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.

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

feat(tls): support ca type issuer and v1alpha* version cert-manager api #561

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ericsyh
Copy link

@ericsyh ericsyh commented Dec 13, 2024

Fixes #

Motivation

The current chart only supports enable tls with self-signed issuer which is not possible for production users. This PR supports ca type issuser which allows users to bound a root certificate (in Secret) to issue required tls certificates for pulsar components.

Modifications

  • support the ca type issuer in the certs.type and add the certs.issuers.ca.secretName field for the root certificate reference.
  • support cert-manager v1alpha[1-3] API versions.

Verifying this change

  • Make sure that the change passes the CI checks.

Signed-off-by: ericsyh <ericshenyuhao@outlook.com>
Signed-off-by: ericsyh <ericshenyuhao@outlook.com>
Signed-off-by: ericsyh <ericshenyuhao@outlook.com>
Signed-off-by: ericsyh <ericshenyuhao@outlook.com>
Signed-off-by: ericsyh <ericshenyuhao@outlook.com>
@ericsyh ericsyh changed the title [feat](tls): support ca type issuer and v1alpha* version cert-manager api feat(tls): support ca type issuer and v1alpha* version cert-manager api Dec 13, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Good work! I added a question regarding backwards compatibility.

Comment on lines -51 to -52
- "*.{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}.{{ template "pulsar.namespace" . }}.svc.{{ .Values.clusterDomain }}"
- "{{ template "pulsar.fullname" . }}-{{ .Values.proxy.component }}"
Copy link
Member

Choose a reason for hiding this comment

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

Is this removal a breaking change? Does this require release note instructions users that upgrade? Would it be possible to retain compatibility?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, it's not a breaking change cause i just move the position to make the format of proxy certificate dnsNames and broker certificate dnsNames to be consistent. It doesn't change the logic and technical this change is optional.

Copy link
Author

@ericsyh ericsyh left a comment

Choose a reason for hiding this comment

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

replay the comment.

Signed-off-by: ericsyh <ericshenyuhao@outlook.com>
Copy link

@ciiiii ciiiii left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants