Skip to content

Configure certificates subject - #714

Merged
lhotari merged 4 commits into
apache:masterfrom
gulecroc:feat/certs-subject
Oct 6, 2026
Merged

lhotari merged 4 commits into
apache:masterfrom
gulecroc:feat/certs-subject

Conversation

@gulecroc

Copy link
Copy Markdown
Contributor

Motivation

Currently only certificates subject organizations is configurable.

This PR allow to configure full subject properties.

Modifications

Move tls.common.organization to tls.common.subject.organizations.

Fail if tls.common.organization is still used.

Verifying this change

  • Make sure that the change passes the CI checks.

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good. I verified locally that the default render is byte-identical to master, the subject block maps correctly onto cert-manager's X509Subject, an empty subject renders nothing rather than subject: null, and values.yaml carries no organization remnant that could trip the guard spuriously.

Render recipe I used, if useful:

helm dependency build charts/pulsar
helm template test charts/pulsar --set "tls.enabled=true,tls.broker.enabled=true,\
tls.proxy.enabled=true,tls.zookeeper.enabled=true,tls.bookie.enabled=true,\
certs.internal_issuer.enabled=true,components.proxy=true" -s templates/tls-certs-internal.yaml

I initially wanted to push back on two things and talked myself out of both — see the inline notes.

One request before merge: document the migration

The diff touches only _certs.tpl and values.yaml. The README's upgrade procedure explicitly says helm get values … > values.yaml and reuse it, which is exactly the path that now aborts. There's good precedent to follow: the "Upgrading to Helm chart version 4.1.0" section covers the structurally identical auth.authentication.provider removal.

Could you add a README "Upgrading to Helm chart version 4.8.0" section showing the before/after:

# before
tls:
  common:
    organization:
      - pulsar
# after
tls:
  common:
    subject:
      organizations:
        - pulsar

Approving so this isn't blocked on me — please land the docs before merging.

Note

#713 also edits _certs.tpl. git merge-tree shows no textual conflict at the current heads, but whichever lands second should be rebased and re-rendered.

Reviewed with Codex gpt-5.6-sol and Claude Opus 5; every finding reproduced locally by rendering the chart.

Comment thread charts/pulsar/templates/_certs.tpl
Comment thread charts/pulsar/values.yaml
@gulecroc

Copy link
Copy Markdown
Contributor Author

Migration documented in 79b949b

@gulecroc
gulecroc requested a review from lhotari August 25, 2026 06:50

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for adding the commented-out X509Subject fields and the upgrade note. Both address my earlier feedback.

@lhotari
lhotari merged commit 0abec65 into apache:master Oct 6, 2026
41 checks passed
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