From 85e6999ab817901b8622d7979b27534f62e8e610 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Wed, 9 Sep 2026 11:50:51 +0200 Subject: [PATCH 1/6] fix: give SigningPolicy and NodeClassifier accurate Ready status Three CRDs play the same role -- a policy resource the Config controller renders into a ConfigMap or Secret -- but only ReportProcessor owned its status. SigningPolicy and NodeClassifier had theirs written by the Config controller, from config_autosign.go and config_enc.go respectively. Both now follow the ReportProcessor pattern: a status-only observer derives Ready from whether the resource actually reached the servers, so no controller writes into another resource's status any more. The Config controller keeps ownership of the rendered Secrets and reports rendering failures as events on the Config (AutosignPolicyRenderFailed, ENCRenderFailed), matching what it already did for the report webhook. updateSigningPolicyStatus and updateNodeClassifierStatus are gone. Rendered Secrets carry an openvox.voxpupuli.org/rendered-from annotation listing the resources their content was built from and the generation each had at the time. The observers match on that rather than re-parsing the rendered file, which matters in three ways: - A failed re-render leaves the previous Secret in place. Matching on a name -- or, for enc.yaml, on the endpoint URL -- still finds the resource there, so the new, unrendered generation would report as Active. That case is RenderedConfigStale. - enc.yaml carries no resource name at all, so any spec change that keeps spec.url, an auth rotation above all, would be invisible to the observer. - The rendered schema is not declared twice, once with yaml.v3 tags on the render side and once with json tags on the parse side, where a key rename would compile cleanly and flip every resource to NotRendered in production. Cases that had no status at all before, because the Config controller only wrote status on the resources it happened to render: NoConfig for a SigningPolicy whose CertificateAuthority no Config references, NotReferenced for an unreferenced NodeClassifier, and the two override reasons for a replaced built-in binary. The override is checked before the Secret is read, since a Secret rendered before the override was set still exists, and it only counts when every referencing Config sets it. Watches and lookups: - nodeClassifiersForConfig reads nodeClassifierRef off the event object rather than re-fetching the Config. Both the old and the new object of an update run through the map function, so repointing or clearing the field enqueues the classifier that lost its Config, not only the one that gained it. - The Secret watches key off the annotation, so an unrelated Secret that merely ends in -enc does not fan out over every NodeClassifier in the namespace. - A Config whose Secret cannot be read does not mask another Config that did render the resource, so the verdict does not depend on listing order. Where several Configs render the same classifier, one still on an earlier generation holds it at RenderedConfigStale. - ObservedGeneration is captured before the observation. updateStatusWithRetry re-reads the object, so a spec edit landing in between would stamp the new generation onto a verdict derived from the old spec. - Reference lookups use the registered field indexes, with a new spec.nodeClassifierRef index for Config. findSigningPolicies, the duplicated Config filters and the twin override helpers collapse into shared functions. No API or RBAC change: config/rbac/role.yaml and the chart's ClusterRole already grant both status subresources. The kubebuilder markers narrow to what the new controllers do, and the Config controller loses its now-unused signingpolicies/status and nodeclassifiers/status markers. --- cmd/main.go | 14 + docs/reference/index.md | 18 ++ docs/reference/nodeclassifier.md | 29 +- docs/reference/signingpolicy.md | 27 +- internal/controller/config_autosign.go | 111 ++------ internal/controller/config_autosign_test.go | 34 --- internal/controller/config_controller.go | 22 +- internal/controller/config_controller_test.go | 2 +- internal/controller/config_enc.go | 63 +---- internal/controller/config_enc_test.go | 35 --- internal/controller/config_reports.go | 2 +- internal/controller/indexers.go | 10 +- .../controller/nodeclassifier_controller.go | 209 ++++++++++++++ .../nodeclassifier_controller_status_test.go | 215 +++++++++++++++ internal/controller/rendered_source.go | 122 +++++++++ internal/controller/rendered_source_test.go | 68 +++++ internal/controller/server_deployment_test.go | 2 +- .../controller/signingpolicy_controller.go | 254 ++++++++++++++++++ .../signingpolicy_controller_status_test.go | 208 ++++++++++++++ internal/controller/testutil_test.go | 12 +- 20 files changed, 1240 insertions(+), 217 deletions(-) create mode 100644 internal/controller/nodeclassifier_controller.go create mode 100644 internal/controller/nodeclassifier_controller_status_test.go create mode 100644 internal/controller/rendered_source.go create mode 100644 internal/controller/rendered_source_test.go create mode 100644 internal/controller/signingpolicy_controller.go create mode 100644 internal/controller/signingpolicy_controller_status_test.go diff --git a/cmd/main.go b/cmd/main.go index 2fd828ae..e6476bd2 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -182,6 +182,20 @@ func main() { os.Exit(1) } + if err = (&controller.SigningPolicyReconciler{ + Client: mgr.GetClient(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "SigningPolicy") + os.Exit(1) + } + + if err = (&controller.NodeClassifierReconciler{ + Client: mgr.GetClient(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "NodeClassifier") + os.Exit(1) + } + if enableWebhooks { if err := webhook.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to set up webhooks") diff --git a/docs/reference/index.md b/docs/reference/index.md index c4654701..c23be002 100644 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -169,5 +169,23 @@ of the status has not caught up with the current spec yet. | `Pool` | `Ready` | At least one ready endpoint is behind the Service | | `SigningPolicy`, `NodeClassifier`, `ReportProcessor` | `Ready` | The resource was rendered into the configuration the servers mount | +These three are policy resources: the Config controller renders them into the +ConfigMaps and Secrets it owns, and each one derives its own `Ready` from +whether it ended up in that rendered output. So a failure to render is reported +as an event on the Config, while the policy resource reports only whether it is +in effect -- including the cases where nothing references it, or where an +`autosignCommand` / `externalNodesCommand` override bypasses it entirely. The +condition's `reason` names the case; see +[SigningPolicy](signingpolicy.md#phases) and +[NodeClassifier](nodeclassifier.md#phases). + +Each rendered Secret carries an `openvox.voxpupuli.org/rendered-from` +annotation listing the resources its content was built from and the +`metadata.generation` each had at the time. That is what a policy resource +matches itself against, so `Ready` distinguishes "my current spec is in effect" +from "an earlier version of it is". A re-render that fails leaves the previous +Secret in place; the resource then reports `RenderedConfigStale` rather than +claiming the new spec reached a server. + Any resource can additionally carry `Paused` -- see [Pausing Reconciliation](../guides/pausing-reconciliation.md). diff --git a/docs/reference/nodeclassifier.md b/docs/reference/nodeclassifier.md index db7b1c84..13a6fc99 100644 --- a/docs/reference/nodeclassifier.md +++ b/docs/reference/nodeclassifier.md @@ -177,7 +177,34 @@ At most one authentication method may be configured. | Phase | Description | |---|---| | `Active` | Classifier configuration is rendered and active | -| `Error` | Configuration error (e.g. referenced Secret not found) | +| `Error` | The classifier is not in effect -- see the `Ready` condition for which case | + +The status is derived from the rendered ENC Secret, so it reports whether this +classifier actually reached a server rather than whether the resource itself is +well-formed. The `Ready` condition carries the reason: + +| Reason | Meaning | +|---|---| +| `Rendered` | The endpoint is present in the rendered Secret, at the classifier's current generation | +| `NotReferenced` | No [Config](config.md) sets `nodeClassifierRef` to this NodeClassifier, so nothing renders it | +| `OverriddenByExternalNodesCommand` | Every Config referencing it sets [`spec.puppet.externalNodesCommand`](config.md), which replaces the built-in binary and bypasses NodeClassifier resources | +| `NotRendered` | No Secret rendered from this NodeClassifier exists yet | +| `RenderedConfigStale` | A Secret was rendered from this NodeClassifier, but from an earlier generation | + +`enc.yaml` carries no resource name, so the Secret's +`openvox.voxpupuli.org/rendered-from` annotation is what ties the rendered file +back to this NodeClassifier and to the generation it was rendered at. That also +catches a Secret left over from a previous `nodeClassifierRef`, which would +otherwise read as active. Where several Configs reference the same classifier, +one Config still on an earlier generation holds the whole resource at +`RenderedConfigStale`: the current spec is not in effect everywhere yet. + +Rendering failures -- an unresolvable auth Secret, for example -- are reported +on the Config that owns the Secret, as an `ENCRenderFailed` event. Since the +failed render leaves the previous Secret untouched, the classifier reports +`RenderedConfigStale` until the edit that broke it is corrected: the servers +are still classifying against the last configuration that rendered cleanly, +which for a rotated credential is the old one. ## How It Works diff --git a/docs/reference/signingpolicy.md b/docs/reference/signingpolicy.md index 002bc8cd..dd33097f 100644 --- a/docs/reference/signingpolicy.md +++ b/docs/reference/signingpolicy.md @@ -215,7 +215,32 @@ Either `value` or `valueFrom` must be set. | Phase | Description | |---|---| | `Active` | Policy is rendered and active | -| `Error` | Policy has a configuration error (e.g. referenced Secret not found) | +| `Error` | Policy is not in effect -- see the `Ready` condition for which case | + +The status is derived from the rendered autosign policy Secret, so it reports +whether this policy actually reached the CA rather than whether the resource +itself is well-formed. The `Ready` condition carries the reason: + +| Reason | Meaning | +|---|---| +| `Rendered` | The policy is present in the rendered Secret, at its current generation | +| `CertificateAuthorityRefMissing` | `spec.certificateAuthorityRef` is empty, so the policy is bound to no CA | +| `CertificateAuthorityNotFound` | `spec.certificateAuthorityRef` points at a CertificateAuthority that does not exist | +| `NoConfig` | No [Config](config.md) references that CertificateAuthority, so nothing renders the policy | +| `OverriddenByAutosignCommand` | Every Config referencing the CA sets [`spec.puppet.autosignCommand`](config.md), which replaces the built-in binary and bypasses SigningPolicy resources | +| `NotRendered` | The Secret does not (yet) contain this policy | +| `RenderedConfigStale` | The Secret contains this policy, but as it was at an earlier generation | + +The Secret's `openvox.voxpupuli.org/rendered-from` annotation names the +policies its content was built from and the generation each was rendered at, +which is what separates `Rendered` from `RenderedConfigStale`. + +Rendering failures -- an unresolvable `csrAttributes` Secret, for example -- +are reported on the Config that owns the Secret, as an +`AutosignPolicyRenderFailed` event. Since the failed render leaves the previous +Secret untouched, the policy reports `RenderedConfigStale` until the edit that +broke it is corrected: the CA is still signing under the last policy that +rendered cleanly. ## How It Works diff --git a/internal/controller/config_autosign.go b/internal/controller/config_autosign.go index 0daf9cf5..5c1e16fe 100644 --- a/internal/controller/config_autosign.go +++ b/internal/controller/config_autosign.go @@ -6,9 +6,8 @@ import ( "sort" "strings" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "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" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -29,25 +28,6 @@ const autosignPolicyDir = "/etc/puppetlabs/puppet/autosign-policy" // with --config. const autosignPolicyPath = autosignPolicyDir + "/autosign-policy.yaml" -// findSigningPolicies returns all SigningPolicies referencing the given CA. -// -// A list error is returned rather than swallowed: an empty policy set renders -// as a deny-all policy, so treating a transient failure as "no policies" would -// overwrite a valid policy Secret and lock every agent out. -func (r *ConfigReconciler) findSigningPolicies(ctx context.Context, ca *openvoxv1alpha1.CertificateAuthority) ([]openvoxv1alpha1.SigningPolicy, error) { - list := &openvoxv1alpha1.SigningPolicyList{} - if err := r.List(ctx, list, client.InNamespace(ca.Namespace)); err != nil { - return nil, fmt.Errorf("listing SigningPolicies in namespace %s: %w", ca.Namespace, err) - } - var result []openvoxv1alpha1.SigningPolicy - for _, sp := range list.Items { - if sp.Spec.CertificateAuthorityRef == ca.Name { - result = append(result, sp) - } - } - return result, nil -} - // reconcileAutosignSecrets reconciles the autosign policy Secret for the CA referenced by this Config. func (r *ConfigReconciler) reconcileAutosignSecrets(ctx context.Context, cfg *openvoxv1alpha1.Config) error { if cfg.Spec.AuthorityRef == "" { @@ -79,27 +59,31 @@ func (r *ConfigReconciler) reconcileAutosignSecrets(ctx context.Context, cfg *op func (r *ConfigReconciler) reconcileAutosignSecret(ctx context.Context, cfg *openvoxv1alpha1.Config, ca *openvoxv1alpha1.CertificateAuthority) error { secretName := fmt.Sprintf("%s-autosign-policy", ca.Name) - policies, err := r.findSigningPolicies(ctx, ca) + policies, err := signingPoliciesForAuthority(ctx, r.Client, ca.Namespace, ca.Name) if err != nil { return err } - // Render policy config YAML + // Rendering failures are reported on the Config, which owns this Secret. The + // SigningPolicy controller derives its own status from whether its policy + // ends up in the rendered Secret. policyYAML, renderErr := r.renderAutosignPolicyConfig(ctx, cfg.Namespace, ca, policies) if renderErr != nil { + r.Recorder.Eventf(cfg, nil, corev1.EventTypeWarning, EventReasonAutosignPolicyRenderFailed, "Reconcile", + "Rendering the autosign policy for CertificateAuthority %s failed: %v", ca.Name, renderErr) return fmt.Errorf("rendering autosign policy config: %w", renderErr) } - // Update SigningPolicy status - for i := range policies { - r.updateSigningPolicyStatus(ctx, &policies[i], nil) - } - data := map[string][]byte{ "autosign-policy.yaml": []byte(policyYAML), } - return r.reconcileSecret(ctx, cfg, secretName, data) + sources := make([]renderSource, 0, len(policies)) + for i := range policies { + sources = append(sources, sourceOf(&policies[i])) + } + + return r.reconcileSecret(ctx, cfg, secretName, data, renderedFromAnnotation(sources)) } // renderAutosignPolicyConfig renders the policy config YAML that openvox-autosign reads. @@ -159,7 +143,6 @@ func (r *ConfigReconciler) renderAutosignPolicyConfig(ctx context.Context, names value, err = resolveSecretKey(ctx, r.Client, namespace, attr.ValueFrom.SecretKeyRef.Name, attr.ValueFrom.SecretKeyRef.Key) if err != nil { - r.updateSigningPolicyStatus(ctx, &p, err) return "", fmt.Errorf("resolving csrAttribute %q for policy %s: %w", attr.Name, p.Name, err) } } @@ -181,67 +164,29 @@ func renderAllowList(sb *strings.Builder, field string, allow []string) { } } -// updateSigningPolicyStatus sets the phase and condition on a SigningPolicy. -func (r *ConfigReconciler) updateSigningPolicyStatus(ctx context.Context, sp *openvoxv1alpha1.SigningPolicy, err error) { - var errMsg string - if err != nil { - errMsg = err.Error() - } - if statusErr := updateStatusWithRetry(ctx, r.Client, sp, func() { - if err != nil { - sp.Status.Phase = openvoxv1alpha1.SigningPolicyPhaseError - meta.SetStatusCondition(&sp.Status.Conditions, metav1.Condition{ - Type: openvoxv1alpha1.ConditionSigningPolicyReady, - Status: metav1.ConditionFalse, - Reason: "Error", - Message: errMsg, - ObservedGeneration: sp.Generation, - }) - } else { - sp.Status.Phase = openvoxv1alpha1.SigningPolicyPhaseActive - meta.SetStatusCondition(&sp.Status.Conditions, metav1.Condition{ - Type: openvoxv1alpha1.ConditionSigningPolicyReady, - Status: metav1.ConditionTrue, - Reason: "PolicyRendered", - Message: "Signing policy is active", - ObservedGeneration: sp.Generation, - }) - } - }); statusErr != nil { - log.FromContext(ctx).Error(statusErr, "failed to update SigningPolicy status", "name", sp.Name) - } -} - // enqueueConfigsForSigningPolicy maps SigningPolicy changes to Config reconciles. -func (r *ConfigReconciler) enqueueConfigsForSigningPolicy(c client.Reader) handler.MapFunc { +func (r *ConfigReconciler) enqueueConfigsForSigningPolicy(c client.Client) handler.MapFunc { return func(ctx context.Context, obj client.Object) []reconcile.Request { sp, ok := obj.(*openvoxv1alpha1.SigningPolicy) - if !ok { - return nil - } - - // Find the CA referenced by this SigningPolicy - ca := &openvoxv1alpha1.CertificateAuthority{} - if err := c.Get(ctx, types.NamespacedName{Name: sp.Spec.CertificateAuthorityRef, Namespace: sp.Namespace}, ca); err != nil { - log.FromContext(ctx).Error(err, "failed to get CertificateAuthority in watcher", "name", sp.Spec.CertificateAuthorityRef) + if !ok || sp.Spec.CertificateAuthorityRef == "" { return nil } - - // Enqueue all Configs whose authorityRef points to this CA - cfgList := &openvoxv1alpha1.ConfigList{} - if err := c.List(ctx, cfgList, client.InNamespace(ca.Namespace)); err != nil { + configs, err := configsReferencingAuthority(ctx, c, sp.Namespace, sp.Spec.CertificateAuthorityRef) + if err != nil { log.FromContext(ctx).Error(err, "failed to list Configs in watcher") return nil } + return configRequests(configs) + } +} - var requests []reconcile.Request - for _, cfg := range cfgList.Items { - if cfg.Spec.AuthorityRef == ca.Name { - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{Name: cfg.Name, Namespace: cfg.Namespace}, - }) - } - } - return requests +// configRequests turns a set of Configs into reconcile requests. +func configRequests(configs []openvoxv1alpha1.Config) []reconcile.Request { + requests := make([]reconcile.Request, 0, len(configs)) + for _, cfg := range configs { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: cfg.Name, Namespace: cfg.Namespace}, + }) } + return requests } diff --git a/internal/controller/config_autosign_test.go b/internal/controller/config_autosign_test.go index 2e9f8533..bb2c10c3 100644 --- a/internal/controller/config_autosign_test.go +++ b/internal/controller/config_autosign_test.go @@ -1,12 +1,10 @@ package controller import ( - "fmt" "strings" "testing" 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" @@ -285,38 +283,6 @@ func TestRenderAutosignPolicyConfig_SortOrder(t *testing.T) { } } -func TestUpdateSigningPolicyStatus_Success(t *testing.T) { - sp := newSigningPolicy("test-policy", "test-ca", true) - c := setupTestClient(sp) - r := newConfigReconciler(c) - - r.updateSigningPolicyStatus(testCtx(), sp, nil) - - updated := &openvoxv1alpha1.SigningPolicy{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "test-policy", Namespace: testNamespace}, updated); err != nil { - t.Fatalf("failed to get SigningPolicy: %v", err) - } - if updated.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseActive { - t.Errorf("expected phase %q, got %q", openvoxv1alpha1.SigningPolicyPhaseActive, updated.Status.Phase) - } -} - -func TestUpdateSigningPolicyStatus_Error(t *testing.T) { - sp := newSigningPolicy("test-policy", "test-ca", true) - c := setupTestClient(sp) - r := newConfigReconciler(c) - - r.updateSigningPolicyStatus(testCtx(), sp, fmt.Errorf("rendering failed")) - - updated := &openvoxv1alpha1.SigningPolicy{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "test-policy", Namespace: testNamespace}, updated); err != nil { - t.Fatalf("failed to get SigningPolicy: %v", err) - } - if updated.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseError { - t.Errorf("expected phase %q, got %q", openvoxv1alpha1.SigningPolicyPhaseError, updated.Status.Phase) - } -} - // TestRenderAutosignPolicy_ReservesTheOperatorCertname is the operator half of // the escalation guard. The CA auth.conf grants admin rights to this certname, // so the rendered policy has to tell the autosign binary never to hand it out. diff --git a/internal/controller/config_controller.go b/internal/controller/config_controller.go index 01773dec..94323cf2 100644 --- a/internal/controller/config_controller.go +++ b/internal/controller/config_controller.go @@ -29,17 +29,22 @@ type ConfigReconciler struct { } // Event reasons for Config. +// +// The Config owns every rendered ConfigMap and Secret, so a rendering failure +// is reported here rather than on the SigningPolicy, NodeClassifier or +// ReportProcessor it was rendered from -- those report only whether they made +// it into the rendered output. const ( - EventReasonReportWebhookRenderFailed = "ReportWebhookRenderFailed" + EventReasonReportWebhookRenderFailed = "ReportWebhookRenderFailed" + EventReasonAutosignPolicyRenderFailed = "AutosignPolicyRenderFailed" + EventReasonENCRenderFailed = "ENCRenderFailed" ) // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=configs,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=configs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=configs/finalizers,verbs=update // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=signingpolicies,verbs=get;list;watch -// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=signingpolicies/status,verbs=get;update;patch // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=nodeclassifiers,verbs=get;list;watch -// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=nodeclassifiers/status,verbs=get;update;patch // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=databases,verbs=get;list;watch // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=certificateauthorities,verbs=get;list;watch // +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=reportprocessors,verbs=get;list;watch @@ -207,7 +212,13 @@ func (r *ConfigReconciler) reconcileConfigMap(ctx context.Context, cfg *openvoxv } // reconcileSecret creates or updates a Secret owned by the given Config. -func (r *ConfigReconciler) reconcileSecret(ctx context.Context, cfg *openvoxv1alpha1.Config, name string, data map[string][]byte) error { +// +// annotations are merged into whatever the Secret already carries. The +// AnnotationRenderedFrom entry is what the status controllers read to tell +// which resources the current content came from; a Secret no status controller +// observes passes nil. +func (r *ConfigReconciler) reconcileSecret(ctx context.Context, cfg *openvoxv1alpha1.Config, name string, + data map[string][]byte, annotations map[string]string) error { logger := log.FromContext(ctx) secret := &corev1.Secret{ @@ -218,6 +229,9 @@ func (r *ConfigReconciler) reconcileSecret(ctx context.Context, cfg *openvoxv1al return err } secret.Labels = configLabels(cfg.Name) + for k, v := range annotations { + metav1.SetMetaDataAnnotation(&secret.ObjectMeta, k, v) + } secret.Data = data return controllerutil.SetControllerReference(cfg, secret, r.Scheme) }) diff --git a/internal/controller/config_controller_test.go b/internal/controller/config_controller_test.go index b7d0ed1f..d9fabf11 100644 --- a/internal/controller/config_controller_test.go +++ b/internal/controller/config_controller_test.go @@ -247,7 +247,7 @@ func TestConfigReconcile_PuppetConfWithENC(t *testing.T) { func TestConfigReconcile_AutosignCommandOverride(t *testing.T) { cfg := newConfig("production", withAuthorityRef("production-ca"), - withAutosignCommand("/usr/local/bin/custom-autosign"), + withAutosignCommand(), ) ca := newCertificateAuthority("production-ca") c := setupTestClient(cfg, ca) diff --git a/internal/controller/config_enc.go b/internal/controller/config_enc.go index c2e85eaa..d21f2ee2 100644 --- a/internal/controller/config_enc.go +++ b/internal/controller/config_enc.go @@ -4,9 +4,8 @@ import ( "context" "fmt" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - "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" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -39,20 +38,23 @@ func (r *ConfigReconciler) reconcileENCSecret(ctx context.Context, cfg *openvoxv return fmt.Errorf("getting NodeClassifier %s: %w", cfg.Spec.NodeClassifierRef, err) } + // Rendering failures are reported on the Config, which owns this Secret. The + // NodeClassifier controller derives its own status from whether its endpoint + // ends up in the rendered Secret. encYAML, renderErr := r.renderENCConfig(ctx, cfg, nc) if renderErr != nil { - r.updateNodeClassifierStatus(ctx, nc, renderErr) + r.Recorder.Eventf(cfg, nil, corev1.EventTypeWarning, EventReasonENCRenderFailed, "Reconcile", + "Rendering the ENC configuration from NodeClassifier %s failed: %v", nc.Name, renderErr) return fmt.Errorf("rendering ENC config: %w", renderErr) } - r.updateNodeClassifierStatus(ctx, nc, nil) - secretName := fmt.Sprintf("%s-enc", cfg.Name) data := map[string][]byte{ "enc.yaml": []byte(encYAML), } - return r.reconcileSecret(ctx, cfg, secretName, data) + return r.reconcileSecret(ctx, cfg, secretName, data, + renderedFromAnnotation([]renderSource{sourceOf(nc)})) } // encYAMLConfig mirrors the YAML structure read by openvox-enc. @@ -165,59 +167,18 @@ func (r *ConfigReconciler) renderENCConfig(ctx context.Context, cfg *openvoxv1al return string(out), nil } -// updateNodeClassifierStatus sets the phase and condition on a NodeClassifier. -func (r *ConfigReconciler) updateNodeClassifierStatus(ctx context.Context, nc *openvoxv1alpha1.NodeClassifier, err error) { - var errMsg string - if err != nil { - errMsg = err.Error() - } - if statusErr := updateStatusWithRetry(ctx, r.Client, nc, func() { - if err != nil { - nc.Status.Phase = openvoxv1alpha1.NodeClassifierPhaseError - meta.SetStatusCondition(&nc.Status.Conditions, metav1.Condition{ - Type: openvoxv1alpha1.ConditionNodeClassifierReady, - Status: metav1.ConditionFalse, - Reason: "Error", - Message: errMsg, - ObservedGeneration: nc.Generation, - }) - } else { - nc.Status.Phase = openvoxv1alpha1.NodeClassifierPhaseActive - meta.SetStatusCondition(&nc.Status.Conditions, metav1.Condition{ - Type: openvoxv1alpha1.ConditionNodeClassifierReady, - Status: metav1.ConditionTrue, - Reason: "ConfigRendered", - Message: "Node classifier configuration is active", - ObservedGeneration: nc.Generation, - }) - } - }); statusErr != nil { - log.FromContext(ctx).Error(statusErr, "failed to update NodeClassifier status", "name", nc.Name) - } -} - // enqueueConfigsForNodeClassifier maps NodeClassifier changes to Config reconciles. -func (r *ConfigReconciler) enqueueConfigsForNodeClassifier(c client.Reader) handler.MapFunc { +func (r *ConfigReconciler) enqueueConfigsForNodeClassifier(c client.Client) handler.MapFunc { return func(ctx context.Context, obj client.Object) []reconcile.Request { nc, ok := obj.(*openvoxv1alpha1.NodeClassifier) if !ok { return nil } - - cfgList := &openvoxv1alpha1.ConfigList{} - if err := c.List(ctx, cfgList, client.InNamespace(nc.Namespace)); err != nil { + configs, err := configsReferencingNodeClassifier(ctx, c, nc.Namespace, nc.Name) + if err != nil { log.FromContext(ctx).Error(err, "failed to list Configs in watcher") return nil } - - var requests []reconcile.Request - for _, cfg := range cfgList.Items { - if cfg.Spec.NodeClassifierRef == nc.Name { - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{Name: cfg.Name, Namespace: cfg.Namespace}, - }) - } - } - return requests + return configRequests(configs) } } diff --git a/internal/controller/config_enc_test.go b/internal/controller/config_enc_test.go index a2cca28d..34e64355 100644 --- a/internal/controller/config_enc_test.go +++ b/internal/controller/config_enc_test.go @@ -1,12 +1,9 @@ package controller import ( - "fmt" "strings" "testing" - "k8s.io/apimachinery/pkg/types" - openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" ) @@ -267,35 +264,3 @@ func TestRenderENCConfig_CacheCustomDirectory(t *testing.T) { t.Errorf("expected custom cache directory in output:\n%s", out) } } - -func TestUpdateNodeClassifierStatus_Success(t *testing.T) { - nc := newNodeClassifier("test-nc", "https://foreman.example.com") - c := setupTestClient(nc) - r := newConfigReconciler(c) - - r.updateNodeClassifierStatus(testCtx(), nc, nil) - - updated := &openvoxv1alpha1.NodeClassifier{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "test-nc", Namespace: testNamespace}, updated); err != nil { - t.Fatalf("failed to get NodeClassifier: %v", err) - } - if updated.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseActive { - t.Errorf("expected phase %q, got %q", openvoxv1alpha1.NodeClassifierPhaseActive, updated.Status.Phase) - } -} - -func TestUpdateNodeClassifierStatus_Error(t *testing.T) { - nc := newNodeClassifier("test-nc", "https://foreman.example.com") - c := setupTestClient(nc) - r := newConfigReconciler(c) - - r.updateNodeClassifierStatus(testCtx(), nc, fmt.Errorf("render failed")) - - updated := &openvoxv1alpha1.NodeClassifier{} - if err := c.Get(testCtx(), types.NamespacedName{Name: "test-nc", Namespace: testNamespace}, updated); err != nil { - t.Fatalf("failed to get NodeClassifier: %v", err) - } - if updated.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseError { - t.Errorf("expected phase %q, got %q", openvoxv1alpha1.NodeClassifierPhaseError, updated.Status.Phase) - } -} diff --git a/internal/controller/config_reports.go b/internal/controller/config_reports.go index 4b4ef518..2c40e393 100644 --- a/internal/controller/config_reports.go +++ b/internal/controller/config_reports.go @@ -69,7 +69,7 @@ func (r *ConfigReconciler) reconcileReportWebhookSecret(ctx context.Context, cfg "report-webhook.yaml": []byte(webhookYAML), } - return r.reconcileSecret(ctx, cfg, secretName, data) + return r.reconcileSecret(ctx, cfg, secretName, data, nil) } // reportWebhookConfig mirrors the YAML structure read by openvox-report. diff --git a/internal/controller/indexers.go b/internal/controller/indexers.go index ed524a66..5ea2b7b0 100644 --- a/internal/controller/indexers.go +++ b/internal/controller/indexers.go @@ -18,9 +18,10 @@ import ( // Indexes are scoped per type, so the same field name is reused where the // reference means the same thing on different resources. const ( - IndexConfigRef = "spec.configRef" - IndexCertificateRef = "spec.certificateRef" - IndexAuthorityRef = "spec.authorityRef" + IndexConfigRef = "spec.configRef" + IndexCertificateRef = "spec.certificateRef" + IndexAuthorityRef = "spec.authorityRef" + IndexNodeClassifierRef = "spec.nodeClassifierRef" // IndexCertname makes the certname collision check a lookup rather than a // full listing. A certname identifies exactly one entry on the CA, so two @@ -60,6 +61,9 @@ func fieldIndexes() []fieldIndex { {&openvoxv1alpha1.Config{}, IndexAuthorityRef, func(o client.Object) []string { return nonEmpty(o.(*openvoxv1alpha1.Config).Spec.AuthorityRef) }}, + {&openvoxv1alpha1.Config{}, IndexNodeClassifierRef, func(o client.Object) []string { + return nonEmpty(o.(*openvoxv1alpha1.Config).Spec.NodeClassifierRef) + }}, {&openvoxv1alpha1.Certificate{}, IndexAuthorityRef, func(o client.Object) []string { return nonEmpty(o.(*openvoxv1alpha1.Certificate).Spec.AuthorityRef) }}, diff --git a/internal/controller/nodeclassifier_controller.go b/internal/controller/nodeclassifier_controller.go new file mode 100644 index 00000000..a52f4fba --- /dev/null +++ b/internal/controller/nodeclassifier_controller.go @@ -0,0 +1,209 @@ +package controller + +import ( + "context" + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// NodeClassifierReconciler owns the status of NodeClassifier objects. +// +// The rendered ENC Secret belongs to the Config controller, which has the +// endpoint credentials. What this controller does is report whether a given +// NodeClassifier actually reached a server -- derived from what it can observe, +// not handed over from another controller. +type NodeClassifierReconciler struct { + client.Client +} + +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=nodeclassifiers,verbs=get;list;watch +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=nodeclassifiers/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=configs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch + +func (r *NodeClassifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + nc := &openvoxv1alpha1.NodeClassifier{} + if err := r.Get(ctx, req.NamespacedName, nc); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("getting NodeClassifier %s: %w", req.NamespacedName, err) + } + + // Pausing comes after the deletion path: a paused resource must still be + // deletable, otherwise the annotation turns into a trap. + if paused, err := reconcilePauseState(ctx, r.Client, nc, &nc.Status.Conditions); err != nil { + return ctrl.Result{}, err + } else if paused { + logger.Info("reconciliation paused by annotation", "name", nc.Name) + return ctrl.Result{}, nil + } + + // The generation the verdict is about, captured before the observation: + // updateStatusWithRetry re-reads the object, so a spec edit landing in + // between would otherwise stamp the new generation onto a verdict derived + // from the old spec. + generation := nc.Generation + + phase, reason, message := r.observe(ctx, nc) + if reason == reasonLookupFailed { + // A transient lookup failure says nothing about the NodeClassifier. + // Leave the status alone and let the backoff retry. + return ctrl.Result{}, fmt.Errorf("%s", message) + } + + if err := updateStatusWithRetry(ctx, r.Client, nc, func() { + nc.Status.ObservedGeneration = generation + nc.Status.Phase = phase + status := metav1.ConditionFalse + if phase == openvoxv1alpha1.NodeClassifierPhaseActive { + status = metav1.ConditionTrue + } + meta.SetStatusCondition(&nc.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionNodeClassifierReady, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: generation, + }) + }); err != nil { + return ctrl.Result{}, fmt.Errorf("updating NodeClassifier status %s: %w", nc.Name, err) + } + + return ctrl.Result{}, nil +} + +// observe derives the NodeClassifier state from the Configs referencing it and +// the ENC Secret rendered for them. +func (r *NodeClassifierReconciler) observe(ctx context.Context, + nc *openvoxv1alpha1.NodeClassifier) (openvoxv1alpha1.NodeClassifierPhase, string, string) { + configs, err := configsReferencingNodeClassifier(ctx, r.Client, nc.Namespace, nc.Name) + if err != nil { + return "", reasonLookupFailed, fmt.Sprintf("listing Configs for NodeClassifier %s: %v", nc.Name, err) + } + if len(configs) == 0 { + return openvoxv1alpha1.NodeClassifierPhaseError, "NotReferenced", + fmt.Sprintf("no Config sets nodeClassifierRef to %s, so no ENC configuration is rendered", nc.Name) + } + + // A custom externalNodesCommand replaces the built-in binary entirely, so + // this NodeClassifier is bypassed. Reported before the Secret is read: a + // Secret rendered before the override was set still exists, and calling that + // "active" would claim an effect this NodeClassifier no longer has. + if allOverride(configs, overrideExternalNodes) { + return openvoxv1alpha1.NodeClassifierPhaseError, "OverriddenByExternalNodesCommand", + fmt.Sprintf("spec.puppet.externalNodesCommand is set on every Config referencing NodeClassifier %s, "+ + "which bypasses NodeClassifier resources", nc.Name) + } + + var rendered, stale []string + for _, cfg := range configs { + if cfg.Spec.Puppet.ExternalNodesCommand != "" { + continue + } + secretName := fmt.Sprintf("%s-enc", cfg.Name) + secret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Name: secretName, Namespace: nc.Namespace}, secret); err != nil { + if apierrors.IsNotFound(err) { + continue + } + return "", reasonLookupFailed, fmt.Sprintf("getting Secret %s: %v", secretName, err) + } + // The annotation names the NodeClassifier the content was rendered from, + // which a Secret left over from a previous nodeClassifierRef does not + // match, and carries the generation it was rendered at. + generation, ok := renderedGeneration(secret.Annotations, nc.Name) + switch { + case !ok: + continue + case generation != nc.Generation: + stale = append(stale, secretName) + default: + rendered = append(rendered, secretName) + } + } + + // Stale outranks rendered: while any server still runs an earlier spec, the + // current one is not in effect, and reporting Ready for a generation that + // has not fully landed is the claim this controller exists to avoid. + if len(stale) > 0 { + return openvoxv1alpha1.NodeClassifierPhaseError, "RenderedConfigStale", + fmt.Sprintf("Secret %s was rendered from an earlier generation of NodeClassifier %s; "+ + "the current spec has not reached a server", strings.Join(stale, ", "), nc.Name) + } + if len(rendered) == 0 { + return openvoxv1alpha1.NodeClassifierPhaseError, "NotRendered", + fmt.Sprintf("no Secret rendered from NodeClassifier %s exists yet", nc.Name) + } + + return openvoxv1alpha1.NodeClassifierPhaseActive, "Rendered", + fmt.Sprintf("Endpoint is present in Secret %s", strings.Join(rendered, ", ")) +} + +// configsReferencingNodeClassifier returns the Configs in a namespace whose +// nodeClassifierRef points at the given NodeClassifier. +func configsReferencingNodeClassifier(ctx context.Context, c client.Client, + namespace, ncName string) ([]openvoxv1alpha1.Config, error) { + cfgList := &openvoxv1alpha1.ConfigList{} + if err := c.List(ctx, cfgList, + client.InNamespace(namespace), + client.MatchingFields{IndexNodeClassifierRef: ncName}); err != nil { + return nil, err + } + return cfgList.Items, nil +} + +func (r *NodeClassifierReconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&openvoxv1alpha1.NodeClassifier{}). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(nodeClassifiersForSecret())). + Watches(&openvoxv1alpha1.Config{}, handler.EnqueueRequestsFromMapFunc(nodeClassifiersForConfig())). + Complete(r) +} + +// nodeClassifiersForSecret maps a rendered ENC Secret back to the +// NodeClassifier it was rendered from. +func nodeClassifiersForSecret() handler.MapFunc { + return func(_ context.Context, obj client.Object) []ctrl.Request { + if !strings.HasSuffix(obj.GetName(), "-enc") { + return nil + } + return renderedSourceRequests(obj) + } +} + +// nodeClassifiersForConfig maps a Config change to the NodeClassifier it +// references. The Config decides whether the ENC configuration is rendered at +// all, so its nodeClassifierRef and externalNodesCommand are both inputs to the +// status. +// +// The reference is read off the event object rather than looked up: an update +// runs this against both the old and the new Config, so repointing or clearing +// nodeClassifierRef enqueues the NodeClassifier that just lost its Config as +// well as the one that gained it. A lookup would resolve both events to the +// current spec and leave the former reporting Active forever. +func nodeClassifiersForConfig() handler.MapFunc { + return func(_ context.Context, obj client.Object) []ctrl.Request { + cfg, ok := obj.(*openvoxv1alpha1.Config) + if !ok || cfg.Spec.NodeClassifierRef == "" { + return nil + } + return []ctrl.Request{{ + NamespacedName: types.NamespacedName{Name: cfg.Spec.NodeClassifierRef, Namespace: cfg.Namespace}, + }} + } +} diff --git a/internal/controller/nodeclassifier_controller_status_test.go b/internal/controller/nodeclassifier_controller_status_test.go new file mode 100644 index 00000000..e2fdc62d --- /dev/null +++ b/internal/controller/nodeclassifier_controller_status_test.go @@ -0,0 +1,215 @@ +package controller + +import ( + "testing" + + 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" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// encSecret builds an ENC Secret as the Config controller renders it: the +// endpoint in enc.yaml, and the NodeClassifier it came from in the annotation. +func encSecret(cfgName, url string, sources ...renderSource) *corev1.Secret { + yaml := "url: " + url + "\nmethod: GET\npath: /node/{certname}\nresponseFormat: yaml\ntimeoutSeconds: 10\n" + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfgName + "-enc", + Namespace: testNamespace, + Annotations: renderedFromAnnotation(sources), + }, + Data: map[string][]byte{"enc.yaml": []byte(yaml)}, + } +} + +func TestNodeClassifierReconcile_Status(t *testing.T) { + const encURL = "https://foreman.example.invalid" + nc := newNodeClassifier("my-enc", encURL) + nc.Generation = 2 + current := renderSource{Name: "my-enc", Generation: 2} + cfg := newConfig("production", withNodeClassifierRef()) + key := types.NamespacedName{Name: "my-enc", Namespace: testNamespace} + + t.Run("active once the ENC config is rendered", func(t *testing.T) { + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), encSecret("production", encURL, current)) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseActive { + t.Errorf("phase = %q, want Active", got.Status.Phase) + } + if !meta.IsStatusConditionTrue(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) { + t.Error("expected a true Ready condition") + } + if got.Status.ObservedGeneration != nc.Generation { + t.Errorf("observedGeneration = %d, want %d", got.Status.ObservedGeneration, nc.Generation) + } + }) + + t.Run("error when no Config references it", func(t *testing.T) { + c := setupTestClient(nc.DeepCopy()) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil || cond.Reason != "NotReferenced" { + t.Errorf("expected reason NotReferenced, got %+v", cond) + } + }) + + t.Run("error while the secret has not been rendered", func(t *testing.T) { + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy()) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil || cond.Reason != "NotRendered" { + t.Errorf("expected reason NotRendered, got %+v", cond) + } + }) + + // A Secret left over from a previous nodeClassifierRef would otherwise read + // as active: it exists, and it is named after the Config. + t.Run("error when the rendered secret belongs to another classifier", func(t *testing.T) { + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), + encSecret("production", "https://someone-else.example.invalid", + renderSource{Name: "someone-else", Generation: 1})) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil || cond.Reason != "NotRendered" { + t.Errorf("expected reason NotRendered, got %+v", cond) + } + }) + + // A failed re-render leaves the previous Secret in place. Matching on the + // name alone would report the new spec as active while the servers still run + // the old one -- including, for an auth rotation, a revoked credential. + t.Run("error when the rendered secret is from an earlier generation", func(t *testing.T) { + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), + encSecret("production", encURL, renderSource{Name: "my-enc", Generation: 1})) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil || cond.Reason != "RenderedConfigStale" { + t.Errorf("expected reason RenderedConfigStale, got %+v", cond) + } + if got.Status.ObservedGeneration != nc.Generation { + t.Errorf("observedGeneration = %d, want the generation that is not in effect (%d)", + got.Status.ObservedGeneration, nc.Generation) + } + }) + + // A stale Secret from before the override was set must not read as active: + // externalNodesCommand replaces the binary that would have consumed it. + t.Run("error when externalNodesCommand bypasses the classifier", func(t *testing.T) { + overridden := newConfig("production", + withNodeClassifierRef(), + withExternalNodesCommand("/usr/local/bin/custom-enc")) + c := setupTestClient(nc.DeepCopy(), overridden, encSecret("production", encURL, current)) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil || cond.Reason != "OverriddenByExternalNodesCommand" { + t.Errorf("expected reason OverriddenByExternalNodesCommand, got %+v", cond) + } + }) + + // A Config that has not rendered anything yet says nothing about the + // classifier, so it must not pull a Config that did render it out of Active. + t.Run("active when a second Config has not rendered yet", func(t *testing.T) { + second := newConfig("staging", withNodeClassifierRef()) + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), second, + encSecret("production", encURL, current)) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseActive { + t.Errorf("phase = %q, want Active", got.Status.Phase) + } + }) + + // Where several Configs render the same classifier, one still on an earlier + // generation holds the whole resource back -- the current spec is not in + // effect everywhere yet. Without that rule the verdict would depend on which + // Config the listing happened to return first. + t.Run("stale in one Config outranks a current render in another", func(t *testing.T) { + second := newConfig("staging", withNodeClassifierRef()) + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), second, + encSecret("production", encURL, current), + encSecret("staging", encURL, renderSource{Name: "my-enc", Generation: 1})) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil || cond.Reason != "RenderedConfigStale" { + t.Errorf("expected reason RenderedConfigStale, got %+v", cond) + } + }) +} + +// Repointing nodeClassifierRef has to enqueue the classifier that just lost its +// Config, not only the one that gained it. controller-runtime runs the map +// function against the old object as well, so reading the reference off the +// event object is what makes the old classifier drop out of Active. +func TestNodeClassifiersForConfig(t *testing.T) { + mapFn := nodeClassifiersForConfig() + + old := newConfig("production", withNodeClassifierRef()) + old.Spec.NodeClassifierRef = "old-enc" + requests := mapFn(testCtx(), old) + if len(requests) != 1 || requests[0].Name != "old-enc" { + t.Errorf("got %v, want a request for old-enc", requests) + } + + cleared := newConfig("production") + if got := mapFn(testCtx(), cleared); len(got) != 0 { + t.Errorf("got %v, want no requests for a Config without a nodeClassifierRef", got) + } +} diff --git a/internal/controller/rendered_source.go b/internal/controller/rendered_source.go new file mode 100644 index 00000000..418a0a0e --- /dev/null +++ b/internal/controller/rendered_source.go @@ -0,0 +1,122 @@ +package controller + +import ( + "fmt" + "sort" + "strconv" + "strings" + + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// AnnotationRenderedFrom records which resources produced the current content +// of a rendered Secret, as sorted "name=generation" pairs. +// +// The status controllers match on this rather than re-parsing the rendered +// file. Parsing answers a weaker question: enc.yaml carries no resource name at +// all, and a file left behind by a failed re-render still describes the old +// spec, so a resource whose current spec never reached a server would read as +// active. The annotation is written in the same update as the data, so the two +// cannot disagree, and the generation says which spec the content came from. +const AnnotationRenderedFrom = "openvox.voxpupuli.org/rendered-from" + +// renderSource identifies one resource a rendered file was built from. +type renderSource struct { + Name string + Generation int64 +} + +// sourceOf describes an object as the source of a rendered file. +func sourceOf(obj client.Object) renderSource { + return renderSource{Name: obj.GetName(), Generation: obj.GetGeneration()} +} + +// renderedFromAnnotation builds the annotation for a set of sources. An empty +// set still yields the key, so a Secret rendered from nothing overwrites what a +// previous render recorded instead of keeping a stale claim. +func renderedFromAnnotation(sources []renderSource) map[string]string { + parts := make([]string, 0, len(sources)) + for _, s := range sources { + parts = append(parts, fmt.Sprintf("%s=%d", s.Name, s.Generation)) + } + // Sorted so an unchanged render produces an unchanged value and does not + // rewrite the Secret. + sort.Strings(parts) + return map[string]string{AnnotationRenderedFrom: strings.Join(parts, ",")} +} + +// renderedGeneration returns the generation the named resource had when the +// file was rendered, and whether it contributed to it at all. +func renderedGeneration(annotations map[string]string, name string) (int64, bool) { + value, ok := annotations[AnnotationRenderedFrom] + if !ok || value == "" { + return 0, false + } + for part := range strings.SplitSeq(value, ",") { + key, gen, found := strings.Cut(part, "=") + if !found || key != name { + continue + } + parsed, err := strconv.ParseInt(gen, 10, 64) + if err != nil { + return 0, false + } + return parsed, true + } + return 0, false +} + +// renderedSourceRequests maps a rendered Secret to the resources it was +// rendered from. +// +// controller-runtime runs a map function against both the old and the new +// object of an update, so a resource dropped from a re-render is enqueued from +// the old annotation just as the one that replaced it is enqueued from the new. +// A Secret the operator did not render carries no annotation and maps to +// nothing, which keeps an unrelated Secret from fanning out over every resource +// in the namespace. +func renderedSourceRequests(obj client.Object) []ctrl.Request { + value := obj.GetAnnotations()[AnnotationRenderedFrom] + if value == "" { + return nil + } + parts := strings.Split(value, ",") + requests := make([]ctrl.Request, 0, len(parts)) + for _, part := range parts { + name, _, found := strings.Cut(part, "=") + if !found || name == "" { + continue + } + requests = append(requests, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: name, Namespace: obj.GetNamespace()}, + }) + } + return requests +} + +// allOverride reports whether every Config replaces the built-in binary that +// would consume a rendered file, as named by command. A single Config that does +// not still renders the file, so the resource is only bypassed when they all +// opt out. +func allOverride(configs []openvoxv1alpha1.Config, command func(openvoxv1alpha1.Config) string) bool { + for _, cfg := range configs { + if command(cfg) == "" { + return false + } + } + return len(configs) > 0 +} + +// overrideAutosign and overrideExternalNodes select the command that replaces +// the built-in binary for each kind of rendered file. +func overrideAutosign(cfg openvoxv1alpha1.Config) string { + return cfg.Spec.Puppet.AutosignCommand +} + +func overrideExternalNodes(cfg openvoxv1alpha1.Config) string { + return cfg.Spec.Puppet.ExternalNodesCommand +} diff --git a/internal/controller/rendered_source_test.go b/internal/controller/rendered_source_test.go new file mode 100644 index 00000000..7f361cbc --- /dev/null +++ b/internal/controller/rendered_source_test.go @@ -0,0 +1,68 @@ +package controller + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestRenderedFromAnnotationRoundTrip(t *testing.T) { + annotations := renderedFromAnnotation([]renderSource{ + {Name: "policy-b", Generation: 7}, + {Name: "policy-a", Generation: 2}, + }) + + // Sorted, so an unchanged render produces an unchanged value and does not + // rewrite the Secret. + if got := annotations[AnnotationRenderedFrom]; got != "policy-a=2,policy-b=7" { + t.Errorf("got %q, want the sources sorted by name", got) + } + + if gen, ok := renderedGeneration(annotations, "policy-b"); !ok || gen != 7 { + t.Errorf("got (%d, %v), want (7, true)", gen, ok) + } + if _, ok := renderedGeneration(annotations, "policy-c"); ok { + t.Error("a resource that did not contribute must not report a generation") + } + + // A CA with no policies still renders a file. The key is written empty + // rather than left out, so a previous render's claim does not survive. + empty := renderedFromAnnotation(nil) + if _, ok := empty[AnnotationRenderedFrom]; !ok { + t.Error("expected the key to be present for an empty source set") + } + if _, ok := renderedGeneration(empty, "policy-a"); ok { + t.Error("an empty annotation must not report a generation") + } + + if _, ok := renderedGeneration(nil, "policy-a"); ok { + t.Error("a Secret the operator did not render must not report a generation") + } +} + +func TestRenderedSourceRequests(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ca-autosign-policy", + Namespace: testNamespace, + Annotations: renderedFromAnnotation([]renderSource{{Name: "policy-a", Generation: 1}, {Name: "policy-b", Generation: 1}}), + }, + } + requests := renderedSourceRequests(secret) + if len(requests) != 2 || requests[0].Name != "policy-a" || requests[1].Name != "policy-b" { + t.Errorf("got %v, want a request per rendered source", requests) + } + if requests[0].Namespace != testNamespace { + t.Errorf("namespace = %q, want %q", requests[0].Namespace, testNamespace) + } + + // An unrelated Secret that merely happens to match the name suffix carries + // no annotation, and must not fan out over every resource in the namespace. + foreign := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "vault-enc", Namespace: testNamespace}, + } + if got := renderedSourceRequests(foreign); len(got) != 0 { + t.Errorf("got %v, want no requests for a Secret the operator did not render", got) + } +} diff --git a/internal/controller/server_deployment_test.go b/internal/controller/server_deployment_test.go index 95776584..cd471267 100644 --- a/internal/controller/server_deployment_test.go +++ b/internal/controller/server_deployment_test.go @@ -201,7 +201,7 @@ func TestBuildPodSpec_MultipleCodeVolumes(t *testing.T) { } func TestBuildPodSpec_AutosignCommandSkipsPolicyMount(t *testing.T) { - cfg := newConfig("production", withAutosignCommand("/usr/local/bin/custom-autosign")) + cfg := newConfig("production", withAutosignCommand()) server := newServer("test-ca", withCA(true), withServerRole(false)) podSpec := testBuildPodSpec(server, cfg) diff --git a/internal/controller/signingpolicy_controller.go b/internal/controller/signingpolicy_controller.go new file mode 100644 index 00000000..0c828439 --- /dev/null +++ b/internal/controller/signingpolicy_controller.go @@ -0,0 +1,254 @@ +package controller + +import ( + "context" + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// SigningPolicyReconciler owns the status of SigningPolicy objects. +// +// The rendered autosign policy Secret belongs to the Config controller, which +// has the CSR-attribute credentials and the full policy list. What this +// controller does is report whether a given SigningPolicy actually reached the +// CA -- derived from what it can observe, not handed over from another +// controller. +type SigningPolicyReconciler struct { + client.Client +} + +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=signingpolicies,verbs=get;list;watch +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=signingpolicies/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=certificateauthorities,verbs=get;list;watch +// +kubebuilder:rbac:groups=openvox.voxpupuli.org,resources=configs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch + +func (r *SigningPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + sp := &openvoxv1alpha1.SigningPolicy{} + if err := r.Get(ctx, req.NamespacedName, sp); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + return ctrl.Result{}, fmt.Errorf("getting SigningPolicy %s: %w", req.NamespacedName, err) + } + + // Pausing comes after the deletion path: a paused resource must still be + // deletable, otherwise the annotation turns into a trap. + if paused, err := reconcilePauseState(ctx, r.Client, sp, &sp.Status.Conditions); err != nil { + return ctrl.Result{}, err + } else if paused { + logger.Info("reconciliation paused by annotation", "name", sp.Name) + return ctrl.Result{}, nil + } + + // The generation the verdict is about, captured before the observation: + // updateStatusWithRetry re-reads the object, so a spec edit landing in + // between would otherwise stamp the new generation onto a verdict derived + // from the old spec. + generation := sp.Generation + + phase, reason, message := r.observe(ctx, sp) + if reason == reasonLookupFailed { + // A transient lookup failure says nothing about the SigningPolicy. + // Leave the status alone and let the backoff retry. + return ctrl.Result{}, fmt.Errorf("%s", message) + } + + if err := updateStatusWithRetry(ctx, r.Client, sp, func() { + sp.Status.ObservedGeneration = generation + sp.Status.Phase = phase + status := metav1.ConditionFalse + if phase == openvoxv1alpha1.SigningPolicyPhaseActive { + status = metav1.ConditionTrue + } + meta.SetStatusCondition(&sp.Status.Conditions, metav1.Condition{ + Type: openvoxv1alpha1.ConditionSigningPolicyReady, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: generation, + }) + }); err != nil { + return ctrl.Result{}, fmt.Errorf("updating SigningPolicy status %s: %w", sp.Name, err) + } + + return ctrl.Result{}, nil +} + +// observe derives the SigningPolicy state from the CA it references and the +// autosign policy Secret rendered for that CA. +func (r *SigningPolicyReconciler) observe(ctx context.Context, + sp *openvoxv1alpha1.SigningPolicy) (openvoxv1alpha1.SigningPolicyPhase, string, string) { + if sp.Spec.CertificateAuthorityRef == "" { + return openvoxv1alpha1.SigningPolicyPhaseError, "CertificateAuthorityRefMissing", + "spec.certificateAuthorityRef is empty" + } + + caName := sp.Spec.CertificateAuthorityRef + ca := &openvoxv1alpha1.CertificateAuthority{} + if err := r.Get(ctx, types.NamespacedName{Name: caName, Namespace: sp.Namespace}, ca); err != nil { + if apierrors.IsNotFound(err) { + return openvoxv1alpha1.SigningPolicyPhaseError, "CertificateAuthorityNotFound", + fmt.Sprintf("CertificateAuthority %s does not exist", caName) + } + return "", reasonLookupFailed, fmt.Sprintf("getting CertificateAuthority %s: %v", caName, err) + } + + // A policy only reaches the CA through a Config: the Config controller is + // what renders the Secret. Without one, nothing is rendered no matter how + // valid the policy is. + configs, err := configsReferencingAuthority(ctx, r.Client, sp.Namespace, caName) + if err != nil { + return "", reasonLookupFailed, fmt.Sprintf("listing Configs for CertificateAuthority %s: %v", caName, err) + } + if len(configs) == 0 { + return openvoxv1alpha1.SigningPolicyPhaseError, "NoConfig", + fmt.Sprintf("no Config references CertificateAuthority %s, so no policy is rendered", caName) + } + + // A custom autosignCommand replaces the built-in binary entirely, so the + // policy is bypassed. Reported before the Secret is read: a Secret rendered + // before the override was set still exists, and calling that "active" would + // claim an effect this policy no longer has. + if allOverride(configs, overrideAutosign) { + return openvoxv1alpha1.SigningPolicyPhaseError, "OverriddenByAutosignCommand", + fmt.Sprintf("spec.puppet.autosignCommand is set on every Config referencing CertificateAuthority %s, "+ + "which bypasses SigningPolicy resources", caName) + } + + secretName := fmt.Sprintf("%s-autosign-policy", caName) + secret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{Name: secretName, Namespace: sp.Namespace}, secret); err != nil { + if apierrors.IsNotFound(err) { + return openvoxv1alpha1.SigningPolicyPhaseError, "NotRendered", + fmt.Sprintf("Secret %s has not been rendered yet", secretName) + } + return "", reasonLookupFailed, fmt.Sprintf("getting Secret %s: %v", secretName, err) + } + + // The annotation names the policies the content was rendered from and the + // generation each was rendered at. Reading it rather than the rendered + // policy list is what separates "my current spec is in effect" from "some + // earlier version of it is", which a name alone cannot tell apart. + generation, ok := renderedGeneration(secret.Annotations, sp.Name) + switch { + case !ok: + return openvoxv1alpha1.SigningPolicyPhaseError, "NotRendered", + fmt.Sprintf("Secret %s does not contain a policy for this SigningPolicy", secretName) + case generation != sp.Generation: + return openvoxv1alpha1.SigningPolicyPhaseError, "RenderedConfigStale", + fmt.Sprintf("Secret %s was rendered from an earlier generation of SigningPolicy %s; "+ + "the current spec has not reached the CA", secretName, sp.Name) + } + + return openvoxv1alpha1.SigningPolicyPhaseActive, "Rendered", + fmt.Sprintf("Policy is present in Secret %s", secretName) +} + +// configsReferencingAuthority returns the Configs in a namespace whose +// authorityRef points at the given CertificateAuthority. +func configsReferencingAuthority(ctx context.Context, c client.Client, + namespace, caName string) ([]openvoxv1alpha1.Config, error) { + cfgList := &openvoxv1alpha1.ConfigList{} + if err := c.List(ctx, cfgList, + client.InNamespace(namespace), + client.MatchingFields{IndexAuthorityRef: caName}); err != nil { + return nil, err + } + return cfgList.Items, nil +} + +// signingPoliciesForAuthority returns the SigningPolicies in a namespace bound +// to the given CertificateAuthority. +// +// A list error is returned rather than swallowed: an empty policy set renders +// as a deny-all policy, so treating a transient failure as "no policies" would +// overwrite a valid policy Secret and lock every agent out. +func signingPoliciesForAuthority(ctx context.Context, c client.Reader, + namespace, caName string) ([]openvoxv1alpha1.SigningPolicy, error) { + list := &openvoxv1alpha1.SigningPolicyList{} + if err := c.List(ctx, list, client.InNamespace(namespace)); err != nil { + return nil, fmt.Errorf("listing SigningPolicies in namespace %s: %w", namespace, err) + } + var result []openvoxv1alpha1.SigningPolicy + for _, sp := range list.Items { + if sp.Spec.CertificateAuthorityRef == caName { + result = append(result, sp) + } + } + return result, nil +} + +func (r *SigningPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&openvoxv1alpha1.SigningPolicy{}). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(signingPoliciesForSecret())). + Watches(&openvoxv1alpha1.Config{}, handler.EnqueueRequestsFromMapFunc( + signingPoliciesForConfig(mgr.GetClient()), + )). + Watches(&openvoxv1alpha1.CertificateAuthority{}, handler.EnqueueRequestsFromMapFunc( + signingPoliciesForCA(mgr.GetClient()), + )). + Complete(r) +} + +// signingPoliciesForSecret maps a rendered autosign policy Secret back to the +// SigningPolicies it was rendered from. +func signingPoliciesForSecret() handler.MapFunc { + return func(_ context.Context, obj client.Object) []ctrl.Request { + if !strings.HasSuffix(obj.GetName(), "-autosign-policy") { + return nil + } + return renderedSourceRequests(obj) + } +} + +// signingPoliciesForConfig maps a Config change to the SigningPolicies bound to +// the CA it references. The Config decides whether a policy is rendered at all, +// so its authorityRef and autosignCommand are both inputs to the status. +func signingPoliciesForConfig(c client.Client) handler.MapFunc { + return func(ctx context.Context, obj client.Object) []ctrl.Request { + cfg, ok := obj.(*openvoxv1alpha1.Config) + if !ok || cfg.Spec.AuthorityRef == "" { + return nil + } + return signingPolicyRequests(ctx, c, cfg.Namespace, cfg.Spec.AuthorityRef) + } +} + +// signingPoliciesForCA maps a CertificateAuthority change to the +// SigningPolicies that reference it. +func signingPoliciesForCA(c client.Client) handler.MapFunc { + return func(ctx context.Context, obj client.Object) []ctrl.Request { + return signingPolicyRequests(ctx, c, obj.GetNamespace(), obj.GetName()) + } +} + +func signingPolicyRequests(ctx context.Context, c client.Client, namespace, caName string) []ctrl.Request { + policies, err := signingPoliciesForAuthority(ctx, c, namespace, caName) + if err != nil { + log.FromContext(ctx).Error(err, "failed to list SigningPolicies in watcher") + return nil + } + requests := make([]ctrl.Request, 0, len(policies)) + for _, sp := range policies { + requests = append(requests, ctrl.Request{ + NamespacedName: types.NamespacedName{Name: sp.Name, Namespace: sp.Namespace}, + }) + } + return requests +} diff --git a/internal/controller/signingpolicy_controller_status_test.go b/internal/controller/signingpolicy_controller_status_test.go new file mode 100644 index 00000000..18dfd395 --- /dev/null +++ b/internal/controller/signingpolicy_controller_status_test.go @@ -0,0 +1,208 @@ +package controller + +import ( + "testing" + + 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" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// testCAName is the CertificateAuthority every case in this file binds to. +const testCAName = "test-ca" + +// autosignPolicySecret builds a policy Secret as the Config controller renders +// it: the policies in autosign-policy.yaml, and the SigningPolicies they came +// from in the annotation. +func autosignPolicySecret(sources ...renderSource) *corev1.Secret { + yaml := "reservedCertnames:\n - \"" + operatorSigningCertname(testCAName) + "\"\npolicies:\n" + for _, s := range sources { + yaml += " - name: \"" + s.Name + "\"\n any: true\n" + } + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCAName + "-autosign-policy", + Namespace: testNamespace, + Annotations: renderedFromAnnotation(sources), + }, + Data: map[string][]byte{"autosign-policy.yaml": []byte(yaml)}, + } +} + +func TestSigningPolicyReconcile_Status(t *testing.T) { + sp := newSigningPolicy("test-policy", testCAName, true) + sp.Generation = 2 + current := renderSource{Name: "test-policy", Generation: 2} + ca := newCertificateAuthority(testCAName) + cfg := newConfig("production", withAuthorityRef(testCAName)) + key := types.NamespacedName{Name: "test-policy", Namespace: testNamespace} + + t.Run("active once the policy is rendered", func(t *testing.T) { + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), cfg.DeepCopy(), + autosignPolicySecret(current)) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseActive { + t.Errorf("phase = %q, want Active", got.Status.Phase) + } + if !meta.IsStatusConditionTrue(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) { + t.Error("expected a true Ready condition") + } + }) + + t.Run("error when the CA is missing", func(t *testing.T) { + c := setupTestClient(sp.DeepCopy()) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil || cond.Reason != "CertificateAuthorityNotFound" { + t.Errorf("expected reason CertificateAuthorityNotFound, got %+v", cond) + } + }) + + // A policy only reaches the CA through a Config. Without one it is inert, and + // saying so is the whole point of giving the resource its own status. + t.Run("error when no Config references the CA", func(t *testing.T) { + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy()) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil || cond.Reason != "NoConfig" { + t.Errorf("expected reason NoConfig, got %+v", cond) + } + }) + + t.Run("error while the secret has not been rendered", func(t *testing.T) { + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), cfg.DeepCopy()) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil || cond.Reason != "NotRendered" { + t.Errorf("expected reason NotRendered, got %+v", cond) + } + }) + + // A failed re-render -- an unresolvable csrAttributes Secret, say -- leaves + // the previous Secret in place. Matching on the name alone would report the + // broken generation as active at the CA. + t.Run("error when the rendered policy is from an earlier generation", func(t *testing.T) { + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), cfg.DeepCopy(), + autosignPolicySecret(renderSource{Name: "test-policy", Generation: 1})) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil || cond.Reason != "RenderedConfigStale" { + t.Errorf("expected reason RenderedConfigStale, got %+v", cond) + } + }) + + t.Run("error when certificateAuthorityRef is empty", func(t *testing.T) { + unbound := sp.DeepCopy() + unbound.Spec.CertificateAuthorityRef = "" + c := setupTestClient(unbound) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil || cond.Reason != "CertificateAuthorityRefMissing" { + t.Errorf("expected reason CertificateAuthorityRefMissing, got %+v", cond) + } + }) + + t.Run("error when another policy was rendered but not this one", func(t *testing.T) { + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), cfg.DeepCopy(), + autosignPolicySecret(renderSource{Name: "someone-else", Generation: 1})) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseError { + t.Errorf("phase = %q, want Error", got.Status.Phase) + } + }) + + // A stale Secret from before the override was set must not read as active: + // autosignCommand replaces the binary that would have consumed the policy. + t.Run("error when autosignCommand bypasses the policy", func(t *testing.T) { + overridden := newConfig("production", + withAuthorityRef(testCAName), + withAutosignCommand()) + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), overridden, + autosignPolicySecret(current)) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil || cond.Reason != "OverriddenByAutosignCommand" { + t.Errorf("expected reason OverriddenByAutosignCommand, got %+v", cond) + } + }) + + // One Config opting out does not disable the policy for the Config that did + // not, so the override only counts when every Config sets it. + t.Run("active when only one of two Configs overrides", func(t *testing.T) { + overridden := newConfig("legacy", + withAuthorityRef(testCAName), + withAutosignCommand()) + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), cfg.DeepCopy(), overridden, + autosignPolicySecret(current)) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseActive { + t.Errorf("phase = %q, want Active", got.Status.Phase) + } + }) +} diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index 5f932fd8..35f84fe9 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -126,9 +126,9 @@ func withReadOnlyRootFS(v bool) configOption { } } -func withAutosignCommand(cmd string) configOption { +func withAutosignCommand() configOption { return func(c *openvoxv1alpha1.Config) { - c.Spec.Puppet.AutosignCommand = cmd + c.Spec.Puppet.AutosignCommand = "/usr/local/bin/custom-autosign" } } @@ -557,6 +557,14 @@ func newReportProcessorReconciler(c client.Client) *ReportProcessorReconciler { } } +func newSigningPolicyReconciler(c client.Client) *SigningPolicyReconciler { + return &SigningPolicyReconciler{Client: c} +} + +func newNodeClassifierReconciler(c client.Client) *NodeClassifierReconciler { + return &NodeClassifierReconciler{Client: c} +} + type databaseOption func(*openvoxv1alpha1.Database) func newDatabase(name string, opts ...databaseOption) *openvoxv1alpha1.Database { From e9166126ad1fa4e82cbc82342adc42466d2fed5b Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Wed, 9 Sep 2026 11:51:12 +0200 Subject: [PATCH 2/6] docs: rename AGENT.md to AGENTS.md and rule out AI attribution AGENTS.md is the name coding agents look for by convention; the singular file was picked up by nothing. Nothing in the repository referenced the old path, so the rename is self-contained. While renaming, spell out that commits, pull request descriptions and comments carry no Co-Authored-By line for an AI assistant, no "Generated with Claude Code" footer and no session links. --- AGENT.md => AGENTS.md | 6 ++++++ 1 file changed, 6 insertions(+) rename AGENT.md => AGENTS.md (94%) diff --git a/AGENT.md b/AGENTS.md similarity index 94% rename from AGENT.md rename to AGENTS.md index 282f1698..7a68255a 100644 --- a/AGENT.md +++ b/AGENTS.md @@ -38,6 +38,8 @@ test: add unit tests for helpers - `feat:` triggers a minor version bump - `fix:` triggers a patch version bump - Append `BREAKING CHANGE:` in the body for major bumps +- No AI attribution trailers. Do not add `Co-Authored-By:` for an AI assistant, + and no `Claude-Session:` or comparable session links. ## Pull Requests @@ -46,6 +48,10 @@ test: add unit tests for helpers - Body should include a `## Summary` with bullet points and a `## Test plan` - Reference related issues with `Closes #` - PRs always target `develop`, not `main` +- No AI attribution footers in the description ("Generated with Claude Code" or + similar) and no session links. The same goes for issue and review comments. + If tooling appends such a footer server-side, remove it from the body + afterwards. ## Build & Test From a1d8135480ce3d3e3f4f9f0ea007f2e41403b8f6 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Wed, 9 Sep 2026 16:18:56 +0200 Subject: [PATCH 3/6] test: cover the rendered-from write path, and fix what it exposed The status tests built their Secret fixtures by calling renderedFromAnnotation themselves, so reader and writer agreed by construction rather than by observation. A mutation run showed how little that proved: reconcileSecret could stop writing the annotation entirely -- or write it only on create, or record generation zero -- and the whole suite still passed. New tests observe what the Config controller actually produces: the ENC and autosign Secrets carry the expected name=generation pairs, only the policies of the CA being rendered, refreshed on every re-render rather than frozen at create; and both halves now run against each other, Config controller rendering first and the status controller reading what it wrote. Each of those mutants is killed by at least one of them. Two defects the review turned up, both now covered: - A Secret rendered before this mechanism existed carries no annotation at all, which is not the same as being rendered from nothing. Reading it as "this resource is not in it" flipped every SigningPolicy and NodeClassifier to NotRendered on upgrade, and for a paused Config -- whose reconcile returns before the render step -- it stayed there. Those now report RenderSourceUnknown until the Config controller re-renders. - A NodeClassifier whose Config rendered a *different* classifier reported "no Secret rendered from X exists yet", which is false; a Secret exists, it just belongs to someone else. The SigningPolicy observer already distinguished the two cases. Also tightened: ObservedGeneration is now proven to come from the generation captured before the observation, by serving a bumped generation from the read updateStatusWithRetry performs -- the previous assertion could not tell the two apart. Error subtests assert Ready=False and not just the reason, which 15 of them failed to notice before. The Secret watches' suffix filters, the malformed annotation values and the request namespace all have tests; the namespace assertion no longer uses the default namespace, where a hardcoded value would have passed. The docs claimed a failed re-render always yields RenderedConfigStale. That holds only when a spec edit triggered it: a credential rotated underneath an unchanged spec leaves the generation untouched, so the resource keeps reporting Active while the old rendered file stays in effect. Both reference pages now say so and point at the Config's render-failure events, which is where that case is visible. This is long-standing behaviour, not new here -- the previous code also returned before writing status on a render error. --- docs/reference/nodeclassifier.md | 19 +- docs/reference/signingpolicy.md | 16 +- internal/controller/config_controller_test.go | 2 +- .../controller/nodeclassifier_controller.go | 28 ++- .../nodeclassifier_controller_status_test.go | 47 ++--- internal/controller/rendered_source.go | 10 ++ .../rendered_source_contract_test.go | 170 ++++++++++++++++++ internal/controller/rendered_source_test.go | 93 +++++++++- .../controller/signingpolicy_controller.go | 6 + .../signingpolicy_controller_status_test.go | 55 +++--- internal/controller/status_generation_test.go | 116 ++++++++++++ internal/controller/testutil_test.go | 35 +++- 12 files changed, 518 insertions(+), 79 deletions(-) create mode 100644 internal/controller/rendered_source_contract_test.go create mode 100644 internal/controller/status_generation_test.go diff --git a/docs/reference/nodeclassifier.md b/docs/reference/nodeclassifier.md index 13a6fc99..3cb7a568 100644 --- a/docs/reference/nodeclassifier.md +++ b/docs/reference/nodeclassifier.md @@ -188,8 +188,9 @@ well-formed. The `Ready` condition carries the reason: | `Rendered` | The endpoint is present in the rendered Secret, at the classifier's current generation | | `NotReferenced` | No [Config](config.md) sets `nodeClassifierRef` to this NodeClassifier, so nothing renders it | | `OverriddenByExternalNodesCommand` | Every Config referencing it sets [`spec.puppet.externalNodesCommand`](config.md), which replaces the built-in binary and bypasses NodeClassifier resources | -| `NotRendered` | No Secret rendered from this NodeClassifier exists yet | +| `NotRendered` | No Secret rendered from this NodeClassifier exists, or the one that exists was rendered from a different one | | `RenderedConfigStale` | A Secret was rendered from this NodeClassifier, but from an earlier generation | +| `RenderSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | `enc.yaml` carries no resource name, so the Secret's `openvox.voxpupuli.org/rendered-from` annotation is what ties the rendered file @@ -200,11 +201,17 @@ one Config still on an earlier generation holds the whole resource at `RenderedConfigStale`: the current spec is not in effect everywhere yet. Rendering failures -- an unresolvable auth Secret, for example -- are reported -on the Config that owns the Secret, as an `ENCRenderFailed` event. Since the -failed render leaves the previous Secret untouched, the classifier reports -`RenderedConfigStale` until the edit that broke it is corrected: the servers -are still classifying against the last configuration that rendered cleanly, -which for a rotated credential is the old one. +on the Config that owns the Secret, as an `ENCRenderFailed` event, and the +failed render leaves the previous Secret untouched. + +What the classifier reports then depends on whether its own spec changed. If a +spec edit triggered the failing render, the generation moved on and the +classifier reports `RenderedConfigStale`. If the spec did not change -- the +referenced auth Secret was rotated or deleted underneath it -- the generation +is unchanged, the previous Secret still matches it, and the classifier keeps +reporting `Active` while the servers classify against the old credential. Watch +the Config's `ENCRenderFailed` events for that case; the classifier's own +status cannot see it. ## How It Works diff --git a/docs/reference/signingpolicy.md b/docs/reference/signingpolicy.md index dd33097f..a15874d1 100644 --- a/docs/reference/signingpolicy.md +++ b/docs/reference/signingpolicy.md @@ -230,6 +230,7 @@ itself is well-formed. The `Ready` condition carries the reason: | `OverriddenByAutosignCommand` | Every Config referencing the CA sets [`spec.puppet.autosignCommand`](config.md), which replaces the built-in binary and bypasses SigningPolicy resources | | `NotRendered` | The Secret does not (yet) contain this policy | | `RenderedConfigStale` | The Secret contains this policy, but as it was at an earlier generation | +| `RenderSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | The Secret's `openvox.voxpupuli.org/rendered-from` annotation names the policies its content was built from and the generation each was rendered at, @@ -237,10 +238,17 @@ which is what separates `Rendered` from `RenderedConfigStale`. Rendering failures -- an unresolvable `csrAttributes` Secret, for example -- are reported on the Config that owns the Secret, as an -`AutosignPolicyRenderFailed` event. Since the failed render leaves the previous -Secret untouched, the policy reports `RenderedConfigStale` until the edit that -broke it is corrected: the CA is still signing under the last policy that -rendered cleanly. +`AutosignPolicyRenderFailed` event, and the failed render leaves the previous +Secret untouched. + +What the policy reports then depends on whether its own spec changed. If a spec +edit triggered the failing render, the generation moved on and the policy +reports `RenderedConfigStale`. If the spec did not change -- the referenced +`csrAttributes` Secret was rotated or deleted underneath it -- the generation is +unchanged, the previous Secret still matches it, and the policy keeps reporting +`Active` while the CA signs under the last policy that rendered cleanly. Watch +the Config's `AutosignPolicyRenderFailed` events for that case; the policy's own +status cannot see it. ## How It Works diff --git a/internal/controller/config_controller_test.go b/internal/controller/config_controller_test.go index d9fabf11..fe7ddaba 100644 --- a/internal/controller/config_controller_test.go +++ b/internal/controller/config_controller_test.go @@ -532,7 +532,7 @@ func TestConfigReconcile_LogbackXML(t *testing.T) { func TestConfigReconcile_AutosignPolicy(t *testing.T) { cfg := newConfig("production", withAuthorityRef("production-ca")) ca := newCertificateAuthority("production-ca") - sp := newSigningPolicy("allow-all", "production-ca", true) + sp := newSigningPolicy("allow-all", "production-ca") c := setupTestClient(cfg, ca, sp) r := newConfigReconciler(c) diff --git a/internal/controller/nodeclassifier_controller.go b/internal/controller/nodeclassifier_controller.go index a52f4fba..c4a2531f 100644 --- a/internal/controller/nodeclassifier_controller.go +++ b/internal/controller/nodeclassifier_controller.go @@ -110,7 +110,7 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, "which bypasses NodeClassifier resources", nc.Name) } - var rendered, stale []string + var rendered, stale, unrecorded, foreign []string for _, cfg := range configs { if cfg.Spec.Puppet.ExternalNodesCommand != "" { continue @@ -123,13 +123,17 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, } return "", reasonLookupFailed, fmt.Sprintf("getting Secret %s: %v", secretName, err) } + if !renderedSourceRecorded(secret.Annotations) { + unrecorded = append(unrecorded, secretName) + continue + } // The annotation names the NodeClassifier the content was rendered from, // which a Secret left over from a previous nodeClassifierRef does not // match, and carries the generation it was rendered at. generation, ok := renderedGeneration(secret.Annotations, nc.Name) switch { case !ok: - continue + foreign = append(foreign, secretName) case generation != nc.Generation: stale = append(stale, secretName) default: @@ -137,21 +141,29 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, } } + switch { // Stale outranks rendered: while any server still runs an earlier spec, the // current one is not in effect, and reporting Ready for a generation that // has not fully landed is the claim this controller exists to avoid. - if len(stale) > 0 { + case len(stale) > 0: return openvoxv1alpha1.NodeClassifierPhaseError, "RenderedConfigStale", fmt.Sprintf("Secret %s was rendered from an earlier generation of NodeClassifier %s; "+ "the current spec has not reached a server", strings.Join(stale, ", "), nc.Name) - } - if len(rendered) == 0 { + case len(rendered) > 0: + return openvoxv1alpha1.NodeClassifierPhaseActive, "Rendered", + fmt.Sprintf("Endpoint is present in Secret %s", strings.Join(rendered, ", ")) + case len(unrecorded) > 0: + return openvoxv1alpha1.NodeClassifierPhaseError, "RenderSourceUnknown", + fmt.Sprintf("Secret %s does not record which resources it was rendered from, so the Config "+ + "controller has not re-rendered it yet; its contents are unchanged in the meantime", + strings.Join(unrecorded, ", ")) + case len(foreign) > 0: return openvoxv1alpha1.NodeClassifierPhaseError, "NotRendered", - fmt.Sprintf("no Secret rendered from NodeClassifier %s exists yet", nc.Name) + fmt.Sprintf("Secret %s was rendered from a different NodeClassifier", strings.Join(foreign, ", ")) } - return openvoxv1alpha1.NodeClassifierPhaseActive, "Rendered", - fmt.Sprintf("Endpoint is present in Secret %s", strings.Join(rendered, ", ")) + return openvoxv1alpha1.NodeClassifierPhaseError, "NotRendered", + fmt.Sprintf("no Secret rendered from NodeClassifier %s exists yet", nc.Name) } // configsReferencingNodeClassifier returns the Configs in a namespace whose diff --git a/internal/controller/nodeclassifier_controller_status_test.go b/internal/controller/nodeclassifier_controller_status_test.go index e2fdc62d..22913022 100644 --- a/internal/controller/nodeclassifier_controller_status_test.go +++ b/internal/controller/nodeclassifier_controller_status_test.go @@ -1,6 +1,7 @@ package controller import ( + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -64,10 +65,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) - if cond == nil || cond.Reason != "NotReferenced" { - t.Errorf("expected reason NotReferenced, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "NotReferenced") }) t.Run("error while the secret has not been rendered", func(t *testing.T) { @@ -80,10 +78,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) - if cond == nil || cond.Reason != "NotRendered" { - t.Errorf("expected reason NotRendered, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") }) // A Secret left over from a previous nodeClassifierRef would otherwise read @@ -100,9 +95,10 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) - if cond == nil || cond.Reason != "NotRendered" { - t.Errorf("expected reason NotRendered, got %+v", cond) + if cond != nil && !strings.Contains(cond.Message, "different NodeClassifier") { + t.Errorf("message = %q, want it to say the Secret belongs to another classifier rather than that none exists", cond.Message) } }) @@ -120,10 +116,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) - if cond == nil || cond.Reason != "RenderedConfigStale" { - t.Errorf("expected reason RenderedConfigStale, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigStale") if got.Status.ObservedGeneration != nc.Generation { t.Errorf("observedGeneration = %d, want the generation that is not in effect (%d)", got.Status.ObservedGeneration, nc.Generation) @@ -145,10 +138,25 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) - if cond == nil || cond.Reason != "OverriddenByExternalNodesCommand" { - t.Errorf("expected reason OverriddenByExternalNodesCommand, got %+v", cond) + requireErrorCondition(t, got.Status.Conditions, "OverriddenByExternalNodesCommand") + }) + + // On upgrade the rendered Secret exists but predates the annotation. Reading + // that as "this classifier is not in it" would flip every classifier to + // NotRendered, and for a paused Config it would stay there. + t.Run("secret from before the annotation reports an unknown source", func(t *testing.T) { + legacy := encSecret("production", encURL) + delete(legacy.Annotations, AnnotationRenderedFrom) + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), legacy) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + requireErrorCondition(t, got.Status.Conditions, "RenderSourceUnknown") }) // A Config that has not rendered anything yet says nothing about the @@ -187,10 +195,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) - if cond == nil || cond.Reason != "RenderedConfigStale" { - t.Errorf("expected reason RenderedConfigStale, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigStale") }) } diff --git a/internal/controller/rendered_source.go b/internal/controller/rendered_source.go index 418a0a0e..5358943e 100644 --- a/internal/controller/rendered_source.go +++ b/internal/controller/rendered_source.go @@ -49,6 +49,16 @@ func renderedFromAnnotation(sources []renderSource) map[string]string { return map[string]string{AnnotationRenderedFrom: strings.Join(parts, ",")} } +// renderedSourceRecorded reports whether a Secret records its render sources at +// all. renderedFromAnnotation always writes the key, empty source set included, +// so a Secret without it was rendered by an operator that predates the +// annotation -- which is not the same as being rendered from nothing, and must +// not be read as "this resource is not in it". +func renderedSourceRecorded(annotations map[string]string) bool { + _, ok := annotations[AnnotationRenderedFrom] + return ok +} + // renderedGeneration returns the generation the named resource had when the // file was rendered, and whether it contributed to it at all. func renderedGeneration(annotations map[string]string, name string) (int64, bool) { diff --git a/internal/controller/rendered_source_contract_test.go b/internal/controller/rendered_source_contract_test.go new file mode 100644 index 00000000..edc9e9da --- /dev/null +++ b/internal/controller/rendered_source_contract_test.go @@ -0,0 +1,170 @@ +package controller + +import ( + "fmt" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// The status controllers read the rendered-from annotation; the Config +// controller writes it. The status tests build their Secret fixtures by calling +// renderedFromAnnotation themselves, so reader and writer agree there by +// construction -- the Config controller could stop writing the annotation +// altogether and those tests would still pass. These tests observe what the +// Config controller actually produces, and then run both halves against each +// other. + +func renderedFrom(t *testing.T, c client.Client, secretName string) string { + t.Helper() + secret := &corev1.Secret{} + if err := c.Get(testCtx(), types.NamespacedName{Name: secretName, Namespace: testNamespace}, secret); err != nil { + t.Fatalf("reading Secret %s: %v", secretName, err) + } + value, ok := secret.Annotations[AnnotationRenderedFrom] + if !ok { + t.Fatalf("Secret %s carries no %s annotation, so nothing can tell which resource it was rendered from", + secretName, AnnotationRenderedFrom) + } + return value +} + +func TestConfigReconcile_ENCSecretRecordsRenderSource(t *testing.T) { + nc := newNodeClassifier("my-enc", "https://foreman.example.invalid") + nc.Generation = 3 + c := setupTestClient(newConfig("production", withNodeClassifierRef()), nc) + + if _, err := newConfigReconciler(c).Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + if got := renderedFrom(t, c, "production-enc"); got != "my-enc=3" { + t.Errorf("rendered-from = %q, want the classifier at the generation it was rendered from", got) + } +} + +func TestConfigReconcile_AutosignSecretRecordsRenderSources(t *testing.T) { + ca := newCertificateAuthority(testCAName) + + alpha := newSigningPolicy("alpha", testCAName) + alpha.Generation = 5 + beta := newSigningPolicy("beta", testCAName) + beta.Generation = 2 + // Bound to a different CA, so it must not appear in this Secret's sources. + foreign := newSigningPolicy("gamma", "other-ca") + foreign.Generation = 9 + + c := setupTestClient(newConfig("production", withAuthorityRef(testCAName)), ca, alpha, beta, foreign) + + if _, err := newConfigReconciler(c).Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("reconcile: %v", err) + } + + if got := renderedFrom(t, c, testCAName+"-autosign-policy"); got != "alpha=5,beta=2" { + t.Errorf("rendered-from = %q, want both policies of this CA, sorted, and no other CA's policy", got) + } +} + +// A re-render has to refresh the annotation. If it were only written when the +// Secret is created, the recorded generation would freeze and every later +// generation would read as RenderedConfigStale forever. +func TestConfigReconcile_RenderSourceFollowsGeneration(t *testing.T) { + nc := newNodeClassifier("my-enc", "https://foreman.example.invalid") + nc.Generation = 3 + c := setupTestClient(newConfig("production", withNodeClassifierRef()), nc) + r := newConfigReconciler(c) + + if _, err := r.Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("first reconcile: %v", err) + } + first := renderedFrom(t, c, "production-enc") + + stored := &openvoxv1alpha1.NodeClassifier{} + key := types.NamespacedName{Name: "my-enc", Namespace: testNamespace} + if err := c.Get(testCtx(), key, stored); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + stored.Generation = 4 + stored.Spec.URL = "https://foreman-2.example.invalid" + if err := c.Update(testCtx(), stored); err != nil { + t.Fatalf("updating NodeClassifier: %v", err) + } + if err := c.Get(testCtx(), key, stored); err != nil { + t.Fatalf("re-reading NodeClassifier: %v", err) + } + if stored.Generation == 3 { + t.Fatalf("generation did not advance, so this test cannot tell a refreshed annotation from a frozen one") + } + + if _, err := r.Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("second reconcile: %v", err) + } + + want := fmt.Sprintf("my-enc=%d", stored.Generation) + got := renderedFrom(t, c, "production-enc") + if got == first { + t.Errorf("rendered-from is still %q after a re-render at a new generation", got) + } + if got != want { + t.Errorf("rendered-from = %q, want %q", got, want) + } +} + +// The two halves run against each other rather than against a shared fixture +// helper: the Config controller renders, then the status controller reads what +// it wrote. +func TestRenderedFromRoundTrip_NodeClassifier(t *testing.T) { + nc := newNodeClassifier("my-enc", "https://foreman.example.invalid") + nc.Generation = 3 + c := setupTestClient(newConfig("production", withNodeClassifierRef()), nc) + + if _, err := newConfigReconciler(c).Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("config reconcile: %v", err) + } + if _, err := newNodeClassifierReconciler(c).Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("nodeclassifier reconcile: %v", err) + } + + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "my-enc", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseActive { + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + t.Fatalf("phase = %q, want Active (condition %+v)", got.Status.Phase, cond) + } + if got.Status.ObservedGeneration != 3 { + t.Errorf("observedGeneration = %d, want 3", got.Status.ObservedGeneration) + } +} + +func TestRenderedFromRoundTrip_SigningPolicy(t *testing.T) { + sp := newSigningPolicy("allow-all", testCAName) + sp.Generation = 4 + c := setupTestClient(newConfig("production", withAuthorityRef(testCAName)), + newCertificateAuthority(testCAName), sp) + + if _, err := newConfigReconciler(c).Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("config reconcile: %v", err) + } + if _, err := newSigningPolicyReconciler(c).Reconcile(testCtx(), testRequest("allow-all")); err != nil { + t.Fatalf("signingpolicy reconcile: %v", err) + } + + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "allow-all", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseActive { + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + t.Fatalf("phase = %q, want Active (condition %+v)", got.Status.Phase, cond) + } + if got.Status.ObservedGeneration != 4 { + t.Errorf("observedGeneration = %d, want 4", got.Status.ObservedGeneration) + } +} diff --git a/internal/controller/rendered_source_test.go b/internal/controller/rendered_source_test.go index 7f361cbc..4c0dc852 100644 --- a/internal/controller/rendered_source_test.go +++ b/internal/controller/rendered_source_test.go @@ -41,28 +41,103 @@ func TestRenderedFromAnnotationRoundTrip(t *testing.T) { } } +// Anything unparsable has to read as "this resource did not contribute", never +// as generation zero, which a resource could legitimately be at. +func TestRenderedGeneration_MalformedValues(t *testing.T) { + tests := []struct { + name string + value string + want int64 + found bool + }{ + {"no separator", "my-enc", 0, false}, + {"generation is not a number", "my-enc=abc", 0, false}, + {"empty generation", "my-enc=", 0, false}, + {"empty value", "", 0, false}, + {"name only matches a prefix", "my-enc-2=4", 0, false}, + {"second entry matches", "other=1,my-enc=5", 5, true}, + {"trailing separator", "my-enc=5,", 5, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gen, ok := renderedGeneration(map[string]string{AnnotationRenderedFrom: tt.value}, "my-enc") + if ok != tt.found || gen != tt.want { + t.Errorf("renderedGeneration(%q) = (%d, %v), want (%d, %v)", tt.value, gen, ok, tt.want, tt.found) + } + }) + } +} + func TestRenderedSourceRequests(t *testing.T) { + // Deliberately not testNamespace, so a hardcoded namespace would show up. + const ns = "openvox-system" secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-ca-autosign-policy", - Namespace: testNamespace, - Annotations: renderedFromAnnotation([]renderSource{{Name: "policy-a", Generation: 1}, {Name: "policy-b", Generation: 1}}), + Name: "test-ca-autosign-policy", + Namespace: ns, + Annotations: renderedFromAnnotation([]renderSource{ + {Name: "policy-a", Generation: 1}, + {Name: "policy-b", Generation: 1}, + }), }, } requests := renderedSourceRequests(secret) if len(requests) != 2 || requests[0].Name != "policy-a" || requests[1].Name != "policy-b" { - t.Errorf("got %v, want a request per rendered source", requests) + t.Fatalf("got %v, want a request per rendered source", requests) } - if requests[0].Namespace != testNamespace { - t.Errorf("namespace = %q, want %q", requests[0].Namespace, testNamespace) + for _, req := range requests { + if req.Namespace != ns { + t.Errorf("namespace = %q, want %q", req.Namespace, ns) + } } - // An unrelated Secret that merely happens to match the name suffix carries - // no annotation, and must not fan out over every resource in the namespace. + // A Secret the operator did not render carries no annotation, so it maps to + // nothing rather than fanning out over every resource in the namespace. foreign := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "vault-enc", Namespace: testNamespace}, + ObjectMeta: metav1.ObjectMeta{Name: "vault-enc", Namespace: ns}, } if got := renderedSourceRequests(foreign); len(got) != 0 { t.Errorf("got %v, want no requests for a Secret the operator did not render", got) } } + +// The two Secret watches share the annotation, so the name suffix is what keeps +// each controller to its own rendered file. Without it an autosign policy +// Secret would map its policy names onto NodeClassifier requests. +func TestSecretWatches_SuffixSelectsTheRightController(t *testing.T) { + annotated := func(name string, sources ...renderSource) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + Annotations: renderedFromAnnotation(sources), + }, + } + } + enc := annotated("production-enc", renderSource{Name: "my-enc", Generation: 1}) + autosign := annotated("test-ca-autosign-policy", renderSource{Name: "allow-all", Generation: 1}) + webhook := annotated("production-report-webhook", renderSource{Name: "beta", Generation: 1}) + + ncFor := nodeClassifiersForSecret() + spFor := signingPoliciesForSecret() + + if got := ncFor(testCtx(), enc); len(got) != 1 || got[0].Name != "my-enc" { + t.Errorf("ENC Secret mapped to %v, want a request for my-enc", got) + } + if got := ncFor(testCtx(), autosign); len(got) != 0 { + t.Errorf("autosign Secret mapped to NodeClassifier requests %v, want none", got) + } + if got := ncFor(testCtx(), webhook); len(got) != 0 { + t.Errorf("report webhook Secret mapped to NodeClassifier requests %v, want none", got) + } + + if got := spFor(testCtx(), autosign); len(got) != 1 || got[0].Name != "allow-all" { + t.Errorf("autosign Secret mapped to %v, want a request for allow-all", got) + } + if got := spFor(testCtx(), enc); len(got) != 0 { + t.Errorf("ENC Secret mapped to SigningPolicy requests %v, want none", got) + } + if got := spFor(testCtx(), webhook); len(got) != 0 { + t.Errorf("report webhook Secret mapped to SigningPolicy requests %v, want none", got) + } +} diff --git a/internal/controller/signingpolicy_controller.go b/internal/controller/signingpolicy_controller.go index 0c828439..a9467586 100644 --- a/internal/controller/signingpolicy_controller.go +++ b/internal/controller/signingpolicy_controller.go @@ -140,6 +140,12 @@ func (r *SigningPolicyReconciler) observe(ctx context.Context, return "", reasonLookupFailed, fmt.Sprintf("getting Secret %s: %v", secretName, err) } + if !renderedSourceRecorded(secret.Annotations) { + return openvoxv1alpha1.SigningPolicyPhaseError, "RenderSourceUnknown", + fmt.Sprintf("Secret %s does not record which resources it was rendered from, so the Config "+ + "controller has not re-rendered it yet; its contents are unchanged in the meantime", secretName) + } + // The annotation names the policies the content was rendered from and the // generation each was rendered at. Reading it rather than the rendered // policy list is what separates "my current spec is in effect" from "some diff --git a/internal/controller/signingpolicy_controller_status_test.go b/internal/controller/signingpolicy_controller_status_test.go index 18dfd395..d5c788c0 100644 --- a/internal/controller/signingpolicy_controller_status_test.go +++ b/internal/controller/signingpolicy_controller_status_test.go @@ -33,7 +33,7 @@ func autosignPolicySecret(sources ...renderSource) *corev1.Secret { } func TestSigningPolicyReconcile_Status(t *testing.T) { - sp := newSigningPolicy("test-policy", testCAName, true) + sp := newSigningPolicy("test-policy", testCAName) sp.Generation = 2 current := renderSource{Name: "test-policy", Generation: 2} ca := newCertificateAuthority(testCAName) @@ -69,10 +69,7 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) - if cond == nil || cond.Reason != "CertificateAuthorityNotFound" { - t.Errorf("expected reason CertificateAuthorityNotFound, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "CertificateAuthorityNotFound") }) // A policy only reaches the CA through a Config. Without one it is inert, and @@ -87,10 +84,7 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) - if cond == nil || cond.Reason != "NoConfig" { - t.Errorf("expected reason NoConfig, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "NoConfig") }) t.Run("error while the secret has not been rendered", func(t *testing.T) { @@ -103,10 +97,7 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) - if cond == nil || cond.Reason != "NotRendered" { - t.Errorf("expected reason NotRendered, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") }) // A failed re-render -- an unresolvable csrAttributes Secret, say -- leaves @@ -123,10 +114,25 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) - if cond == nil || cond.Reason != "RenderedConfigStale" { - t.Errorf("expected reason RenderedConfigStale, got %+v", cond) + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigStale") + }) + + // On upgrade the rendered Secret exists but predates the annotation. Reading + // that as "this policy is not in it" would flip every policy to NotRendered, + // and for a paused Config it would stay there. + t.Run("secret from before the annotation reports an unknown source", func(t *testing.T) { + legacy := autosignPolicySecret(current) + delete(legacy.Annotations, AnnotationRenderedFrom) + c := setupTestClient(sp.DeepCopy(), ca.DeepCopy(), cfg.DeepCopy(), legacy) + r := newSigningPolicyReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + requireErrorCondition(t, got.Status.Conditions, "RenderSourceUnknown") }) t.Run("error when certificateAuthorityRef is empty", func(t *testing.T) { @@ -141,10 +147,7 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) - if cond == nil || cond.Reason != "CertificateAuthorityRefMissing" { - t.Errorf("expected reason CertificateAuthorityRefMissing, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "CertificateAuthorityRefMissing") }) t.Run("error when another policy was rendered but not this one", func(t *testing.T) { @@ -158,9 +161,10 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - if got.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseError { - t.Errorf("phase = %q, want Error", got.Status.Phase) - } + // The reason is asserted, not just the phase: a policy the Secret was + // never rendered from and one rendered at an older generation are both + // Error, so the phase alone cannot tell which branch produced it. + requireErrorCondition(t, got.Status.Conditions, "NotRendered") }) // A stale Secret from before the override was set must not read as active: @@ -179,10 +183,7 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) - if cond == nil || cond.Reason != "OverriddenByAutosignCommand" { - t.Errorf("expected reason OverriddenByAutosignCommand, got %+v", cond) - } + requireErrorCondition(t, got.Status.Conditions, "OverriddenByAutosignCommand") }) // One Config opting out does not disable the policy for the Config that did diff --git a/internal/controller/status_generation_test.go b/internal/controller/status_generation_test.go new file mode 100644 index 00000000..0dd8cb23 --- /dev/null +++ b/internal/controller/status_generation_test.go @@ -0,0 +1,116 @@ +package controller + +import ( + "context" + "testing" + + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" +) + +// The status controllers capture metadata.generation before observing, because +// updateStatusWithRetry re-reads the object and a spec edit landing in between +// would otherwise stamp the new generation onto a verdict derived from the old +// spec. Reading the generation off the re-read object passes an ordinary test, +// where nothing changes in between -- so these tests inject the edit, by +// serving a bumped generation from every read after the first. + +// generationBumpingClient serves the given objects, but hands out a newer +// metadata.generation for the watched type from the second read onwards. That +// is exactly the read updateStatusWithRetry performs. +func generationBumpingClient(t *testing.T, bumped int64, isWatched func(client.Object) bool, + objs ...client.Object) (client.Client, func() int) { + t.Helper() + reads := 0 + c := testClientBuilder(objs...). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, + obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + if isWatched(obj) { + reads++ + if reads > 1 { + obj.SetGeneration(bumped) + } + } + return nil + }, + }). + Build() + return c, func() int { return reads } +} + +func TestNodeClassifierReconcile_ObservedGenerationPredatesTheEdit(t *testing.T) { + const observed int64 = 2 + nc := newNodeClassifier("my-enc", "https://foreman.example.invalid") + nc.Generation = observed + + c, reads := generationBumpingClient(t, 7, + func(o client.Object) bool { _, ok := o.(*openvoxv1alpha1.NodeClassifier); return ok }, + nc, newConfig("production", withNodeClassifierRef()), + encSecret("production", "https://foreman.example.invalid", + renderSource{Name: "my-enc", Generation: observed})) + + if _, err := newNodeClassifierReconciler(c).Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + if reads() < 2 { + t.Fatalf("only %d read(s) of the NodeClassifier, so no re-read happened and this test proves nothing", reads()) + } + + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "my-enc", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + if got.Status.ObservedGeneration != observed { + t.Errorf("observedGeneration = %d, want %d -- the generation the verdict was derived from, not the one that landed during the status write", + got.Status.ObservedGeneration, observed) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond == nil { + t.Fatal("expected a Ready condition") + } + if cond.ObservedGeneration != observed { + t.Errorf("condition observedGeneration = %d, want %d", cond.ObservedGeneration, observed) + } +} + +func TestSigningPolicyReconcile_ObservedGenerationPredatesTheEdit(t *testing.T) { + const observed int64 = 2 + sp := newSigningPolicy("test-policy", testCAName) + sp.Generation = observed + + c, reads := generationBumpingClient(t, 7, + func(o client.Object) bool { _, ok := o.(*openvoxv1alpha1.SigningPolicy); return ok }, + sp, newCertificateAuthority(testCAName), newConfig("production", withAuthorityRef(testCAName)), + autosignPolicySecret(renderSource{Name: "test-policy", Generation: observed})) + + if _, err := newSigningPolicyReconciler(c).Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + if reads() < 2 { + t.Fatalf("only %d read(s) of the SigningPolicy, so no re-read happened and this test proves nothing", reads()) + } + + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-policy", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + if got.Status.ObservedGeneration != observed { + t.Errorf("observedGeneration = %d, want %d -- the generation the verdict was derived from, not the one that landed during the status write", + got.Status.ObservedGeneration, observed) + } + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionSigningPolicyReady) + if cond == nil { + t.Fatal("expected a Ready condition") + } + if cond.ObservedGeneration != observed { + t.Errorf("condition observedGeneration = %d, want %d", cond.ObservedGeneration, observed) + } +} diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index 35f84fe9..a8d4e018 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "testing" corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" @@ -36,6 +37,34 @@ func testScheme() *runtime.Scheme { // setupTestClient creates a fake client pre-loaded with the given objects. // StatusSubresource is enabled for all CRD types that use status updates. func setupTestClient(objs ...client.Object) client.Client { + return testClientBuilder(objs...).Build() +} + +// requireErrorCondition asserts a resource reports the given failure reason and +// is genuinely not ready. Checking the reason alone would let a condition that +// names the failure while still reporting Ready=True pass unnoticed. +// +// The condition type is not a parameter because every resource with a +// readiness condition names it "Ready"; see ConditionSigningPolicyReady and +// its siblings. +func requireErrorCondition(t *testing.T, conditions []metav1.Condition, reason string) { + t.Helper() + const condType = openvoxv1alpha1.ConditionSigningPolicyReady + cond := meta.FindStatusCondition(conditions, condType) + if cond == nil { + t.Fatalf("expected a %s condition, got none", condType) + } + if cond.Reason != reason { + t.Errorf("reason = %q, want %q", cond.Reason, reason) + } + if cond.Status != metav1.ConditionFalse { + t.Errorf("status = %q for reason %q, want False", cond.Status, reason) + } +} + +// testClientBuilder is setupTestClient stopping short of Build, for tests that +// need to add interceptors on top of the same scheme, subresources and indexes. +func testClientBuilder(objs ...client.Object) *fake.ClientBuilder { b := fake.NewClientBuilder(). WithScheme(testScheme()). WithObjects(objs...). @@ -55,7 +84,7 @@ func setupTestClient(objs ...client.Object) client.Client { for _, idx := range fieldIndexes() { b = b.WithIndex(idx.obj, idx.field, idx.extract) } - return b.Build() + return b } // testRecorder returns a fake event recorder. @@ -416,7 +445,7 @@ func newCertificateAuthority(name string, opts ...caOption) *openvoxv1alpha1.Cer return ca } -func newSigningPolicy(name, caRef string, any bool) *openvoxv1alpha1.SigningPolicy { +func newSigningPolicy(name, caRef string) *openvoxv1alpha1.SigningPolicy { return &openvoxv1alpha1.SigningPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -424,7 +453,7 @@ func newSigningPolicy(name, caRef string, any bool) *openvoxv1alpha1.SigningPoli }, Spec: openvoxv1alpha1.SigningPolicySpec{ CertificateAuthorityRef: caRef, - Any: any, + Any: true, }, } } From 7a879eac3b2b90bf1c575178110dc7c7ea97bd39 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Wed, 9 Sep 2026 23:04:31 +0200 Subject: [PATCH 4/6] fix: stop a healthy Config masking a broken one in NodeClassifier status The verdict ranked a current render above a Secret rendered from a different classifier and above one recording no source at all, while ranking a stale render below it. Those three are the same statement -- a server is running something that is not this generation -- so the ordering contradicted the rule it stated one line above, and one Config that renders cleanly hid the others. The case is persistent rather than a startup race. Two Configs reference the same classifier; the second fails its reconcile before the ENC step, so its Secret keeps whatever it held before. The classifier reported Ready=True and named only the healthy Secret in its message, leaving nothing to point at the Config that was actually stuck. A Secret that exists and does not match the current generation now holds the resource out of Ready whichever of the three forms it takes, and the message names it. A Config with no Secret at all still does not hold anything back: nothing is mounted there, so it contradicts nothing. Also from the review: - docs/reference/index.md still carried the claim the two reference pages had already been corrected for: that a failed re-render always yields RenderedConfigStale. It holds only when a spec edit caused the failure. The shared page now says so, and covers RenderSourceUnknown, which it omitted. - docs/reference/nodeclassifier.md stated the multi-Config rule for stale renders only, which read as a guarantee that Ready means every referencing Config is current. - The comment justifying requireErrorCondition's hardcoded condition type claimed every readiness condition is named "Ready". ConditionCAReady and ConditionConfigReady are not; the two types it is used on are. --- docs/reference/index.md | 17 +++++-- docs/reference/nodeclassifier.md | 7 ++- .../controller/nodeclassifier_controller.go | 25 +++++++---- .../nodeclassifier_controller_status_test.go | 44 +++++++++++++++++++ internal/controller/testutil_test.go | 7 +-- 5 files changed, 83 insertions(+), 17 deletions(-) diff --git a/docs/reference/index.md b/docs/reference/index.md index c23be002..b7501105 100644 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -183,9 +183,20 @@ Each rendered Secret carries an `openvox.voxpupuli.org/rendered-from` annotation listing the resources its content was built from and the `metadata.generation` each had at the time. That is what a policy resource matches itself against, so `Ready` distinguishes "my current spec is in effect" -from "an earlier version of it is". A re-render that fails leaves the previous -Secret in place; the resource then reports `RenderedConfigStale` rather than -claiming the new spec reached a server. +from "an earlier version of it is": a spec edit whose re-render fails leaves the +previous Secret in place, and the resource reports `RenderedConfigStale` rather +than claiming the new spec reached a server. + +Because the annotation records the generation, this only covers failures a spec +edit caused. A render that starts failing under an *unchanged* spec -- a +referenced credential Secret rotated out from under it -- leaves the generation +matching, so the resource keeps reporting `Ready`. The render failure is an +event on the Config, which is where that case is visible. + +A Secret rendered before this mechanism existed carries no annotation at all, +which is not the same as being rendered from nothing. Those resources report +`RenderSourceUnknown` until the Config controller re-renders, rather than +claiming they were left out. Any resource can additionally carry `Paused` -- see [Pausing Reconciliation](../guides/pausing-reconciliation.md). diff --git a/docs/reference/nodeclassifier.md b/docs/reference/nodeclassifier.md index 3cb7a568..c431d712 100644 --- a/docs/reference/nodeclassifier.md +++ b/docs/reference/nodeclassifier.md @@ -197,8 +197,11 @@ well-formed. The `Ready` condition carries the reason: back to this NodeClassifier and to the generation it was rendered at. That also catches a Secret left over from a previous `nodeClassifierRef`, which would otherwise read as active. Where several Configs reference the same classifier, -one Config still on an earlier generation holds the whole resource at -`RenderedConfigStale`: the current spec is not in effect everywhere yet. +any one of them holding a Secret that does not match the current generation -- +an earlier generation, a different classifier, or one that records no source at +all -- holds the whole resource out of `Ready`: the current spec is not in +effect everywhere yet. A Config that has rendered no Secret at all is the +exception, since nothing is mounted there to contradict it. Rendering failures -- an unresolvable auth Secret, for example -- are reported on the Config that owns the Secret, as an `ENCRenderFailed` event, and the diff --git a/internal/controller/nodeclassifier_controller.go b/internal/controller/nodeclassifier_controller.go index c4a2531f..62f37456 100644 --- a/internal/controller/nodeclassifier_controller.go +++ b/internal/controller/nodeclassifier_controller.go @@ -141,25 +141,32 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, } } + // A Secret that exists but does not match this NodeClassifier's current + // generation means a server is running something else -- an earlier + // generation, another classifier, or content of unknown vintage. All three + // outrank a Config that did render the current spec: reporting Ready while + // one server is demonstrably not on it is the claim this controller exists + // to avoid, and a healthy Config must not mask a broken one. + // + // A Config with no Secret at all is different. Nothing is mounted there, so + // it contradicts nothing and does not hold the resource back. switch { - // Stale outranks rendered: while any server still runs an earlier spec, the - // current one is not in effect, and reporting Ready for a generation that - // has not fully landed is the claim this controller exists to avoid. case len(stale) > 0: return openvoxv1alpha1.NodeClassifierPhaseError, "RenderedConfigStale", fmt.Sprintf("Secret %s was rendered from an earlier generation of NodeClassifier %s; "+ "the current spec has not reached a server", strings.Join(stale, ", "), nc.Name) - case len(rendered) > 0: - return openvoxv1alpha1.NodeClassifierPhaseActive, "Rendered", - fmt.Sprintf("Endpoint is present in Secret %s", strings.Join(rendered, ", ")) + case len(foreign) > 0: + return openvoxv1alpha1.NodeClassifierPhaseError, "NotRendered", + fmt.Sprintf("Secret %s was rendered from a different NodeClassifier, so NodeClassifier %s "+ + "is not in effect for every Config referencing it", strings.Join(foreign, ", "), nc.Name) case len(unrecorded) > 0: return openvoxv1alpha1.NodeClassifierPhaseError, "RenderSourceUnknown", fmt.Sprintf("Secret %s does not record which resources it was rendered from, so the Config "+ "controller has not re-rendered it yet; its contents are unchanged in the meantime", strings.Join(unrecorded, ", ")) - case len(foreign) > 0: - return openvoxv1alpha1.NodeClassifierPhaseError, "NotRendered", - fmt.Sprintf("Secret %s was rendered from a different NodeClassifier", strings.Join(foreign, ", ")) + case len(rendered) > 0: + return openvoxv1alpha1.NodeClassifierPhaseActive, "Rendered", + fmt.Sprintf("Endpoint is present in Secret %s", strings.Join(rendered, ", ")) } return openvoxv1alpha1.NodeClassifierPhaseError, "NotRendered", diff --git a/internal/controller/nodeclassifier_controller_status_test.go b/internal/controller/nodeclassifier_controller_status_test.go index 22913022..983c7194 100644 --- a/internal/controller/nodeclassifier_controller_status_test.go +++ b/internal/controller/nodeclassifier_controller_status_test.go @@ -178,6 +178,50 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { } }) + // A healthy Config must not mask a broken one. Both cases below are a server + // demonstrably running something that is not this generation, exactly like a + // stale render, so neither may be hidden behind a Config that is current. + t.Run("an unrecorded render in one Config outranks a current one in another", func(t *testing.T) { + second := newConfig("staging", withNodeClassifierRef()) + legacy := encSecret("staging", encURL) + delete(legacy.Annotations, AnnotationRenderedFrom) + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), second, + encSecret("production", encURL, current), legacy) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + requireErrorCondition(t, got.Status.Conditions, "RenderSourceUnknown") + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond != nil && !strings.Contains(cond.Message, "staging-enc") { + t.Errorf("message = %q, want it to name the Secret that is holding the classifier back", cond.Message) + } + }) + + t.Run("a foreign render in one Config outranks a current one in another", func(t *testing.T) { + second := newConfig("staging", withNodeClassifierRef()) + c := setupTestClient(nc.DeepCopy(), cfg.DeepCopy(), second, + encSecret("production", encURL, current), + encSecret("staging", encURL, renderSource{Name: "someone-else", Generation: 1})) + r := newNodeClassifierReconciler(c) + if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + if cond != nil && !strings.Contains(cond.Message, "staging-enc") { + t.Errorf("message = %q, want it to name the Secret that is holding the classifier back", cond.Message) + } + }) + // Where several Configs render the same classifier, one still on an earlier // generation holds the whole resource back -- the current spec is not in // effect everywhere yet. Without that rule the verdict would depend on which diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index a8d4e018..3f8525b1 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -44,9 +44,10 @@ func setupTestClient(objs ...client.Object) client.Client { // is genuinely not ready. Checking the reason alone would let a condition that // names the failure while still reporting Ready=True pass unnoticed. // -// The condition type is not a parameter because every resource with a -// readiness condition names it "Ready"; see ConditionSigningPolicyReady and -// its siblings. +// The condition type is not a parameter because the resources this is used on +// -- SigningPolicy and NodeClassifier -- both name their readiness condition +// "Ready". Others do not (ConditionCAReady, ConditionConfigReady), so a caller +// from elsewhere fails at the Fatalf below rather than asserting nothing. func requireErrorCondition(t *testing.T, conditions []metav1.Condition, reason string) { t.Helper() const condType = openvoxv1alpha1.ConditionSigningPolicyReady From 3bbbc7475b978b6cdaa81ef08c2e072d91dfe83a Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Wed, 9 Sep 2026 23:24:14 +0200 Subject: [PATCH 5/6] fix: put ReportProcessor on the same footing, and stop calling overrides errors Three independent reviews of this branch converged on the same two gaps. ReportProcessor is the third resource of this kind and was left matching by endpoint name alone, which is exactly what the rest of this branch argues against: a spec edit whose re-render fails leaves the previous file in place, the name still matches, and the processor reports Active while the servers post to the old endpoint. It now carries the rendered-from annotation and reads it like its two siblings, so all three answer the same question the same way and the last hand-rolled parser of a rendered file is gone. It also picks up the generation-captured-before-observation fix the other two got, and a test for it. A deliberate override is not a fault. Setting autosignCommand or externalNodesCommand replaces the built-in binary on purpose, and reporting the bypassed resource as phase Error made a supported configuration -- one the charts exercise in CI, see charts/openvox-stack/ci/puppet-command-overrides -values.yaml -- look permanently broken in the Phase column. Both CRDs gain a Disabled phase for that case. Ready stays False, because the resource genuinely is not in effect. Two more from the same reviews: - The Configs come back from the field index in map iteration order, so joining their Secret names into the condition Message made it a non-deterministic function of unchanged state. Every reordering was a real status write that re-enqueued this controller and fanned a Config re-render out behind it. The slices are sorted before they reach a message, which is what the render side already does. - The upgrade path -- adopting a Secret the previous version wrote, without the annotation -- was the most upgrade-critical behaviour in the branch and had no test. It does now, including the Config controller stamping the source and the resource going Active afterwards. Docs: docs/getting-started/installation.md gains an upgrade section covering the transient RenderedConfigSourceUnknown window, the paused-Config case, the renamed reason strings and the new Disabled phase. The reference pages get a reason table for ReportProcessor, which had none, and the shared mechanism on the index page is now true of all three resources rather than only two. --- api/v1alpha1/nodeclassifier_types.go | 8 +- api/v1alpha1/signingpolicy_types.go | 8 +- ...openvox.voxpupuli.org_nodeclassifiers.yaml | 1 + ...openvox.voxpupuli.org_signingpolicies.yaml | 1 + ...openvox.voxpupuli.org_nodeclassifiers.yaml | 1 + ...openvox.voxpupuli.org_signingpolicies.yaml | 1 + docs/getting-started/installation.md | 27 ++++++ docs/reference/index.md | 14 ++- docs/reference/nodeclassifier.md | 9 +- docs/reference/reportprocessor.md | 26 +++++- docs/reference/signingpolicy.md | 7 +- internal/controller/config_controller_test.go | 2 +- internal/controller/config_reports.go | 7 +- .../controller/nodeclassifier_controller.go | 17 +++- .../nodeclassifier_controller_status_test.go | 8 +- .../rendered_source_contract_test.go | 92 +++++++++++++++++++ .../controller/reportprocessor_controller.go | 73 +++++++-------- .../reportprocessor_controller_status_test.go | 36 +++----- internal/controller/server_deployment_test.go | 2 +- .../controller/signingpolicy_controller.go | 4 +- .../signingpolicy_controller_status_test.go | 2 +- internal/controller/status_generation_test.go | 33 +++++++ internal/controller/testutil_test.go | 4 +- 23 files changed, 292 insertions(+), 91 deletions(-) diff --git a/api/v1alpha1/nodeclassifier_types.go b/api/v1alpha1/nodeclassifier_types.go index 2a0d1a9c..b150fd11 100644 --- a/api/v1alpha1/nodeclassifier_types.go +++ b/api/v1alpha1/nodeclassifier_types.go @@ -149,12 +149,16 @@ type NodeClassifierCache struct { } // NodeClassifierPhase represents the current lifecycle phase. -// +kubebuilder:validation:Enum=Active;Error +// +kubebuilder:validation:Enum=Active;Disabled;Error type NodeClassifierPhase string const ( NodeClassifierPhaseActive NodeClassifierPhase = "Active" - NodeClassifierPhaseError NodeClassifierPhase = "Error" + // NodeClassifierPhaseDisabled marks a classifier that is deliberately + // bypassed rather than broken, so an intentional configuration does not + // read as a fault in the Phase column. + NodeClassifierPhaseDisabled NodeClassifierPhase = "Disabled" + NodeClassifierPhaseError NodeClassifierPhase = "Error" ) // NodeClassifierStatus defines the observed state of NodeClassifier. diff --git a/api/v1alpha1/signingpolicy_types.go b/api/v1alpha1/signingpolicy_types.go index cd33c3d9..1f7947d3 100644 --- a/api/v1alpha1/signingpolicy_types.go +++ b/api/v1alpha1/signingpolicy_types.go @@ -124,12 +124,16 @@ type SecretKeyRef struct { } // SigningPolicyPhase represents the current lifecycle phase of a SigningPolicy. -// +kubebuilder:validation:Enum=Active;Error +// +kubebuilder:validation:Enum=Active;Disabled;Error type SigningPolicyPhase string const ( SigningPolicyPhaseActive SigningPolicyPhase = "Active" - SigningPolicyPhaseError SigningPolicyPhase = "Error" + // SigningPolicyPhaseDisabled marks a policy that is deliberately bypassed + // rather than broken, so an intentional configuration does not read as a + // fault in the Phase column. + SigningPolicyPhaseDisabled SigningPolicyPhase = "Disabled" + SigningPolicyPhaseError SigningPolicyPhase = "Error" ) // SigningPolicyStatus defines the observed state of SigningPolicy. diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_nodeclassifiers.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_nodeclassifiers.yaml index 219dcce7..0059f43f 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_nodeclassifiers.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_nodeclassifiers.yaml @@ -296,6 +296,7 @@ spec: the phase does not change what the operator does. enum: - Active + - Disabled - Error type: string type: object diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_signingpolicies.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_signingpolicies.yaml index 69fb2547..d9a345a7 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_signingpolicies.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_signingpolicies.yaml @@ -286,6 +286,7 @@ spec: the phase does not change what the operator does. enum: - Active + - Disabled - Error type: string type: object diff --git a/config/crd/bases/openvox.voxpupuli.org_nodeclassifiers.yaml b/config/crd/bases/openvox.voxpupuli.org_nodeclassifiers.yaml index 219dcce7..0059f43f 100644 --- a/config/crd/bases/openvox.voxpupuli.org_nodeclassifiers.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_nodeclassifiers.yaml @@ -296,6 +296,7 @@ spec: the phase does not change what the operator does. enum: - Active + - Disabled - Error type: string type: object diff --git a/config/crd/bases/openvox.voxpupuli.org_signingpolicies.yaml b/config/crd/bases/openvox.voxpupuli.org_signingpolicies.yaml index 69fb2547..d9a345a7 100644 --- a/config/crd/bases/openvox.voxpupuli.org_signingpolicies.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_signingpolicies.yaml @@ -286,6 +286,7 @@ spec: the phase does not change what the operator does. enum: - Active + - Disabled - Error type: string type: object diff --git a/docs/getting-started/installation.md b/docs/getting-started/installation.md index 7ba04419..414f16bb 100644 --- a/docs/getting-started/installation.md +++ b/docs/getting-started/installation.md @@ -55,6 +55,33 @@ In namespace mode the operator uses Role/RoleBinding instead of ClusterRole/Clus ## Upgrading +### Status of SigningPolicy, NodeClassifier and ReportProcessor + +From the version that introduced the `openvox.voxpupuli.org/rendered-from` +annotation, these three resources derive their `Ready` condition from the +Secrets the Config controller renders, and match themselves against that +annotation. Two things follow for an upgrade: + +- Secrets rendered by the previous version carry no annotation, so every + SigningPolicy, NodeClassifier and ReportProcessor reports + `RenderedConfigSourceUnknown` with `Ready=False` until the Config controller + re-renders -- normally seconds after the new operator starts. A Config that is + [paused](../guides/pausing-reconciliation.md) never re-renders, so its + resources stay in that state until it is resumed. Where several Configs + reference one NodeClassifier, it can briefly flip to `Ready=False` while the + Configs are re-rendered one at a time. +- The condition `reason` strings changed. `PolicyRendered` and `ConfigRendered` + became `Rendered`, and the single catch-all `Error` reason was split into + specific cases. Automation matching the old strings needs updating; see the + reason tables in the [CRD reference](../reference/index.md). + +A resource that is deliberately bypassed by an `autosignCommand` or +`externalNodesCommand` override now reports `phase: Disabled` rather than +`Active`, with `Ready=False` and a reason naming the override. Alerting on +`phase: Error` is unaffected by that case; alerting on `Ready=True` is not. + +### CRDs are not upgraded by Helm + Helm installs the CRDs from the chart's `crds/` directory on the first install, but [does not update them on `helm upgrade`](https://helm.sh/docs/chart_best_practices/custom_resource_definitions/). An operator upgraded with `helm upgrade` alone keeps running against the CRDs of diff --git a/docs/reference/index.md b/docs/reference/index.md index b7501105..e007e5c2 100644 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -185,7 +185,10 @@ annotation listing the resources its content was built from and the matches itself against, so `Ready` distinguishes "my current spec is in effect" from "an earlier version of it is": a spec edit whose re-render fails leaves the previous Secret in place, and the resource reports `RenderedConfigStale` rather -than claiming the new spec reached a server. +than claiming the new spec reached a server. A Secret rendered before this +mechanism existed carries no annotation at all, which is not the same as being +rendered from nothing; those resources report `RenderedConfigSourceUnknown` +until the Config controller re-renders. Because the annotation records the generation, this only covers failures a spec edit caused. A render that starts failing under an *unchanged* spec -- a @@ -193,10 +196,11 @@ referenced credential Secret rotated out from under it -- leaves the generation matching, so the resource keeps reporting `Ready`. The render failure is an event on the Config, which is where that case is visible. -A Secret rendered before this mechanism existed carries no annotation at all, -which is not the same as being rendered from nothing. Those resources report -`RenderSourceUnknown` until the Config controller re-renders, rather than -claiming they were left out. +A resource that is deliberately bypassed reports `phase: Disabled` rather than +`Error` -- an `autosignCommand` or `externalNodesCommand` override replaces the +built-in binary, which is a configuration choice, not a fault. `Ready` is still +`False`, because the resource genuinely is not in effect. Everything else that +is not `Active` is `Error`. Any resource can additionally carry `Paused` -- see [Pausing Reconciliation](../guides/pausing-reconciliation.md). diff --git a/docs/reference/nodeclassifier.md b/docs/reference/nodeclassifier.md index c431d712..633a2b41 100644 --- a/docs/reference/nodeclassifier.md +++ b/docs/reference/nodeclassifier.md @@ -177,6 +177,7 @@ At most one authentication method may be configured. | Phase | Description | |---|---| | `Active` | Classifier configuration is rendered and active | +| `Disabled` | Deliberately bypassed by an [`externalNodesCommand`](config.md) override -- a configuration choice, not a fault | | `Error` | The classifier is not in effect -- see the `Ready` condition for which case | The status is derived from the rendered ENC Secret, so it reports whether this @@ -186,11 +187,15 @@ well-formed. The `Ready` condition carries the reason: | Reason | Meaning | |---|---| | `Rendered` | The endpoint is present in the rendered Secret, at the classifier's current generation | -| `NotReferenced` | No [Config](config.md) sets `nodeClassifierRef` to this NodeClassifier, so nothing renders it | +| `NoConfig` | No [Config](config.md) sets `nodeClassifierRef` to this NodeClassifier, so nothing renders it | | `OverriddenByExternalNodesCommand` | Every Config referencing it sets [`spec.puppet.externalNodesCommand`](config.md), which replaces the built-in binary and bypasses NodeClassifier resources | | `NotRendered` | No Secret rendered from this NodeClassifier exists, or the one that exists was rendered from a different one | | `RenderedConfigStale` | A Secret was rendered from this NodeClassifier, but from an earlier generation | -| `RenderSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | +| `RenderedConfigSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | + +Automation upgrading from an earlier operator version should note that these +reasons replace the previous two: `ConfigRendered` became `Rendered`, and a +single catch-all `Error` reason was split into the specific cases above. `enc.yaml` carries no resource name, so the Secret's `openvox.voxpupuli.org/rendered-from` annotation is what ties the rendered file diff --git a/docs/reference/reportprocessor.md b/docs/reference/reportprocessor.md index e757d7d3..0a8eb3c0 100644 --- a/docs/reference/reportprocessor.md +++ b/docs/reference/reportprocessor.md @@ -183,8 +183,30 @@ Either `value` or `valueFrom` may be set, not both. | Phase | Description | |---|---| -| `Active` | Report processor configuration is rendered and active | -| `Error` | Configuration error (e.g. referenced Secret not found) | +| `Active` | The endpoint is rendered and active | +| `Error` | The endpoint is not in effect -- see the `Ready` condition for which case | + +The status is derived from the rendered report-webhook Secret, so it reports +whether this processor actually reached a server rather than whether the +resource itself is well-formed. The `Ready` condition carries the reason: + +| Reason | Meaning | +|---|---| +| `Rendered` | The endpoint is present in the rendered Secret, at this processor's current generation | +| `ConfigRefMissing` | `spec.configRef` is empty, so the processor is bound to no Config | +| `ConfigNotFound` | `spec.configRef` points at a [Config](config.md) that does not exist | +| `NotRendered` | The Secret does not (yet) contain an endpoint for this processor | +| `RenderedConfigStale` | The Secret contains this processor, but as it was at an earlier generation | +| `RenderedConfigSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | + +The Secret's `openvox.voxpupuli.org/rendered-from` annotation names the +processors its content was built from and the generation each was rendered at, +which is what separates `Rendered` from `RenderedConfigStale`. Rendering +failures are reported on the Config that owns the Secret, as a +`ReportWebhookRenderFailed` event; as with the other policy resources, a render +that fails under an unchanged spec leaves the generation matching, so the +processor keeps reporting `Active`. See the +[shared mechanism](index.md#status-phases-and-conditions). ## Processor Types diff --git a/docs/reference/signingpolicy.md b/docs/reference/signingpolicy.md index a15874d1..505ce4a1 100644 --- a/docs/reference/signingpolicy.md +++ b/docs/reference/signingpolicy.md @@ -215,6 +215,7 @@ Either `value` or `valueFrom` must be set. | Phase | Description | |---|---| | `Active` | Policy is rendered and active | +| `Disabled` | Deliberately bypassed by an [`autosignCommand`](config.md) override -- a configuration choice, not a fault | | `Error` | Policy is not in effect -- see the `Ready` condition for which case | The status is derived from the rendered autosign policy Secret, so it reports @@ -230,7 +231,11 @@ itself is well-formed. The `Ready` condition carries the reason: | `OverriddenByAutosignCommand` | Every Config referencing the CA sets [`spec.puppet.autosignCommand`](config.md), which replaces the built-in binary and bypasses SigningPolicy resources | | `NotRendered` | The Secret does not (yet) contain this policy | | `RenderedConfigStale` | The Secret contains this policy, but as it was at an earlier generation | -| `RenderSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | +| `RenderedConfigSourceUnknown` | The Secret predates this mechanism and does not record what it was rendered from; it resolves once the Config controller re-renders | + +Automation upgrading from an earlier operator version should note that these +reasons replace the previous two: `PolicyRendered` became `Rendered`, and a +single catch-all `Error` reason was split into the specific cases above. The Secret's `openvox.voxpupuli.org/rendered-from` annotation names the policies its content was built from and the generation each was rendered at, diff --git a/internal/controller/config_controller_test.go b/internal/controller/config_controller_test.go index fe7ddaba..0799d6bf 100644 --- a/internal/controller/config_controller_test.go +++ b/internal/controller/config_controller_test.go @@ -280,7 +280,7 @@ func TestConfigReconcile_AutosignCommandOverride(t *testing.T) { func TestConfigReconcile_ExternalNodesCommandOverride(t *testing.T) { cfg := newConfig("production", withNodeClassifierRef(), - withExternalNodesCommand("/usr/local/bin/custom-enc"), + withExternalNodesCommand(), ) nc := newNodeClassifier("my-enc", "https://enc.example.com") c := setupTestClient(cfg, nc) diff --git a/internal/controller/config_reports.go b/internal/controller/config_reports.go index 2c40e393..0b6f5a46 100644 --- a/internal/controller/config_reports.go +++ b/internal/controller/config_reports.go @@ -69,7 +69,12 @@ func (r *ConfigReconciler) reconcileReportWebhookSecret(ctx context.Context, cfg "report-webhook.yaml": []byte(webhookYAML), } - return r.reconcileSecret(ctx, cfg, secretName, data, nil) + sources := make([]renderSource, 0, len(processors)) + for i := range processors { + sources = append(sources, sourceOf(&processors[i])) + } + + return r.reconcileSecret(ctx, cfg, secretName, data, renderedFromAnnotation(sources)) } // reportWebhookConfig mirrors the YAML structure read by openvox-report. diff --git a/internal/controller/nodeclassifier_controller.go b/internal/controller/nodeclassifier_controller.go index 62f37456..b44b1084 100644 --- a/internal/controller/nodeclassifier_controller.go +++ b/internal/controller/nodeclassifier_controller.go @@ -3,6 +3,7 @@ package controller import ( "context" "fmt" + "sort" "strings" corev1 "k8s.io/api/core/v1" @@ -96,7 +97,7 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, return "", reasonLookupFailed, fmt.Sprintf("listing Configs for NodeClassifier %s: %v", nc.Name, err) } if len(configs) == 0 { - return openvoxv1alpha1.NodeClassifierPhaseError, "NotReferenced", + return openvoxv1alpha1.NodeClassifierPhaseError, "NoConfig", fmt.Sprintf("no Config sets nodeClassifierRef to %s, so no ENC configuration is rendered", nc.Name) } @@ -105,7 +106,7 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, // Secret rendered before the override was set still exists, and calling that // "active" would claim an effect this NodeClassifier no longer has. if allOverride(configs, overrideExternalNodes) { - return openvoxv1alpha1.NodeClassifierPhaseError, "OverriddenByExternalNodesCommand", + return openvoxv1alpha1.NodeClassifierPhaseDisabled, "OverriddenByExternalNodesCommand", fmt.Sprintf("spec.puppet.externalNodesCommand is set on every Config referencing NodeClassifier %s, "+ "which bypasses NodeClassifier resources", nc.Name) } @@ -141,6 +142,16 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, } } + // The Configs come back in the field index's map iteration order, so the + // slices are sorted before they reach a message. An unsorted join makes the + // condition Message a non-deterministic function of the same state, and each + // reordering is a real status write that re-enqueues this controller and + // fans a Config re-render out behind it. + sort.Strings(rendered) + sort.Strings(stale) + sort.Strings(unrecorded) + sort.Strings(foreign) + // A Secret that exists but does not match this NodeClassifier's current // generation means a server is running something else -- an earlier // generation, another classifier, or content of unknown vintage. All three @@ -160,7 +171,7 @@ func (r *NodeClassifierReconciler) observe(ctx context.Context, fmt.Sprintf("Secret %s was rendered from a different NodeClassifier, so NodeClassifier %s "+ "is not in effect for every Config referencing it", strings.Join(foreign, ", "), nc.Name) case len(unrecorded) > 0: - return openvoxv1alpha1.NodeClassifierPhaseError, "RenderSourceUnknown", + return openvoxv1alpha1.NodeClassifierPhaseError, "RenderedConfigSourceUnknown", fmt.Sprintf("Secret %s does not record which resources it was rendered from, so the Config "+ "controller has not re-rendered it yet; its contents are unchanged in the meantime", strings.Join(unrecorded, ", ")) diff --git a/internal/controller/nodeclassifier_controller_status_test.go b/internal/controller/nodeclassifier_controller_status_test.go index 983c7194..f3c4722d 100644 --- a/internal/controller/nodeclassifier_controller_status_test.go +++ b/internal/controller/nodeclassifier_controller_status_test.go @@ -65,7 +65,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - requireErrorCondition(t, got.Status.Conditions, "NotReferenced") + requireErrorCondition(t, got.Status.Conditions, "NoConfig") }) t.Run("error while the secret has not been rendered", func(t *testing.T) { @@ -128,7 +128,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { t.Run("error when externalNodesCommand bypasses the classifier", func(t *testing.T) { overridden := newConfig("production", withNodeClassifierRef(), - withExternalNodesCommand("/usr/local/bin/custom-enc")) + withExternalNodesCommand()) c := setupTestClient(nc.DeepCopy(), overridden, encSecret("production", encURL, current)) r := newNodeClassifierReconciler(c) if _, err := r.Reconcile(testCtx(), testRequest("my-enc")); err != nil { @@ -156,7 +156,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - requireErrorCondition(t, got.Status.Conditions, "RenderSourceUnknown") + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigSourceUnknown") }) // A Config that has not rendered anything yet says nothing about the @@ -195,7 +195,7 @@ func TestNodeClassifierReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading NodeClassifier: %v", err) } - requireErrorCondition(t, got.Status.Conditions, "RenderSourceUnknown") + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigSourceUnknown") cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) if cond != nil && !strings.Contains(cond.Message, "staging-enc") { t.Errorf("message = %q, want it to name the Secret that is holding the classifier back", cond.Message) diff --git a/internal/controller/rendered_source_contract_test.go b/internal/controller/rendered_source_contract_test.go index edc9e9da..9354d2e7 100644 --- a/internal/controller/rendered_source_contract_test.go +++ b/internal/controller/rendered_source_contract_test.go @@ -8,6 +8,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" ) @@ -168,3 +169,94 @@ func TestRenderedFromRoundTrip_SigningPolicy(t *testing.T) { t.Errorf("observedGeneration = %d, want 4", got.Status.ObservedGeneration) } } + +// The upgrade path: a Secret the previous operator version wrote carries no +// annotation, so the resource cannot tell whether it is in it. It must report +// that rather than claim it was left out, and the first Config reconcile must +// adopt the Secret and clear the state. +func TestRenderedFromAdoptsAnUnannotatedSecret(t *testing.T) { + nc := newNodeClassifier("my-enc", "https://foreman.example.invalid") + nc.Generation = 3 + cfg := newConfig("production", withNodeClassifierRef()) + + // As the previous operator version left it: owned by the Config, holding the + // right content, but with no record of what it was rendered from. + legacy := encSecret("production", "https://foreman.example.invalid") + delete(legacy.Annotations, AnnotationRenderedFrom) + if err := controllerutil.SetControllerReference(cfg, legacy, testScheme()); err != nil { + t.Fatalf("setting owner reference: %v", err) + } + c := setupTestClient(cfg, nc, legacy) + + key := types.NamespacedName{Name: "my-enc", Namespace: testNamespace} + got := &openvoxv1alpha1.NodeClassifier{} + + if _, err := newNodeClassifierReconciler(c).Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("nodeclassifier reconcile before adoption: %v", err) + } + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigSourceUnknown") + + // The Config controller re-renders on its first pass and stamps the source. + if _, err := newConfigReconciler(c).Reconcile(testCtx(), testRequest("production")); err != nil { + t.Fatalf("config reconcile: %v", err) + } + if want := "my-enc=3"; renderedFrom(t, c, "production-enc") != want { + t.Fatalf("rendered-from = %q, want %q after adoption", renderedFrom(t, c, "production-enc"), want) + } + + if _, err := newNodeClassifierReconciler(c).Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("nodeclassifier reconcile after adoption: %v", err) + } + if err := c.Get(testCtx(), key, got); err != nil { + t.Fatalf("re-reading NodeClassifier: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseActive { + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + t.Errorf("phase = %q, want Active once the Secret is adopted (condition %+v)", got.Status.Phase, cond) + } +} + +// A deliberate override is a configuration choice, not a fault, so it must not +// land in the Error phase where it would trip phase-based alerting. +func TestOverriddenResourcesReportDisabledNotError(t *testing.T) { + t.Run("SigningPolicy", func(t *testing.T) { + sp := newSigningPolicy("test-policy", testCAName) + sp.Generation = 1 + c := setupTestClient(sp, newCertificateAuthority(testCAName), + newConfig("production", withAuthorityRef(testCAName), withAutosignCommand()), + autosignPolicySecret(renderSource{Name: "test-policy", Generation: 1})) + if _, err := newSigningPolicyReconciler(c).Reconcile(testCtx(), testRequest("test-policy")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.SigningPolicy{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "test-policy", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading SigningPolicy: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.SigningPolicyPhaseDisabled { + t.Errorf("phase = %q, want Disabled", got.Status.Phase) + } + requireErrorCondition(t, got.Status.Conditions, "OverriddenByAutosignCommand") + }) + + t.Run("NodeClassifier", func(t *testing.T) { + nc := newNodeClassifier("my-enc", "https://foreman.example.invalid") + nc.Generation = 1 + c := setupTestClient(nc, + newConfig("production", withNodeClassifierRef(), withExternalNodesCommand()), + encSecret("production", "https://foreman.example.invalid", renderSource{Name: "my-enc", Generation: 1})) + if _, err := newNodeClassifierReconciler(c).Reconcile(testCtx(), testRequest("my-enc")); err != nil { + t.Fatalf("reconcile: %v", err) + } + got := &openvoxv1alpha1.NodeClassifier{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "my-enc", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading NodeClassifier: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.NodeClassifierPhaseDisabled { + t.Errorf("phase = %q, want Disabled", got.Status.Phase) + } + requireErrorCondition(t, got.Status.Conditions, "OverriddenByExternalNodesCommand") + }) +} diff --git a/internal/controller/reportprocessor_controller.go b/internal/controller/reportprocessor_controller.go index eab02ca3..42827129 100644 --- a/internal/controller/reportprocessor_controller.go +++ b/internal/controller/reportprocessor_controller.go @@ -3,7 +3,6 @@ package controller import ( "context" "fmt" - "slices" "strings" corev1 "k8s.io/api/core/v1" @@ -17,7 +16,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/yaml" openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" ) @@ -60,6 +58,12 @@ func (r *ReportProcessorReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } + // The generation the verdict is about, captured before the observation: + // updateStatusWithRetry re-reads the object, so a spec edit landing in + // between would otherwise stamp the new generation onto a verdict derived + // from the old spec. + generation := rp.Generation + phase, reason, message := r.observe(ctx, rp) if reason == reasonLookupFailed { // A transient lookup failure says nothing about the ReportProcessor. @@ -68,7 +72,7 @@ func (r *ReportProcessorReconciler) Reconcile(ctx context.Context, req ctrl.Requ } if err := updateStatusWithRetry(ctx, r.Client, rp, func() { - rp.Status.ObservedGeneration = rp.Generation + rp.Status.ObservedGeneration = generation rp.Status.Phase = phase status := metav1.ConditionFalse if phase == openvoxv1alpha1.ReportProcessorPhaseActive { @@ -79,7 +83,7 @@ func (r *ReportProcessorReconciler) Reconcile(ctx context.Context, req ctrl.Requ Status: status, Reason: reason, Message: message, - ObservedGeneration: rp.Generation, + ObservedGeneration: generation, }) }); err != nil { return ctrl.Result{}, fmt.Errorf("updating ReportProcessor status %s: %w", rp.Name, err) @@ -118,46 +122,35 @@ func (r *ReportProcessorReconciler) observe(ctx context.Context, rp *openvoxv1al return "", reasonLookupFailed, fmt.Sprintf("getting Secret %s: %v", secretName, err) } - rendered, err := renderedEndpointNames(secret.Data["report-webhook.yaml"]) - if err != nil { - return openvoxv1alpha1.ReportProcessorPhaseError, "RenderedConfigUnreadable", - fmt.Sprintf("Secret %s does not contain a readable report-webhook.yaml: %v", secretName, err) - } - if slices.Contains(rendered, rp.Name) { - return openvoxv1alpha1.ReportProcessorPhaseActive, "Rendered", - fmt.Sprintf("Endpoint is present in Secret %s", secretName) + if !renderedSourceRecorded(secret.Annotations) { + return openvoxv1alpha1.ReportProcessorPhaseError, "RenderedConfigSourceUnknown", + fmt.Sprintf("Secret %s does not record which resources it was rendered from, so the Config "+ + "controller has not re-rendered it yet; its contents are unchanged in the meantime", secretName) } - return openvoxv1alpha1.ReportProcessorPhaseError, "NotRendered", - fmt.Sprintf("Secret %s does not contain an endpoint for this ReportProcessor", secretName) -} - -// renderedEndpointNames extracts the endpoint names from a rendered -// report-webhook.yaml. -func renderedEndpointNames(data []byte) ([]string, error) { - if len(data) == 0 { - return nil, fmt.Errorf("report-webhook.yaml is empty") - } - var parsed struct { - Endpoints []struct { - Name string `json:"name"` - } `json:"endpoints"` + // The annotation names the processors the content was rendered from and the + // generation each was rendered at. An endpoint name alone cannot tell a + // current spec from one whose re-render failed and left the old file behind. + generation, ok := renderedGeneration(secret.Annotations, rp.Name) + switch { + case !ok: + return openvoxv1alpha1.ReportProcessorPhaseError, "NotRendered", + fmt.Sprintf("Secret %s does not contain an endpoint for this ReportProcessor", secretName) + case generation != rp.Generation: + return openvoxv1alpha1.ReportProcessorPhaseError, "RenderedConfigStale", + fmt.Sprintf("Secret %s was rendered from an earlier generation of ReportProcessor %s; "+ + "the current spec has not reached a server", secretName, rp.Name) } - if err := yaml.Unmarshal(data, &parsed); err != nil { - return nil, err - } - names := make([]string, 0, len(parsed.Endpoints)) - for _, ep := range parsed.Endpoints { - names = append(names, ep.Name) - } - return names, nil + + return openvoxv1alpha1.ReportProcessorPhaseActive, "Rendered", + fmt.Sprintf("Endpoint is present in Secret %s", secretName) } func (r *ReportProcessorReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&openvoxv1alpha1.ReportProcessor{}). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc( - reportProcessorsForSecret(mgr.GetClient()), + reportProcessorsForSecret(), )). Watches(&openvoxv1alpha1.Config{}, handler.EnqueueRequestsFromMapFunc( reportProcessorsForConfig(mgr.GetClient()), @@ -167,14 +160,12 @@ func (r *ReportProcessorReconciler) SetupWithManager(mgr ctrl.Manager) error { // reportProcessorsForSecret maps a rendered report-webhook Secret back to the // ReportProcessors it was rendered from. -func reportProcessorsForSecret(c client.Client) handler.MapFunc { - return func(ctx context.Context, obj client.Object) []ctrl.Request { - name := obj.GetName() - if !strings.HasSuffix(name, "-report-webhook") { +func reportProcessorsForSecret() handler.MapFunc { + return func(_ context.Context, obj client.Object) []ctrl.Request { + if !strings.HasSuffix(obj.GetName(), "-report-webhook") { return nil } - cfgName := strings.TrimSuffix(name, "-report-webhook") - return reportProcessorRequests(ctx, c, obj.GetNamespace(), cfgName) + return renderedSourceRequests(obj) } } diff --git a/internal/controller/reportprocessor_controller_status_test.go b/internal/controller/reportprocessor_controller_status_test.go index 592d02b1..fbcbb4e7 100644 --- a/internal/controller/reportprocessor_controller_status_test.go +++ b/internal/controller/reportprocessor_controller_status_test.go @@ -11,24 +11,32 @@ import ( openvoxv1alpha1 "github.com/slauger/openvox-operator/api/v1alpha1" ) -func webhookSecret(cfgName string, endpointNames ...string) *corev1.Secret { +// webhookSecret builds a report-webhook Secret as the Config controller renders +// it: the endpoints in report-webhook.yaml, and the ReportProcessors they came +// from in the annotation. +func webhookSecret(cfgName string, sources ...renderSource) *corev1.Secret { yaml := "endpoints:\n" - for _, n := range endpointNames { - yaml += " - name: " + n + "\n url: https://example.invalid\n" + for _, s := range sources { + yaml += " - name: " + s.Name + "\n url: https://example.invalid\n" } return &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: cfgName + "-report-webhook", Namespace: testNamespace}, - Data: map[string][]byte{"report-webhook.yaml": []byte(yaml)}, + ObjectMeta: metav1.ObjectMeta{ + Name: cfgName + "-report-webhook", + Namespace: testNamespace, + Annotations: renderedFromAnnotation(sources), + }, + Data: map[string][]byte{"report-webhook.yaml": []byte(yaml)}, } } func TestReportProcessorReconcile_Status(t *testing.T) { rp := newReportProcessor("test-rp", "https://puppetdb.example.invalid") + rp.Generation = 1 cfg := newConfig("production") key := types.NamespacedName{Name: "test-rp", Namespace: testNamespace} t.Run("active once the endpoint is rendered", func(t *testing.T) { - c := setupTestClient(rp.DeepCopy(), cfg.DeepCopy(), webhookSecret("production", "test-rp")) + c := setupTestClient(rp.DeepCopy(), cfg.DeepCopy(), webhookSecret("production", renderSource{Name: "test-rp", Generation: 1})) r := &ReportProcessorReconciler{Client: c, Scheme: testScheme(), Recorder: testRecorder()} if _, err := r.Reconcile(testCtx(), testRequest("test-rp")); err != nil { t.Fatalf("reconcile: %v", err) @@ -78,7 +86,7 @@ func TestReportProcessorReconcile_Status(t *testing.T) { }) t.Run("error when another processor was rendered but not this one", func(t *testing.T) { - c := setupTestClient(rp.DeepCopy(), cfg.DeepCopy(), webhookSecret("production", "someone-else")) + c := setupTestClient(rp.DeepCopy(), cfg.DeepCopy(), webhookSecret("production", renderSource{Name: "someone-else", Generation: 1})) r := &ReportProcessorReconciler{Client: c, Scheme: testScheme(), Recorder: testRecorder()} if _, err := r.Reconcile(testCtx(), testRequest("test-rp")); err != nil { t.Fatalf("reconcile: %v", err) @@ -92,17 +100,3 @@ func TestReportProcessorReconcile_Status(t *testing.T) { } }) } - -func TestRenderedEndpointNames(t *testing.T) { - names, err := renderedEndpointNames([]byte("endpoints:\n - name: a\n url: https://a.invalid\n - name: b\n url: https://b.invalid\n")) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if len(names) != 2 || names[0] != "a" || names[1] != "b" { - t.Errorf("got %v, want [a b]", names) - } - - if _, err := renderedEndpointNames(nil); err == nil { - t.Error("empty input should be an error") - } -} diff --git a/internal/controller/server_deployment_test.go b/internal/controller/server_deployment_test.go index cd471267..3a9ed598 100644 --- a/internal/controller/server_deployment_test.go +++ b/internal/controller/server_deployment_test.go @@ -221,7 +221,7 @@ func TestBuildPodSpec_AutosignCommandSkipsPolicyMount(t *testing.T) { func TestBuildPodSpec_ExternalNodesCommandSkipsENCMount(t *testing.T) { cfg := newConfig("production", withNodeClassifierRef(), - withExternalNodesCommand("/usr/local/bin/custom-enc"), + withExternalNodesCommand(), ) server := newServer("test-server", withServerRole(true)) diff --git a/internal/controller/signingpolicy_controller.go b/internal/controller/signingpolicy_controller.go index a9467586..c3c8d622 100644 --- a/internal/controller/signingpolicy_controller.go +++ b/internal/controller/signingpolicy_controller.go @@ -125,7 +125,7 @@ func (r *SigningPolicyReconciler) observe(ctx context.Context, // before the override was set still exists, and calling that "active" would // claim an effect this policy no longer has. if allOverride(configs, overrideAutosign) { - return openvoxv1alpha1.SigningPolicyPhaseError, "OverriddenByAutosignCommand", + return openvoxv1alpha1.SigningPolicyPhaseDisabled, "OverriddenByAutosignCommand", fmt.Sprintf("spec.puppet.autosignCommand is set on every Config referencing CertificateAuthority %s, "+ "which bypasses SigningPolicy resources", caName) } @@ -141,7 +141,7 @@ func (r *SigningPolicyReconciler) observe(ctx context.Context, } if !renderedSourceRecorded(secret.Annotations) { - return openvoxv1alpha1.SigningPolicyPhaseError, "RenderSourceUnknown", + return openvoxv1alpha1.SigningPolicyPhaseError, "RenderedConfigSourceUnknown", fmt.Sprintf("Secret %s does not record which resources it was rendered from, so the Config "+ "controller has not re-rendered it yet; its contents are unchanged in the meantime", secretName) } diff --git a/internal/controller/signingpolicy_controller_status_test.go b/internal/controller/signingpolicy_controller_status_test.go index d5c788c0..78ad2a96 100644 --- a/internal/controller/signingpolicy_controller_status_test.go +++ b/internal/controller/signingpolicy_controller_status_test.go @@ -132,7 +132,7 @@ func TestSigningPolicyReconcile_Status(t *testing.T) { if err := c.Get(testCtx(), key, got); err != nil { t.Fatalf("reading SigningPolicy: %v", err) } - requireErrorCondition(t, got.Status.Conditions, "RenderSourceUnknown") + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigSourceUnknown") }) t.Run("error when certificateAuthorityRef is empty", func(t *testing.T) { diff --git a/internal/controller/status_generation_test.go b/internal/controller/status_generation_test.go index 0dd8cb23..bc84a6b5 100644 --- a/internal/controller/status_generation_test.go +++ b/internal/controller/status_generation_test.go @@ -114,3 +114,36 @@ func TestSigningPolicyReconcile_ObservedGenerationPredatesTheEdit(t *testing.T) t.Errorf("condition observedGeneration = %d, want %d", cond.ObservedGeneration, observed) } } + +// ReportProcessor matches by endpoint name rather than by annotation, but the +// generation it stamps has to be captured the same way -- the re-read inside +// updateStatusWithRetry is identical for all three. +func TestReportProcessorReconcile_ObservedGenerationPredatesTheEdit(t *testing.T) { + const observed int64 = 2 + rp := newReportProcessor("beta", "https://beta.example.invalid/reports") + rp.Generation = observed + + c, reads := generationBumpingClient(t, 7, + func(o client.Object) bool { _, ok := o.(*openvoxv1alpha1.ReportProcessor); return ok }, + rp, newConfig("production"), + webhookSecret("production", renderSource{Name: "beta", Generation: observed})) + + if _, err := newReportProcessorReconciler(c).Reconcile(testCtx(), testRequest("beta")); err != nil { + t.Fatalf("reconcile: %v", err) + } + if reads() < 2 { + t.Fatalf("only %d read(s) of the ReportProcessor, so no re-read happened and this test proves nothing", reads()) + } + + got := &openvoxv1alpha1.ReportProcessor{} + if err := c.Get(testCtx(), types.NamespacedName{Name: "beta", Namespace: testNamespace}, got); err != nil { + t.Fatalf("reading ReportProcessor: %v", err) + } + if got.Status.Phase != openvoxv1alpha1.ReportProcessorPhaseActive { + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionReportProcessorReady) + t.Fatalf("phase = %q, want Active (condition %+v)", got.Status.Phase, cond) + } + if got.Status.ObservedGeneration != observed { + t.Errorf("observedGeneration = %d, want %d -- the generation the verdict was derived from", got.Status.ObservedGeneration, observed) + } +} diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index 3f8525b1..14951d31 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -162,9 +162,9 @@ func withAutosignCommand() configOption { } } -func withExternalNodesCommand(cmd string) configOption { +func withExternalNodesCommand() configOption { return func(c *openvoxv1alpha1.Config) { - c.Spec.Puppet.ExternalNodesCommand = cmd + c.Spec.Puppet.ExternalNodesCommand = "/usr/local/bin/custom-enc" } } From aea7cc75cb4b5c8f5a1120a9413a3f86c17fb2f1 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Wed, 9 Sep 2026 23:26:02 +0200 Subject: [PATCH 6/6] chore: run go mod tidy after dropping the last direct yaml import The rendered-file parsers were the only direct users of sigs.k8s.io/yaml. Reading the rendered-from annotation instead removed the last one, so the dependency moves to indirect and the CI tidy check goes green again. --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 8dee44fa..be1c787d 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,6 @@ require ( k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 sigs.k8s.io/controller-runtime v0.25.0 sigs.k8s.io/gateway-api v1.6.2 - sigs.k8s.io/yaml v1.6.0 ) require ( @@ -113,6 +112,7 @@ require ( sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.4.2 // indirect + sigs.k8s.io/yaml v1.6.0 // indirect ) tool (