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

set template for ca issuer name and secret name + geo-replication installation example #565

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

Conversation

gulecroc
Copy link

Motivation

To test geo-replication, I would like to install the pulsar chart 3 times :

  • 1 for global zookeeper
  • 2 for 2 pulsar clusters

I also would like to use TLS. The global zookeeper install will create the CA issuer and certificate, and the 2 pulsar clusters will use it to create their certificates.

Modifications

  • add template to override CA issuer name and certificate secret name
  • add example to create a geo-replication installation

Verifying this change

  • Make sure that the change passes the CI checks.

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.

Thanks for the PR, @gulecroc . Please remove the geo-replication changes out of this PR since that will simplify handling of this PR so that we first get the TLS templating changes made.
We can handle the geo-replication examples in a separate PR.

I really appreciate your effort in making a Geo-replication example available directly in the Pulsar helm chart.
One minor detail is that global Zookeeper isn't required in Pulsar to use geo-replication. For completeness of the examples, it would be useful to then have examples of both cases. The Pulsar documentation doesn't clearly explain the different ways Pulsar geo-replication can be configured.

Comment on lines +20 to +40
{{/*
Define the pulsar certs ca issuer name
*/}}
{{- define "pulsar.certs.issuers.ca.name" -}}
{{- if .Values.certs.issuers.ca.name -}}
{{- .Values.certs.issuers.ca.name -}}
{{- else -}}
{{ template "pulsar.fullname" . }}-{{ .Values.certs.internal_issuer.component }}-ca-issuer
{{- end -}}
{{- end -}}

{{/*
Define the pulsar certs ca issuer secret name
*/}}
{{- define "pulsar.certs.issuers.ca.secretName" -}}
{{- if .Values.certs.issuers.ca.secretName -}}
{{- .Values.certs.issuers.ca.secretName -}}
{{- else -}}
{{ printf "%s-%s" .Release.Name .Values.tls.ca_suffix }}
{{- end -}}
{{- end -}}
Copy link
Member

Choose a reason for hiding this comment

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

This is a great improvement.
One additional request would be to handle both .Values.certs.internal_issuer.type cases in these templates, for both "selfsigning" and "ca". That would make this PR more consistent. (#561 recently added/improved "ca" handling).

@gulecroc
Copy link
Author

Hi @lhotari,

I remove the geo-replication doc.
I don't think it's necessarry to add .Values.certs.internal_issuer.type cases in the certs template because we could also set ca name and secretName values for selfsigning ?

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.

2 participants