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

[jaeger-operator] Fix certificate condition in deployment file for #375 #376

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[jaeger-operator] Fix certificate condition in deployment file for #375 #376

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: 1 addition & 1 deletion charts/jaeger-operator/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
apiVersion: v1
description: jaeger-operator Helm chart for Kubernetes
name: jaeger-operator
version: 2.32.2
version: 2.32.3
appVersion: 1.34.1
home: https://www.jaegertracing.io/
icon: https://www.jaegertracing.io/img/jaeger-icon-reverse-color.svg
Expand Down
4 changes: 4 additions & 0 deletions charts/jaeger-operator/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,12 @@ spec:
- containerPort: 9443
name: webhook-server
protocol: TCP
{{- if .Values.certs.certificate.create }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Check out the https://github.com/jaegertracing/helm-charts/tree/main/charts/jaeger-operator#prerequisites for the operator chart and my comment in conversation. In case you don't want to use cert-manager you should create certificate for webhook service in a secret instead.

Copy link
Author

@AshutoshNirkhe AshutoshNirkhe Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@czomo thanks for the explanation. I have already configured it to use webhooks and self-signed issuer/certs now so am good.
Basically it started with if we can disable both webhooks and hence certificates for webhooks.
Like what if I disable both the webhooks ? (https://github.com/jaegertracing/helm-charts/blob/jaeger-operator-2.32.2/charts/jaeger-operator/values.yaml#L22)
Why should I still care about certificates ? So if webhooks are mandatory, shall we get rid off those flags from values file to enable/disable them or update the Readme ?

Copy link
Contributor

Choose a reason for hiding this comment

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

jaeger-operator can be deployed in multiple ways, that's why there is option to disable specific components.

webhooks in general are integrated part of jaeger because they are responsible for validating jeager resources(validating webhook) or in case of mutating webhooks adding jaeger sidecars. However there are some cases when you want do disable them because of conflicts with other resources. Be carefull about that.
One of the purpose of certs is handling secure connection between service and webhooks, more features will be added in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Ok so I think its wiser to not disable any of that :) We don't this PR anymore in that case.

Copy link

Choose a reason for hiding this comment

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

We should be able to disable creating the certificates so people can use their own cert-manager issuers and settings. Is this PR being merged?

volumeMounts:
- mountPath: /tmp/k8s-webhook-server/serving-certs
name: cert
readOnly: true
{{- end }}
args: ["start"]
env:
- name: WATCH_NAMESPACE
Expand All @@ -77,11 +79,13 @@ spec:
{{- end }}
resources:
{{ toYaml .Values.resources | indent 12 }}
{{- if .Values.certs.certificate.create }}
volumes:
- name: cert
secret:
defaultMode: 420
secretName: {{ default "jaeger-operator-service-cert" .Values.certs.certificate.secretName }}
{{- end }}
{{- with .Values.nodeSelector }}
nodeSelector:
{{ toYaml . | indent 8 }}
Expand Down