feat: add config to overwrite grpc certificate

[K3das]

• read the CONTRIBUTING guidelines
• raised a GitHub issue or discussed it on the projects chat beforehand
• added unit tests
• added integration tests
• updated documentation if needed
• updated CHANGELOG.md

This adds two config options to overwrite the TLS certificate for the gRPC API

# Use separate a certificate for gRPC, this overwrites
# the global certificate.
grpc_tls_cert_path: ""
grpc_tls_key_path: ""

(closes #1709)

[ohdearaugustin]

Looks good to me.

[ohdearaugustin]

Suggested change

	# Use separate a certificate for gRPC, this overwrites
	# Use separate a x509 certificate for gRPC, this overwrites

[kradalby]

Overwrite sounds incorrect, it's more like using separate certs for grpc.

What is the use case for this, why would you want it?

It will also need some sort of integration tests.

[K3das]

I used "overwrite" since that would imply that, by default, it would use certificates defined in the other settings, I'm happy to change this wording.

My use case is described in the attached issue; I don't want to configure TLS for the HTTP API endpoints, but configure it for the gRPC endpoints.

I'll add integration tests if you still consider this an acceptable change.

[kradalby]

My use case is described in the attached issue; I don't want to configure TLS for the HTTP API endpoints, but configure it for the gRPC endpoints.

OK, I am not super stoked about this as we mainly encourage less knobs to configure this kind of stuff as users seem to find them, try to use them and generate support load. I appreciate that it is useful for power users whom understand how to use them, we have no good mechanism for preventing everyone else from doing it.

I can particular imagine that a lot of people using varieties of proxies will run into things here.

I'll add integration tests if you still consider this an acceptable change.

I do not know exactly how I want them to look or how to test something like this, so this might be a challenging exercise in it self, but if you want to take a stab at it, you are welcome to do so.

[kradalby]

At this point, with the growing amount of options it should end up like:

Suggested change

	grpc_tls_cert_path: ""
	grpc_tls_key_path: ""
	grpc:
	allow_insecure: false
	tls:
	cert_path: ""
	tls_key_path: ""

[K3das]

agreed, though this would create breaking changes, which @ohdearaugustin asked to avoid

[kradalby]

It should be possible to map the allow insecure to the new value.

[K3das]

I do not know exactly how I want them to look or how to test something like this, so this might be a challenging exercise in it self, but if you want to take a stab at it, you are welcome to do so.

Not sure either, but this could probably involve working on a better test suite for the TLS implementation, since from what I understand, ACME is completely untested. Alternatively, integration tests could generate and trust a certificate for gRPC like they do for the HTTP API, then just check the fingerprint(?).

Outside of CLI tests, are there tests for gRPC at all?

[kradalby]

This looks reasonable, but please add a new test so we dont change already known ones.

[K3das]

@kradalby just to confirm, is it fine that this is breaking gRPC config. Would it be a better idea to put this with the TLS settings and break those (by making them a map)?

[kradalby]

@kradalby just to confirm, is it fine that this is breaking gRPC config. Would it be a better idea to put this with the TLS settings and break those (by making them a map)?

It should not require breaking it, you should be able to set up an alias in cobra/viper to handle it.

[K3das]

Alright, I'll see what I can do, still, would it be a better idea to keep this with TLS configs instead?

[kradalby]

Alright, I'll see what I can do, still, would it be a better idea to keep this with TLS configs instead?

Sorry, forgot to answer this. Might be, I kind of want this feature to be "available, but not accessible", as I mentioned earlier that I am concerned people would use it in the wrong way. Maybe "hiding" it in tls makes sense.

[K3das]

closing this for now, I don't have a significant need for this anymore, and given new changes it would require a bit of effort to merge and update this patch.