K8SPXC-1848: update existing cert-manager Certificate CRs on operator upgrade#2414
Open
larainema wants to merge 8 commits into
Open
K8SPXC-1848: update existing cert-manager Certificate CRs on operator upgrade#2414larainema wants to merge 8 commits into
larainema wants to merge 8 commits into
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
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
egegunes
left a comment
egegunes
left a comment
Contributor
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
egegunes
left a comment
egegunes
left a comment
Contributor
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
4 times, most recently
from
41d97bd to
d346d57
Compare
gkech
reviewed
Apr 9, 2026
larainema
force-pushed
the
fix/update-cert-manager-certificate-specs
branch
from
d346d57 to
eb96e2e
Compare
pull-request-size
Bot
added
size/XL
larainema
force-pushed
the
fix/update-cert-manager-certificate-specs
branch
3 times, most recently
from
3fe1e9b to
2647bfc
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
2647bfc to
3c5b880
Compare
Contributor
Author
Added. The test now: Deploys the cluster with certValidityDuration: 1160h and caValidityDuration: 16280h |
Member
|
Hi @larainema please resolve the conflicts on this PR |
Resolve conflicts with percona#2412 (CA rotation) which has been merged to main: - pkg/controller/pxc/tls.go: combine reconcileCARotation + naming helpers from percona#2412 with reconcileCertManagerCertificateSpecs + build helpers from this PR. Build helpers now use the shared naming constants. - e2e-tests/tls-issue-cert-manager/run: keep cert-manager readiness check, inline cluster creation, and full-pod restart for CA rotation from percona#2412; preserve certificate spec update test from this PR.
Contributor
|
hello @larainema can you please resolve conflicts and check e2e tests results? |
…ager-certificate-specs
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes the operator reconcile the spec of cert-manager Certificate CRs on subsequent reconciles (e.g. after operator upgrade or CR TLS changes), rather than only creating them once. The cert creation logic is also refactored into smaller builder helpers.
Changes:
- Refactor
createSSLByCertManagerto use new helpers (resolveTLSCertConfig,buildCACertificate,buildSSLCertificate,buildSSLInternalCertificate) and a genericcreateOrUpdateCertificatebased oncontrollerutil.CreateOrUpdate. - Add
reconcileCertManagerCertificateSpecsinvoked when the credentials secret already exists, to update existingCertificateCR specs in place. - Add an e2e test variant (
*-updated.ymlconfig + new compare files) that changes durations / SANs and asserts theCertificateCRs are updated.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/pxc/tls.go | Refactors cert-manager certificate creation and adds in-place spec reconciliation. |
| e2e-tests/tls-issue-cert-manager/run | Adds an "update TLS spec" step and asserts CA/SSL Certificate CRs are updated. |
| e2e-tests/tls-issue-cert-manager/conf/some-name-tls-issue-updated.yml | New CR variant with changed durations and an extra SAN. |
| e2e-tests/tls-issue-cert-manager/compare/certificate_some-name-tls-issue-ssl-updated.yml | Expected SSL Certificate after update (gen=2, new SAN, new duration). |
| e2e-tests/tls-issue-cert-manager/compare/certificate_some-name-tls-issue-ca-cert-updated.yml | Expected CA Certificate after update (gen=2, new duration). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+60
to
+64
| if cr.Spec.TLS != nil && cr.Spec.TLS.IssuerConf != nil { | ||
| return fmt.Errorf("reconcile cert-manager certificates: %w", err) | ||
| } | ||
| log := logf.FromContext(ctx) | ||
| log.Info("Failed to reconcile cert-manager certificates, skipping (certs may be manually managed)", "error", err) |
Comment on lines
+276
to
+290
| _, err := controllerutil.CreateOrUpdate(ctx, r.client, existing, func() error { | ||
| existing.Labels = desired.Labels | ||
|
|
||
| // Preserve IssuerRef.Group if the API server defaulted it | ||
| // (cert-manager >= 1.19 defaults empty group to "cert-manager.io"). | ||
| // Without this, every reconcile would see a diff and trigger | ||
| // an unnecessary Update, which increments the Certificate's | ||
| // generation and may cause cert-manager to re-issue. | ||
| if desired.Spec.IssuerRef.Group == "" && existing.Spec.IssuerRef.Group != "" { | ||
| desired.Spec.IssuerRef.Group = existing.Spec.IssuerRef.Group | ||
| } | ||
|
|
||
| existing.Spec = desired.Spec | ||
| return nil | ||
| }) |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" |
Comment on lines
+143
to
+144
| sleep 30 | ||
|
|
Comment on lines
+275
to
+284
| // Preserve IssuerRef.Group if the API server defaulted it | ||
| // (cert-manager >= 1.19 defaults empty group to "cert-manager.io"). | ||
| // Without this, every reconcile would see a diff and trigger | ||
| // an unnecessary Update, which increments the Certificate's | ||
| // generation and may cause cert-manager to re-issue. | ||
| desiredSpec := desired.Spec | ||
| if desiredSpec.PrivateKey != nil { | ||
| privateKey := *desiredSpec.PrivateKey | ||
| desiredSpec.PrivateKey = &privateKey | ||
| } |
Comment on lines
+289
to
+296
| // A CA rotation is triggered by patching rotationPolicy=Always. | ||
| // Keep that explicit override instead of resetting it on the next reconcile. | ||
| if existing.Spec.PrivateKey != nil && desiredSpec.PrivateKey != nil && | ||
| desiredSpec.PrivateKey.RotationPolicy == cm.RotationPolicyNever && | ||
| existing.Spec.PrivateKey.RotationPolicy != "" && | ||
| existing.Spec.PrivateKey.RotationPolicy != desiredSpec.PrivateKey.RotationPolicy { | ||
| desiredSpec.PrivateKey.RotationPolicy = existing.Spec.PrivateKey.RotationPolicy | ||
| } |
Comment on lines
+468
to
+480
| func (r *ReconcilePerconaXtraDBCluster) reconcileCertManagerCertificateSpecs(ctx context.Context, cr *api.PerconaXtraDBCluster) error { | ||
| // Only reconcile if the SSL certificate was created by cert-manager. | ||
| sslCert := &cm.Certificate{} | ||
| if err := r.client.Get(ctx, types.NamespacedName{ | ||
| Namespace: cr.Namespace, | ||
| Name: naming.SSLCertificateName(cr), | ||
| }, sslCert); err != nil { | ||
| if k8serr.IsNotFound(err) { | ||
| // Certificate CR doesn't exist — certs were created manually. | ||
| return nil | ||
| } | ||
| return fmt.Errorf("get ssl certificate: %w", err) | ||
| } |
| } | ||
| _, err := controllerutil.CreateOrUpdate(ctx, r.client, existing, func() error { | ||
| existing.Labels = mergeCertificateLabels(existing.Labels, desired.Labels) | ||
|
|
Contributor
Author
|
Follow-up pushed in 9106e17. Resolved the latest review feedback:
Local checks passed:
GitHub Actions and Jenkins restarted on the new head; Jenkins is PR-2414/18. |
Collaborator
commit: 5e68ee7 |
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.
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