K8SPG-1045: add .spec.tls.certManagementPolicy field#1679
Conversation
| ); err != nil { | ||
| return nil, errors.Wrapf(err, "get user-provided instance TLS secret %s", existing.Name) | ||
| } | ||
| return existing, nil |
There was a problem hiding this comment.
we're returning a nil *corev1.Secret with a nil error. this is unexpected for caller. can we improve if possible or at least comment in function's godoc
| _, hasCluster := obj.GetLabels()[naming.LabelCluster] | ||
| return hasCluster | ||
| }))). | ||
| Watches(&corev1.Pod{}, r.watchPods()). |
There was a problem hiding this comment.
What would be if I create secret manually? As I understand we don't have labels in secret in this case.
There was a problem hiding this comment.
We are going to expect user to set this cluster label: #1679 (comment)
|
@pooknull please fix conflicts |
| clusters := &v1beta1.PostgresClusterList{} | ||
| if err := r.Client.List(ctx, clusters, client.InNamespace(obj.GetNamespace())); err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| requests := make([]reconcile.Request, 0, len(clusters.Items)) | ||
| for i := range clusters.Items { | ||
| requests = append(requests, reconcile.Request{ | ||
| NamespacedName: client.ObjectKeyFromObject(&clusters.Items[i]), | ||
| }) | ||
| } | ||
| return requests |
There was a problem hiding this comment.
are we sure that we want to do this? i'd force user to add cluster label to the secret instead of blindly triggering reconcile
There was a problem hiding this comment.
I agree with you. I have reverted this change: 1c0bbf6
This reverts commit 3e8b007.
commit: 312ca39 |
There was a problem hiding this comment.
Pull request overview
Adds a new .spec.tls.certManagementPolicy API knob to control whether the operator manages TLS secrets (auto, default) or requires all TLS secrets to be user-provided (userProvidedOnly) and pauses reconciliation until prerequisites are met.
Changes:
- Introduces
certManagementPolicyto the PostgresCluster/PerconaPGCluster CRDs and Go types with defaulting + enum validation. - Adds controller logic to detect missing user-provided TLS secrets, set a
TLSSecretsReadycondition, and pause reconciliation viaProgressing=Falsewhen secrets are missing. - Adds KUTTL E2E coverage for the new policy, and updates generated CRD/bundle artifacts.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go | Adds certManagementPolicy type/enum, defaulting helper, and a new condition type constant. |
| internal/controller/postgrescluster/watches.go | Adds a Secret watch to trigger reconcile when user-managed Secrets change. |
| internal/controller/postgrescluster/pki.go | Implements userProvidedOnly behavior: required-secret detection/conditions and user-provided certificate handling. |
| internal/controller/postgrescluster/pgbouncer.go | Honors userProvidedOnly by requiring an existing PgBouncer TLS secret rather than generating one. |
| internal/controller/postgrescluster/pgbackrest.go | Honors userProvidedOnly by requiring an existing pgBackRest secret when repo-host is present. |
| internal/controller/postgrescluster/patroni.go | Requires an existing replication TLS secret when userProvidedOnly is set. |
| internal/controller/postgrescluster/instance.go | Allows StatefulSet creation while deferring instance cert Secret enforcement to subsequent reconciles. |
| internal/controller/postgrescluster/controller.go | Pauses reconciliation when TLSSecretsReady=False and wires up the new Secret watch. |
| e2e-tests/tests/cert-management-policy/00-deploy-operator.yaml | Test setup for cert-management-policy scenario. |
| e2e-tests/tests/cert-management-policy/00-assert.yaml | Asserts operator deployment readiness for the scenario. |
| e2e-tests/tests/cert-management-policy/01-create-initial-cluster.yaml | Creates an initial cluster in auto mode and enables components for the test. |
| e2e-tests/tests/cert-management-policy/01-assert.yaml | Verifies the baseline cluster is healthy and expected secrets/resources exist. |
| e2e-tests/tests/cert-management-policy/02-save-secrets.yaml | Captures generated TLS secrets for later reuse as “user-provided” secrets. |
| e2e-tests/tests/cert-management-policy/03-delete-cluster-and-secrets.yaml | Deletes the cluster and related resources to prepare for the userProvidedOnly flow. |
| e2e-tests/tests/cert-management-policy/04-create-user-provided-cluster.yaml | Recreates the cluster with certManagementPolicy=userProvidedOnly. |
| e2e-tests/tests/cert-management-policy/04-assert.yaml | Asserts the cluster pauses with TLSSecretsReady=False and Progressing=False. |
| e2e-tests/tests/cert-management-policy/05-restore-secrets.yaml | Restores the saved TLS secrets (except instance certs) to resume partial progress. |
| e2e-tests/tests/cert-management-policy/05-assert.yaml | Asserts the cluster remains paused until instance cert secrets are restored. |
| e2e-tests/tests/cert-management-policy/06-restore-instance-secret.yaml | Restores instance cert secret using the generated StatefulSet name. |
| e2e-tests/tests/cert-management-policy/06-assert.yaml | Verifies full readiness after all user-provided secrets are present. |
| e2e-tests/tests/cert-management-policy/99-remove-cluster-gracefully.yaml | Cleanup step for the E2E scenario (removes resources, checks operator health). |
| e2e-tests/run-release.csv | Adds the new E2E test suite to release test runs. |
| e2e-tests/run-pr.csv | Adds the new E2E test suite to PR test runs. |
| deploy/cw-bundle.yaml | Regenerated bundle content to include the new CRD field. |
| deploy/crd.yaml | Regenerated CRD manifest to include the new CRD field. |
| deploy/cr.yaml | Updates example CR comments to mention certManagementPolicy. |
| deploy/bundle.yaml | Regenerated bundle content to include the new CRD field. |
| config/crd/bases/upstream.pgv2.percona.com_postgresclusters.yaml | Adds certManagementPolicy to the upstream PostgresCluster CRD schema. |
| config/crd/bases/pgv2.percona.com_perconapgclusters.yaml | Adds certManagementPolicy to the PerconaPGCluster CRD schema. |
| build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml | Updates generated CRD output to include certManagementPolicy. |
| if err := checkSecret(nil, naming.PGBackRestSecret(cluster).Name); err != nil { | ||
| return errors.Wrap(err, "check pgBackRest TLS secret") | ||
| } | ||
|
|
| func (r *Reconciler) reconcileTLSCertManagementPolicy(ctx context.Context, cluster *v1beta1.PostgresCluster) error { | ||
| cond := metav1.Condition{ | ||
| Type: v1beta1.ConditionTypeTLSSecretsReady, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "TLSSecretsFound", | ||
| ObservedGeneration: cluster.GetGeneration(), | ||
| } | ||
|
|
||
| if cluster.Spec.TLS.GetCertManagementPolicy() != v1beta1.CertManagementUserProvidedOnly { | ||
| cond.Message = "certManagementPolicy is " + string(cluster.Spec.TLS.GetCertManagementPolicy()) | ||
| meta.SetStatusCondition(&cluster.Status.Conditions, cond) | ||
| return nil | ||
| } |
| // watchClusterSecrets returns a handler.EventHandler for Secrets that are | ||
| // labeled with a PostgresCluster name but intentionally have no owner reference. | ||
| func (*Reconciler) watchClusterSecrets() handler.EventHandler { |
| meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ | ||
| Type: v1beta1.PostgresClusterProgressing, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "Paused", |
There was a problem hiding this comment.
To not confuse with cluster paused which is different
| Reason: "Paused", | |
| Reason: "ReconcilePaused", |
| if cond := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionTypeTLSSecretsReady); cond != nil { | ||
| if cond.Status == metav1.ConditionFalse { | ||
| meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ | ||
| Type: v1beta1.PostgresClusterProgressing, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "Paused", | ||
| Message: "Reconciliation is paused. Check `TLSSecretsReady` condition", | ||
| ObservedGeneration: cluster.GetGeneration(), | ||
| }) | ||
| return runtime.ErrorWithBackoff(patchClusterStatus()) | ||
| } | ||
| meta.RemoveStatusCondition(&cluster.Status.Conditions, v1beta1.PostgresClusterProgressing) | ||
| } | ||
| } |
There was a problem hiding this comment.
ifs can be combined by using IsStatusConditionPresentAndEqual
| if cond := meta.FindStatusCondition(cluster.Status.Conditions, v1beta1.ConditionTypeTLSSecretsReady); cond != nil { | |
| if cond.Status == metav1.ConditionFalse { | |
| meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ | |
| Type: v1beta1.PostgresClusterProgressing, | |
| Status: metav1.ConditionFalse, | |
| Reason: "Paused", | |
| Message: "Reconciliation is paused. Check `TLSSecretsReady` condition", | |
| ObservedGeneration: cluster.GetGeneration(), | |
| }) | |
| return runtime.ErrorWithBackoff(patchClusterStatus()) | |
| } | |
| meta.RemoveStatusCondition(&cluster.Status.Conditions, v1beta1.PostgresClusterProgressing) | |
| } | |
| } | |
| if meta.IsStatusConditionPresentAndEqual(cluster.Status.Conditions, v1beta1.ConditionTypeTLSSecretsReady, metav1.ConditionFalse) { | |
| meta.SetStatusCondition(&cluster.Status.Conditions, metav1.Condition{ | |
| Type: v1beta1.PostgresClusterProgressing, | |
| Status: metav1.ConditionFalse, | |
| Reason: "Paused", | |
| Message: "Reconciliation is paused. Check `TLSSecretsReady` condition", | |
| ObservedGeneration: cluster.GetGeneration(), | |
| }) | |
| return runtime.ErrorWithBackoff(patchClusterStatus()) | |
| } | |
| meta.RemoveStatusCondition(&cluster.Status.Conditions, v1beta1.PostgresClusterProgressing) | |
| } | |
| ) | ||
|
|
||
| // K8SPG-1045 | ||
| func (r *Reconciler) reconcileTLSCertManagementPolicy(ctx context.Context, cluster *v1beta1.PostgresCluster) error { |
There was a problem hiding this comment.
since this is only reconciling the status condition
| func (r *Reconciler) reconcileTLSCertManagementPolicy(ctx context.Context, cluster *v1beta1.PostgresCluster) error { | |
| func (r *Reconciler) reconcileTLSCondition(ctx context.Context, cluster *v1beta1.PostgresCluster) error { |
| ) | ||
|
|
||
| // K8SPG-1045 | ||
| func (r *Reconciler) reconcileTLSCertManagementPolicy(ctx context.Context, cluster *v1beta1.PostgresCluster) error { |
There was a problem hiding this comment.
We should add a unit test for this, should be easy with fake client and mock objects
| ) | ||
|
|
||
| // watchClusterSecrets returns a handler.EventHandler for Secrets that are | ||
| // labeled with a PostgresCluster name but intentionally have no owner reference. |
There was a problem hiding this comment.
we're not asserting anything about ownerRefs in the function, is it expected?
| _ = root.Certificate.UnmarshalText(existing.Data[certificateKey]) | ||
| _ = root.PrivateKey.UnmarshalText(existing.Data[privateKey]) |
There was a problem hiding this comment.
processing the errors could be quite helpful for figuring out what is wrong exactly and make it easier for user to fix it, so maybe we shouldn't skip the errors
https://perconadev.atlassian.net/browse/K8SPG-1045
DESCRIPTION
This PR adds
.spec.tls.certManagementPolicywhich controls how TLS secrets are managed. The following values are supported:auto- the operator creates and maintains TLS secrets. This is the default.userProvidedOnly- the operator never generates or modifies TLS secrets. Reconciliation pauses until every required secret is provided by the user.When secrets are missing (when
userProvidedOnlyis used), the cluster reports:The
TLSSecretsReadymessage lists the missing secret names.Once these secrets are present, the operator creates the instance
StatefulSetso its generated name can be seen by the user.After that, user should create corresponding
<statefulset-name>-certssecret.User's secrets should contain
postgres-operator.crunchydata.com/cluster: <cluster-name>label to trigger reconcileCHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability