Repository navigation
migrate-certs: carry over node cert SANs and reissue the client cert - #689
Open
isaactwong wants to merge 5 commits into
Open
isaactwong wants to merge 5 commits into
isaactwong wants to merge 5 commits into
Conversation
GenerateNodeCert inlined the hosts every node certificate must carry, and the generator's tests kept their own copy of the same list. The migration helper is about to need the list too, to tell which SANs on an existing certificate are extras worth carrying over. Give it a name, NodeCertHosts, and make the generator and the tests use it. No behavior change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Drive the self-signer branch of GenerateCertsForOperator against the objects the legacy chart leaves behind: a node certificate that carries SANs the chart's additionalSANs setting added, and a ready 672h root client certificate. Record what comes out today: the regenerated node certificate gains the join names but loses the extra SANs, and the client certificate is kept as is although the migrated values disable the self-signer and nothing will rotate it. The next commit fixes both and flips these assertions, so the fix shows up in its own diff. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GenerateClientCert issued certificates from an inline closure. Lift it into generateClientCert so a second entry point can issue without the ready check. No behavior change; TestCertificateRotation now also pins the path that keeps a ready, not-yet-due client secret, which nothing covered before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
migrate-certs builds its GenerateCert by hand rather than through the self-signer's getInitialConfig, so it never learned the ADDITIONAL_SANS the legacy chart passed at install time, and it had nowhere to learn them from: the migration does not see the old values. The regenerated node certificate carried only the standard hosts and the join names, and clients that verified against an extra SAN failed after migration. The cert-manager branch of the same migration already preserves existing SANs by appending the join names to the Certificate CR. Do the same for the self-signer branch, from the only record there is: the existing node certificate (<name>-node-secret for the legacy chart, <name>-node for the public operator). InheritSANs adds everything the certificate carries beyond NodeCertHosts to AdditionalSANs before the node certificate is regenerated, and the command logs what it carried over so the names can be copied into additionalSANs before the self-signer is re-enabled. The migration also kept the legacy 672h client certificate because it was still valid, while the exported values disable the self-signer, so nothing would ever rotate it. Reissue it unconditionally (RegenerateClientCert, with the user named explicitly rather than read from USER_NAME) and default --client-duration to --node-duration so both certificates last a year. The command's help text and the two manual-migration runbooks say so; the node-duration help text's "365h" typo is fixed in passing. Restoring rotation for migrated self-signer clusters is #688. Fixes #687 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The migrate-certs changes in this branch live in pkg/generator, which the self-signer image is built from. CI's Tag Change gate requires the chart values to move to a new tag whenever that package changes, so that the merge builds and publishes an image carrying the fix. Bump both charts and their generated-template sources from 1.11 to 1.12 and record it in the CHANGELOGs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Oct 22, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After
migrate-certsruns on a cluster the legacy chart's self-signer managed, two things go wrong. Clients that verify the server against a name fromtls.certs.selfSigner.additionalSANsfail TLS verification, because the regenerated node certificate carries only the standard hosts and the join names. And the legacy 672h root client certificate is kept as is, while the exported values disable the self-signer, so nothing rotates it and it expires within 28 days. The migration doc promised a one-year TTL it did not deliver for the client certificate.Root cause
migrate-certsbuilds itsGenerateCertby hand instead of through the self-signer'sgetInitialConfig, so it never learned theADDITIONAL_SANSthe install Job passed, and it had nowhere to learn them from: the migration does not see the old chart values. The cert-manager branch of the same migration already preserves existing SANs by appending the join names to the Certificate CR'sDNSNames; the self-signer branch regenerated from scratch.Fix
The existing node certificate is the record.
NodeCertHostsnames the standard host list (one definition, also replacing a copy the generator's tests kept), andInheritSANsadds everything the existing certificate carries beyond it toAdditionalSANsbefore the node certificate is regenerated. The migration looks in<name>-node-secret(legacy chart) and<name>-node(public operator), logs the names it carried over, and warns if it finds no certificate at all.The client certificate is reissued unconditionally through the new
RegenerateClientCert, which names the user explicitly rather than readingUSER_NAMEfrom the environment, and--client-durationnow defaults to--node-durationso both certificates last a year. The two manual-migration runbooks describe the behavior and tell users to copy the carried-over names intocockroachdb.tls.selfSigner.additionalSANsbefore re-enabling the self-signer, since the self-signer rebuilds certificates from configuration, not from the previous certificate.Commits
generator: name the node certificate's standard host list. Mechanical; no behavior change.migrate: test what migrate-certs carries over from the legacy secrets. Passes onmasterby asserting today's behavior: extras dropped, client certificate kept. Reading its assertions, then the flip in commit 4, is the fastest way to see the change.generator: lift GenerateClientCert's issuing closure into a method. Mechanical; adds the keep-ready checkTestCertificateRotationwas missing.migrate: carry the node cert's SANs over and reissue the client cert. The fix.Behavior changes
<name>-client-secretout of the cluster must fetch it again.--client-durationdefaults to--node-durationinstead of 672h. An explicit flag is still honored.--node-durationhelp text's "365h" typo is fixed in passing.Scope
Self-signer branch of
GenerateCertsForOperatoronly. The cert-manager and provided-certificates branches are untouched. This PR does not restore rotation for migrated self-signer clusters; it gives them a year to adoptcockroachdb.tls.selfSigner.enabled: true. #688 tracks restoring rotation. The controller-migration runbooks describe the operator's in-process migration controller, which lives elsewhere, and were left alone.Testing
go test ./pkg/generator/ ./pkg/migrate/ ./cmd/migrate/... ./cmd/self-signer/on every commit, fake controller-runtime client, no cluster. The migration test seeds the legacy artifacts (Helm release annotation, CA secret with key, node secret with extra SANs and the self-signer's annotations, ready 672h client secret) and covers three shapes: legacy chart, no existing node secret, and public operator. Removing the<name>-nodefallback fails the public-operator case. No k3d migration e2e was run.Fixes #687
Epic: none
🤖 Generated with Claude Code
Part of a sweep of fixes for one bug shape: two code paths that should build the same thing from the same inputs were written as separate copies and drifted. Each PR in the set follows one arc: the first commit adds a test that exercises both paths side by side and records today's divergence (it passes on
master); the second commit makes the paths share one definition and flips the test to equality.Siblings: helm-charts #691, cockroach-operator #1167, cockroach-operator #1169, cockroach-operator #1172.