Create serving certs for headless services on OpenShift (#818) #824
Conversation
|
|
||
| // copy to avoid modifying params.Instance.Annotations | ||
| annotations := map[string]string{ | ||
| "service.beta.openshift.io/serving-cert-secret-name": fmt.Sprintf("%s-tls", h.Name), |
There was a problem hiding this comment.
This annotation should be present only if the collector is deployed on the OpenShift.
IIRC there is already some machinery in the project that recognizes the platform.
There was a problem hiding this comment.
I had considered this, but noticed in https://github.com/jaegertracing/jaeger-operator/blob/main/pkg/service/collector.go#L28 that jaeger-operator adds the annotation regardless of the platform. My understanding is that unrecognized annotations are silently ignored, so I don't think there is any harm. Also, making this conditional would require addition unit test code. But I can make it conditional if required.
|
@rkukura is there any documentation needed for this? Not familiar users might ask how to use those certs? Why does only the headless service have them? |
…ry#818) Add annotation to create serving cerfificates for services on OpenShift. Resolves: open-telemetry#818 Signed-off-by: Robert Kukura <[email protected]>
@pavolloffay I added some text to README.md. The headless service is what is typically used within the cluster, such as for connections from agents. The generated certificate is only trusted within the cluster. |
| @@ -57,6 +57,8 @@ The `config` node holds the `YAML` that should be passed down as-is to the under | |||
|
|
|||
| At this point, the Operator does *not* validate the contents of the configuration file: if the configuration is invalid, the instance will still be created but the underlying OpenTelemetry Collector might crash. | |||
|
|
|||
| The Operator does examine the configuration file to discover configured receivers and their ports. If it finds receivers with ports, it creates a pair of kubernetes services, one headless, exposing those ports within the cluster. The headless service contains a `service.beta.openshift.io/serving-cert-secret-name` annotation that will cause OpenShift to create a secret containing a certificate and key. This secret can be mounted as a volume and the certificate and key used in those receivers' TLS configurations. | |||
There was a problem hiding this comment.
The text looks good/ it explains what the annotation does. I would it makes sense here also document why someone should be doing something like this/waht is the common use case? Users are often searching for solutions for use-cases.
There was a problem hiding this comment.
More general discussion of configuring and securing the collector is not specific to the operator, so probably doesn't belong in this repo. We could add some mention of this operator functionality in https://opentelemetry.io/docs/collector/configuration/#setting-up-certificates after this PR is merged. Adding new documentation in this repo that covers common operator use cases (including but not limited to securing OTLP between agent and gateway collectors) seems beyond the scope of this PR.
There was a problem hiding this comment.
More general discussion of configuring and securing the collector is not specific to the operator, so probably doesn't belong in this repo.
Our users should understand why there is a headless service, when to use it and what is the use case for using those certificates.
We could add some mention of this operator functionality in https://opentelemetry.io/docs/collector/configuration/#setting-up-certificates
I don't think that operator documentation belongs there. You could try to work with the docs/website sig to add an operator page there. So far there are no operator docs in opentelemetry.io.
There was a problem hiding this comment.
@pavolloffay The headless service has been created by this operator for a long time. I agree the operator's documentation on it is lacking. But does that need to be resolved before this PR can be merged? Can we create an issue to improve the documentation?
There was a problem hiding this comment.
Maybe I was not clear, I wanted a better docs for the certificates - what is the use-case for them etc.
There was a problem hiding this comment.
You can create an additional PR to add them
…ry#818) (open-telemetry#824) Add annotation to create serving cerfificates for services on OpenShift. Resolves: open-telemetry#818 Signed-off-by: Robert Kukura <[email protected]>
resolves #818
Signed-off-by: Robert Kukura [email protected]