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 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/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/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 c4654701..e007e5c2 100644 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -169,5 +169,38 @@ 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 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. 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 +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 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 db7b1c84..633a2b41 100644 --- a/docs/reference/nodeclassifier.md +++ b/docs/reference/nodeclassifier.md @@ -177,7 +177,49 @@ 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) | +| `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 +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 | +| `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 | +| `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 +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, +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 +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/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 002bc8cd..505ce4a1 100644 --- a/docs/reference/signingpolicy.md +++ b/docs/reference/signingpolicy.md @@ -215,7 +215,45 @@ 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) | +| `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 +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 | +| `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, +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, 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/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 ( 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..0799d6bf 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) @@ -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) @@ -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/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..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) + 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/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..b44b1084 --- /dev/null +++ b/internal/controller/nodeclassifier_controller.go @@ -0,0 +1,239 @@ +package controller + +import ( + "context" + "fmt" + "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" + 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, "NoConfig", + 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.NodeClassifierPhaseDisabled, "OverriddenByExternalNodesCommand", + fmt.Sprintf("spec.puppet.externalNodesCommand is set on every Config referencing NodeClassifier %s, "+ + "which bypasses NodeClassifier resources", nc.Name) + } + + var rendered, stale, unrecorded, foreign []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) + } + 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: + foreign = append(foreign, secretName) + case generation != nc.Generation: + stale = append(stale, secretName) + default: + rendered = append(rendered, secretName) + } + } + + // 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 + // 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 { + 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(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, "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, ", ")) + case len(rendered) > 0: + 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 +// 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..f3c4722d --- /dev/null +++ b/internal/controller/nodeclassifier_controller_status_test.go @@ -0,0 +1,264 @@ +package controller + +import ( + "strings" + "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) + } + requireErrorCondition(t, got.Status.Conditions, "NoConfig") + }) + + 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) + } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") + }) + + // 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) + } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") + cond := meta.FindStatusCondition(got.Status.Conditions, openvoxv1alpha1.ConditionNodeClassifierReady) + 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) + } + }) + + // 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) + } + 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) + } + }) + + // 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()) + 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) + } + 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, "RenderedConfigSourceUnknown") + }) + + // 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) + } + }) + + // 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, "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) + } + }) + + 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 + // 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) + } + requireErrorCondition(t, got.Status.Conditions, "RenderedConfigStale") + }) +} + +// 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..5358943e --- /dev/null +++ b/internal/controller/rendered_source.go @@ -0,0 +1,132 @@ +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, ",")} +} + +// 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) { + 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_contract_test.go b/internal/controller/rendered_source_contract_test.go new file mode 100644 index 00000000..9354d2e7 --- /dev/null +++ b/internal/controller/rendered_source_contract_test.go @@ -0,0 +1,262 @@ +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" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + 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) + } +} + +// 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/rendered_source_test.go b/internal/controller/rendered_source_test.go new file mode 100644 index 00000000..4c0dc852 --- /dev/null +++ b/internal/controller/rendered_source_test.go @@ -0,0 +1,143 @@ +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") + } +} + +// 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: 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.Fatalf("got %v, want a request per rendered source", requests) + } + for _, req := range requests { + if req.Namespace != ns { + t.Errorf("namespace = %q, want %q", req.Namespace, ns) + } + } + + // 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: 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/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 95776584..3a9ed598 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) @@ -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 new file mode 100644 index 00000000..c3c8d622 --- /dev/null +++ b/internal/controller/signingpolicy_controller.go @@ -0,0 +1,260 @@ +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.SigningPolicyPhaseDisabled, "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) + } + + if !renderedSourceRecorded(secret.Annotations) { + 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) + } + + // 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..78ad2a96 --- /dev/null +++ b/internal/controller/signingpolicy_controller_status_test.go @@ -0,0 +1,209 @@ +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) + 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) + } + requireErrorCondition(t, got.Status.Conditions, "CertificateAuthorityNotFound") + }) + + // 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) + } + requireErrorCondition(t, got.Status.Conditions, "NoConfig") + }) + + 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) + } + requireErrorCondition(t, got.Status.Conditions, "NotRendered") + }) + + // 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) + } + 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, "RenderedConfigSourceUnknown") + }) + + 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) + } + requireErrorCondition(t, got.Status.Conditions, "CertificateAuthorityRefMissing") + }) + + 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) + } + // 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: + // 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) + } + requireErrorCondition(t, got.Status.Conditions, "OverriddenByAutosignCommand") + }) + + // 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/status_generation_test.go b/internal/controller/status_generation_test.go new file mode 100644 index 00000000..bc84a6b5 --- /dev/null +++ b/internal/controller/status_generation_test.go @@ -0,0 +1,149 @@ +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) + } +} + +// 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 5f932fd8..14951d31 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,35 @@ 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 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 + 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 +85,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. @@ -126,15 +156,15 @@ 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" } } -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" } } @@ -416,7 +446,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 +454,7 @@ func newSigningPolicy(name, caRef string, any bool) *openvoxv1alpha1.SigningPoli }, Spec: openvoxv1alpha1.SigningPolicySpec{ CertificateAuthorityRef: caRef, - Any: any, + Any: true, }, } } @@ -557,6 +587,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 {