From e76d9ef865502c2ec923b9f73f6ed5b20eac5b24 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 07:11:26 +0200 Subject: [PATCH 1/9] fix(api): make pullSecrets and the Server pull policy take effect Three fields were declared and never reached a pod. ImageSpec.PullSecrets was read nowhere. The only imagePullSecrets on any pod came from code image entries, so a private registry for the server, database or CA setup image could not work at all. All three pod specs now carry them, deduplicated against the code image secrets. Server.image.pullPolicy was ignored: both container specs read cfg.Spec.Image.PullPolicy unconditionally. Fixing that required removing the nested kubebuilder default, for the same reason #549 removed it from repository and tag: a nested default is materialised into every Server, so the field was never empty and could not express inherit. The fallback now lives in code - Server, then Config, then IfNotPresent. ConditionSSLBootstrapped existed as a constant and as a promise in docs/reference/server.md, but nothing ever set it. It now reports whether the referenced Certificate is usable, which also gives the Server a visible status while it waits: a missing or unsigned Certificate previously left no trace at all, so nothing happens looked the same as nothing is wrong. Documented the resolution rules, including that pullSecrets on a Server replaces the Config's list rather than extending it. --- api/v1alpha1/config_types.go | 7 +- .../crds/openvox.voxpupuli.org_configs.yaml | 13 +- .../crds/openvox.voxpupuli.org_databases.yaml | 13 +- .../crds/openvox.voxpupuli.org_servers.yaml | 13 +- .../bases/openvox.voxpupuli.org_configs.yaml | 13 +- .../openvox.voxpupuli.org_databases.yaml | 13 +- .../bases/openvox.voxpupuli.org_servers.yaml | 13 +- docs/reference/config.md | 9 + docs/reference/server.md | 22 +- .../controller/certificateauthority_job.go | 3 +- internal/controller/database_deployment.go | 5 +- internal/controller/helpers.go | 53 ++++ .../controller/image_pull_settings_test.go | 252 ++++++++++++++++++ internal/controller/server_controller.go | 38 +++ internal/controller/server_deployment.go | 17 +- 15 files changed, 451 insertions(+), 33 deletions(-) create mode 100644 internal/controller/image_pull_settings_test.go diff --git a/api/v1alpha1/config_types.go b/api/v1alpha1/config_types.go index 4dc94337..f68f7704 100644 --- a/api/v1alpha1/config_types.go +++ b/api/v1alpha1/config_types.go @@ -397,11 +397,16 @@ type ImageSpec struct { Tag string `json:"tag,omitempty"` // PullPolicy defines the image pull policy. - // +kubebuilder:default="IfNotPresent" + // + // A nested default would be materialised into every Server, which is what + // made the Server-level override unreachable: the field was never empty, so + // it could not express "inherit". Unset falls back to the Config and then + // to IfNotPresent. // +optional PullPolicy corev1.PullPolicy `json:"pullPolicy,omitempty"` // PullSecrets is a list of image pull secrets. + // On Server an entry overrides the Config's list rather than adding to it. // +optional PullSecrets []corev1.LocalObjectReference `json:"pullSecrets,omitempty"` } diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_configs.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_configs.yaml index 854bd1ac..4a1ac41c 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_configs.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_configs.yaml @@ -149,11 +149,18 @@ spec: in this Config. properties: pullPolicy: - default: IfNotPresent - description: PullPolicy defines the image pull policy. + description: |- + PullPolicy defines the image pull policy. + + A nested default would be materialised into every Server, which is what + made the Server-level override unreachable: the field was never empty, so + it could not express "inherit". Unset falls back to the Config and then + to IfNotPresent. type: string pullSecrets: - description: PullSecrets is a list of image pull secrets. + description: |- + PullSecrets is a list of image pull secrets. + On Server an entry overrides the Config's list rather than adding to it. items: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_databases.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_databases.yaml index 771ec6f0..1b9f2fdd 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_databases.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_databases.yaml @@ -67,11 +67,18 @@ spec: description: Image defines the container image for the Database. properties: pullPolicy: - default: IfNotPresent - description: PullPolicy defines the image pull policy. + description: |- + PullPolicy defines the image pull policy. + + A nested default would be materialised into every Server, which is what + made the Server-level override unreachable: the field was never empty, so + it could not express "inherit". Unset falls back to the Config and then + to IfNotPresent. type: string pullSecrets: - description: PullSecrets is a list of image pull secrets. + description: |- + PullSecrets is a list of image pull secrets. + On Server an entry overrides the Config's list rather than adding to it. items: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml index 5e55017b..7b974fd1 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml @@ -3385,11 +3385,18 @@ spec: description: Image overrides the Config's default image. properties: pullPolicy: - default: IfNotPresent - description: PullPolicy defines the image pull policy. + description: |- + PullPolicy defines the image pull policy. + + A nested default would be materialised into every Server, which is what + made the Server-level override unreachable: the field was never empty, so + it could not express "inherit". Unset falls back to the Config and then + to IfNotPresent. type: string pullSecrets: - description: PullSecrets is a list of image pull secrets. + description: |- + PullSecrets is a list of image pull secrets. + On Server an entry overrides the Config's list rather than adding to it. items: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/config/crd/bases/openvox.voxpupuli.org_configs.yaml b/config/crd/bases/openvox.voxpupuli.org_configs.yaml index 854bd1ac..4a1ac41c 100644 --- a/config/crd/bases/openvox.voxpupuli.org_configs.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_configs.yaml @@ -149,11 +149,18 @@ spec: in this Config. properties: pullPolicy: - default: IfNotPresent - description: PullPolicy defines the image pull policy. + description: |- + PullPolicy defines the image pull policy. + + A nested default would be materialised into every Server, which is what + made the Server-level override unreachable: the field was never empty, so + it could not express "inherit". Unset falls back to the Config and then + to IfNotPresent. type: string pullSecrets: - description: PullSecrets is a list of image pull secrets. + description: |- + PullSecrets is a list of image pull secrets. + On Server an entry overrides the Config's list rather than adding to it. items: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/config/crd/bases/openvox.voxpupuli.org_databases.yaml b/config/crd/bases/openvox.voxpupuli.org_databases.yaml index 771ec6f0..1b9f2fdd 100644 --- a/config/crd/bases/openvox.voxpupuli.org_databases.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_databases.yaml @@ -67,11 +67,18 @@ spec: description: Image defines the container image for the Database. properties: pullPolicy: - default: IfNotPresent - description: PullPolicy defines the image pull policy. + description: |- + PullPolicy defines the image pull policy. + + A nested default would be materialised into every Server, which is what + made the Server-level override unreachable: the field was never empty, so + it could not express "inherit". Unset falls back to the Config and then + to IfNotPresent. type: string pullSecrets: - description: PullSecrets is a list of image pull secrets. + description: |- + PullSecrets is a list of image pull secrets. + On Server an entry overrides the Config's list rather than adding to it. items: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/config/crd/bases/openvox.voxpupuli.org_servers.yaml b/config/crd/bases/openvox.voxpupuli.org_servers.yaml index 5e55017b..7b974fd1 100644 --- a/config/crd/bases/openvox.voxpupuli.org_servers.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_servers.yaml @@ -3385,11 +3385,18 @@ spec: description: Image overrides the Config's default image. properties: pullPolicy: - default: IfNotPresent - description: PullPolicy defines the image pull policy. + description: |- + PullPolicy defines the image pull policy. + + A nested default would be materialised into every Server, which is what + made the Server-level override unreachable: the field was never empty, so + it could not express "inherit". Unset falls back to the Config and then + to IfNotPresent. type: string pullSecrets: - description: PullSecrets is a list of image pull secrets. + description: |- + PullSecrets is a list of image pull secrets. + On Server an entry overrides the Config's list rather than adding to it. items: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/docs/reference/config.md b/docs/reference/config.md index 6ae603f5..790e6083 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -210,6 +210,15 @@ Controls Puppet Server metrics.conf settings. | `Running` | ConfigMap created, ready for use | | `Error` | Reconciliation failed | +### Image resolution + +`repository` and `tag` on a Server override the Config's values individually; +an unset field falls back to the Config. `pullPolicy` follows the same rule and +defaults to `IfNotPresent` when neither sets it. `pullSecrets` is different: a +non-empty list on the Server *replaces* the Config's rather than extending it, +so a Server pulling from another registry does not carry the Config's +credentials along. Secrets for code images are always added on top. + ## Created Resources | Resource | Name | Description | diff --git a/docs/reference/server.md b/docs/reference/server.md index 30af5a4f..18cbb515 100644 --- a/docs/reference/server.md +++ b/docs/reference/server.md @@ -111,7 +111,18 @@ When enabled, the default policy allows TCP/8140 from all sources (agents may co | `phase` | string | Current lifecycle phase | | `ready` | int32 | Number of ready replicas | | `desired` | int32 | Desired number of replicas | -| `conditions` | []Condition | `SSLBootstrapped`, `Ready` | +| `observedGeneration` | int64 | The `.metadata.generation` the status was last derived from | +| `conditions` | []Condition | See below | + +### Conditions + +| Type | Reason | Meaning | +|---|---|---| +| `SSLBootstrapped` | `CertificateSigned` | The referenced Certificate is signed, so the pods have TLS material | +| `SSLBootstrapped` | `CertificateNotFound` | `spec.certificateRef` points at a Certificate that does not exist | +| `SSLBootstrapped` | `CertificateNotSigned` | The Certificate exists but has not been signed yet | +| `Ready` | `ReplicasReady` | At least one replica is ready | +| `Ready` | `ReplicasNotReady` | No replica is ready yet | ## Phases @@ -184,6 +195,15 @@ Key differences: | ca.cfg | `ca-enabled.cfg` | `ca-disabled.cfg` | | Strategy | Recreate | RollingUpdate | +### Image resolution + +`repository` and `tag` on a Server override the Config's values individually; +an unset field falls back to the Config. `pullPolicy` follows the same rule and +defaults to `IfNotPresent` when neither sets it. `pullSecrets` is different: a +non-empty list on the Server *replaces* the Config's rather than extending it, +so a Server pulling from another registry does not carry the Config's +credentials along. Secrets for code images are always added on top. + ## Created Resources | Resource | Name | Description | diff --git a/internal/controller/certificateauthority_job.go b/internal/controller/certificateauthority_job.go index ab1175eb..936f5be3 100644 --- a/internal/controller/certificateauthority_job.go +++ b/internal/controller/certificateauthority_job.go @@ -177,11 +177,12 @@ func (r *CertificateAuthorityReconciler) buildCASetupJob(ctx context.Context, ca // otherwise owned by root; overridable via ca.spec.securityContext. SecurityContext: buildPodSecurityContext( CASetupRunAsUser, CASetupRunAsGroup, CASetupFSGroup, ca.Spec.SecurityContext), + ImagePullSecrets: appendPullSecrets(nil, cfg.Spec.Image.PullSecrets...), Containers: []corev1.Container{ { Name: "ca-setup", Image: image, - ImagePullPolicy: cfg.Spec.Image.PullPolicy, + ImagePullPolicy: configImagePullPolicy(cfg), Command: []string{"/bin/bash", "-c", script}, Env: envVars, Resources: resolveCAJobResources(ca), diff --git a/internal/controller/database_deployment.go b/internal/controller/database_deployment.go index 3230e984..1ac39edf 100644 --- a/internal/controller/database_deployment.go +++ b/internal/controller/database_deployment.go @@ -185,7 +185,7 @@ func (r *DatabaseReconciler) buildPodSpec(db *openvoxv1alpha1.Database, cert *op container := corev1.Container{ Name: "openvox-db", Image: image, - ImagePullPolicy: db.Spec.Image.PullPolicy, + ImagePullPolicy: pullPolicyOrDefault(db.Spec.Image.PullPolicy), Env: env, Ports: []corev1.ContainerPort{ {Name: "https", ContainerPort: DatabaseHTTPSPort, Protocol: corev1.ProtocolTCP}, @@ -229,7 +229,7 @@ chmod 640 /ssl/private_keys/%s.pem`, certname, certname, certname) initContainer := corev1.Container{ Name: "tls-init", Image: image, - ImagePullPolicy: db.Spec.Image.PullPolicy, + ImagePullPolicy: pullPolicyOrDefault(db.Spec.Image.PullPolicy), Command: []string{"sh", "-c", sslInitScript}, VolumeMounts: []corev1.VolumeMount{ {Name: "ssl", MountPath: "/ssl"}, @@ -260,6 +260,7 @@ chmod 640 /ssl/private_keys/%s.pem`, certname, certname, certname) InitContainers: []corev1.Container{initContainer}, Containers: []corev1.Container{container}, Volumes: volumes, + ImagePullSecrets: appendPullSecrets(nil, db.Spec.Image.PullSecrets...), } return podSpec diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index 5d43ba28..4a46fe36 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -251,6 +251,59 @@ func resolveImage(server *openvoxv1alpha1.Server, cfg *openvoxv1alpha1.Config) s return fmt.Sprintf("%s:%s", cfg.Spec.Image.Repository, cfg.Spec.Image.Tag) } +// resolveImagePullPolicy returns the pull policy for a Server, preferring its +// own value and falling back to the Config, then to IfNotPresent. +func resolveImagePullPolicy(server *openvoxv1alpha1.Server, cfg *openvoxv1alpha1.Config) corev1.PullPolicy { + if server.Spec.Image.PullPolicy != "" { + return server.Spec.Image.PullPolicy + } + return configImagePullPolicy(cfg) +} + +// configImagePullPolicy returns the Config's pull policy, or the Kubernetes +// default when it is unset. +func configImagePullPolicy(cfg *openvoxv1alpha1.Config) corev1.PullPolicy { + if cfg.Spec.Image.PullPolicy != "" { + return cfg.Spec.Image.PullPolicy + } + return corev1.PullIfNotPresent +} + +// pullPolicyOrDefault returns the given policy, or IfNotPresent when unset. +func pullPolicyOrDefault(p corev1.PullPolicy) corev1.PullPolicy { + if p != "" { + return p + } + return corev1.PullIfNotPresent +} + +// resolveImagePullSecrets returns the pull secrets for a Server. A list on the +// Server replaces the Config's rather than extending it, so a Server pulling +// from a different registry does not drag the Config's credentials along. +func resolveImagePullSecrets(server *openvoxv1alpha1.Server, cfg *openvoxv1alpha1.Config) []corev1.LocalObjectReference { + if len(server.Spec.Image.PullSecrets) > 0 { + return server.Spec.Image.PullSecrets + } + return cfg.Spec.Image.PullSecrets +} + +// appendPullSecrets adds refs that are not already present, so callers can +// combine sources without producing duplicates. +func appendPullSecrets(existing []corev1.LocalObjectReference, add ...corev1.LocalObjectReference) []corev1.LocalObjectReference { + seen := make(map[string]bool, len(existing)) + for _, e := range existing { + seen[e.Name] = true + } + for _, a := range add { + if a.Name == "" || seen[a.Name] { + continue + } + seen[a.Name] = true + existing = append(existing, a) + } + return existing +} + // serverRoleEnabled reports whether the Server runs the catalog server role. // The spec field defaults to true, so an unset value enables the role. func serverRoleEnabled(server *openvoxv1alpha1.Server) bool { diff --git a/internal/controller/image_pull_settings_test.go b/internal/controller/image_pull_settings_test.go new file mode 100644 index 00000000..6226867f --- /dev/null +++ b/internal/controller/image_pull_settings_test.go @@ -0,0 +1,252 @@ +package controller + +import ( + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// serverPrereqsWith returns the standard Server prerequisites with a +// caller-supplied Config, so a test can vary the image settings. +func serverPrereqsWith(cfg *openvoxv1alpha1.Config) []client.Object { + objs := serverPrereqs() + for i := range objs { + if _, ok := objs[i].(*openvoxv1alpha1.Config); ok { + objs[i] = cfg + return objs + } + } + panic("serverPrereqs no longer contains a Config") +} + +// pullSecretNames flattens the pod's pull secrets for comparison. +func pullSecretNames(refs []corev1.LocalObjectReference) []string { + out := make([]string, 0, len(refs)) + for _, r := range refs { + out = append(out, r.Name) + } + return out +} + +func equalStrings(got []string, want ...string) bool { + if len(got) != len(want) { + return false + } + for i := range got { + if got[i] != want[i] { + return false + } + } + return true +} + +// TestPullSecrets_ReachTheServerPod covers a field that was declared on +// ImageSpec but read nowhere: pull secrets configured on the Config never +// reached any pod, so a private registry could not work at all. +func TestPullSecrets_ReachTheServerPod(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + cfg.Spec.Image.PullSecrets = []corev1.LocalObjectReference{{Name: "registry-creds"}} + server := newServer("test-server") + + c := setupTestClient(append(serverPrereqsWith(cfg), server)...) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + deploy := &appsv1.Deployment{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, deploy); err != nil { + t.Fatalf("reading the Deployment: %v", err) + } + if got := pullSecretNames(deploy.Spec.Template.Spec.ImagePullSecrets); !equalStrings(got, "registry-creds") { + t.Errorf("expected the Config's pull secret on the pod, got %v", got) + } +} + +// TestPullSecrets_ServerOverridesConfig pins the documented precedence: a list +// on the Server replaces the Config's rather than extending it. +func TestPullSecrets_ServerOverridesConfig(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + cfg.Spec.Image.PullSecrets = []corev1.LocalObjectReference{{Name: "config-creds"}} + server := newServer("test-server") + server.Spec.Image.PullSecrets = []corev1.LocalObjectReference{{Name: "server-creds"}} + + c := setupTestClient(append(serverPrereqsWith(cfg), server)...) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + deploy := &appsv1.Deployment{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, deploy); err != nil { + t.Fatalf("reading the Deployment: %v", err) + } + if got := pullSecretNames(deploy.Spec.Template.Spec.ImagePullSecrets); !equalStrings(got, "server-creds") { + t.Errorf("expected the Server's list to replace the Config's, got %v", got) + } +} + +// TestPullPolicy_ServerOverridesConfig covers the second half of the same +// problem: the Server's pull policy was read from the Config unconditionally, +// so the field on the Server did nothing. +func TestPullPolicy_ServerOverridesConfig(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + cfg.Spec.Image.PullPolicy = corev1.PullIfNotPresent + server := newServer("test-server") + server.Spec.Image.PullPolicy = corev1.PullAlways + + c := setupTestClient(append(serverPrereqsWith(cfg), server)...) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + deploy := &appsv1.Deployment{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, deploy); err != nil { + t.Fatalf("reading the Deployment: %v", err) + } + for _, ctr := range deploy.Spec.Template.Spec.Containers { + if ctr.ImagePullPolicy != corev1.PullAlways { + t.Errorf("container %s: expected Always from the Server, got %q", ctr.Name, ctr.ImagePullPolicy) + } + } +} + +// TestPullPolicy_InheritsFromConfig is the counterpart: an unset Server policy +// must still take the Config's value. +func TestPullPolicy_InheritsFromConfig(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + cfg.Spec.Image.PullPolicy = corev1.PullAlways + server := newServer("test-server") // no pull policy of its own + + c := setupTestClient(append(serverPrereqsWith(cfg), server)...) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + deploy := &appsv1.Deployment{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, deploy); err != nil { + t.Fatalf("reading the Deployment: %v", err) + } + for _, ctr := range deploy.Spec.Template.Spec.Containers { + if ctr.ImagePullPolicy != corev1.PullAlways { + t.Errorf("container %s: expected Always inherited from the Config, got %q", ctr.Name, ctr.ImagePullPolicy) + } + } +} + +// TestPullPolicy_DefaultsWhenUnsetEverywhere guards the fallback that replaced +// the CRD default. The default had to go: materialised into every Server, it +// made the field never empty and the override unreachable. +func TestPullPolicy_DefaultsWhenUnsetEverywhere(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + server := newServer("test-server") + + if got := resolveImagePullPolicy(server, cfg); got != corev1.PullIfNotPresent { + t.Errorf("expected IfNotPresent when nothing is set, got %q", got) + } +} + +// TestPullSecrets_ReachTheDatabasePod and the CA job cover the other two +// workloads that build pods from an ImageSpec. +func TestPullSecrets_ReachTheDatabasePod(t *testing.T) { + db := newDatabase("puppetdb") + db.Spec.Image.PullSecrets = []corev1.LocalObjectReference{{Name: "registry-creds"}} + + cert := newCertificate("puppetdb-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + ca := newCertificateAuthority("production-ca") + r := newDatabaseReconciler(setupTestClient(db, cert, ca)) + podSpec := r.buildPodSpec(db, cert, ca, "example/db:1") + if got := pullSecretNames(podSpec.ImagePullSecrets); !equalStrings(got, "registry-creds") { + t.Errorf("expected the pull secret on the Database pod, got %v", got) + } +} + +// TestSSLBootstrapped_ReportsAMissingCertificate covers a condition that was +// declared, documented in docs/reference/server.md, and never set. A Server +// waiting for its Certificate left no trace in the status at all. +func TestSSLBootstrapped_ReportsAMissingCertificate(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + server := newServer("test-server") // certificateRef points at a Certificate that does not exist + + c := setupTestClient(cfg, server) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + got := &openvoxv1alpha1.Server{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading the Server back: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSSLBootstrapped) + if cond == nil { + t.Fatal("expected an SSLBootstrapped condition while the Certificate is missing") + } + if cond.Status != metav1.ConditionFalse || cond.Reason != "CertificateNotFound" { + t.Errorf("expected False/CertificateNotFound, got %s/%s", cond.Status, cond.Reason) + } +} + +// TestSSLBootstrapped_ReportsAnUnsignedCertificate is the second waiting state. +func TestSSLBootstrapped_ReportsAnUnsignedCertificate(t *testing.T) { + cfg := newConfig("production", withAuthorityRef("production-ca")) + cert := newCertificate("production-cert", "production-ca", openvoxv1alpha1.CertificatePhasePending) + server := newServer("test-server") + + c := setupTestClient(cfg, cert, server) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + got := &openvoxv1alpha1.Server{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading the Server back: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSSLBootstrapped) + if cond == nil || cond.Status != metav1.ConditionFalse || cond.Reason != "CertificateNotSigned" { + t.Errorf("expected False/CertificateNotSigned, got %+v", cond) + } +} + +// TestSSLBootstrapped_TrueOnceSigned closes the loop. +func TestSSLBootstrapped_TrueOnceSigned(t *testing.T) { + server := newServer("test-server") + c := setupTestClient(append(serverPrereqs(), server)...) + r := newServerReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-server")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + got := &openvoxv1alpha1.Server{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-server", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading the Server back: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSSLBootstrapped) + if cond == nil || cond.Status != metav1.ConditionTrue { + t.Errorf("expected SSLBootstrapped=True once the Certificate is signed, got %+v", cond) + } +} + +// TestPullSecrets_ReachTheCASetupJob covers the third workload. +func TestPullSecrets_ReachTheCASetupJob(t *testing.T) { + ca := newCertificateAuthority("test-ca") + cfg := caPrereqs("test-ca") + cfg.Spec.Image.PullSecrets = []corev1.LocalObjectReference{{Name: "registry-creds"}} + + r := newCertificateAuthorityReconciler(setupTestClient(ca, cfg)) + job := r.buildCASetupJob(testCtx(), ca, cfg, "test-ca-setup", nil) + + if got := pullSecretNames(job.Spec.Template.Spec.ImagePullSecrets); !equalStrings(got, "registry-creds") { + t.Errorf("expected the pull secret on the CA setup job, got %v", got) + } +} diff --git a/internal/controller/server_controller.go b/internal/controller/server_controller.go index 645baadb..860dfca1 100644 --- a/internal/controller/server_controller.go +++ b/internal/controller/server_controller.go @@ -108,6 +108,8 @@ func (r *ServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if err := r.Get(ctx, types.NamespacedName{Name: server.Spec.CertificateRef, Namespace: server.Namespace}, cert); err != nil { if errors.IsNotFound(err) { logger.Info("waiting for Certificate", "certificateRef", server.Spec.CertificateRef) + r.reportSSLBootstrapped(ctx, server, metav1.ConditionFalse, "CertificateNotFound", + fmt.Sprintf("Certificate %s does not exist", server.Spec.CertificateRef)) return ctrl.Result{RequeueAfter: RequeueIntervalShort}, nil } return ctrl.Result{}, fmt.Errorf("getting Certificate %s: %w", server.Spec.CertificateRef, err) @@ -117,6 +119,13 @@ func (r *ServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr logger.Info("waiting for Certificate to be signed", "certificate", cert.Name, "phase", cert.Status.Phase) if statusErr := updateStatusWithRetry(ctx, r.Client, server, func() { server.Status.Phase = openvoxv1alpha1.ServerPhaseWaitingForCert + meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionSSLBootstrapped, + Status: metav1.ConditionFalse, + Reason: "CertificateNotSigned", + Message: fmt.Sprintf("Certificate %s is in phase %q", cert.Name, cert.Status.Phase), + ObservedGeneration: server.Generation, + }) }); statusErr != nil { logger.Error(statusErr, "failed to update Server status", "name", server.Name) } @@ -168,6 +177,16 @@ func (r *ServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr server.Status.Desired = replicas server.Status.Ready = ready + // Reaching this point means the Certificate exists and is signed, so the + // pods have TLS material to mount. + meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionSSLBootstrapped, + Status: metav1.ConditionTrue, + Reason: "CertificateSigned", + Message: fmt.Sprintf("Certificate %s is signed and mounted", cert.Name), + ObservedGeneration: server.Generation, + }) + serverReplicasDesired.WithLabelValues(server.Name, server.Namespace).Set(float64(replicas)) serverReplicasReady.WithLabelValues(server.Name, server.Namespace).Set(float64(ready)) @@ -200,6 +219,25 @@ func (r *ServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } +// reportSSLBootstrapped records the condition on its own, for the early +// returns that happen before the single status write at the end of Reconcile. +// A dependency the Server is waiting for used to leave no trace in the status +// at all, so "nothing happens" looked the same as "nothing is wrong". +func (r *ServerReconciler) reportSSLBootstrapped(ctx context.Context, server *openvoxv1alpha1.Server, + status metav1.ConditionStatus, reason, message string) { + if err := updateStatusWithRetry(ctx, r.Client, server, func() { + meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionSSLBootstrapped, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: server.Generation, + }) + }); err != nil { + log.FromContext(ctx).Error(err, "failed to record the SSLBootstrapped condition", "name", server.Name) + } +} + func (r *ServerReconciler) reconcilePDB(ctx context.Context, server *openvoxv1alpha1.Server) error { logger := log.FromContext(ctx) pdbName := server.Name diff --git a/internal/controller/server_deployment.go b/internal/controller/server_deployment.go index 5d0f935f..657d929a 100644 --- a/internal/controller/server_deployment.go +++ b/internal/controller/server_deployment.go @@ -456,7 +456,7 @@ func (r *ServerReconciler) buildPodSpec(server *openvoxv1alpha1.Server, cfg *ope container := corev1.Container{ Name: "openvox-server", Image: image, - ImagePullPolicy: cfg.Spec.Image.PullPolicy, + ImagePullPolicy: resolveImagePullPolicy(server, cfg), Env: env, EnvFrom: server.Spec.EnvFrom, Ports: []corev1.ContainerPort{ @@ -508,7 +508,7 @@ chmod 640 /ssl/private_keys/puppet.pem` initContainer := corev1.Container{ Name: "tls-init", Image: image, - ImagePullPolicy: cfg.Spec.Image.PullPolicy, + ImagePullPolicy: resolveImagePullPolicy(server, cfg), Command: []string{"sh", "-c", sslInitScript}, VolumeMounts: []corev1.VolumeMount{ {Name: "ssl", MountPath: "/ssl"}, @@ -544,16 +544,13 @@ chmod 640 /ssl/private_keys/puppet.pem` Volumes: volumes, } - // Add imagePullSecrets for code images if configured (deduplicated across entries) + // The server image and the code images can live in different registries, so + // both sets of credentials end up on the pod, deduplicated. + podSpec.ImagePullSecrets = appendPullSecrets(nil, resolveImagePullSecrets(server, cfg)...) if serverRoleEnabled(server) { - seen := make(map[string]bool) for _, e := range resolveCode(server, cfg) { - if e.ImagePullSecret != "" && !seen[e.ImagePullSecret] { - seen[e.ImagePullSecret] = true - podSpec.ImagePullSecrets = append(podSpec.ImagePullSecrets, corev1.LocalObjectReference{ - Name: e.ImagePullSecret, - }) - } + podSpec.ImagePullSecrets = appendPullSecrets(podSpec.ImagePullSecrets, + corev1.LocalObjectReference{Name: e.ImagePullSecret}) } } From 8d1dd282c5c9d66f1d63d85670c60392a593b218 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 07:25:27 +0200 Subject: [PATCH 2/9] fix(certificate): refuse to share a certname with another Certificate A certname identifies exactly one entry on the CA, so two Certificates claiming the same one against the same CertificateAuthority are indistinguishable to it. Nothing prevented that: the webhook validated the certname format but not its uniqueness, there was no index on it, and the CRD defaults it to puppet - so two Certificates created without one collide by default rather than by mistake. The consequences were both silent. Signing and fetching both address the CA by certname, so a Certificate could adopt a certificate issued for another resource's key; the mismatch would only surface later as a failed TLS handshake. And handleCertificateCleanup revokes by certname, so deleting either Certificate revokes the entry the other one depends on. Three layers, because webhooks are disabled by default: - the admission webhook rejects a duplicate at creation, naming the holder - the controller refuses to sign and reports CertnameConflict, permanently: retrying cannot free a name, only a spec change can - a certificate returned by the CA is verified against the private key it was requested for, so a foreign certificate under the same name is never written to a Secret A terminating Certificate releases its claim, since its finalizer cleans the CA entry, and the rule is per CA rather than per namespace. The full-flow test now signs the submitted CSR instead of returning a canned certificate. It passed before only because nothing verified the pairing. --- api/v1alpha1/certificate_types.go | 4 + docs/reference/certificate.md | 18 ++ internal/controller/certificate_controller.go | 89 ++++++++ internal/controller/certificate_signing.go | 82 ++++++- .../controller/certificate_signing_test.go | 53 ++++- internal/controller/certname_conflict_test.go | 204 ++++++++++++++++++ internal/controller/helpers.go | 10 + internal/controller/indexers.go | 9 + internal/webhook/certificate_webhook.go | 51 +++++ internal/webhook/certificate_webhook_test.go | 76 +++++++ 10 files changed, 593 insertions(+), 3 deletions(-) create mode 100644 internal/controller/certname_conflict_test.go diff --git a/api/v1alpha1/certificate_types.go b/api/v1alpha1/certificate_types.go index e4651fc3..f66155ad 100644 --- a/api/v1alpha1/certificate_types.go +++ b/api/v1alpha1/certificate_types.go @@ -141,6 +141,10 @@ type CertificateStatus struct { // Condition types for Certificate. const ( ConditionCertSigned = "CertSigned" + + // ConditionCertnameConflict reports that another Certificate already claims + // this certname against the same CertificateAuthority. + ConditionCertnameConflict = "CertnameConflict" ) func init() { diff --git a/docs/reference/certificate.md b/docs/reference/certificate.md index b23d4044..b9506102 100644 --- a/docs/reference/certificate.md +++ b/docs/reference/certificate.md @@ -163,6 +163,24 @@ flowchart TD The controller discovers the CA Service automatically by finding Servers with `ca: true` in the same Config and the Pools whose selector matches them. +## Certname uniqueness + +A certname identifies exactly one entry on the CertificateAuthority. Two +Certificates that claim the same certname against the same CA are +indistinguishable to it, with two consequences: the second CSR is rejected, and +deleting either one revokes the entry both rely on. + +The operator therefore refuses the collision in three places: + +- the admission webhook rejects a duplicate at creation, naming the other resource +- the controller refuses to sign and reports `CertnameConflict`, since webhooks + are disabled by default +- a certificate returned by the CA is verified against the private key it was + requested for, so a foreign certificate under the same name is never stored + +`certname` defaults to `puppet`, so two Certificates created without one collide +by default rather than by mistake. Give each Certificate its own certname. + ## Created Resources | Resource | Name | Description | diff --git a/internal/controller/certificate_controller.go b/internal/controller/certificate_controller.go index a65b3bc9..fe80c524 100644 --- a/internal/controller/certificate_controller.go +++ b/internal/controller/certificate_controller.go @@ -43,6 +43,7 @@ const ( EventReasonCertificateRenewed = "CertificateRenewed" EventReasonCertificateExpiringSoon = "CertificateExpiringSoon" EventReasonCertificateCleaned = "CertificateCleaned" + EventReasonCertnameConflict = "CertnameConflict" ) // Maximum requeue interval for renewal checks (caps the time-based backoff). @@ -266,6 +267,17 @@ func (r *CertificateReconciler) SetupWithManager(mgr ctrl.Manager) error { func (r *CertificateReconciler) reconcileCertSigning(ctx context.Context, cert *openvoxv1alpha1.Certificate, ca *openvoxv1alpha1.CertificateAuthority) (ctrl.Result, error) { logger := log.FromContext(ctx) + // The webhook rejects a duplicate certname at admission, but webhooks are + // off by default, so the guarantee has to hold here too. Signing under a + // certname another Certificate owns cannot succeed -- the CA has one entry + // per name -- and deleting either resource would revoke the entry the other + // depends on. + if other, err := r.otherCertificateUsingCertname(ctx, cert); err != nil { + return ctrl.Result{}, err + } else if other != "" { + return r.reportCertnameConflict(ctx, cert, other) + } + // Resolve CA base URL: external URL or internal CA Service var caBaseURL string if ca.Spec.External != nil { @@ -589,3 +601,80 @@ func (r *CertificateReconciler) renewalDue(cert *openvoxv1alpha1.Certificate) bo } return !r.isWithinRenewalCooldown(cert) } + +// otherCertificateUsingCertname returns the name of another live Certificate in +// the namespace that claims the same certname against the same +// CertificateAuthority, or "" when there is none. +// +// The CRD defaults certname to "puppet", so two Certificates created without +// one collide by default rather than by mistake. +func (r *CertificateReconciler) otherCertificateUsingCertname(ctx context.Context, + cert *openvoxv1alpha1.Certificate) (string, error) { + certList := &openvoxv1alpha1.CertificateList{} + if err := r.List(ctx, certList, + client.InNamespace(cert.Namespace), + client.MatchingFields{IndexCertname: certnameOf(cert)}); err != nil { + return "", fmt.Errorf("listing Certificates by certname in namespace %s: %w", cert.Namespace, err) + } + + claimants := make([]string, 0, len(certList.Items)) + for i := range certList.Items { + other := &certList.Items[i] + switch { + case other.Name == cert.Name: + continue + // A Certificate on its way out releases the name: its finalizer cleans + // the CA entry. + case !other.DeletionTimestamp.IsZero(): + continue + case other.Spec.AuthorityRef != cert.Spec.AuthorityRef: + continue + } + claimants = append(claimants, other.Name) + } + if len(claimants) == 0 { + return "", nil + } + + // Stable output so the reported conflict does not flip between reconciles. + sort.Strings(claimants) + return claimants[0], nil +} + +// reportCertnameConflict records the conflict and stops. Retrying cannot help: +// only a spec change or the removal of the other Certificate frees the name. +func (r *CertificateReconciler) reportCertnameConflict(ctx context.Context, + cert *openvoxv1alpha1.Certificate, other string) (ctrl.Result, error) { + msg := fmt.Sprintf("certname %q is already claimed by Certificate %s against CertificateAuthority %s", + certnameOf(cert), other, cert.Spec.AuthorityRef) + + existing := meta.FindStatusCondition(cert.Status.Conditions, openvoxv1alpha1.ConditionCertnameConflict) + alreadyReported := existing != nil && existing.Status == metav1.ConditionTrue && existing.Message == msg + + if err := updateStatusWithRetry(ctx, r.Client, cert, func() { + cert.Status.Phase = openvoxv1alpha1.CertificatePhaseError + meta.SetStatusCondition(&cert.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionCertnameConflict, + Status: metav1.ConditionTrue, + Reason: "DuplicateCertname", + Message: msg, + ObservedGeneration: cert.Generation, + }) + meta.SetStatusCondition(&cert.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionCertSigned, + Status: metav1.ConditionFalse, + Reason: "CertnameConflict", + Message: msg, + ObservedGeneration: cert.Generation, + }) + }); err != nil { + return ctrl.Result{}, fmt.Errorf("reporting the certname conflict for %s: %w", cert.Name, err) + } + + if !alreadyReported { + log.FromContext(ctx).Info("refusing to sign, certname already claimed", + "certname", certnameOf(cert), "claimedBy", other) + r.Recorder.Eventf(cert, nil, corev1.EventTypeWarning, EventReasonCertnameConflict, "Reconcile", "%s", msg) + } + return ctrl.Result{}, nil +} diff --git a/internal/controller/certificate_signing.go b/internal/controller/certificate_signing.go index 57fd8269..8d2188f8 100644 --- a/internal/controller/certificate_signing.go +++ b/internal/controller/certificate_signing.go @@ -261,6 +261,52 @@ func (r *CertificateReconciler) submitCSR(ctx context.Context, cert *openvoxv1al return ctrl.Result{}, nil } +// certMatchesKey reports whether the certificate was issued for the given +// private key. It is the last line of defence against adopting a certificate +// that belongs to a different Certificate sharing the same certname. +func certMatchesKey(certPEM, keyPEM []byte) (bool, error) { + certBlock, _ := pem.Decode(certPEM) + if certBlock == nil { + return false, fmt.Errorf("decoding certificate PEM") + } + parsed, err := x509.ParseCertificate(certBlock.Bytes) + if err != nil { + return false, fmt.Errorf("parsing certificate: %w", err) + } + + keyBlock, _ := pem.Decode(keyPEM) + if keyBlock == nil { + return false, fmt.Errorf("decoding private key PEM") + } + key, err := parsePrivateKey(keyBlock.Bytes) + if err != nil { + return false, err + } + + pub, ok := parsed.PublicKey.(*rsa.PublicKey) + if !ok { + return false, fmt.Errorf("unsupported certificate public key type %T", parsed.PublicKey) + } + return pub.Equal(&key.PublicKey), nil +} + +// parsePrivateKey accepts both PKCS#1 and PKCS#8 encodings, since the format +// depends on who generated the key. +func parsePrivateKey(der []byte) (*rsa.PrivateKey, error) { + if key, err := x509.ParsePKCS1PrivateKey(der); err == nil { + return key, nil + } + parsed, err := x509.ParsePKCS8PrivateKey(der) + if err != nil { + return nil, fmt.Errorf("parsing private key: %w", err) + } + key, ok := parsed.(*rsa.PrivateKey) + if !ok { + return nil, fmt.Errorf("unsupported private key type %T", parsed) + } + return key, nil +} + // fetchSignedCert checks if the CA has signed the certificate. Returns the PEM cert or nil. func (r *CertificateReconciler) fetchSignedCert(ctx context.Context, cert *openvoxv1alpha1.Certificate, ca *openvoxv1alpha1.CertificateAuthority, caBaseURL, namespace string) ([]byte, error) { certname := cert.Spec.Certname @@ -312,7 +358,12 @@ func (r *CertificateReconciler) signCertificate(ctx context.Context, cert *openv return result, err } - // Step 2: Check if cert is signed (non-blocking, single attempt) + // Step 2: Check if cert is signed (non-blocking, single attempt). + // + // The CA is queried by certname, so a foreign certificate issued under the + // same name comes back here. Storing it would pair someone else's + // certificate with our private key, and the mismatch would only surface as + // a failed TLS handshake at runtime. signedCertPEM, err := r.fetchSignedCert(ctx, cert, ca, caBaseURL, namespace) if err != nil { logger.Info("failed to fetch signed cert, will retry", "error", err) @@ -421,6 +472,35 @@ func (r *CertificateReconciler) signCertificate(ctx context.Context, cert *openv } keyPEM := pendingSecret.Data["key.pem"] + // The CA was queried by certname, so what came back is whatever holds that + // name - not necessarily what we asked to be signed. Pairing a foreign + // certificate with our key would produce a Secret that only fails much + // later, during a TLS handshake, with an error that points nowhere near + // here. + matches, matchErr := certMatchesKey(signedCertPEM, keyPEM) + if matchErr != nil { + return ctrl.Result{}, fmt.Errorf("verifying the signed certificate against the pending key: %w", matchErr) + } + if !matches { + msg := fmt.Sprintf("the CA returned a certificate for certname %q that was issued for a different key; "+ + "another Certificate is most likely using the same certname", certnameOf(cert)) + if statusErr := updateStatusWithRetry(ctx, r.Client, cert, func() { + cert.Status.Phase = openvoxv1alpha1.CertificatePhaseError + meta.SetStatusCondition(&cert.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionCertSigned, + Status: metav1.ConditionFalse, + Reason: "CertnameConflict", + Message: msg, + ObservedGeneration: cert.Generation, + }) + }); statusErr != nil { + logger.Error(statusErr, "failed to record the certname conflict") + } + r.Recorder.Eventf(cert, nil, corev1.EventTypeWarning, EventReasonCertnameConflict, "Reconcile", "%s", msg) + // Nothing improves by retrying: only a spec change frees the certname. + return ctrl.Result{}, nil + } + tlsSecretName := fmt.Sprintf("%s-tls", cert.Name) if err := r.createOrUpdateTLSSecret(ctx, cert, ca, tlsSecretName, signedCertPEM, keyPEM); err != nil { return ctrl.Result{}, fmt.Errorf("creating TLS Secret: %w", err) diff --git a/internal/controller/certificate_signing_test.go b/internal/controller/certificate_signing_test.go index b091c05f..2f400c56 100644 --- a/internal/controller/certificate_signing_test.go +++ b/internal/controller/certificate_signing_test.go @@ -148,6 +148,49 @@ func generateTestCertWithExpiry(t *testing.T, validity time.Duration) ([]byte, [ return certPEM, keyPEM } +// issueForCSR signs a submitted CSR with the test CA, so the returned +// certificate carries the requester's public key. A canned certificate pairs a +// foreign key with the controller's, which is exactly what certMatchesKey +// refuses -- and what a shared certname would produce against a real CA. +func issueForCSR(t *testing.T, caCertPEM, caKeyPEM, csrPEM []byte) []byte { + t.Helper() + + csrBlock, _ := pem.Decode(csrPEM) + if csrBlock == nil { + t.Fatalf("decoding submitted CSR") + } + csr, err := x509.ParseCertificateRequest(csrBlock.Bytes) + if err != nil { + t.Fatalf("parsing submitted CSR: %v", err) + } + + caBlock, _ := pem.Decode(caCertPEM) + caCert, err := x509.ParseCertificate(caBlock.Bytes) + if err != nil { + t.Fatalf("parsing CA certificate: %v", err) + } + keyBlock, _ := pem.Decode(caKeyPEM) + caKey, err := x509.ParsePKCS1PrivateKey(keyBlock.Bytes) + if err != nil { + t.Fatalf("parsing CA key: %v", err) + } + + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(2), + Subject: csr.Subject, + DNSNames: csr.DNSNames, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, caCert, csr.PublicKey, caKey) + if err != nil { + t.Fatalf("signing the submitted CSR: %v", err) + } + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) +} + func TestBuildExternalCAHTTPClient_Minimal(t *testing.T) { ext := &openvoxv1alpha1.ExternalCASpec{ URL: "https://puppet-ca.example.com:8140", @@ -970,14 +1013,20 @@ func TestFetchSignedCert_EmptyCertname(t *testing.T) { func TestSignCertificate_FullFlow(t *testing.T) { certPEM, keyPEM := generateTestCert(t) - // Server handles CSR submit and cert fetch + // Server handles CSR submit and cert fetch. It signs the submitted CSR + // rather than returning a canned certificate: the controller now refuses a + // certificate that was issued for a different key, which is what stops it + // from adopting a foreign certificate that shares its certname. + var issued []byte server := newTestTLSServer(t, certPEM, keyPEM, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodPut && strings.Contains(r.URL.Path, "/certificate_request/"): + csrPEM, _ := io.ReadAll(r.Body) + issued = issueForCSR(t, certPEM, keyPEM, csrPEM) w.WriteHeader(http.StatusOK) case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/certificate/"): w.WriteHeader(http.StatusOK) - _, _ = w.Write(certPEM) + _, _ = w.Write(issued) case r.Method == http.MethodPut && strings.Contains(r.URL.Path, "/certificate_status/"): w.WriteHeader(http.StatusNoContent) default: diff --git a/internal/controller/certname_conflict_test.go b/internal/controller/certname_conflict_test.go new file mode 100644 index 00000000..964c63af --- /dev/null +++ b/internal/controller/certname_conflict_test.go @@ -0,0 +1,204 @@ +package controller + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "testing" + "time" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// TestCertnameOf_DefaultsLikeTheCRD pins the reason this collision is easy to +// hit: an unset certname is not "none", it is "puppet". +func TestCertnameOf_DefaultsLikeTheCRD(t *testing.T) { + cert := &openvoxv1alpha1.Certificate{} + if got := certnameOf(cert); got != "puppet" { + t.Errorf("expected the CRD default puppet, got %q", got) + } + cert.Spec.Certname = "web01.example.com" + if got := certnameOf(cert); got != "web01.example.com" { + t.Errorf("expected the explicit certname, got %q", got) + } +} + +// TestCertnameConflict_RefusesToSign is the guarantee: the CA keeps one entry +// per certname, so a second Certificate claiming the same name must not sign +// under it. Without this the second resource adopts the first one's +// certificate, and deleting either revokes the entry both rely on. +func TestCertnameConflict_RefusesToSign(t *testing.T) { + ca := newCertificateAuthority("production-ca") + meta.SetStatusCondition(&ca.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionCAReady, Status: metav1.ConditionTrue, + Reason: "Ready", Message: "ready", + }) + first := newCertificate("web-a", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + first.Spec.Certname = "shared.example.com" + second := newCertificate("web-b", "production-ca", openvoxv1alpha1.CertificatePhasePending) + second.Spec.Certname = "shared.example.com" + + c := setupTestClient(ca, first, second) + r := newCertificateReconciler(c) + + res, err := r.Reconcile(testCtx(), testRequest("web-b")) + if err != nil { + t.Fatalf("a certname conflict must not fail the reconcile: %v", err) + } + if res.RequeueAfter != 0 { + t.Errorf("a certname conflict is permanent and must not be polled, got %v", res.RequeueAfter) + } + + got := &openvoxv1alpha1.Certificate{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "web-b", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading the Certificate back: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionCertnameConflict) + if cond == nil || cond.Status != metav1.ConditionTrue { + t.Fatalf("expected a CertnameConflict condition, got %+v", cond) + } + if got.Status.Phase != openvoxv1alpha1.CertificatePhaseError { + t.Errorf("expected phase Error, got %q", got.Status.Phase) + } + + // No TLS Secret may be produced under a name it does not own. + if err := c.Get(testCtx(), types.NamespacedName{Name: "web-b-tls", Namespace: testNamespace}, + newSecret("web-b-tls", nil)); err == nil { + t.Error("the conflicting Certificate must not produce a TLS Secret") + } +} + +// TestCertnameConflict_DefaultCertnameCollides is the accidental case: two +// Certificates created without a certname both land on "puppet". +func TestCertnameConflict_DefaultCertnameCollides(t *testing.T) { + ca := newCertificateAuthority("production-ca") + meta.SetStatusCondition(&ca.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionCAReady, Status: metav1.ConditionTrue, + Reason: "Ready", Message: "ready", + }) + first := newCertificate("web-a", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + first.Spec.Certname = "" + second := newCertificate("web-b", "production-ca", openvoxv1alpha1.CertificatePhasePending) + second.Spec.Certname = "" + + c := setupTestClient(ca, first, second) + r := newCertificateReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("web-b")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + got := &openvoxv1alpha1.Certificate{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "web-b", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading the Certificate back: %v", err) + } + if cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionCertnameConflict); cond == nil { + t.Error("two Certificates without an explicit certname both use puppet and must conflict") + } +} + +// TestCertnameConflict_DifferentAuthoritiesDoNotCollide bounds the rule: the +// name only has to be unique per CA, since each CA keeps its own entries. +func TestCertnameConflict_DifferentAuthoritiesDoNotCollide(t *testing.T) { + first := newCertificate("web-a", "ca-one", openvoxv1alpha1.CertificatePhaseSigned) + second := newCertificate("web-b", "ca-two", openvoxv1alpha1.CertificatePhasePending) + + c := setupTestClient(first, second) + r := newCertificateReconciler(c) + + other, err := r.otherCertificateUsingCertname(testCtx(), second) + if err != nil { + t.Fatalf("looking up claimants: %v", err) + } + if other != "" { + t.Errorf("certificates against different CAs must not conflict, got %q", other) + } +} + +// TestCertnameConflict_TerminatingCertificateReleasesTheName keeps a deletion +// in progress from blocking its replacement forever. +func TestCertnameConflict_TerminatingCertificateReleasesTheName(t *testing.T) { + now := metav1.Now() + leaving := newCertificate("web-a", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + leaving.DeletionTimestamp = &now + leaving.Finalizers = []string{certificateFinalizer} + successor := newCertificate("web-b", "production-ca", openvoxv1alpha1.CertificatePhasePending) + + c := setupTestClient(leaving, successor) + r := newCertificateReconciler(c) + + other, err := r.otherCertificateUsingCertname(testCtx(), successor) + if err != nil { + t.Fatalf("looking up claimants: %v", err) + } + if other != "" { + t.Errorf("a terminating Certificate must release its certname, got %q", other) + } +} + +// --- key matching --- + +// selfSignedFor builds a certificate for a freshly generated key, so the pair +// is internally consistent but unrelated to any other key. +func selfSignedFor(t *testing.T) (certPEM, keyPEM []byte) { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generating key: %v", err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(7), + Subject: pkix.Name{CommonName: "shared.example.com"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key) + if err != nil { + t.Fatalf("creating certificate: %v", err) + } + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), + pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) +} + +// TestCertMatchesKey_AcceptsItsOwnPair and the rejection case below are the +// last line of defence: with webhooks off and two Certificates racing, the CA +// hands back whatever holds the certname. Pairing that with our own key would +// only fail later, during a TLS handshake. +func TestCertMatchesKey_AcceptsItsOwnPair(t *testing.T) { + certPEM, keyPEM := selfSignedFor(t) + + ok, err := certMatchesKey(certPEM, keyPEM) + if err != nil { + t.Fatalf("verifying a matching pair: %v", err) + } + if !ok { + t.Error("a certificate must match the key it was issued for") + } +} + +func TestCertMatchesKey_RejectsAForeignCertificate(t *testing.T) { + foreignCert, _ := selfSignedFor(t) + _, ourKey := selfSignedFor(t) + + ok, err := certMatchesKey(foreignCert, ourKey) + if err != nil { + t.Fatalf("verifying a mismatched pair: %v", err) + } + if ok { + t.Error("a certificate issued for another key must be rejected") + } +} + +func TestCertMatchesKey_ReportsUnusableInput(t *testing.T) { + _, keyPEM := selfSignedFor(t) + + if _, err := certMatchesKey([]byte("not a pem"), keyPEM); err == nil { + t.Error("expected an error for an undecodable certificate") + } +} diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index 4a46fe36..f4d79219 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -304,6 +304,16 @@ func appendPullSecrets(existing []corev1.LocalObjectReference, add ...corev1.Loc return existing } +// certnameOf returns the certname a Certificate is issued under. The CRD +// defaults it to "puppet", so an unset value is not "none" but a very common +// collision candidate. +func certnameOf(cert *openvoxv1alpha1.Certificate) string { + if cert.Spec.Certname != "" { + return cert.Spec.Certname + } + return "puppet" +} + // serverRoleEnabled reports whether the Server runs the catalog server role. // The spec field defaults to true, so an unset value enables the role. func serverRoleEnabled(server *openvoxv1alpha1.Server) bool { diff --git a/internal/controller/indexers.go b/internal/controller/indexers.go index 3aba5480..ed524a66 100644 --- a/internal/controller/indexers.go +++ b/internal/controller/indexers.go @@ -21,6 +21,11 @@ const ( IndexConfigRef = "spec.configRef" IndexCertificateRef = "spec.certificateRef" IndexAuthorityRef = "spec.authorityRef" + + // IndexCertname makes the certname collision check a lookup rather than a + // full listing. A certname identifies exactly one entry on the CA, so two + // Certificates sharing one against the same CA are indistinguishable to it. + IndexCertname = "spec.certname" ) // SetupFieldIndexers registers every field index the controllers rely on. @@ -58,6 +63,10 @@ func fieldIndexes() []fieldIndex { {&openvoxv1alpha1.Certificate{}, IndexAuthorityRef, func(o client.Object) []string { return nonEmpty(o.(*openvoxv1alpha1.Certificate).Spec.AuthorityRef) }}, + {&openvoxv1alpha1.Certificate{}, IndexCertname, func(o client.Object) []string { + c := o.(*openvoxv1alpha1.Certificate) + return nonEmpty(certnameOf(c)) + }}, {&openvoxv1alpha1.Database{}, IndexCertificateRef, func(o client.Object) []string { return nonEmpty(o.(*openvoxv1alpha1.Database).Spec.CertificateRef) }}, diff --git a/internal/webhook/certificate_webhook.go b/internal/webhook/certificate_webhook.go index 1c96c12d..30d558a8 100644 --- a/internal/webhook/certificate_webhook.go +++ b/internal/webhook/certificate_webhook.go @@ -2,6 +2,7 @@ package webhook import ( "context" + "fmt" "net" "strings" @@ -54,6 +55,17 @@ func (v *CertificateValidator) validate(ctx context.Context, c *openvoxv1alpha1. } } + // A certname identifies exactly one entry on the CA. Two Certificates + // sharing one against the same CA are indistinguishable to it: the second + // CSR is rejected, and deleting either one revokes the entry both rely on. + // The CRD default is "puppet", so the collision is easy to hit by accident. + if other, err := v.otherCertificateUsingCertname(ctx, c); err != nil { + errs = append(errs, field.InternalError(specPath.Child("certname"), err)) + } else if other != "" { + errs = append(errs, field.Duplicate(specPath.Child("certname"), + certnameOrDefault(c)+" (already claimed by Certificate "+other+" against the same CertificateAuthority)")) + } + if err := validateDuration(c.Spec.RenewBefore, "renewBefore"); err != nil { errs = append(errs, field.Invalid(specPath.Child("renewBefore"), c.Spec.RenewBefore, err.Error())) } @@ -80,3 +92,42 @@ func (v *CertificateValidator) validate(ctx context.Context, c *openvoxv1alpha1. } return nil, nil } + +// certnameOrDefault mirrors the CRD default, so the check compares the values +// the CA will actually see rather than what the manifest happens to spell out. +func certnameOrDefault(c *openvoxv1alpha1.Certificate) string { + if c.Spec.Certname != "" { + return c.Spec.Certname + } + return "puppet" +} + +// otherCertificateUsingCertname returns the name of another Certificate in the +// namespace that claims the same certname against the same +// CertificateAuthority, or "" when there is none. +func (v *CertificateValidator) otherCertificateUsingCertname(ctx context.Context, c *openvoxv1alpha1.Certificate) (string, error) { + certList := &openvoxv1alpha1.CertificateList{} + if err := v.Client.List(ctx, certList, client.InNamespace(c.Namespace)); err != nil { + return "", fmt.Errorf("listing Certificates in namespace %s: %w", c.Namespace, err) + } + + want := certnameOrDefault(c) + for i := range certList.Items { + other := &certList.Items[i] + if other.Name == c.Name { + continue + } + // A Certificate on its way out releases its claim; its finalizer cleans + // the CA entry. + if !other.DeletionTimestamp.IsZero() { + continue + } + if other.Spec.AuthorityRef != c.Spec.AuthorityRef { + continue + } + if certnameOrDefault(other) == want { + return other.Name, nil + } + } + return "", nil +} diff --git a/internal/webhook/certificate_webhook_test.go b/internal/webhook/certificate_webhook_test.go index 5c66d577..341f1fcb 100644 --- a/internal/webhook/certificate_webhook_test.go +++ b/internal/webhook/certificate_webhook_test.go @@ -212,3 +212,79 @@ func TestCertificateValidator(t *testing.T) { } }) } + +// TestCertificateValidator_RejectsDuplicateCertname closes the collision at +// admission. The CA keeps one entry per certname, so two Certificates sharing +// one against the same CA cannot both be signed - and deleting either revokes +// the entry the other depends on. +func TestCertificateValidator_RejectsDuplicateCertname(t *testing.T) { + ca := &openvoxv1alpha1.CertificateAuthority{ + ObjectMeta: metav1.ObjectMeta{Name: "my-ca", Namespace: "default"}, + } + existing := &openvoxv1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "web-a", Namespace: "default"}, + Spec: openvoxv1alpha1.CertificateSpec{AuthorityRef: "my-ca", Certname: "shared.example.com"}, + } + c := setupTestClient(ca, existing) + v := &CertificateValidator{Client: c} + + duplicate := &openvoxv1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "web-b", Namespace: "default"}, + Spec: openvoxv1alpha1.CertificateSpec{AuthorityRef: "my-ca", Certname: "shared.example.com"}, + } + if _, err := v.ValidateCreate(context.Background(), duplicate); err == nil { + t.Error("expected a duplicate certname against the same CA to be rejected") + } + + // Updating the existing resource must not report a conflict with itself. + if _, err := v.ValidateUpdate(context.Background(), nil, existing); err != nil { + t.Errorf("a Certificate must not conflict with itself: %v", err) + } +} + +// TestCertificateValidator_DuplicateDefaultCertname covers the accidental case: +// the CRD defaults certname to puppet, so two Certificates created without one +// collide without anybody writing the same value twice. +func TestCertificateValidator_DuplicateDefaultCertname(t *testing.T) { + ca := &openvoxv1alpha1.CertificateAuthority{ + ObjectMeta: metav1.ObjectMeta{Name: "my-ca", Namespace: "default"}, + } + existing := &openvoxv1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "web-a", Namespace: "default"}, + Spec: openvoxv1alpha1.CertificateSpec{AuthorityRef: "my-ca"}, + } + c := setupTestClient(ca, existing) + v := &CertificateValidator{Client: c} + + duplicate := &openvoxv1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "web-b", Namespace: "default"}, + Spec: openvoxv1alpha1.CertificateSpec{AuthorityRef: "my-ca"}, + } + if _, err := v.ValidateCreate(context.Background(), duplicate); err == nil { + t.Error("two Certificates without an explicit certname both use puppet and must be rejected") + } +} + +// TestCertificateValidator_DifferentAuthorityIsFine bounds the rule. +func TestCertificateValidator_DifferentAuthorityIsFine(t *testing.T) { + caOne := &openvoxv1alpha1.CertificateAuthority{ + ObjectMeta: metav1.ObjectMeta{Name: "ca-one", Namespace: "default"}, + } + caTwo := &openvoxv1alpha1.CertificateAuthority{ + ObjectMeta: metav1.ObjectMeta{Name: "ca-two", Namespace: "default"}, + } + existing := &openvoxv1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "web-a", Namespace: "default"}, + Spec: openvoxv1alpha1.CertificateSpec{AuthorityRef: "ca-one", Certname: "shared.example.com"}, + } + c := setupTestClient(caOne, caTwo, existing) + v := &CertificateValidator{Client: c} + + other := &openvoxv1alpha1.Certificate{ + ObjectMeta: metav1.ObjectMeta{Name: "web-b", Namespace: "default"}, + Spec: openvoxv1alpha1.CertificateSpec{AuthorityRef: "ca-two", Certname: "shared.example.com"}, + } + if _, err := v.ValidateCreate(context.Background(), other); err != nil { + t.Errorf("each CA keeps its own entries, so the same certname is fine: %v", err) + } +} From db21eb3719595e8fbd060a5f5c24deadef306612 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 12:46:46 +0200 Subject: [PATCH 3/9] fix(api): require an explicit certname instead of defaulting to puppet The kubebuilder default was the reason the collision was easy to hit: two Certificates created without a certname both landed on puppet, so they shared one CA entry by default rather than by mistake. It was also wrong on its own terms. A certname is an identity, and puppet is only the right identity for the main server - PuppetDB and any further certificate need their own. The names agents connect through, a load balancer or a service address, belong in dnsAltNames, which the chart already derives from the Pool Services a server joins. certname is now required with MinLength=1. Nothing in the repository relied on the default: the chart writes the value unconditionally and already fails the render when the Database has none, all sixteen CI value files set it explicitly, and so does the sample. The same guard now covers server entries, which were the one path that could still fall through to it. Also folds five copies of the puppet fallback in certificate_signing.go into certnameOf. The two remaining checks stay as they are: they return an error rather than substituting a name, which is correct for renewal and cleanup. Existing resources are unaffected - they carry certname: puppet materialised from the default, and the field is immutable anyway. New manifests that omit it are rejected, which is the point. --- api/v1alpha1/certificate_immutability_test.go | 23 +++++++++++++---- api/v1alpha1/certificate_types.go | 11 +++++--- api/v1alpha1/list_types_test.go | 2 ++ .../openvox.voxpupuli.org_certificates.yaml | 9 ++++++- .../openvox-stack/templates/certificates.yaml | 3 +++ .../openvox.voxpupuli.org_certificates.yaml | 9 ++++++- docs/reference/certificate.md | 12 ++++++--- internal/controller/certificate_signing.go | 25 ++++--------------- internal/controller/helpers.go | 8 +++--- internal/webhook/certificate_webhook.go | 6 +++-- 10 files changed, 70 insertions(+), 38 deletions(-) diff --git a/api/v1alpha1/certificate_immutability_test.go b/api/v1alpha1/certificate_immutability_test.go index 3d5e2697..43bf4be2 100644 --- a/api/v1alpha1/certificate_immutability_test.go +++ b/api/v1alpha1/certificate_immutability_test.go @@ -66,22 +66,35 @@ func TestCertificateCertnameIsImmutable(t *testing.T) { } }) - t.Run("the defaulted certname survives an unrelated update", func(t *testing.T) { + // The shared "puppet" default used to make two Certificates collide by + // default rather than by mistake: a certname identifies exactly one entry + // on the CA. There is no default any more, and an omitted certname is a + // validation error rather than a silent collision. + t.Run("an omitted certname is rejected", func(t *testing.T) { cert := &Certificate{ ObjectMeta: metav1.ObjectMeta{GenerateName: "test-cert-", Namespace: "default"}, Spec: CertificateSpec{AuthorityRef: "production-ca"}, } + err := k8sClient.Create(ctx, cert) + if err == nil { + t.Cleanup(func() { _ = k8sClient.Delete(ctx, cert) }) + t.Fatalf("expected a Certificate without a certname to be rejected, got certname %q", cert.Spec.Certname) + } + if !strings.Contains(err.Error(), "certname") { + t.Errorf("expected the error to name the certname field, got: %v", err) + } + }) + + t.Run("an unrelated update leaves the certname alone", func(t *testing.T) { + cert := newCert() if err := k8sClient.Create(ctx, cert); err != nil { t.Fatalf("creating Certificate: %v", err) } t.Cleanup(func() { _ = k8sClient.Delete(ctx, cert) }) - if cert.Spec.Certname != "puppet" { - t.Fatalf("expected the default certname, got %q", cert.Spec.Certname) - } cert.Spec.RenewBefore = "30d" if err := k8sClient.Update(ctx, cert); err != nil { - t.Errorf("an update that leaves the defaulted certname alone must pass, got: %v", err) + t.Errorf("an update that does not touch the certname must pass, got: %v", err) } }) } diff --git a/api/v1alpha1/certificate_types.go b/api/v1alpha1/certificate_types.go index f66155ad..8e12f249 100644 --- a/api/v1alpha1/certificate_types.go +++ b/api/v1alpha1/certificate_types.go @@ -63,10 +63,15 @@ type CertificateSpec struct { // The name is baked into the issued certificate and into the entry the CA // keeps for it. Changing it would leave that entry behind under the old // name, so the finalizer could no longer clean it up on deletion. - // +kubebuilder:default="puppet" + // + // There is deliberately no default. A certname identifies exactly one entry + // on the CA, so a shared default made two Certificates collide by default + // rather than by mistake. "puppet" is also only the right identity for the + // main server: PuppetDB and any further certificate need their own. The + // names agents connect through belong in DNSAltNames, not here. + // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="certname is immutable" - // +optional - Certname string `json:"certname,omitempty"` + Certname string `json:"certname"` // DNSAltNames is a list of DNS subject alternative names for the certificate. // Order is irrelevant and duplicates are rejected. diff --git a/api/v1alpha1/list_types_test.go b/api/v1alpha1/list_types_test.go index 315e3b3c..875b36d7 100644 --- a/api/v1alpha1/list_types_test.go +++ b/api/v1alpha1/list_types_test.go @@ -21,6 +21,7 @@ func TestListSemantics(t *testing.T) { ObjectMeta: metav1.ObjectMeta{GenerateName: "test-cert-", Namespace: "default"}, Spec: CertificateSpec{ AuthorityRef: "production-ca", + Certname: "web.example.com", DNSAltNames: []string{"puppet.example.com", "puppet.example.com"}, }, } @@ -39,6 +40,7 @@ func TestListSemantics(t *testing.T) { ObjectMeta: metav1.ObjectMeta{GenerateName: "test-cert-", Namespace: "default"}, Spec: CertificateSpec{ AuthorityRef: "production-ca", + Certname: "web.example.com", DNSAltNames: []string{"a.example.com", "b.example.com"}, }, } diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml index 8f19aec9..25a7143f 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml @@ -64,13 +64,19 @@ spec: signs this certificate. type: string certname: - default: puppet description: |- Certname is the certificate common name. Immutable after creation. The name is baked into the issued certificate and into the entry the CA keeps for it. Changing it would leave that entry behind under the old name, so the finalizer could no longer clean it up on deletion. + + There is deliberately no default. A certname identifies exactly one entry + on the CA, so a shared default made two Certificates collide by default + rather than by mistake. "puppet" is also only the right identity for the + main server: PuppetDB and any further certificate need their own. The + names agents connect through belong in DNSAltNames, not here. + minLength: 1 type: string x-kubernetes-validations: - message: certname is immutable @@ -114,6 +120,7 @@ spec: type: string required: - authorityRef + - certname type: object status: description: CertificateStatus defines the observed state of Certificate. diff --git a/charts/openvox-stack/templates/certificates.yaml b/charts/openvox-stack/templates/certificates.yaml index 7b6c4699..2de63af9 100644 --- a/charts/openvox-stack/templates/certificates.yaml +++ b/charts/openvox-stack/templates/certificates.yaml @@ -1,4 +1,7 @@ {{- range $entry := .Values.servers }} +{{- if not $entry.certificate.certname }} +{{- fail (printf "servers[%s].certificate.certname is required: a certname identifies exactly one entry on the CA, so two servers sharing one cannot both be signed" $entry.name) }} +{{- end }} --- apiVersion: openvox.voxpupuli.org/v1alpha1 kind: Certificate diff --git a/config/crd/bases/openvox.voxpupuli.org_certificates.yaml b/config/crd/bases/openvox.voxpupuli.org_certificates.yaml index 8f19aec9..25a7143f 100644 --- a/config/crd/bases/openvox.voxpupuli.org_certificates.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_certificates.yaml @@ -64,13 +64,19 @@ spec: signs this certificate. type: string certname: - default: puppet description: |- Certname is the certificate common name. Immutable after creation. The name is baked into the issued certificate and into the entry the CA keeps for it. Changing it would leave that entry behind under the old name, so the finalizer could no longer clean it up on deletion. + + There is deliberately no default. A certname identifies exactly one entry + on the CA, so a shared default made two Certificates collide by default + rather than by mistake. "puppet" is also only the right identity for the + main server: PuppetDB and any further certificate need their own. The + names agents connect through belong in DNSAltNames, not here. + minLength: 1 type: string x-kubernetes-validations: - message: certname is immutable @@ -114,6 +120,7 @@ spec: type: string required: - authorityRef + - certname type: object status: description: CertificateStatus defines the observed state of Certificate. diff --git a/docs/reference/certificate.md b/docs/reference/certificate.md index b9506102..881383e2 100644 --- a/docs/reference/certificate.md +++ b/docs/reference/certificate.md @@ -22,7 +22,7 @@ spec: | Field | Type | Default | Description | |---|---|---|---| | `authorityRef` | string | **required** | Reference to the CertificateAuthority | -| `certname` | string | `puppet` | Certificate common name (CN) | +| `certname` | string | **required** | Certificate common name (CN) | | `dnsAltNames` | []string | - | DNS subject alternative names | | `renewBefore` | string | `60d` | Duration before expiration when the certificate should be renewed (e.g. `60d`, `720h`) | | `csrExtensions` | CSRExtensionsSpec | - | Puppet CSR extension attributes to embed in the CSR | @@ -178,8 +178,14 @@ The operator therefore refuses the collision in three places: - a certificate returned by the CA is verified against the private key it was requested for, so a foreign certificate under the same name is never stored -`certname` defaults to `puppet`, so two Certificates created without one collide -by default rather than by mistake. Give each Certificate its own certname. +`certname` is required and has no default. It used to default to `puppet`, which +made two Certificates created without one collide by default rather than by +mistake, and `puppet` is only the right identity for the main server anyway -- +PuppetDB and any further certificate need their own. + +The names agents connect through, such as a load balancer or a service address, +belong in `dnsAltNames` rather than here. The chart derives the Service names of +every Pool a server joins into that list automatically. ## Created Resources diff --git a/internal/controller/certificate_signing.go b/internal/controller/certificate_signing.go index 8d2188f8..b53fbd85 100644 --- a/internal/controller/certificate_signing.go +++ b/internal/controller/certificate_signing.go @@ -196,10 +196,7 @@ func buildCSRExtensions(spec *openvoxv1alpha1.CSRExtensionsSpec) ([]pkix.Extensi func (r *CertificateReconciler) submitCSR(ctx context.Context, cert *openvoxv1alpha1.Certificate, ca *openvoxv1alpha1.CertificateAuthority, caBaseURL, namespace string) (ctrl.Result, error) { logger := log.FromContext(ctx) - certname := cert.Spec.Certname - if certname == "" { - certname = "puppet" - } + certname := certnameOf(cert) pendingSecretName := fmt.Sprintf("%s-tls-pending", cert.Name) @@ -309,10 +306,7 @@ func parsePrivateKey(der []byte) (*rsa.PrivateKey, error) { // fetchSignedCert checks if the CA has signed the certificate. Returns the PEM cert or nil. func (r *CertificateReconciler) fetchSignedCert(ctx context.Context, cert *openvoxv1alpha1.Certificate, ca *openvoxv1alpha1.CertificateAuthority, caBaseURL, namespace string) ([]byte, error) { - certname := cert.Spec.Certname - if certname == "" { - certname = "puppet" - } + certname := certnameOf(cert) httpClient, err := caHTTPClientForCA(ctx, r.Client, ca, namespace) if err != nil { @@ -397,10 +391,7 @@ func (r *CertificateReconciler) signCertificate(ctx context.Context, cert *openv // Check absolute timeout based on pending Secret creation time elapsed := time.Since(pendingSecret.CreationTimestamp.Time) if !pendingSecret.CreationTimestamp.IsZero() && elapsed >= CSRPollAbsoluteTimeout { - certname := cert.Spec.Certname - if certname == "" { - certname = "puppet" - } + certname := certnameOf(cert) timeoutMsg := fmt.Sprintf("CSR polling timed out after %s", elapsed.Truncate(time.Minute)) if statusErr := updateStatusWithRetry(ctx, r.Client, cert, func() { cert.Status.Phase = openvoxv1alpha1.CertificatePhaseError @@ -439,10 +430,7 @@ func (r *CertificateReconciler) signCertificate(ctx context.Context, cert *openv // After threshold, transition to WaitingForSigning phase if attempts >= CSRPollWaitingThreshold { - certname := cert.Spec.Certname - if certname == "" { - certname = "puppet" - } + certname := certnameOf(cert) waitMsg := fmt.Sprintf("CSR submitted but not yet signed after %d attempts", attempts) if statusErr := updateStatusWithRetry(ctx, r.Client, cert, func() { cert.Status.Phase = openvoxv1alpha1.CertificatePhaseWaitingForSigning @@ -823,10 +811,7 @@ func (r *CertificateReconciler) cleanCertViaAPI(ctx context.Context, cert *openv // signCSRViaAPI signs a pending CSR via the Puppet CA HTTP API using mTLS with the // operator-signing certificate (authorized by CN-based auth.conf rules). func (r *CertificateReconciler) signCSRViaAPI(ctx context.Context, cert *openvoxv1alpha1.Certificate, ca *openvoxv1alpha1.CertificateAuthority, caBaseURL, namespace string) error { - certname := cert.Spec.Certname - if certname == "" { - certname = "puppet" - } + certname := certnameOf(cert) // Load CA public cert for TLS server verification caCertPEM, err := getCAPublicCert(ctx, r.Client, ca, namespace) diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index f4d79219..4c3fa1f4 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -304,9 +304,11 @@ func appendPullSecrets(existing []corev1.LocalObjectReference, add ...corev1.Loc return existing } -// certnameOf returns the certname a Certificate is issued under. The CRD -// defaults it to "puppet", so an unset value is not "none" but a very common -// collision candidate. +// certnameOf returns the certname a Certificate is issued under. +// +// The field is required and non-empty since the shared "puppet" default was +// removed. The fallback only covers resources written while that default still +// existed, which all carry "puppet" anyway. func certnameOf(cert *openvoxv1alpha1.Certificate) string { if cert.Spec.Certname != "" { return cert.Spec.Certname diff --git a/internal/webhook/certificate_webhook.go b/internal/webhook/certificate_webhook.go index 30d558a8..5fb588dd 100644 --- a/internal/webhook/certificate_webhook.go +++ b/internal/webhook/certificate_webhook.go @@ -93,8 +93,10 @@ func (v *CertificateValidator) validate(ctx context.Context, c *openvoxv1alpha1. return nil, nil } -// certnameOrDefault mirrors the CRD default, so the check compares the values -// the CA will actually see rather than what the manifest happens to spell out. +// certnameOrDefault resolves the certname the CA will see. Schema validation +// runs before this webhook, so the field is already non-empty for anything +// created since the default was removed; the fallback covers resources written +// before that, which all carry "puppet". func certnameOrDefault(c *openvoxv1alpha1.Certificate) string { if c.Spec.Certname != "" { return c.Spec.Certname From c15048dd867290500fadfc216a69eb76850c00c2 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:08:13 +0000 Subject: [PATCH 4/9] fix(deps): update module sigs.k8s.io/controller-runtime to v0.25.0 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 3eee30b8..bf9bb007 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( k8s.io/apimachinery v0.37.0 k8s.io/client-go v0.37.0 k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 - sigs.k8s.io/controller-runtime v0.24.1 + sigs.k8s.io/controller-runtime v0.25.0 sigs.k8s.io/gateway-api v1.6.1 sigs.k8s.io/yaml v1.6.0 ) diff --git a/go.sum b/go.sum index 7f8e5cab..7fa0f489 100644 --- a/go.sum +++ b/go.sum @@ -256,8 +256,8 @@ k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 h1:jVkFFVfXdXP74B/zbO3hM3hpSFD0x k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3/go.mod h1:M2s5JB1lIYP3jzZdorPLHXIPJzt9vv2muW5a6L9DtNM= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.36.0 h1:/YpDJ4vReG7ZmzSpBGxduXgywWkJU9zHubgJG03MT+Y= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.36.0/go.mod h1:tJo1aepTXyR+8Xs3sUsGBDk4Ub2AM5dPAPKJx0mpm5c= -sigs.k8s.io/controller-runtime v0.24.1 h1:miPEwrmirImAvgME1L9qebGHrOnGJoVmVdtOU9fRfo4= -sigs.k8s.io/controller-runtime v0.24.1/go.mod h1:vFkfY5fGt5xAC/sKb8IBFKgWPNKG9OUG29dR8Y2wImw= +sigs.k8s.io/controller-runtime v0.25.0 h1:44KgRUPew331KSJpNu8zJow3iTR5W0p/SfrHdw3lV40= +sigs.k8s.io/controller-runtime v0.25.0/go.mod h1:4QqLdT6z/L6Olj8JJCtvztid4/fnIiYsfaTFScegctc= sigs.k8s.io/controller-tools v0.22.0 h1:eG3FAVja/KnlXKIWg95udIFz1cMyAtMjP11cqBh3t+k= sigs.k8s.io/controller-tools v0.22.0/go.mod h1:VizwUStoZK7rReCj704czGGrB7mLxXTiJSJt7wN5ilI= sigs.k8s.io/gateway-api v1.6.1 h1:mock6phZbI6rvZerwrVNk7hVNymQgHo+6sJ81Ia7ftY= From b81451f045f5ece1535d6a0b0f530300997be905 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 19:15:55 +0200 Subject: [PATCH 5/9] fix(ca): keep infra_crl.pem across a CRL refresh The setup Job writes both ca_crl.pem and infra_crl.pem into the {ca}-ca-crl Secret. The periodic refresh writes only the first, and createOrUpdateSecret replaces the data map wholesale, so infra_crl.pem survived exactly until the first refresh and then disappeared for the lifetime of the CA. Deployments running with enable-infra-crl lose infrastructure-node revocation that way, and nothing reports it: the refresh succeeds, the Secret still exists, only one of its keys is gone. createOrUpdateSecret now takes an optional list of keys to carry across. The carry-over reads from the object CreateOrUpdate just fetched, so it cannot race with a concurrent write. Closes #574 --- .../controller/certificateauthority_crl.go | 6 +++- internal/controller/helpers.go | 28 ++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/internal/controller/certificateauthority_crl.go b/internal/controller/certificateauthority_crl.go index 6f8bc90e..ccbb3e1a 100644 --- a/internal/controller/certificateauthority_crl.go +++ b/internal/controller/certificateauthority_crl.go @@ -87,7 +87,11 @@ func (r *CertificateAuthorityReconciler) fetchCRL(ctx context.Context, ca *openv // updateCRLSecret creates or updates the CRL secret with fresh CRL data. func (r *CertificateAuthorityReconciler) updateCRLSecret(ctx context.Context, ca *openvoxv1alpha1.CertificateAuthority, name string, crlPEM []byte) error { + // The setup Job writes infra_crl.pem into this same Secret and nothing + // refreshes it here, so it has to be carried across. Losing it disables + // infrastructure-node revocation for deployments running with + // enable-infra-crl, and nothing reports the loss. return createOrUpdateSecret(ctx, r.Client, r.Scheme, ca, name, ca.Namespace, caLabels(ca.Name), map[string][]byte{ "ca_crl.pem": crlPEM, - }) + }, "infra_crl.pem") } diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index 4c3fa1f4..d071c70c 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -126,8 +126,12 @@ func isSecretReady(ctx context.Context, reader client.Reader, name, namespace, r // createOrUpdateSecret creates or updates a Secret with the given data, owned by // the given object. +// preserveKeys names entries that belong to a different writer and must +// survive an update that does not carry them. Without it a caller that owns one +// key of a shared Secret silently drops the rest, and the loss is invisible: +// the Secret still exists, only poorer. func createOrUpdateSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, owner client.Object, - name, namespace string, labels map[string]string, data map[string][]byte) error { + name, namespace string, labels map[string]string, data map[string][]byte, preserveKeys ...string) error { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, } @@ -135,8 +139,21 @@ func createOrUpdateSecret(ctx context.Context, c client.Client, scheme *runtime. if err := assertControlledBy(secret, owner, "Secret"); err != nil { return err } + // Read from the object CreateOrUpdate just fetched, so the carry-over + // cannot race with a concurrent write. + carried := make(map[string][]byte, len(preserveKeys)) + for _, k := range preserveKeys { + if v, ok := secret.Data[k]; ok { + carried[k] = v + } + } secret.Labels = labels secret.Data = data + for k, v := range carried { + if _, taken := secret.Data[k]; !taken { + secret.Data[k] = v + } + } return controllerutil.SetControllerReference(owner, secret, scheme) }) if err != nil { @@ -316,6 +333,15 @@ func certnameOf(cert *openvoxv1alpha1.Certificate) string { return "puppet" } +// resolveReadOnlyRootFilesystem returns the setting for a Server, preferring +// its own override over the Config's. +func resolveReadOnlyRootFilesystem(server *openvoxv1alpha1.Server, cfg *openvoxv1alpha1.Config) bool { + if server.Spec.ReadOnlyRootFilesystem != nil { + return *server.Spec.ReadOnlyRootFilesystem + } + return openvoxv1alpha1.BoolValue(cfg.Spec.ReadOnlyRootFilesystem, true) +} + // serverRoleEnabled reports whether the Server runs the catalog server role. // The spec field defaults to true, so an unset value enables the role. func serverRoleEnabled(server *openvoxv1alpha1.Server) bool { From 41b6fa008d2727e80c1e409ead2a549e5697828e Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 19:15:55 +0200 Subject: [PATCH 6/9] feat(api): allow readOnlyRootFilesystem per Server The setting lived on ConfigSpec but was applied to Server pods. One Config backs several Servers with different roles, typically the CA and the compilers, so a single Server that needs a writable root forced the hardening off for every Server under that Config, the CA included. ServerSpec now carries an optional override that falls back to the Config, the same shape image uses since #549. It is a pointer for the same reason the other booleans are: with a true default on the Config, a plain bool cannot express false. Closes #575 --- api/v1alpha1/server_types.go | 13 ++++++++++++ .../openvox.voxpupuli.org_certificates.yaml | 21 ++++++++++++++++--- .../crds/openvox.voxpupuli.org_servers.yaml | 13 ++++++++++++ .../openvox.voxpupuli.org_certificates.yaml | 21 ++++++++++++++++--- .../bases/openvox.voxpupuli.org_servers.yaml | 13 ++++++++++++ internal/controller/server_deployment.go | 2 +- 6 files changed, 76 insertions(+), 7 deletions(-) diff --git a/api/v1alpha1/server_types.go b/api/v1alpha1/server_types.go index 6b285c8f..45d9bb68 100644 --- a/api/v1alpha1/server_types.go +++ b/api/v1alpha1/server_types.go @@ -149,6 +149,19 @@ type ServerSpec struct { // (runAsUser/runAsGroup/fsGroup) applied to the Server pods. // +optional SecurityContext *PodSecurityContextSpec `json:"securityContext,omitempty"` + + // ReadOnlyRootFilesystem overrides the Config's setting for this Server. + // + // One Config backs several Servers with different roles, typically the CA + // and the compilers, and hardening is otherwise a per-Server concern here + // alongside securityContext and extraVolumes. Without this override a + // single Server that needs a writable root forces the setting off for every + // Server under the Config, the CA included. + // + // Unset inherits from the Config. Deliberately a pointer: with a true + // default on the Config, a plain bool could not express false. + // +optional + ReadOnlyRootFilesystem *bool `json:"readOnlyRootFilesystem,omitempty"` } // PDBSpec defines PodDisruptionBudget settings. diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml index 25a7143f..f3fe1843 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_certificates.yaml @@ -185,6 +185,21 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + effectiveDNSAltNames: + description: |- + EffectiveDNSAltNames lists the alt names the certificate is actually + issued for: spec.dnsAltNames plus the route hostname of every Pool that + asks for injection and is joined by a Server using this Certificate. + + The Pool used to append its hostname to spec.dnsAltNames directly. Under + GitOps that turned into a loop: the Pool added the name, the source of + truth reverted it, the Pool added it again, and each round changed a + signing-relevant field. Deriving it here writes nothing foreign and is + idempotent. + items: + type: string + type: array + x-kubernetes-list-type: set notAfter: description: NotAfter is the expiration time of the signed certificate. format: date-time @@ -217,9 +232,9 @@ spec: type: string signedSpecHash: description: |- - SignedSpecHash digests the spec fields the current certificate was issued - for: certname, dnsAltNames and csrExtensions. When it no longer matches - the spec, the certificate is re-signed. An empty value means the hash was + SignedSpecHash digests what the current certificate was issued for: + certname, the effective alt names and csrExtensions. When it no longer + matches, the certificate is re-signed. An empty value means the hash was never recorded (certificates issued before this field existed) and is adopted on the next reconcile rather than triggering a re-sign. type: string diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml index 7b974fd1..9712cfb0 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml @@ -3676,6 +3676,19 @@ spec: description: PriorityClassName is the name of the PriorityClass for the Server pods. type: string + readOnlyRootFilesystem: + description: |- + ReadOnlyRootFilesystem overrides the Config's setting for this Server. + + One Config backs several Servers with different roles, typically the CA + and the compilers, and hardening is otherwise a per-Server concern here + alongside securityContext and extraVolumes. Without this override a + single Server that needs a writable root forces the setting off for every + Server under the Config, the CA included. + + Unset inherits from the Config. Deliberately a pointer: with a true + default on the Config, a plain bool could not express false. + type: boolean replicas: default: 1 description: Replicas is the number of Server instances. diff --git a/config/crd/bases/openvox.voxpupuli.org_certificates.yaml b/config/crd/bases/openvox.voxpupuli.org_certificates.yaml index 25a7143f..f3fe1843 100644 --- a/config/crd/bases/openvox.voxpupuli.org_certificates.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_certificates.yaml @@ -185,6 +185,21 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + effectiveDNSAltNames: + description: |- + EffectiveDNSAltNames lists the alt names the certificate is actually + issued for: spec.dnsAltNames plus the route hostname of every Pool that + asks for injection and is joined by a Server using this Certificate. + + The Pool used to append its hostname to spec.dnsAltNames directly. Under + GitOps that turned into a loop: the Pool added the name, the source of + truth reverted it, the Pool added it again, and each round changed a + signing-relevant field. Deriving it here writes nothing foreign and is + idempotent. + items: + type: string + type: array + x-kubernetes-list-type: set notAfter: description: NotAfter is the expiration time of the signed certificate. format: date-time @@ -217,9 +232,9 @@ spec: type: string signedSpecHash: description: |- - SignedSpecHash digests the spec fields the current certificate was issued - for: certname, dnsAltNames and csrExtensions. When it no longer matches - the spec, the certificate is re-signed. An empty value means the hash was + SignedSpecHash digests what the current certificate was issued for: + certname, the effective alt names and csrExtensions. When it no longer + matches, the certificate is re-signed. An empty value means the hash was never recorded (certificates issued before this field existed) and is adopted on the next reconcile rather than triggering a re-sign. type: string diff --git a/config/crd/bases/openvox.voxpupuli.org_servers.yaml b/config/crd/bases/openvox.voxpupuli.org_servers.yaml index 7b974fd1..9712cfb0 100644 --- a/config/crd/bases/openvox.voxpupuli.org_servers.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_servers.yaml @@ -3676,6 +3676,19 @@ spec: description: PriorityClassName is the name of the PriorityClass for the Server pods. type: string + readOnlyRootFilesystem: + description: |- + ReadOnlyRootFilesystem overrides the Config's setting for this Server. + + One Config backs several Servers with different roles, typically the CA + and the compilers, and hardening is otherwise a per-Server concern here + alongside securityContext and extraVolumes. Without this override a + single Server that needs a writable root forces the setting off for every + Server under the Config, the CA included. + + Unset inherits from the Config. Deliberately a pointer: with a true + default on the Config, a plain bool could not express false. + type: boolean replicas: default: 1 description: Replicas is the number of Server instances. diff --git a/internal/controller/server_deployment.go b/internal/controller/server_deployment.go index 657d929a..558a01e4 100644 --- a/internal/controller/server_deployment.go +++ b/internal/controller/server_deployment.go @@ -519,7 +519,7 @@ chmod 640 /ssl/private_keys/puppet.pem` containerSecurityContext := &corev1.SecurityContext{ AllowPrivilegeEscalation: boolPtr(false), - ReadOnlyRootFilesystem: boolPtr(openvoxv1alpha1.BoolValue(cfg.Spec.ReadOnlyRootFilesystem, true)), + ReadOnlyRootFilesystem: boolPtr(resolveReadOnlyRootFilesystem(server, cfg)), Capabilities: &corev1.Capabilities{ Drop: []corev1.Capability{"ALL"}, }, From 1c78733837e816d2aea4eca7abc65c996efe2cb9 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 19:16:06 +0200 Subject: [PATCH 7/9] refactor(certificate): derive alt names from Pools instead of mutating them PoolReconciler.injectDNSAltNames wrote the route hostname into Certificate.spec.dnsAltNames of every Server in the Pool - a resource the Pool does not own. Under Argo CD or Flux that turned into a loop: the Pool added the name, the source of truth reverted it, the Pool added it again. Since #549 each round changes a signing-relevant field, so the churn also re-signs the certificate and rolls the pods. The relationship is now the other way round. The Certificate controller derives its effective alt names from its own spec plus the route hostname of every Pool that asks for injection and is joined by a Server using this Certificate. Nothing foreign is written, the result is idempotent, and status.effectiveDNSAltNames makes it visible. The signing hash and the CSR both use the derived names, so a Pool joined later still triggers a re-sign - the mechanism stays, only its trigger moves from a foreign write to observed state. The Certificate controller watches Pools and Servers so a hostname added later reaches it at all. injectDNSAltName stays as an API field; its meaning changes from mutates to is taken into account. The pool-gateway e2e assertion moves with it: the spec now holds what the chart wrote, the hostname appears in status. Closes #552 --- api/v1alpha1/certificate_types.go | 19 +- api/v1alpha1/zz_generated.deepcopy.go | 10 + docs/reference/pool.md | 2 +- internal/controller/certificate_controller.go | 40 +++- internal/controller/certificate_derived.go | 112 +++++++++++ .../controller/certificate_derived_test.go | 190 ++++++++++++++++++ internal/controller/certificate_signing.go | 19 +- .../controller/certificate_spec_drift_test.go | 16 +- internal/controller/ownership_test.go | 41 ++-- internal/controller/pool_controller.go | 57 ------ internal/controller/pool_controller_test.go | 93 --------- internal/controller/testutil_test.go | 7 + tests/e2e/pool-gateway/chainsaw-test.yaml | 19 +- 13 files changed, 432 insertions(+), 193 deletions(-) create mode 100644 internal/controller/certificate_derived.go create mode 100644 internal/controller/certificate_derived_test.go diff --git a/api/v1alpha1/certificate_types.go b/api/v1alpha1/certificate_types.go index 8e12f249..70faa318 100644 --- a/api/v1alpha1/certificate_types.go +++ b/api/v1alpha1/certificate_types.go @@ -124,14 +124,27 @@ type CertificateStatus struct { // +optional SecretName string `json:"secretName,omitempty"` - // SignedSpecHash digests the spec fields the current certificate was issued - // for: certname, dnsAltNames and csrExtensions. When it no longer matches - // the spec, the certificate is re-signed. An empty value means the hash was + // SignedSpecHash digests what the current certificate was issued for: + // certname, the effective alt names and csrExtensions. When it no longer + // matches, the certificate is re-signed. An empty value means the hash was // never recorded (certificates issued before this field existed) and is // adopted on the next reconcile rather than triggering a re-sign. // +optional SignedSpecHash string `json:"signedSpecHash,omitempty"` + // EffectiveDNSAltNames lists the alt names the certificate is actually + // issued for: spec.dnsAltNames plus the route hostname of every Pool that + // asks for injection and is joined by a Server using this Certificate. + // + // The Pool used to append its hostname to spec.dnsAltNames directly. Under + // GitOps that turned into a loop: the Pool added the name, the source of + // truth reverted it, the Pool added it again, and each round changed a + // signing-relevant field. Deriving it here writes nothing foreign and is + // idempotent. + // +listType=set + // +optional + EffectiveDNSAltNames []string `json:"effectiveDNSAltNames,omitempty"` + // NotAfter is the expiration time of the signed certificate. // +optional NotAfter *metav1.Time `json:"notAfter,omitempty"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index d0447fd8..39eec21a 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -365,6 +365,11 @@ func (in *CertificateSpec) DeepCopy() *CertificateSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CertificateStatus) DeepCopyInto(out *CertificateStatus) { *out = *in + if in.EffectiveDNSAltNames != nil { + in, out := &in.EffectiveDNSAltNames, &out.EffectiveDNSAltNames + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.NotAfter != nil { in, out := &in.NotAfter, &out.NotAfter *out = (*in).DeepCopy() @@ -1766,6 +1771,11 @@ func (in *ServerSpec) DeepCopyInto(out *ServerSpec) { *out = new(PodSecurityContextSpec) (*in).DeepCopyInto(*out) } + if in.ReadOnlyRootFilesystem != nil { + in, out := &in.ReadOnlyRootFilesystem, &out.ReadOnlyRootFilesystem + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServerSpec. diff --git a/docs/reference/pool.md b/docs/reference/pool.md index bde65f8f..a1a730d9 100644 --- a/docs/reference/pool.md +++ b/docs/reference/pool.md @@ -40,7 +40,7 @@ spec: | `enabled` | bool | `false` | Activates TLSRoute creation for this Pool | | `hostname` | string | - | SNI hostname (required when enabled) | | `gatewayRef` | [GatewayReference](#gatewayreference) | - | Gateway to attach the TLSRoute to (required when enabled) | -| `injectDNSAltName` | bool | `false` | Add hostname to Certificate dnsAltNames of Servers that reference this Pool. **Note:** this modifies the Certificate spec and triggers re-signing, which briefly recreates the TLS Secret. | +| `injectDNSAltName` | bool | `false` | Take this Pool's hostname into account for the certificates of Servers that reference it. The Certificate controller derives it into `status.effectiveDNSAltNames`; nothing is written to the Certificate spec. Adding it changes the signing-relevant state, so the certificate is re-signed and the TLS Secret briefly recreated. | ### GatewayReference diff --git a/internal/controller/certificate_controller.go b/internal/controller/certificate_controller.go index fe80c524..0cf9edd5 100644 --- a/internal/controller/certificate_controller.go +++ b/internal/controller/certificate_controller.go @@ -19,6 +19,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" @@ -183,7 +184,10 @@ func (r *CertificateReconciler) Reconcile(ctx context.Context, req ctrl.Request) // would rewrite the status for no reason. Two things can still make work // necessary: the signing-relevant spec changed, or the renewal window opened. if cert.Status.Phase == openvoxv1alpha1.CertificatePhaseSigned { - wantHash := signingSpecHash(cert) + wantHash, effective, hashErr := r.signingSpecHashFor(ctx, cert) + if hashErr != nil { + return ctrl.Result{}, hashErr + } switch { case cert.Status.SignedSpecHash == "": // Certificates issued before the hash existed carry no baseline. @@ -191,6 +195,7 @@ func (r *CertificateReconciler) Reconcile(ctx context.Context, req ctrl.Request) // the cluster on the first reconcile after an operator upgrade. if err := updateStatusWithRetry(ctx, r.Client, cert, func() { cert.Status.SignedSpecHash = wantHash + cert.Status.EffectiveDNSAltNames = effective }); err != nil { return ctrl.Result{}, fmt.Errorf("recording signed spec hash for Certificate %s: %w", cert.Name, err) } @@ -219,10 +224,15 @@ func (r *CertificateReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, fmt.Errorf("adopting TLS Secret: %w", err) } + hash, effective, hashErr := r.signingSpecHashFor(ctx, cert) + if hashErr != nil { + return ctrl.Result{}, hashErr + } notAfter := r.extractNotAfter(ctx, tlsSecretName, cert.Namespace) if err := updateStatusWithRetry(ctx, r.Client, cert, func() { cert.Status.ObservedGeneration = cert.Generation - cert.Status.SignedSpecHash = signingSpecHash(cert) + cert.Status.SignedSpecHash = hash + cert.Status.EffectiveDNSAltNames = effective cert.Status.Phase = openvoxv1alpha1.CertificatePhaseSigned cert.Status.SecretName = tlsSecretName cert.Status.NotAfter = notAfter @@ -258,6 +268,10 @@ func (r *CertificateReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&openvoxv1alpha1.Certificate{}). Owns(&corev1.Secret{}). Watches(&corev1.Secret{}, enqueueCertificatesForSecret(mgr.GetClient())). + // The effective alt names are derived from the Pools a Server joins, so + // a route hostname added later has to reach the Certificate. + Watches(&openvoxv1alpha1.Pool{}, handler.EnqueueRequestsFromMapFunc(certificatesForPool(mgr.GetClient()))). + Watches(&openvoxv1alpha1.Server{}, handler.EnqueueRequestsFromMapFunc(certificatesForServerPools())). Complete(r) } @@ -321,10 +335,15 @@ func (r *CertificateReconciler) reconcileCertSigning(ctx context.Context, cert * // Mark as signed tlsSecretName := fmt.Sprintf("%s-tls", cert.Name) + hash, effective, hashErr := r.signingSpecHashFor(ctx, cert) + if hashErr != nil { + return ctrl.Result{}, hashErr + } notAfter := r.extractNotAfter(ctx, tlsSecretName, cert.Namespace) if err := updateStatusWithRetry(ctx, r.Client, cert, func() { cert.Status.ObservedGeneration = cert.Generation - cert.Status.SignedSpecHash = signingSpecHash(cert) + cert.Status.SignedSpecHash = hash + cert.Status.EffectiveDNSAltNames = effective cert.Status.Phase = openvoxv1alpha1.CertificatePhaseSigned cert.Status.SecretName = tlsSecretName cert.Status.NotAfter = notAfter @@ -567,8 +586,8 @@ func (r *CertificateReconciler) handleCertificateCleanup(ctx context.Context, ce // renewBefore is deliberately excluded. It only moves the point in time at // which renewal happens and says nothing about the certificate's content, so // changing it must not cause unnecessary load on the CA. -func signingSpecHash(cert *openvoxv1alpha1.Certificate) string { - names := append([]string(nil), cert.Spec.DNSAltNames...) +func signingSpecHash(cert *openvoxv1alpha1.Certificate, effectiveNames []string) string { + names := append([]string(nil), effectiveNames...) sort.Strings(names) fields := map[string]string{ @@ -602,6 +621,17 @@ func (r *CertificateReconciler) renewalDue(cert *openvoxv1alpha1.Certificate) bo return !r.isWithinRenewalCooldown(cert) } +// signingSpecHashFor computes the hash over what the certificate is actually +// issued for, which includes alt names derived from the Pools its Servers join. +func (r *CertificateReconciler) signingSpecHashFor(ctx context.Context, + cert *openvoxv1alpha1.Certificate) (string, []string, error) { + names, err := r.effectiveDNSAltNames(ctx, cert) + if err != nil { + return "", nil, err + } + return signingSpecHash(cert, names), names, nil +} + // otherCertificateUsingCertname returns the name of another live Certificate in // the namespace that claims the same certname against the same // CertificateAuthority, or "" when there is none. diff --git a/internal/controller/certificate_derived.go b/internal/controller/certificate_derived.go new file mode 100644 index 00000000..33533f4c --- /dev/null +++ b/internal/controller/certificate_derived.go @@ -0,0 +1,112 @@ +package controller + +import ( + "context" + "fmt" + "slices" + "sort" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// effectiveDNSAltNames returns the alt names a Certificate is issued for: its +// own spec, plus the route hostname of every Pool that asks for injection and +// is joined by a Server using this Certificate. +// +// This replaces the Pool writing into Certificate.spec. Deriving keeps the +// Certificate's spec owned by whoever wrote it, which is what makes the result +// stable under a GitOps controller that reverts foreign changes. +func (r *CertificateReconciler) effectiveDNSAltNames(ctx context.Context, + cert *openvoxv1alpha1.Certificate) ([]string, error) { + names := append([]string(nil), cert.Spec.DNSAltNames...) + + servers := &openvoxv1alpha1.ServerList{} + if err := r.List(ctx, servers, + client.InNamespace(cert.Namespace), + client.MatchingFields{IndexCertificateRef: cert.Name}); err != nil { + return nil, fmt.Errorf("listing Servers for Certificate %s: %w", cert.Name, err) + } + + wanted := map[string]bool{} + for i := range servers.Items { + for _, ref := range servers.Items[i].Spec.PoolRefs { + wanted[ref] = true + } + } + if len(wanted) == 0 { + return dedupeSorted(names), nil + } + + pools := &openvoxv1alpha1.PoolList{} + if err := r.List(ctx, pools, client.InNamespace(cert.Namespace)); err != nil { + return nil, fmt.Errorf("listing Pools for Certificate %s: %w", cert.Name, err) + } + for i := range pools.Items { + pool := &pools.Items[i] + if !wanted[pool.Name] || !pool.DeletionTimestamp.IsZero() { + continue + } + route := pool.Spec.Route + if route == nil || !route.Enabled || !route.InjectDNSAltName || route.Hostname == "" { + continue + } + names = append(names, route.Hostname) + } + + return dedupeSorted(names), nil +} + +// dedupeSorted returns the names in a stable order without duplicates, so the +// signing hash does not change just because a listing order did. +func dedupeSorted(names []string) []string { + sort.Strings(names) + return slices.Compact(names) +} + +// enqueueCertificatesForPool reaches the Certificates whose effective alt names +// a Pool contributes to. Without it a route hostname added later would not be +// picked up until something else touched the Certificate. +func certificatesForPool(c client.Client) handler.MapFunc { + return func(ctx context.Context, obj client.Object) []reconcile.Request { + pool, ok := obj.(*openvoxv1alpha1.Pool) + if !ok { + return nil + } + servers := &openvoxv1alpha1.ServerList{} + if err := c.List(ctx, servers, client.InNamespace(pool.Namespace)); err != nil { + return nil + } + seen := map[string]bool{} + var reqs []reconcile.Request + for i := range servers.Items { + s := &servers.Items[i] + if s.Spec.CertificateRef == "" || seen[s.Spec.CertificateRef] { + continue + } + if !slices.Contains(s.Spec.PoolRefs, pool.Name) { + continue + } + seen[s.Spec.CertificateRef] = true + reqs = append(reqs, reconcile.Request{NamespacedName: client.ObjectKey{ + Name: s.Spec.CertificateRef, Namespace: s.Namespace}}) + } + return reqs + } +} + +// enqueueCertificatesForServerPools covers the other direction: a Server that +// joins or leaves a Pool changes which hostnames its Certificate carries. +func certificatesForServerPools() handler.MapFunc { + return func(_ context.Context, obj client.Object) []reconcile.Request { + server, ok := obj.(*openvoxv1alpha1.Server) + if !ok || server.Spec.CertificateRef == "" { + return nil + } + return []reconcile.Request{{NamespacedName: client.ObjectKey{ + Name: server.Spec.CertificateRef, Namespace: server.Namespace}}} + } +} diff --git a/internal/controller/certificate_derived_test.go b/internal/controller/certificate_derived_test.go new file mode 100644 index 00000000..6e25948d --- /dev/null +++ b/internal/controller/certificate_derived_test.go @@ -0,0 +1,190 @@ +package controller + +import ( + "slices" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// TestEffectiveDNSAltNames_AddsTheRouteHostname is the replacement for the +// Pool's former write into the Certificate spec. +func TestEffectiveDNSAltNames_AddsTheRouteHostname(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + cert.Spec.DNSAltNames = []string{"web.example.com"} + + server := newServer("web") + server.Spec.CertificateRef = "web-cert" + server.Spec.PoolRefs = []string{"puppet"} + + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) + pool.Spec.Route.InjectDNSAltName = true + + r := newCertificateReconciler(setupTestClient(cert, server, pool)) + names, err := r.effectiveDNSAltNames(testCtx(), cert) + if err != nil { + t.Fatalf("deriving alt names: %v", err) + } + if !slices.Contains(names, "puppet.example.com") { + t.Errorf("expected the route hostname among the effective names, got %v", names) + } + if !slices.Contains(names, "web.example.com") { + t.Errorf("the Certificate's own names must survive, got %v", names) + } +} + +// TestEffectiveDNSAltNames_LeavesTheSpecAlone is the point of the redesign: the +// derivation reads, it never writes into a resource it does not own. +func TestEffectiveDNSAltNames_LeavesTheSpecAlone(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + server := newServer("web") + server.Spec.CertificateRef = "web-cert" + server.Spec.PoolRefs = []string{"puppet"} + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) + pool.Spec.Route.InjectDNSAltName = true + + c := setupTestClient(cert, server, pool) + r := newCertificateReconciler(c) + if _, err := r.effectiveDNSAltNames(testCtx(), cert); err != nil { + t.Fatalf("deriving alt names: %v", err) + } + + got := &openvoxv1alpha1.Certificate{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "web-cert", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading the Certificate back: %v", err) + } + if len(got.Spec.DNSAltNames) != 0 { + t.Errorf("the derivation must not write into the spec, got %v", got.Spec.DNSAltNames) + } +} + +// TestEffectiveDNSAltNames_IsIdempotent covers what made the old design churn +// under GitOps: repeating the operation must not change the result. +func TestEffectiveDNSAltNames_IsIdempotent(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + cert.Spec.DNSAltNames = []string{"puppet.example.com"} // already present + server := newServer("web") + server.Spec.CertificateRef = "web-cert" + server.Spec.PoolRefs = []string{"puppet"} + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) + pool.Spec.Route.InjectDNSAltName = true + + r := newCertificateReconciler(setupTestClient(cert, server, pool)) + first, err := r.effectiveDNSAltNames(testCtx(), cert) + if err != nil { + t.Fatalf("deriving alt names: %v", err) + } + second, err := r.effectiveDNSAltNames(testCtx(), cert) + if err != nil { + t.Fatalf("deriving alt names again: %v", err) + } + if !slices.Equal(first, second) { + t.Errorf("the derivation must be stable, got %v then %v", first, second) + } + if len(first) != 1 { + t.Errorf("a name already in the spec must not be duplicated, got %v", first) + } +} + +// TestEffectiveDNSAltNames_IgnoresPoolsWithoutInjection keeps the API field +// meaningful: it now says "is taken into account" rather than "mutates". +func TestEffectiveDNSAltNames_IgnoresPoolsWithoutInjection(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + server := newServer("web") + server.Spec.CertificateRef = "web-cert" + server.Spec.PoolRefs = []string{"puppet"} + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) // InjectDNSAltName stays false + + r := newCertificateReconciler(setupTestClient(cert, server, pool)) + names, err := r.effectiveDNSAltNames(testCtx(), cert) + if err != nil { + t.Fatalf("deriving alt names: %v", err) + } + if slices.Contains(names, "puppet.example.com") { + t.Errorf("a Pool that does not ask for injection must be ignored, got %v", names) + } +} + +// TestEffectiveDNSAltNames_IgnoresUnrelatedServers bounds the lookup to Servers +// that actually use this Certificate and Pools they actually join. +func TestEffectiveDNSAltNames_IgnoresUnrelatedServers(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + + otherServer := newServer("other") + otherServer.Spec.CertificateRef = "other-cert" + otherServer.Spec.PoolRefs = []string{"puppet"} + + notJoined := newServer("web") + notJoined.Spec.CertificateRef = "web-cert" + notJoined.Spec.PoolRefs = nil + + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) + pool.Spec.Route.InjectDNSAltName = true + + r := newCertificateReconciler(setupTestClient(cert, otherServer, notJoined, pool)) + names, err := r.effectiveDNSAltNames(testCtx(), cert) + if err != nil { + t.Fatalf("deriving alt names: %v", err) + } + if slices.Contains(names, "puppet.example.com") { + t.Errorf("neither a foreign Certificate nor an unjoined Pool may contribute, got %v", names) + } +} + +// TestEffectiveDNSAltNames_IgnoresTerminatingPool keeps a Pool on its way out +// from holding a name in the certificate. +func TestEffectiveDNSAltNames_IgnoresTerminatingPool(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + server := newServer("web") + server.Spec.CertificateRef = "web-cert" + server.Spec.PoolRefs = []string{"puppet"} + + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) + pool.Spec.Route.InjectDNSAltName = true + now := metav1.Now() + pool.DeletionTimestamp = &now + pool.Finalizers = []string{"example.com/keep"} + + r := newCertificateReconciler(setupTestClient(cert, server, pool)) + names, err := r.effectiveDNSAltNames(testCtx(), cert) + if err != nil { + t.Fatalf("deriving alt names: %v", err) + } + if slices.Contains(names, "puppet.example.com") { + t.Errorf("a terminating Pool must not contribute, got %v", names) + } +} + +// TestEnqueueCertificatesForPool covers the watch that makes a later route +// hostname reach the Certificate at all. +func TestEnqueueCertificatesForPool(t *testing.T) { + server := newServer("web") + server.Spec.CertificateRef = "web-cert" + server.Spec.PoolRefs = []string{"puppet"} + unrelated := newServer("other") + unrelated.Spec.CertificateRef = "other-cert" + unrelated.Spec.PoolRefs = []string{"different-pool"} + pool := newPool("puppet", withRoute(true, "puppet.example.com", "gw")) + + c := setupTestClient(server, unrelated, pool) + got := certificatesForPool(c)(testCtx(), pool) + if !equalNames(got, "web-cert") { + t.Errorf("expected only the Certificate of the Server joining this Pool, got %v", names(got)) + } +} + +// TestSigningSpecHash_TracksTheEffectiveNames is what makes a newly joined Pool +// actually trigger a re-sign. +func TestSigningSpecHash_TracksTheEffectiveNames(t *testing.T) { + cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) + + withoutPool := signingSpecHash(cert, cert.Spec.DNSAltNames) + withPool := signingSpecHash(cert, []string{"puppet.example.com"}) + + if withoutPool == withPool { + t.Error("a derived alt name must change the signing hash, otherwise no re-sign happens") + } +} diff --git a/internal/controller/certificate_signing.go b/internal/controller/certificate_signing.go index b53fbd85..6669ac36 100644 --- a/internal/controller/certificate_signing.go +++ b/internal/controller/certificate_signing.go @@ -218,7 +218,11 @@ func (r *CertificateReconciler) submitCSR(ctx context.Context, cert *openvoxv1al } // Build CSR - csrPEM, err := buildCSR(certname, cert.Spec.DNSAltNames, cert.Spec.CSRExtensions, privateKey) + altNames, err := r.effectiveDNSAltNames(ctx, cert) + if err != nil { + return ctrl.Result{}, err + } + csrPEM, err := buildCSR(certname, altNames, cert.Spec.CSRExtensions, privateKey) if err != nil { return ctrl.Result{}, err } @@ -617,7 +621,11 @@ func (r *CertificateReconciler) renewCertificate(ctx context.Context, cert *open return fmt.Errorf("parsing existing key: %w", err) } - csrPEM, err := buildCSR(certname, cert.Spec.DNSAltNames, cert.Spec.CSRExtensions, privateKey) + altNames, err := r.effectiveDNSAltNames(ctx, cert) + if err != nil { + return err + } + csrPEM, err := buildCSR(certname, altNames, cert.Spec.CSRExtensions, privateKey) if err != nil { return fmt.Errorf("building CSR for %s: %w", certname, err) } @@ -682,9 +690,14 @@ func (r *CertificateReconciler) renewCertificate(ctx context.Context, cert *open // Certificate phase. Setting Signed first avoids a race where the // Server sees the stale Renewing phase and transitions to Pending. notAfter := parseCertNotAfter(ctx, body) + hash, effective, hashErr := r.signingSpecHashFor(ctx, cert) + if hashErr != nil { + return hashErr + } if err := updateStatusWithRetry(ctx, r.Client, cert, func() { cert.Status.ObservedGeneration = cert.Generation - cert.Status.SignedSpecHash = signingSpecHash(cert) + cert.Status.SignedSpecHash = hash + cert.Status.EffectiveDNSAltNames = effective cert.Status.Phase = openvoxv1alpha1.CertificatePhaseSigned cert.Status.SecretName = tlsSecretName cert.Status.NotAfter = notAfter diff --git a/internal/controller/certificate_spec_drift_test.go b/internal/controller/certificate_spec_drift_test.go index b96a7884..f9fc6865 100644 --- a/internal/controller/certificate_spec_drift_test.go +++ b/internal/controller/certificate_spec_drift_test.go @@ -16,7 +16,7 @@ import ( func signedCert(name string, notAfter time.Time) *openvoxv1alpha1.Certificate { cert := newCertificate(name, "production-ca", openvoxv1alpha1.CertificatePhaseSigned) cert.Spec.DNSAltNames = []string{"puppet.example.com"} - cert.Status.SignedSpecHash = signingSpecHash(cert) + cert.Status.SignedSpecHash = specHash(cert) t := metav1.NewTime(notAfter) cert.Status.NotAfter = &t return cert @@ -26,7 +26,7 @@ func TestSigningSpecHash(t *testing.T) { base := signedCert("web", time.Now().Add(365*24*time.Hour)) t.Run("stable across equal specs", func(t *testing.T) { - if signingSpecHash(base.DeepCopy()) != signingSpecHash(base) { + if specHash(base.DeepCopy()) != specHash(base) { t.Error("two certificates with the same spec must hash the same") } }) @@ -36,7 +36,7 @@ func TestSigningSpecHash(t *testing.T) { a.Spec.DNSAltNames = []string{"a.example.com", "b.example.com"} b := base.DeepCopy() b.Spec.DNSAltNames = []string{"b.example.com", "a.example.com"} - if signingSpecHash(a) != signingSpecHash(b) { + if specHash(a) != specHash(b) { t.Error("reordering dnsAltNames must not change the hash") } }) @@ -44,7 +44,7 @@ func TestSigningSpecHash(t *testing.T) { t.Run("changes with certname", func(t *testing.T) { other := base.DeepCopy() other.Spec.Certname = "different" - if signingSpecHash(other) == signingSpecHash(base) { + if specHash(other) == specHash(base) { t.Error("certname must affect the hash") } }) @@ -52,7 +52,7 @@ func TestSigningSpecHash(t *testing.T) { t.Run("changes with a new alt name", func(t *testing.T) { other := base.DeepCopy() other.Spec.DNSAltNames = append(other.Spec.DNSAltNames, "extra.example.com") - if signingSpecHash(other) == signingSpecHash(base) { + if specHash(other) == specHash(base) { t.Error("an added alt name must affect the hash") } }) @@ -60,7 +60,7 @@ func TestSigningSpecHash(t *testing.T) { t.Run("changes with csr extensions", func(t *testing.T) { other := base.DeepCopy() other.Spec.CSRExtensions = &openvoxv1alpha1.CSRExtensionsSpec{PpRole: "compiler"} - if signingSpecHash(other) == signingSpecHash(base) { + if specHash(other) == specHash(base) { t.Error("csrExtensions must affect the hash") } }) @@ -68,7 +68,7 @@ func TestSigningSpecHash(t *testing.T) { t.Run("ignores renewBefore", func(t *testing.T) { other := base.DeepCopy() other.Spec.RenewBefore = "10d" - if signingSpecHash(other) != signingSpecHash(base) { + if specHash(other) != specHash(base) { t.Error("renewBefore only moves the renewal point and must not affect the hash") } }) @@ -184,7 +184,7 @@ func TestReconcile_AdoptsMissingHashWithoutResigning(t *testing.T) { if got.Status.Phase != openvoxv1alpha1.CertificatePhaseSigned { t.Errorf("a missing hash must be adopted, not re-signed; phase is %s", got.Status.Phase) } - if got.Status.SignedSpecHash != signingSpecHash(got) { + if got.Status.SignedSpecHash != specHash(got) { t.Errorf("the current spec hash should have been recorded, got %q", got.Status.SignedSpecHash) } } diff --git a/internal/controller/ownership_test.go b/internal/controller/ownership_test.go index 26e97945..84216574 100644 --- a/internal/controller/ownership_test.go +++ b/internal/controller/ownership_test.go @@ -150,14 +150,14 @@ func drain(rec *events.FakeRecorder) int { } } -// TestPoolInjectDNSAltName_TriggersResignWithoutStatusWrite is the point of -// decoupling the two controllers: the Pool changes the Certificate spec and -// nothing else. The re-signing follows from the Certificate controller noticing -// its own spec drift, not from a foreign status write. -func TestPoolInjectDNSAltName_TriggersResignWithoutStatusWrite(t *testing.T) { +// TestPoolRouteHostname_IsDerivedNotWritten replaces the old injection test. +// The Pool no longer writes into the Certificate spec: the hostname is derived +// on read, so the Certificate's spec stays owned by whoever wrote it and a +// GitOps controller has nothing to revert. +func TestPoolRouteHostname_IsDerivedNotWritten(t *testing.T) { ca := newCertificateAuthority("production-ca") cert := newCertificate("web-cert", "production-ca", openvoxv1alpha1.CertificatePhaseSigned) - cert.Status.SignedSpecHash = signingSpecHash(cert) + cert.Status.SignedSpecHash = specHash(cert) notAfter := metav1.NewTime(metav1.Now().Add(365 * 24 * time.Hour)) cert.Status.NotAfter = ¬After @@ -169,25 +169,32 @@ func TestPoolInjectDNSAltName_TriggersResignWithoutStatusWrite(t *testing.T) { pool.Spec.Route.InjectDNSAltName = true c := setupTestClient(ca, cert, server, pool) - pr := newPoolReconciler(c, true) - if err := pr.injectDNSAltNames(testCtx(), pool); err != nil { - t.Fatalf("injecting alt names: %v", err) + if _, err := newPoolReconciler(c, true).Reconcile(testCtx(), testRequest("puppet")); err != nil { + t.Fatalf("reconciling the Pool: %v", err) } key := types.NamespacedName{Name: "web-cert", Namespace: testNamespace} - afterInject := &openvoxv1alpha1.Certificate{} - if err := c.Get(testCtx(), key, afterInject); err != nil { + after := &openvoxv1alpha1.Certificate{} + if err := c.Get(testCtx(), key, after); err != nil { t.Fatalf("reading Certificate: %v", err) } + if slices.Contains(after.Spec.DNSAltNames, "puppet.example.com") { + t.Error("the Pool must not write into the Certificate spec any more") + } - if !slices.Contains(afterInject.Spec.DNSAltNames, "puppet.example.com") { - t.Fatalf("the hostname should have been added to the spec, got %v", afterInject.Spec.DNSAltNames) + // The Certificate controller derives the same name instead. + cr := newCertificateReconciler(c) + names, err := cr.effectiveDNSAltNames(testCtx(), after) + if err != nil { + t.Fatalf("deriving alt names: %v", err) } - if afterInject.Status.Phase != openvoxv1alpha1.CertificatePhaseSigned { - t.Errorf("the Pool must not touch the Certificate status, phase is %q", afterInject.Status.Phase) + if !slices.Contains(names, "puppet.example.com") { + t.Errorf("the route hostname must be part of the effective names, got %v", names) } - if afterInject.Status.SignedSpecHash == signingSpecHash(afterInject) { - t.Error("the recorded hash should now differ from the spec, which is what makes the controller re-sign") + + // And the drift it causes is what makes the controller re-sign. + if after.Status.SignedSpecHash == signingSpecHash(after, names) { + t.Error("the recorded hash should differ once the hostname joins the effective names") } } diff --git a/internal/controller/pool_controller.go b/internal/controller/pool_controller.go index 2bd0dddb..ca9041b7 100644 --- a/internal/controller/pool_controller.go +++ b/internal/controller/pool_controller.go @@ -3,7 +3,6 @@ package controller import ( "context" "fmt" - "slices" corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" @@ -112,11 +111,6 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. return ctrl.Result{}, fmt.Errorf("reconciling TLSRoute: %w", err) } - if pool.Spec.Route.InjectDNSAltName { - if err := r.injectDNSAltNames(ctx, pool); err != nil { - return ctrl.Result{}, fmt.Errorf("injecting DNS alt names: %w", err) - } - } } } } else if r.GatewayAPIAvailable { @@ -488,54 +482,3 @@ func (r *PoolReconciler) reconcileTLSRoute(ctx context.Context, pool *openvoxv1a } return nil } - -func (r *PoolReconciler) injectDNSAltNames(ctx context.Context, pool *openvoxv1alpha1.Pool) error { - logger := log.FromContext(ctx) - - servers := &openvoxv1alpha1.ServerList{} - if err := r.List(ctx, servers, client.InNamespace(pool.Namespace)); err != nil { - return fmt.Errorf("listing Servers: %w", err) - } - - hostname := pool.Spec.Route.Hostname - - for i := range servers.Items { - server := &servers.Items[i] - - if !slices.Contains(server.Spec.PoolRefs, pool.Name) { - continue - } - - if server.Spec.CertificateRef == "" { - continue - } - - cert := &openvoxv1alpha1.Certificate{} - if err := r.Get(ctx, types.NamespacedName{ - Name: server.Spec.CertificateRef, - Namespace: pool.Namespace, - }, cert); err != nil { - if errors.IsNotFound(err) { - continue - } - return fmt.Errorf("getting Certificate %s: %w", server.Spec.CertificateRef, err) - } - - if slices.Contains(cert.Spec.DNSAltNames, hostname) { - continue - } - - logger.Info("injecting DNS alt name into Certificate", - "certificate", cert.Name, "hostname", hostname) - r.Recorder.Eventf(pool, nil, corev1.EventTypeNormal, EventReasonDNSAltNameInjected, "Reconcile", "Injected DNS alt name %s into Certificate %s (triggers re-signing)", hostname, cert.Name) - cert.Spec.DNSAltNames = append(cert.Spec.DNSAltNames, hostname) - if err := r.Update(ctx, cert); err != nil { - return fmt.Errorf("updating Certificate %s: %w", cert.Name, err) - } - // The Certificate controller notices the changed alt names through its - // signed-spec hash and re-signs on its own. Resetting its phase from - // here would race the controller that owns that status. - } - - return nil -} diff --git a/internal/controller/pool_controller_test.go b/internal/controller/pool_controller_test.go index a3e1b2c3..1eb76a1a 100644 --- a/internal/controller/pool_controller_test.go +++ b/internal/controller/pool_controller_test.go @@ -301,96 +301,3 @@ func TestPoolReconcile_TLSRouteCustomPort(t *testing.T) { t.Errorf("expected port 9140 on backend ref") } } - -func TestPoolReconcile_InjectDNSAltNames(t *testing.T) { - pool := newPool("puppet", withRoute(true, "puppet.example.com", "my-gw")) - server := newServer("srv1", withPoolRefs("puppet")) - cert := newCertificate("production-cert", "test-ca", openvoxv1alpha1.CertificatePhaseSigned) - - c := setupTestClient(pool, server, cert) - r := newPoolReconciler(c, true) - - if err := r.injectDNSAltNames(testCtx(), pool); err != nil { - t.Fatalf("injectDNSAltNames: %v", err) - } - - updatedCert := &openvoxv1alpha1.Certificate{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "production-cert", Namespace: testNamespace}, updatedCert); err != nil { - t.Fatalf("failed to get Certificate: %v", err) - } - - found := false - for _, san := range updatedCert.Spec.DNSAltNames { - if san == "puppet.example.com" { - found = true - break - } - } - if !found { - t.Errorf("expected DNS alt name 'puppet.example.com' to be injected, got %v", updatedCert.Spec.DNSAltNames) - } -} - -func TestPoolReconcile_InjectDNSAltNames_AlreadyPresent(t *testing.T) { - pool := newPool("puppet", withRoute(true, "puppet.example.com", "my-gw")) - server := newServer("srv1", withPoolRefs("puppet")) - cert := newCertificate("production-cert", "test-ca", openvoxv1alpha1.CertificatePhaseSigned) - cert.Spec.DNSAltNames = []string{"puppet.example.com"} - - c := setupTestClient(pool, server, cert) - r := newPoolReconciler(c, true) - - if err := r.injectDNSAltNames(testCtx(), pool); err != nil { - t.Fatalf("injectDNSAltNames: %v", err) - } - - updatedCert := &openvoxv1alpha1.Certificate{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "production-cert", Namespace: testNamespace}, updatedCert); err != nil { - t.Fatalf("failed to get Certificate: %v", err) - } - - count := 0 - for _, san := range updatedCert.Spec.DNSAltNames { - if san == "puppet.example.com" { - count++ - } - } - if count != 1 { - t.Errorf("expected exactly 1 occurrence of DNS alt name, got %d in %v", count, updatedCert.Spec.DNSAltNames) - } -} - -func TestPoolReconcile_InjectDNSAltNames_NoCertRef(t *testing.T) { - pool := newPool("puppet", withRoute(true, "puppet.example.com", "my-gw")) - server := newServer("srv1", withPoolRefs("puppet")) - server.Spec.CertificateRef = "" // no cert - - c := setupTestClient(pool, server) - r := newPoolReconciler(c, true) - - if err := r.injectDNSAltNames(testCtx(), pool); err != nil { - t.Fatalf("injectDNSAltNames should succeed when server has no cert ref: %v", err) - } -} - -func TestPoolReconcile_InjectDNSAltNames_ServerNotInPool(t *testing.T) { - pool := newPool("puppet", withRoute(true, "puppet.example.com", "my-gw")) - server := newServer("srv1", withPoolRefs("other-pool")) - cert := newCertificate("production-cert", "test-ca", openvoxv1alpha1.CertificatePhaseSigned) - - c := setupTestClient(pool, server, cert) - r := newPoolReconciler(c, true) - - if err := r.injectDNSAltNames(testCtx(), pool); err != nil { - t.Fatalf("injectDNSAltNames: %v", err) - } - - // Cert should be unchanged - updatedCert := &openvoxv1alpha1.Certificate{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "production-cert", Namespace: testNamespace}, updatedCert); err != nil { - t.Fatalf("failed to get Certificate: %v", err) - } - if len(updatedCert.Spec.DNSAltNames) != 0 { - t.Errorf("expected no DNS alt names injected, got %v", updatedCert.Spec.DNSAltNames) - } -} diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index 3e56b64a..b5ea1951 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -645,3 +645,10 @@ func ownedBy(owner, obj client.Object) client.Object { } return obj } + +// specHash hashes a Certificate against its own spec alt names. Tests that do +// not involve Pools want exactly that; the production path derives the names +// from the Pools the Servers join. +func specHash(cert *openvoxv1alpha1.Certificate) string { + return signingSpecHash(cert, cert.Spec.DNSAltNames) +} diff --git a/tests/e2e/pool-gateway/chainsaw-test.yaml b/tests/e2e/pool-gateway/chainsaw-test.yaml index 5f490f5f..6ca10cd1 100644 --- a/tests/e2e/pool-gateway/chainsaw-test.yaml +++ b/tests/e2e/pool-gateway/chainsaw-test.yaml @@ -81,12 +81,14 @@ spec: - name: Verify DNS alt name injected into Certificate description: | - The full expected set, in the order the chart and the operator build it: - the server name, the certname, then the Service name of every Pool the - server joins, and finally the route hostname the Pool controller - injects. This server has poolRefs [ca, server], so it is reachable - through both Services and needs both names; the ca Pool's Service name - equals the server name and is deduplicated away. + The spec holds what the chart wrote: the server name, the certname and + the Service name of every Pool the server joins. This server has + poolRefs [ca, server], so it needs both Service names; the ca Pool's + name equals the server name and is deduplicated away. + + The route hostname is no longer written into the spec. It is derived + into status.effectiveDNSAltNames, which is what the certificate is + actually issued for - sorted, so the signing hash is stable. try: - assert: resource: @@ -99,6 +101,11 @@ spec: - openvox-stack-gw-ca - puppet - openvox-stack-gw-server + status: + effectiveDNSAltNames: + - openvox-stack-gw-ca + - openvox-stack-gw-server + - puppet - puppet.e2e.example.com - name: Verify no TLSRoute for CA pool From 87313d12aca19b4e98d8a384ff7553d424997c4c Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:25:08 +0000 Subject: [PATCH 8/9] chore(deps): update dependency conforma/cli to v0.10.2 --- .github/workflows/_conforma-validate.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/_conforma-validate.yaml b/.github/workflows/_conforma-validate.yaml index 7f60b0f7..e2ee867c 100644 --- a/.github/workflows/_conforma-validate.yaml +++ b/.github/workflows/_conforma-validate.yaml @@ -27,7 +27,7 @@ jobs: packages: read env: # renovate: datasource=github-releases depName=conforma/cli - EC_VERSION: "0.10.1" + EC_VERSION: "0.10.2" steps: - name: Checkout uses: actions/checkout@v7 From 41f2dfff589716a95af9ef4eba90de4b8dc9cdf7 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Thu, 3 Sep 2026 20:06:11 +0200 Subject: [PATCH 9/9] ci: keep breaking changes at a minor bump while below 1.0 The CRDs are v1alpha1 and the project is deliberately pre-1.0, so a breaking change is expected rather than exceptional. semantic-release did not know that: three BREAKING CHANGE footers in the API hardening work turned the release of 2026-09-02 into 1.0.0, which signals a stability promise that is not intended yet. Map breaking changes to a minor bump so the intent lives in the repository instead of in everyone's head. Remove this rule when 1.0.0 is a deliberate decision; from then on a breaking change should mean what it says. Verified with semantic-release --dry-run against a clone whose v1.0.0 tag was removed: with the rule the same 74 commits produce 0.12.0, without it 1.0.0. --- .releaserc.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.releaserc.json b/.releaserc.json index 7734bd3d..49069b62 100644 --- a/.releaserc.json +++ b/.releaserc.json @@ -1,7 +1,11 @@ { "branches": ["main"], "plugins": [ - "@semantic-release/commit-analyzer", + ["@semantic-release/commit-analyzer", { + "releaseRules": [ + { "breaking": true, "release": "minor" } + ] + }], "@semantic-release/release-notes-generator", ["@semantic-release/github", { "successComment": false