Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create serving certs for headless services on OpenShift (#818) #824

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Create serving certs for headless services on OpenShift (#818) #824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rkukura rkukura Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I was not clear, I wanted a better docs for the certificates - what is the use-case for them etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create an additional PR to add them



### Upgrades

Expand Down
10 changes: 10 additions & 0 deletions pkg/collector/reconcile/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,16 @@ func headless(ctx context.Context, params Params) *corev1.Service {
}

h.Name = naming.HeadlessService(params.Instance)

// 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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
for k, v := range h.Annotations {
annotations[k] = v
}
h.Annotations = annotations

h.Spec.ClusterIP = "None"
return h
}
Expand Down
1 change: 1 addition & 0 deletions pkg/collector/reconcile/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ func TestDeleteServices(t *testing.T) {
func TestHeadlessService(t *testing.T) {
t.Run("should return headless service", func(t *testing.T) {
actual := headless(context.Background(), params())
assert.Equal(t, actual.Annotations["service.beta.openshift.io/serving-cert-secret-name"], "test-collector-headless-tls")
assert.Equal(t, actual.Spec.ClusterIP, "None")
})
}
Expand Down