K8SPSMDB-1606: Add CA key persistence for manual TLS to support safe cert re-signing on SAN changes#2277
Merged
hors merged 12 commits intoJun 8, 2026
Conversation
myJamong
requested review from
egegunes,
eleo007,
gkech,
hors,
jvpasinatto,
mayankshah1607,
nmarukovich,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
egegunes
changed the title
egegunes
requested changes
Mar 9, 2026
Comment on lines
459
to
461
| if cr.CompareVersion("1.17.0") < 0 { | ||
| secretObj.Labels = nil | ||
| caLabels = nil | ||
| } |
Contributor
There was a problem hiding this comment.
this condition can be removed
Contributor
Author
There was a problem hiding this comment.
Removed the condition - 0a1aa64
It seems like this line slipped in by accident.
Comment on lines
+477
to
+479
| if cr.CompareVersion("1.17.0") < 0 { | ||
| secretLabels = nil | ||
| } |
Contributor
There was a problem hiding this comment.
we don't need this condition
Contributor
Author
There was a problem hiding this comment.
Removed the condition - 0a1aa64
It seems like this line slipped in by accident.
pooknull
previously approved these changes
Apr 14, 2026
egegunes
previously approved these changes
May 11, 2026
hors
previously approved these changes
May 15, 2026
gkech
previously approved these changes
May 25, 2026
gkech
left a comment
gkech
left a comment
Contributor
There was a problem hiding this comment.
approving, but minor comment for future consideration
| } | ||
|
|
||
| // GetSANsFromCert extracts DNS SANs from a PEM-encoded TLS certificate. | ||
| func GetSANsFromCert(tlsCertPEM []byte) ([]string, error) { |
Contributor
There was a problem hiding this comment.
Got confused by this function name. Either rename to GetDNSNamesFromCert or return all SAN types
Contributor
Author
The function returns only cert.DNSNames, not all SAN types. Renaming to match actual behavior per PR review feedback.
| } | ||
|
|
||
| // needsManualSSLUpdate checks if the TLS certificate SANs differ from the expected SANs. | ||
| func (r *ReconcilePerconaServerMongoDB) needsManualSSLUpdate(ctx context.Context, cr *api.PerconaServerMongoDB, sslSecret *corev1.Secret) bool { |
Contributor
Author
There was a problem hiding this comment.
@mayankshah1607 Thanks. Used ctx to add the missing error log below — that resolves your other comment too. Fixed in f502c50
| currentSANs, err := tls.GetDNSNamesFromCert(sslSecret.Data["tls.crt"]) | ||
| if err != nil { | ||
| return errors.Wrap(err, "create TLS internal secret") | ||
| return false |
Member
There was a problem hiding this comment.
If we are swallowing the error, we should at least log it here (in that case we will need the context logger)
Contributor
Author
There was a problem hiding this comment.
@mayankshah1607 Thanks. Added an error log via the context logger. Kept the return false since the caller treats this as advisory and retrying wouldn't recover a corrupt cert. Fixed in f502c50
egegunes
approved these changes
Jun 1, 2026
mayankshah1607
approved these changes
Jun 5, 2026
gkech
approved these changes
Jun 5, 2026
Collaborator
commit: 1aff7fd |
Collaborator
|
@myJamong thank you for the contribution |
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
7 participants
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.
CHANGE DESCRIPTION
Problem:
When cert-manager is not installed, the operator's manual TLS path has two issues: (1) it never detects SAN changes (e.g., when splitHorizons are added), because it skips reconciliation if TLS secrets already exist, and (2) if secrets are manually deleted to force regeneration, a completely new CA is generated without merging with the old one, causing TLS verification failures during SmartUpdate rolling restarts.
I also made an issue here: #2278
Cause:
The manual TLS code path (createSSLManually) only creates secrets when they don't exist and returns immediately if they do — there is no SAN change detection. Additionally, each call to tls.Issue() generates an independent CA, meaning ssl and ssl-internal use different CAs, and any regeneration produces a CA that existing pods cannot trust.
Solution:
Introduce a persistent CA secret ({name}-ca-cert) for manual TLS management, mirroring the cert-manager CA secret structure. The CA key is preserved so that when SANs change (e.g., splitHorizon additions), the operator re-signs TLS certificates using the same CA — no CA merge is needed and rolling restarts are safe. Key changes:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability