K8SPXC-1848: update existing cert-manager Certificate CRs on operator upgrade#2414
Open
larainema wants to merge 1 commit intopercona:mainfrom
Open
K8SPXC-1848: update existing cert-manager Certificate CRs on operator upgrade#2414larainema wants to merge 1 commit intopercona:mainfrom
larainema wants to merge 1 commit intopercona:mainfrom
Conversation
larainema
requested review from
egegunes,
gkech,
hors,
mayankshah1607,
nmarukovich,
oksana-grishchenko and
pooknull
as code owners
|
|
|
|
larainema
changed the title
egegunes
requested changes
Apr 1, 2026
pkg/controller/pxc/tls.go
Outdated
| return fmt.Errorf("create CA certificate: %v", err) | ||
| err := r.createOrUpdateCertificate(ctx, caCert) | ||
| if err != nil { | ||
| return fmt.Errorf("create or update CA certificate: %v", err) |
Contributor
There was a problem hiding this comment.
please wrap errors here other changed lines
| return nil | ||
| } | ||
|
|
||
| func (r *ReconcilePerconaXtraDBCluster) createOrUpdateCertificate(ctx context.Context, desired *cm.Certificate) error { |
Contributor
There was a problem hiding this comment.
would it be better to use controllerutil.CreateOrUpdate?
egegunes
changed the title
larainema
force-pushed
the
fix/update-cert-manager-certificate-specs
branch
from
65f04b9 to
6feb7f0
Compare
egegunes
reviewed
Apr 2, 2026
Contributor
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
Code changes LGTM.
We need to add a case into e2e tests though, probably to tls-issue-cert-manager:
- Patch PerconaXtraDBCluster to change something under
spec.tls, potentially the duration. - Check if certificates are updated.
- Ensure cluster is still available.
@eleo007 wdyt?
egegunes
requested changes
Apr 6, 2026
Contributor
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
@larainema please add a case into tls-issue-cert-manager e2e tests to check certificate updates
larainema
requested review from
eleo007,
jvpasinatto and
valmiranogueira
as code owners
larainema
force-pushed
the
fix/update-cert-manager-certificate-specs
branch
3 times, most recently
from
d345033 to
41d97bd
Compare
createSSLByCertManager() uses r.client.Create() with an IsAlreadyExists guard, so any spec changes (duration, renewBefore, SANs, labels) introduced in newer operator versions are never applied to Certificate CRs that were created by an older version. The most significant impact is the CA certificate duration: clusters originally deployed with operator < 1.15.0 still carry duration=8760h (1 year) instead of the current default of 26280h (3 years). Replace the create-and-ignore pattern with a createOrUpdateCertificate helper that attempts Create first, and on AlreadyExists fetches the existing Certificate, compares Spec and Labels, and issues an Update when they differ. Fixes percona#2413
larainema
force-pushed
the
fix/update-cert-manager-certificate-specs
branch
from
41d97bd to
d346d57
Compare
Collaborator
commit: d346d57 |
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
5 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.
Problem
createSSLByCertManager()usesr.client.Create()with anIsAlreadyExistsguard for all three cert-manager Certificate resources (CA, ssl, ssl-internal). When the Certificate CR already exists, the function silently skips it — any spec changes introduced in newer operator versions are never applied.The most significant impact is the CA certificate duration: clusters originally deployed with operator < 1.15.0 still carry
duration: 8760h(1 year) instead of the current default26280h(3 years), because the Certificate CR was never updated after the operator upgrade.Fixes #2413
Root Cause
The same pattern is used for all three Certificate CRs. On
AlreadyExists, the new desired spec is discarded.Fix
Add a
createOrUpdateCertificate()helper that:Createfirst (fast path for new clusters)AlreadyExists: fetches the existing Certificate CRSpecandLabelsusingreflect.DeepEqualUpdateAll three call sites in
createSSLByCertManagernow use this helper.Affected Fields (now reconciled on upgrade)
spec.duration— CA validity (most impactful: 1yr → 3yr)spec.renewBeforespec.dnsNames(SANs)spec.commonNamelabelsRelated