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

Jump to bottom

chore: Simplify cert-manager certs and support existingIssuer #442

Closed
wants to merge 1 commit into from
Closed

chore: Simplify cert-manager certs and support existingIssuer #442

wants to merge 1 commit into from

Conversation

mkilchhofer
Copy link
Contributor

@mkilchhofer mkilchhofer commented May 15, 2023 •
edited
Loading

Improve 2 things related to cert-manager certificates:

  • Simplify the cert-manager resources. 2 of 4 resources are not required and I never seen such setups in the community
  • add support for an existing Issuer / ClusterIssuer.

Somehow related to #371 as this introduced the usage of cert-manager.

Checklist

  • I have verified that my change is according to the deprecations & breaking changes policy
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • README is updated with new configuration values (if applicable)
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)

Fixes #

mkilchhofer
mkilchhofer commented May 15, 2023
View reviewed changes
keda/templates/metrics-server/apiservice.yaml
{{- else }}
cert-manager.io/inject-ca-from-secret: {{ .Release.Namespace }}/{{ .Values.certificates.certManager.caSecretName }}
{{- end }}
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ .Values.operator.name }}-tls-certificates
Copy link
Contributor Author

@mkilchhofer mkilchhofer May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This wasn't configured like its documented inside the cert-manager docs.

I'd suggest to follow 1:1 the docs of cert-manager:
https://cert-manager.io/docs/concepts/ca-injector/#injecting-ca-data-from-a-certificate-resource

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor Author

@mkilchhofer mkilchhofer May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The annotation cert-manager.io/inject-ca-from has the value <namespace-name>/<certificate-name>

In the current implementation of the chart the value is <namespace-name>/<secret-name> which seems also working but it does not follow 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.

you are right

@mkilchhofer mkilchhofer force-pushed the feature/existing_cert-manager_issuer branch from cb03763 to 84d5c43 Compare May 15, 2023 22:07
@JorTurFer
Copy link
Member

JorTurFer commented May 26, 2023 •
edited
Loading

I believe that we will agree with a v3 of the chart. If finally we continue with the new version, I think that it'll be the good moment to address this change too, not needing to maintain backward compatibility

@mkilchhofer mkilchhofer force-pushed the feature/existing_cert-manager_issuer branch from 84d5c43 to 37d7bff Compare June 23, 2023 14:26
@mkilchhofer
Copy link
Contributor Author

Rebased :-)

@JorTurFer
Copy link
Member

I still think that we should work on KEDA chart v3 and remove all this code that we think it's not okey. It brings us the flexibility to break everything.
I like your approach of removing the intermediate issuer, but it's a breaking change for users who are using the secret with their own CA to generate the certificates...
@tomkerkhove @zroubalik , do we have consensus about working on a chart v3 from scratch (this will be a breaking change and also will detach versioning between core and chart)

@zroubalik
Copy link
Member

I still think that we should work on KEDA chart v3 and remove all this code that we think it's not okey. It brings us the flexibility to break everything. I like your approach of removing the intermediate issuer, but it's a breaking change for users who are using the secret with their own CA to generate the certificates... @tomkerkhove @zroubalik , do we have consensus about working on a chart v3 from scratch (this will be a breaking change and also will detach versioning between core and chart)

I am all in for Charts simplification. I am not sure what we should do with the versioning though...mabye we can even start thinking about KEDA v3? ... there are several proposal that would be breaking and actually great kedacore/keda#4771
But I don't know...

@mkilchhofer
Copy link
Contributor Author

Reusing an existing issuer is now possible via:

Closing my issue now 👍

@mkilchhofer mkilchhofer deleted the feature/existing_cert-manager_issuer branch May 15, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkilchhofer @JorTurFer @zroubalik