K8SPG-951: add spec.issuerConf#1684
Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
There was a problem hiding this comment.
Pull request overview
Adds spec.tls.issuerConf to the PostgresCluster API to support multiple cert-manager issuer modes (managed namespaced Issuer, managed cluster-scoped ClusterIssuer, and external/third-party issuers), and updates reconciliation logic so leaf certificates can reference the resolved issuerRef. The PR also adjusts controller-runtime client caching to allow direct API-server Get on ClusterIssuers without requiring cluster-wide list/watch RBAC, and adds tests covering the new behaviors.
Changes:
- Introduce
spec.tls.issuerConf(CRD + Go types + deepcopy) to configure issuer references. - Implement issuer-mode resolution and apply logic for Issuer/ClusterIssuer/external issuers, plus CA handling fallbacks for external issuers.
- Disable controller-runtime cache for
cert-manager.io/v1 ClusterIssuerand add/extend unit tests.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go | Deepcopy updates for the new TLSSpec.IssuerConf field. |
| pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go | Adds TLSSpec.IssuerConf to the API type. |
| percona/runtime/runtime.go | Disables informer cache for ClusterIssuer via ClientCacheOptions() and applies it to managers. |
| percona/runtime/runtime_test.go | Adds tests around cache options / manager creation. |
| percona/certmanager/certmanager.go | Implements issuer modes, issuerRef resolution, and managed/external behaviors across Apply* methods. |
| percona/certmanager/certmanager_test.go | Adds/extends tests for issuer modes, namespace resolution, drift updates, and external issuer behavior. |
| internal/naming/names.go | Adds naming helpers for cluster-scoped CA issuer/secret. |
| internal/naming/names_test.go | Tests for the new naming helpers. |
| internal/controller/postgrescluster/pki.go | Makes PKI reconciliation issuer-mode aware, including managed-cluster secret locations and external mode handling. |
| internal/controller/postgrescluster/pki_test.go | Adds issuer-mode awareness tests (with envtest/fake-client workarounds). |
| internal/controller/postgrescluster/pgbackrest.go | Adds CA selection helper for pgBackRest when using external issuers (no operator root CA). |
| internal/controller/postgrescluster/pgbackrest_test.go | Unit tests for pgBackRestCACert. |
| internal/controller/postgrescluster/instance.go | Adds CA selection helper for instance cert embedding when using external issuers. |
| internal/controller/postgrescluster/instance_test.go | Unit tests for instanceCACert. |
| internal/controller/postgrescluster/controller.go | Ensures cert-manager watch registration logic runs even when rootCA is nil (external mode). |
| deploy/cw-bundle.yaml | CRD schema update for spec.tls.issuerConf. |
| deploy/crd.yaml | CRD schema update for spec.tls.issuerConf. |
| deploy/bundle.yaml | CRD schema update for spec.tls.issuerConf. |
| config/crd/bases/upstream.pgv2.percona.com_postgresclusters.yaml | Base CRD schema update for spec.tls.issuerConf. |
| config/crd/bases/pgv2.percona.com_perconapgclusters.yaml | Base CRD schema update for spec.tls.issuerConf. |
| build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml | Generated CRD schema update for spec.tls.issuerConf. |
Files not reviewed (1)
- pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go: Generated file
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
| if err == nil { | ||
| needsUpdate := false | ||
|
|
||
| hasOwnerRef, err := controllerutil.HasOwnerReference(existing.OwnerReferences, cluster, c.scheme) | ||
| if err != nil { | ||
| return errors.Wrap(err, "check owner reference") | ||
| } | ||
|
|
||
| if !hasOwnerRef { | ||
| gvk := v1beta1.SchemeBuilder.GroupVersion.WithKind("PostgresCluster") | ||
| existing.OwnerReferences = []metav1.OwnerReference{{ | ||
| APIVersion: gvk.GroupVersion().String(), | ||
| Kind: gvk.Kind, | ||
| Name: cluster.GetName(), | ||
| UID: cluster.GetUID(), | ||
| BlockOwnerDeletion: ptr.To(true), | ||
| Controller: ptr.To(true), | ||
| }} | ||
| needsUpdate = true | ||
| if !clusterScoped { | ||
| hasOwnerRef, err := controllerutil.HasOwnerReference(existing.OwnerReferences, cluster, c.scheme) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
| if rootCA != nil { | ||
| caCert, err := rootCA.Certificate.MarshalText() | ||
| return caCert, errors.Wrap(err, "failed to marshal root CA certificate") | ||
| } | ||
|
|
||
| if ca := clientSecret.Data[corev1.ServiceAccountRootCAKey]; len(ca) > 0 { | ||
| return ca, nil | ||
| } | ||
| if ca := repoSecret.Data[corev1.ServiceAccountRootCAKey]; len(ca) > 0 { | ||
| return ca, nil | ||
| } | ||
|
|
||
| return nil, errors.New("external issuer did not return a CA certificate for pgBackRest") |
There was a problem hiding this comment.
why do we need these kind of fallbacks here and in instance.go? i'd expect rootCA to be passed properly by the CA issued by ClusterIssuer
There was a problem hiding this comment.
rootCA can be nil in case of external issuers or ClusterIssuer that are not managed by the operator (missing RBAC, created externally).. So in this case we just try to look at the issued leaf cert and get the ca.crt from there
There was a problem hiding this comment.
wouldn't it be better to stop reconciliation on a upper level if rootCA is nil rather than this fallback logic?
There was a problem hiding this comment.
If we return early, how would we know the root CA then? This fallback exists so that the root CA can be inferred from the externally issued leaf certs. Am I missing something?
There was a problem hiding this comment.
maybe i am missing something.
so we have a separate certificate+secret for root CA. for some reason it's nil which means we don't know if it exists. in this case we try to detect the CA from leaf certificates even though we don't have a way to reliably know if those certs are signed by the expected root CA.
rootCA can be nil in case of external issuers or ClusterIssuer that are not managed by the operator (missing RBAC, created externally)..
maybe i don't really understand this scenario:
- if this is a misconfigured Issuer/ClusterIssuer scenario, i don't think we need to continue reconciliation.
- if this is inherent to any kind of custom Issuer/ClusterIssuer, i'm confused because i don't remember this in the implementations in K8SPSMDB and K8SPS
There was a problem hiding this comment.
Ok, I'll try to explain from the top:
This reconcileCertManagerPGBackrestSecret (where this function is called) is responsible for reconciling an internal pgBackRest Secret for its internal TLS (represented by the intent parameter). Among other things, one thing it needs is the rootCA. That can be obtained in 2 ways:
- When the rootCA is managed by the operator: that can be Issuer, ClusterIssuer (if RBAC is present) or even Custom Secret (not via cert-manager). In this case
rootCAis never nil. If it is, the reconciliation is halted atreconcileRootCertificateitself. - When the rootCA not managed by the operator: this is the case when issuer type is some external issuer (or RBAC for ClusterIssuer is missing). In this case, operator has no idea what
rootCAis, and hence it is nil (note this isn't actually an error state). But we need CA cert for pgBackRest, so it just reads theca.crtfrom the leaf cert issued by the external issuer. If the external issuer itself was misconfigured, then the leaf cert will be missing (error on line 2305 and 2312) or ifca.crtis missing in both (error in line 2351)
So with this context, to answer your questions:
if this is a misconfigured Issuer/ClusterIssuer scenario, i don't think we need to continue reconciliation.
We won't reconcile, appropriate errors are thrown when the required certs are missing as I explained above
if this is inherent to any kind of custom Issuer/ClusterIssuer, i'm confused because i don't remember this in the implementations in K8SPSMDB and K8SPS
We don't have this kind of Secret reconciliation (internal pgBackRest Secret) in either operators, so this approach is unique, but the overall behaviour is still consistent with both operators
There was a problem hiding this comment.
ok I see reconcileRootCertificate returns (nil, nil) if issuer is external. what's the practical difference between issuer/clusterIssuer managed by the operator and created externally?
There was a problem hiding this comment.
what's the practical difference between issuer/clusterIssuer managed by the operator and created externally?
Operator managed is always self-signed, whereas externally created can be anything (ACME, Vault, any in-tree types, or an external type which is a different CR. We won't always have a rootCA Secret in this case)
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
commit: 1112177 |
CHANGE DESCRIPTION
Problem:
Adds support for specifying cert-manager issuer conf:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability