Skip to content

feat(helm): allow cert-manager to use a public ACME issuer - #514

Merged
minhtule merged 3 commits into
masterfrom
feat/mt/acme-sans
Sep 10, 2026
Merged

feat(helm): allow cert-manager to use a public ACME issuer#514
minhtule merged 3 commits into
masterfrom
feat/mt/acme-sans

Conversation

@minhtule

@minhtule minhtule commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Add the kubernetes Service DNS names to the certificate only when the Gateway serves Kubernetes resources (kubernetes.enabled or twingateOperator.kubernetesResource.enabled), since a public CA refuses them.
    • kubernetes, kubernetes.default, kubernetes.default.svc, kubernetes.default.svc.<clusterDomain>
  • Emit tls.autoGenerated.certManager.commonName only when set, instead of defaulting to the chart fullname, which is equally unissuable by a public CA.
  • Fail the render with an actionable message when neither tls.dnsNames nor tls.ipAddresses leaves the certificate any identifier.

Notes

  • Existing installs on the helm TLS engine (the default) regenerate tls.crt, tls.key and ca.crt on the first upgrade, because the alternative-names hash now covers a different string and no longer matches the checksum/alternativeNames annotation on the existing Secret. Releases with twingateOperator.gateway.enabled: true re-register the new ca.crt through the operator; those that registered the CA by hand have to re-register it.

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.70%. Comparing base (47bfdd0) to head (be662e6).
⚠️ Report is 7 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
- Coverage   93.74%   93.70%   -0.04%     
==========================================
  Files          42       42              
  Lines        2684     2684              
==========================================
- Hits         2516     2515       -1     
- Misses        167      168       +1     
  Partials        1        1              
Flag Coverage Δ
integration 64.63% <ø> (-0.04%) ⬇️
unit 88.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Enables public ACME issuers by omitting unissuable internal names unless Kubernetes access requires them.

Changes:

  • Centralizes TLS SAN generation and validation.
  • Makes cert-manager common names optional.
  • Updates Helm tests, documentation, and snapshots.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Summary
deploy/gateway/values.yaml Documents conditional Kubernetes SANs.
deploy/gateway/values.schema.json Updates TLS field descriptions.
deploy/gateway/tests/tls-secret_test.yaml Tests Helm-generated certificates.
deploy/gateway/tests/NOTES_test.yaml Updates SAN hash fixtures.
deploy/gateway/tests/cert-manager_test.yaml Tests cert-manager behavior; IP-only validation lacks direct coverage.
deploy/gateway/tests/__snapshot__/cert-manager_test.yaml.snap Updates expected certificate output.
deploy/gateway/templates/tls-secret.yaml Uses shared SAN handling.
deploy/gateway/templates/cert-manager.yaml Supports public issuers and optional common names.
deploy/gateway/templates/_helpers.tpl Centralizes SAN generation, validation, and hashing.
Suppressed comments (1)

deploy/gateway/tests/cert-manager_test.yaml:152

  • This test still supplies tls.dnsNames, so it would pass even if the new requireTlsNames guard incorrectly rejected certificates whose sole identifier is an IP address. Make this case IP-only so the newly supported tls.ipAddresses branch is covered.
        dnsNames:
          - resource.corp.internal
        ipAddresses:
          - 10.0.0.1

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@minhtule
minhtule force-pushed the feat/mt/acme-sans branch 2 times, most recently from 5519af2 to be226c2 Compare September 6, 2026 23:36
@minhtule
minhtule requested a balanced review from Copilot September 7, 2026 00:37
@minhtule
minhtule marked this pull request as ready for review September 7, 2026 00:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread deploy/gateway/templates/_helpers.tpl
Copilot AI review requested due to automatic review settings September 7, 2026 01:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@minhtule
minhtule requested a review from clement0010 September 7, 2026 01:08

@clement0010 clement0010 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍🏻

One question: Since public CA can't sign the K8s DNS name, does that mean we can't enable both k8s and web app for this specifc setup?

"items": {
"type": "string"
"type": "string",
"minLength": 1

@clement0010 clement0010 Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could add a simple unit test for this:

set:
  tls:
    autoGenerated:
      enabled: true
    dnsNames: [""]

in tls-secret_test.yaml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in be662e6

Comment on lines +462 to +463
"type": "string",
"minLength": 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@minhtule

minhtule commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Since public CA can't sign the K8s DNS name, does that mean we can't enable both k8s and web app for this specifc setup?

No, not until we support multiple CAs per Gateway in the backend.

Copilot AI review requested due to automatic review settings September 7, 2026 17:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@minhtule
minhtule merged commit 0c362d1 into master Sep 10, 2026
14 checks passed
@minhtule
minhtule deleted the feat/mt/acme-sans branch September 10, 2026 01:01
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