diff --git a/.coverage-baseline b/.coverage-baseline index f9f577b8..29179ad4 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -76.7 +77.0 diff --git a/docs/design/watchrule-source-namespace/README.md b/docs/design/watchrule-source-namespace/README.md index 80616cd0..d042e680 100644 --- a/docs/design/watchrule-source-namespace/README.md +++ b/docs/design/watchrule-source-namespace/README.md @@ -117,7 +117,7 @@ are pre-existing defect fixes that carry no API change; the last two are the fea |---|---|---|---|---| | 1 | [Namespace-scoped resync](pr1-namespace-scoped-resync.md) | A per-namespace replay must not sweep other namespaces' manifests of the same type. Bug fix, no API. | — | **landed** | | 2 | [Stream-scope collapse](pr2-stream-scope-collapse.md) | A cluster-wide selection stops silently widening a co-resident named-namespace stream for the same GVR. Bug fix, no API. | 1 | **landed** | -| 3 | [ClusterWatchRule target admission](pr3-clusterwatchrule-target-admission.md) | A ClusterWatchRule may no longer attach to a GitTarget whose namespace its ClusterProvider does not admit. Bug fix, no API. | — | not started | +| 3 | [ClusterWatchRule target admission](pr3-clusterwatchrule-target-admission.md) | A ClusterWatchRule may no longer attach to a GitTarget whose namespace its ClusterProvider does not admit. Bug fix, no API. | — | **landed** | | 4 | [The sourceNamespace field and gate](pr4-source-namespace-field.md) | `WatchRule.spec.sourceNamespace`, `GitTarget.spec.allowedSourceNamespaces`, the delegation flag, the gate, `SourceNamespaceAuthorized`, and the source-scope service. | 1, 2 | not started | | 5 | [The ClusterWatchRule ceiling](pr5-clusterwatchrule-source-ceiling.md) | A declared `allowedSourceNamespaces` narrows a ClusterWatchRule's namespaced streams. | 1, 2, 4 | not started | diff --git a/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md b/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md index 6926831b..97b9e95c 100644 --- a/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md +++ b/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md @@ -2,6 +2,15 @@ > Phase 3 of [source-namespace addressing](README.md). **Depends on:** nothing — independent of the > other PRs and orderable at any point. Bug fix — no API change, no CRD regeneration. +> +> **Status: landed.** The admission decision moved to `internal/authz.GitTargetAdmitted` and is now +> applied by all three call sites that compile a rule or start a data plane for a GitTarget — the +> GitTarget reconciler, the ClusterWatchRule reconciler, and the watch manager's startup bootstrap. +> A refusal stops the data plane *before* it publishes `GitTargetNamespaceNotAuthorized`, and two new +> mappers (ClusterProvider → ClusterWatchRules, Namespace → ClusterWatchRules) make a revocation +> converge on the event rather than on the ~10 minute periodic reconcile. The rest of this page is +> the record of what was wrong and what shipped; sections below are past-tense by design, so a +> regression is recognisable against them. ## What this PR is, precisely diff --git a/internal/authz/clusterprovider_admission.go b/internal/authz/clusterprovider_admission.go new file mode 100644 index 00000000..a332b2c7 --- /dev/null +++ b/internal/authz/clusterprovider_admission.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package authz holds the ClusterProvider namespace-admission decision. +// +// A ClusterProvider is cluster-scoped and holds a credential that can read a lot of a source +// cluster; its spec.allowedNamespaces is that provider's explicit, deny-by-default admission of +// the GitTarget NAMESPACES permitted to mirror through it. +// +// The decision lives in its own package — outside both internal/controller and internal/watch — +// because more than one call site compiles rules or starts a data plane for a GitTarget: the +// GitTarget reconciler, the ClusterWatchRule reconciler, and the watch manager's startup +// bootstrap. A gate that only one of them runs is not a gate, so every such call site routes +// through GitTargetAdmitted and one policy read answers all of them identically. +package authz + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + k8stypes "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +const ( + // ReasonClusterProviderNotFound is the denial reason when a GitTarget's referenced + // ClusterProvider does not exist. This is a HARD GATE: a GitTarget may mirror a source cluster + // ONLY through an existing ClusterProvider, "default" included. The operator never creates one, + // so a target whose provider was never declared is denied rather than mirroring on an implicit + // local identity. + ReasonClusterProviderNotFound = "ClusterProviderNotFound" + + // ReasonNamespaceNotAuthorized is the denial reason when the GitTarget's namespace is not + // admitted by its ClusterProvider's spec.allowedNamespaces — including the case where that + // policy carries a selector the apiserver accepted but that does not convert to a selector. + ReasonNamespaceNotAuthorized = "NamespaceNotAuthorized" +) + +// Decision is the outcome of a ClusterProvider namespace-admission check. A denial always carries +// a Reason and an operator-legible Message; an admission carries neither. +type Decision struct { + // Allowed reports whether the GitTarget's namespace may mirror through its ClusterProvider. + Allowed bool + // Reason is a CamelCase condition reason, set only when Allowed is false. + Reason string + // Message explains the denial to an operator, set only when Allowed is false. + Message string +} + +// GitTargetAdmitted reports whether target's namespace is admitted by the ClusterProvider that +// target references, reading both the provider and the target's Namespace through reader. +// +// It is evaluated on every reconcile rather than only at admission, so a policy TIGHTENED after a +// GitTarget was created revokes it too. The two denial paths are deliberate and ordered: a missing +// provider is denied before any namespace policy is consulted, because an absent provider has no +// policy to consult and defaulting to "allow" would make an undeclared provider a bypass. +// +// A non-NotFound read error is returned as err so the caller requeues instead of tearing down a +// running data plane on a transient apiserver failure. A NotFound Namespace is NOT an error: the +// policy is then evaluated against empty labels, so a `names` entry still admits a namespace that +// has not been created yet while a selector correctly does not match it. +func GitTargetAdmitted( + ctx context.Context, + reader client.Reader, + target *configv1alpha3.GitTarget, +) (Decision, error) { + providerName := target.SourceCluster() + + var provider configv1alpha3.ClusterProvider + if err := reader.Get(ctx, k8stypes.NamespacedName{Name: providerName}, &provider); err != nil { + if apierrors.IsNotFound(err) { + return Decision{ + Reason: ReasonClusterProviderNotFound, + Message: fmt.Sprintf( + "referenced ClusterProvider %q was not found; a GitTarget may mirror a source cluster "+ + "only through an existing ClusterProvider. The operator never creates one: declare it "+ + "yourself, or let the chart render %q via clusterProvider.createDefault", + providerName, configv1alpha3.DefaultClusterProviderName), + }, nil + } + return Decision{}, fmt.Errorf("read ClusterProvider %q: %w", providerName, err) + } + + nsLabels := map[string]string{} + var ns corev1.Namespace + if err := reader.Get(ctx, k8stypes.NamespacedName{Name: target.Namespace}, &ns); err != nil { + if !apierrors.IsNotFound(err) { + return Decision{}, fmt.Errorf("read namespace %q: %w", target.Namespace, err) + } + } else { + nsLabels = ns.Labels + } + + allowed, selErr := provider.AllowsNamespace(target.Namespace, nsLabels) + if selErr != nil { + return Decision{ + Reason: ReasonNamespaceNotAuthorized, + Message: fmt.Sprintf( + "ClusterProvider %q allowedNamespaces selector is invalid: %v", providerName, selErr), + }, nil + } + if !allowed { + return Decision{ + Reason: ReasonNamespaceNotAuthorized, + Message: fmt.Sprintf( + "namespace %q is not permitted to reference ClusterProvider %q (spec.allowedNamespaces)", + target.Namespace, providerName), + }, nil + } + + return Decision{Allowed: true}, nil +} diff --git a/internal/authz/clusterprovider_admission_test.go b/internal/authz/clusterprovider_admission_test.go new file mode 100644 index 00000000..ccf9d7fe --- /dev/null +++ b/internal/authz/clusterprovider_admission_test.go @@ -0,0 +1,301 @@ +// SPDX-License-Identifier: Apache-2.0 + +package authz + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +const testNS = "team-a" + +func admissionScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + require.NoError(t, configv1alpha3.AddToScheme(s)) + require.NoError(t, corev1.AddToScheme(s)) + return s +} + +// targetIn builds a GitTarget in testNS referencing providerName ("" omits +// clusterProviderRef entirely, which resolves to the reserved "default" provider). +func targetIn(providerName string) *configv1alpha3.GitTarget { + t := &configv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "mirror", Namespace: testNS}, + } + if providerName != "" { + t.Spec.ClusterProviderRef = &configv1alpha3.ClusterProviderReference{Name: providerName} + } + return t +} + +func providerNamed(name string, policy *configv1alpha3.AllowedNamespaces) *configv1alpha3.ClusterProvider { + return &configv1alpha3.ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: configv1alpha3.ClusterProviderSpec{AllowedNamespaces: policy}, + } +} + +func namespaceLabeled(labels map[string]string) *corev1.Namespace { + return &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNS, Labels: labels}} +} + +func TestGitTargetAdmitted_Policy(t *testing.T) { + tests := []struct { + name string + objects []client.Object + target *configv1alpha3.GitTarget + wantAllowed bool + wantReason string + wantMessage string + }{ + { + name: "provider missing is a hard denial, not an implicit allow", + objects: []client.Object{namespaceLabeled(nil)}, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonClusterProviderNotFound, + wantMessage: `referenced ClusterProvider "prod-eu-1" was not found`, + }, + { + // A GitTarget with no clusterProviderRef resolves to "default"; that name is not a + // bypass — an undeclared "default" is denied exactly like any other missing provider. + name: "omitted clusterProviderRef resolves to default and is still gated", + objects: []client.Object{namespaceLabeled(nil)}, + target: targetIn(""), + wantAllowed: false, + wantReason: ReasonClusterProviderNotFound, + wantMessage: `referenced ClusterProvider "default" was not found`, + }, + { + name: "omitted clusterProviderRef is admitted by an admitting default provider", + objects: []client.Object{ + providerNamed(configv1alpha3.DefaultClusterProviderName, + &configv1alpha3.AllowedNamespaces{Names: []string{"team-a"}}), + namespaceLabeled(nil), + }, + target: targetIn(""), + wantAllowed: true, + }, + { + name: "nil allowedNamespaces denies by default", + objects: []client.Object{ + providerNamed("prod-eu-1", nil), + namespaceLabeled(nil), + }, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonNamespaceNotAuthorized, + wantMessage: `namespace "team-a" is not permitted to reference ClusterProvider "prod-eu-1"`, + }, + { + name: "empty policy struct denies by default", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{}), + namespaceLabeled(nil), + }, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonNamespaceNotAuthorized, + }, + { + name: "names entry admits", + objects: []client.Object{ + providerNamed("prod-eu-1", + &configv1alpha3.AllowedNamespaces{Names: []string{"team-b", "team-a"}}), + namespaceLabeled(nil), + }, + target: targetIn("prod-eu-1"), + wantAllowed: true, + }, + { + name: "names list not containing the namespace denies", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{Names: []string{"team-b"}}), + namespaceLabeled(nil), + }, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonNamespaceNotAuthorized, + }, + { + // The chart default. An empty selector matches every label set, so it admits every + // namespace — the single-cluster install's "no policy configured" shape. + name: "empty selector admits every namespace", + objects: []client.Object{ + providerNamed("prod-eu-1", + &configv1alpha3.AllowedNamespaces{Selector: &metav1.LabelSelector{}}), + namespaceLabeled(nil), + }, + target: targetIn("prod-eu-1"), + wantAllowed: true, + }, + { + name: "selector matching namespace labels admits", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"tier": "prod"}}, + }), + namespaceLabeled(map[string]string{"tier": "prod"}), + }, + target: targetIn("prod-eu-1"), + wantAllowed: true, + }, + { + name: "selector not matching namespace labels denies", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"tier": "prod"}}, + }), + namespaceLabeled(map[string]string{"tier": "dev"}), + }, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonNamespaceNotAuthorized, + }, + { + // Names and selector are ORed: a listed namespace is admitted even when the selector + // rejects it. Guards against a future refactor turning the OR into an AND. + name: "names admits even when the selector does not match", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{ + Names: []string{"team-a"}, + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"tier": "prod"}}, + }), + namespaceLabeled(map[string]string{"tier": "dev"}), + }, + target: targetIn("prod-eu-1"), + wantAllowed: true, + }, + { + name: "invalid selector denies with a legible message rather than admitting", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{ + Selector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: "tier", Operator: "NotARealOperator", + }}}, + }), + namespaceLabeled(nil), + }, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonNamespaceNotAuthorized, + wantMessage: "allowedNamespaces selector is invalid", + }, + { + // A missing Namespace object is not an error: `names` is evaluated against the NAME, + // which exists in the reference even before the namespace object does. + name: "absent namespace object still admitted by a names entry", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{Names: []string{"team-a"}}), + }, + target: targetIn("prod-eu-1"), + wantAllowed: true, + }, + { + // ...but a selector has no labels to match against, so it correctly does not admit. + name: "absent namespace object is not admitted by a label selector", + objects: []client.Object{ + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"tier": "prod"}}, + }), + }, + target: targetIn("prod-eu-1"), + wantAllowed: false, + wantReason: ReasonNamespaceNotAuthorized, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(admissionScheme(t)).WithObjects(tc.objects...).Build() + + decision, err := GitTargetAdmitted(context.Background(), cl, tc.target) + + require.NoError(t, err) + assert.Equal(t, tc.wantAllowed, decision.Allowed) + if tc.wantAllowed { + assert.Empty(t, decision.Reason, "an admission carries no reason") + assert.Empty(t, decision.Message, "an admission carries no message") + return + } + assert.Equal(t, tc.wantReason, decision.Reason) + assert.NotEmpty(t, decision.Message, "a denial must always explain itself") + if tc.wantMessage != "" { + assert.Contains(t, decision.Message, tc.wantMessage) + } + }) + } +} + +// TestGitTargetAdmitted_ReadErrorsSurfaceAsError pins the difference that decides whether a +// transient apiserver failure tears down a running data plane: a read error must come back as err +// (caller requeues, keeps the stream) and never as a denial (caller stops the stream). +func TestGitTargetAdmitted_ReadErrorsSurfaceAsError(t *testing.T) { + boom := errors.New("apiserver unavailable") + + tests := []struct { + name string + failOn func(key client.ObjectKey, obj client.Object) bool + wantMsg string + }{ + { + name: "ClusterProvider read failure", + failOn: func(_ client.ObjectKey, obj client.Object) bool { + _, ok := obj.(*configv1alpha3.ClusterProvider) + return ok + }, + wantMsg: `read ClusterProvider "prod-eu-1"`, + }, + { + name: "Namespace read failure", + failOn: func(_ client.ObjectKey, obj client.Object) bool { + _, ok := obj.(*corev1.Namespace) + return ok + }, + wantMsg: `read namespace "team-a"`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(admissionScheme(t)). + WithObjects( + providerNamed("prod-eu-1", &configv1alpha3.AllowedNamespaces{Names: []string{"team-a"}}), + namespaceLabeled(nil), + ). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func( + ctx context.Context, c client.WithWatch, key client.ObjectKey, + obj client.Object, opts ...client.GetOption, + ) error { + if tc.failOn(key, obj) { + return boom + } + return c.Get(ctx, key, obj, opts...) + }, + }).Build() + + decision, err := GitTargetAdmitted(context.Background(), cl, targetIn("prod-eu-1")) + + require.Error(t, err) + require.ErrorIs(t, err, boom) + assert.Contains(t, err.Error(), tc.wantMsg) + assert.False(t, decision.Allowed, "an errored decision must not read as admitted") + assert.Empty(t, decision.Reason, + "an errored decision must carry no denial reason: callers must requeue, not refuse") + }) + } +} diff --git a/internal/controller/clusterwatchrule_admission_test.go b/internal/controller/clusterwatchrule_admission_test.go new file mode 100644 index 00000000..7784ca9b --- /dev/null +++ b/internal/controller/clusterwatchrule_admission_test.go @@ -0,0 +1,513 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/authz" + "github.com/ConfigButler/gitops-reverser/internal/rulestore" + internaltypes "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/watch" +) + +// cwaWatchManager is a WatchManagerInterface double that records replans. onReconcile lets a test +// observe the world at the exact moment the data plane is replanned, which is how the +// stop-before-status ordering contract is asserted. +type cwaWatchManager struct { + replans int + replanErr error + onReconcile func() +} + +func (m *cwaWatchManager) ReconcileForRuleChange(context.Context) error { + m.replans++ + if m.onReconcile != nil { + m.onReconcile() + } + return m.replanErr +} + +func (m *cwaWatchManager) ResolveWatchRuleResources( + context.Context, configbutleraiv1alpha3.WatchRule, +) (bool, string) { + return true, "resolved" +} + +func (m *cwaWatchManager) ResolveClusterWatchRuleResources( + context.Context, configbutleraiv1alpha3.ClusterWatchRule, +) (bool, string) { + return true, "resolved" +} + +func (m *cwaWatchManager) StreamSummaryForGitTarget(internaltypes.ResourceReference) watch.StreamSummary { + return watch.StreamSummary{} +} + +func (m *cwaWatchManager) StreamSummaryForWatchRule( + configbutleraiv1alpha3.WatchRule, +) watch.StreamSummary { + return cwaRunningSummary() +} + +func (m *cwaWatchManager) StreamSummaryForClusterWatchRule( + configbutleraiv1alpha3.ClusterWatchRule, +) watch.StreamSummary { + return cwaRunningSummary() +} + +func cwaRunningSummary() watch.StreamSummary { + return watch.StreamSummary{ + Total: 1, Ready: 1, Reason: "Streaming", Message: "1/1 streams running", + } +} + +// --- fixtures --------------------------------------------------------------------------------- + +const ( + cwaRuleName = "mirror-everything" + cwaTargetName = "prod-mirror" + cwaTargetNS = "team-a" + cwaProviderName = "prod-eu-1" +) + +func cwaGitTarget() *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: cwaTargetName, Namespace: cwaTargetNS}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + ProviderRef: configbutleraiv1alpha3.GitProviderReference{Name: "git"}, + ClusterProviderRef: &configbutleraiv1alpha3.ClusterProviderReference{Name: cwaProviderName}, + Branch: "main", + Path: "clusters/prod", + }, + } +} + +func cwaGitProvider() *configbutleraiv1alpha3.GitProvider { + return &configbutleraiv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "git", Namespace: cwaTargetNS}, + } +} + +func cwaClusterProvider(policy *configbutleraiv1alpha3.AllowedNamespaces) *configbutleraiv1alpha3.ClusterProvider { + return &configbutleraiv1alpha3.ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: cwaProviderName}, + Spec: configbutleraiv1alpha3.ClusterProviderSpec{AllowedNamespaces: policy}, + } +} + +func cwaClusterWatchRule() *configbutleraiv1alpha3.ClusterWatchRule { + return &configbutleraiv1alpha3.ClusterWatchRule{ + ObjectMeta: metav1.ObjectMeta{Name: cwaRuleName, Generation: 1}, + Spec: configbutleraiv1alpha3.ClusterWatchRuleSpec{ + TargetRef: configbutleraiv1alpha3.NamespacedTargetReference{ + Kind: "GitTarget", Name: cwaTargetName, Namespace: cwaTargetNS, + }, + Rules: []configbutleraiv1alpha3.ClusterResourceRule{{ + Scope: configbutleraiv1alpha3.ResourceScopeNamespaced, + Resources: []string{"configmaps"}, + }}, + }, + } +} + +// cwaAdmitting is the policy that admits cwaTargetNS; cwaDenying admits some other namespace. +func cwaAdmitting() *configbutleraiv1alpha3.AllowedNamespaces { + return &configbutleraiv1alpha3.AllowedNamespaces{Names: []string{cwaTargetNS}} +} + +func cwaDenying() *configbutleraiv1alpha3.AllowedNamespaces { + return &configbutleraiv1alpha3.AllowedNamespaces{Names: []string{"some-other-namespace"}} +} + +type cwaFixture struct { + reconciler *ClusterWatchRuleReconciler + store *rulestore.RuleStore + wm *cwaWatchManager + client client.Client +} + +func newCWAFixture( + t *testing.T, + objects []client.Object, + interceptors interceptor.Funcs, +) *cwaFixture { + t.Helper() + + cl := fake.NewClientBuilder(). + WithScheme(scScheme(t)). + WithObjects(objects...). + WithStatusSubresource(&configbutleraiv1alpha3.ClusterWatchRule{}). + WithInterceptorFuncs(interceptors). + Build() + + store := rulestore.NewStore() + wm := &cwaWatchManager{} + + return &cwaFixture{ + reconciler: &ClusterWatchRuleReconciler{ + Client: cl, + Scheme: cl.Scheme(), + RuleStore: store, + WatchManager: wm, + }, + store: store, + wm: wm, + client: cl, + } +} + +func (f *cwaFixture) reconcile(ctx context.Context) (ctrl.Result, error) { + return f.reconciler.Reconcile(ctx, ctrl.Request{ + NamespacedName: k8stypes.NamespacedName{Name: cwaRuleName}, + }) +} + +func (f *cwaFixture) compiledNames() []string { + names := []string{} + for _, r := range f.store.SnapshotClusterWatchRules() { + names = append(names, r.Source.Name) + } + return names +} + +func (f *cwaFixture) reloadRule(ctx context.Context, t *testing.T) *configbutleraiv1alpha3.ClusterWatchRule { + t.Helper() + var rule configbutleraiv1alpha3.ClusterWatchRule + require.NoError(t, f.client.Get(ctx, k8stypes.NamespacedName{Name: cwaRuleName}, &rule)) + return &rule +} + +// assertTerminalRefusal pins the whole published verdict: the kstatus trio plus the two projected +// conditions, all under the one reason an operator greps for. +func assertTerminalRefusal(t *testing.T, rule *configbutleraiv1alpha3.ClusterWatchRule) { + t.Helper() + + for _, want := range []struct { + conditionType string + status metav1.ConditionStatus + }{ + {ConditionTypeGitTargetReady, metav1.ConditionFalse}, + {ConditionTypeStreamsRunning, metav1.ConditionFalse}, + {ConditionTypeReady, metav1.ConditionFalse}, + {ConditionTypeReconciling, metav1.ConditionFalse}, + {ConditionTypeStalled, metav1.ConditionTrue}, + } { + cond := apimeta.FindStatusCondition(rule.Status.Conditions, want.conditionType) + require.NotNil(t, cond, "condition %s must be published", want.conditionType) + assert.Equal(t, want.status, cond.Status, "condition %s status", want.conditionType) + assert.Equal(t, ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized, cond.Reason, + "condition %s reason", want.conditionType) + } +} + +// --- tests ------------------------------------------------------------------------------------ + +// TestReconcile_ClusterWatchRuleRefusedWhenTargetNamespaceUnauthorized is the direct-refusal case: +// a ClusterWatchRule may not compile against a GitTarget whose namespace the ClusterProvider never +// admitted, even though the rule itself is cluster-scoped and names the target freely. +func TestReconcile_ClusterWatchRuleRefusedWhenTargetNamespaceUnauthorized(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaClusterProvider(cwaDenying()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{}) + + _, err := f.reconcile(ctx) + require.NoError(t, err, "a refusal is terminal, not an error to retry") + + assert.Empty(t, f.compiledNames(), "a refused rule must leave no compiled rule behind") + assert.Positive(t, f.wm.replans, "the watch manager must be replanned so no stream survives") + assertTerminalRefusal(t, f.reloadRule(ctx, t)) +} + +// TestReconcile_ClusterWatchRuleRefusedWhenClusterProviderMissing covers the other half of the +// shared gate: an undeclared provider is a hard denial, so an operator cannot sidestep +// allowedNamespaces by simply never creating the provider. +func TestReconcile_ClusterWatchRuleRefusedWhenClusterProviderMissing(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{}) + + _, err := f.reconcile(ctx) + require.NoError(t, err) + + assert.Empty(t, f.compiledNames()) + rule := f.reloadRule(ctx, t) + assertTerminalRefusal(t, rule) + cond := apimeta.FindStatusCondition(rule.Status.Conditions, ConditionTypeGitTargetReady) + assert.Contains(t, cond.Message, "was not found", + "the message must name which of the two provider-side causes it was") +} + +// TestReconcile_RefusalStopsDataPlaneBeforePublishingStatus is the ordering guard the design calls +// for: a gate that only writes a condition is not a gate. It fails a status-only implementation and +// it fails an implementation that publishes the refusal while the stream is still planned. +func TestReconcile_RefusalStopsDataPlaneBeforePublishingStatus(t *testing.T) { + ctx := context.Background() + + var ( + replannedAt int + statusWrittenAt int + seq int + compiledAtStatusWrite int + ) + + var f *cwaFixture + f = newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaClusterProvider(cwaDenying()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{ + SubResourceUpdate: func( + ctx context.Context, c client.Client, _ string, + obj client.Object, opts ...client.SubResourceUpdateOption, + ) error { + seq++ + statusWrittenAt = seq + compiledAtStatusWrite = len(f.store.SnapshotClusterWatchRules()) + return c.Status().Update(ctx, obj, opts...) + }, + }) + + // Pre-seed the store as an earlier, admitted reconcile would have left it, so "no compiled + // rule at the end" cannot pass vacuously. + f.store.AddOrUpdateClusterWatchRule( + *cwaClusterWatchRule(), cwaTargetName, cwaTargetNS, "git", cwaTargetNS, "main", "clusters/prod") + require.Len(t, f.compiledNames(), 1, "precondition: the rule starts out compiled") + + f.wm.onReconcile = func() { + seq++ + replannedAt = seq + } + + _, err := f.reconcile(ctx) + require.NoError(t, err) + + require.Positive(t, replannedAt, "the watch manager must be replanned on refusal") + require.Positive(t, statusWrittenAt, "the refusal must be published") + assert.Less(t, replannedAt, statusWrittenAt, + "the data plane must be stopped BEFORE the refusal is published") + assert.Zero(t, compiledAtStatusWrite, + "the compiled rule must already be gone when the terminal condition becomes observable") +} + +// TestReconcile_AdmittedClusterWatchRuleStillCompiles is the regression guard for a helper that is +// accidentally too strict: the admitted path must be untouched. +func TestReconcile_AdmittedClusterWatchRuleStillCompiles(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaClusterProvider(cwaAdmitting()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{}) + + _, err := f.reconcile(ctx) + require.NoError(t, err) + + assert.Equal(t, []string{cwaRuleName}, f.compiledNames(), "an admitted rule must compile") + + rule := f.reloadRule(ctx, t) + stalled := apimeta.FindStatusCondition(rule.Status.Conditions, ConditionTypeStalled) + require.NotNil(t, stalled) + assert.Equal(t, metav1.ConditionFalse, stalled.Status, "an admitted rule must not be stalled") + assert.NotEqual(t, ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized, stalled.Reason) +} + +// TestReconcile_AdmittedBySelectorOnNamespaceLabels proves the gate reads the live Namespace's +// labels, not just the names list — the path the Namespace watch exists to re-trigger. +func TestReconcile_AdmittedBySelectorOnNamespaceLabels(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaClusterProvider(&configbutleraiv1alpha3.AllowedNamespaces{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"mirror": "yes"}}, + }), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: cwaTargetNS, Labels: map[string]string{"mirror": "yes"}, + }}, + }, interceptor.Funcs{}) + + _, err := f.reconcile(ctx) + require.NoError(t, err) + assert.Equal(t, []string{cwaRuleName}, f.compiledNames()) +} + +// TestReconcile_RevocationRemovesCompiledRule is the revocation case: a rule that was admitted and +// running must be torn down when the provider's allowedNamespaces stops admitting its target's +// namespace. Same terminal status as an initial denial. +func TestReconcile_RevocationRemovesCompiledRule(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaClusterProvider(cwaAdmitting()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{}) + + // Round 1: admitted and compiled. + _, err := f.reconcile(ctx) + require.NoError(t, err) + require.Equal(t, []string{cwaRuleName}, f.compiledNames(), "precondition: the rule is running") + replansAfterAdmission := f.wm.replans + + // Revoke: the namespace leaves allowedNamespaces. + var provider configbutleraiv1alpha3.ClusterProvider + require.NoError(t, f.client.Get(ctx, k8stypes.NamespacedName{Name: cwaProviderName}, &provider)) + provider.Spec.AllowedNamespaces = cwaDenying() + require.NoError(t, f.client.Update(ctx, &provider)) + + // Round 2: the same reconcile now refuses. + _, err = f.reconcile(ctx) + require.NoError(t, err) + + assert.Empty(t, f.compiledNames(), "revocation must remove the compiled rule") + assert.Greater(t, f.wm.replans, replansAfterAdmission, + "revocation must replan the watch manager so the stream stops") + assertTerminalRefusal(t, f.reloadRule(ctx, t)) +} + +// TestReconcile_AdmissionReadErrorRequeuesAndKeepsCompiledRule pins the fail-safe direction. A +// transient apiserver failure is NOT a denial: tearing the stream down on it would turn every +// blip into an outage, so the rule stays compiled and the error requeues. +func TestReconcile_AdmissionReadErrorRequeuesAndKeepsCompiledRule(t *testing.T) { + ctx := context.Background() + boom := errors.New("apiserver unavailable") + + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaClusterProvider(cwaAdmitting()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{ + Get: func( + ctx context.Context, c client.WithWatch, key client.ObjectKey, + obj client.Object, opts ...client.GetOption, + ) error { + if _, ok := obj.(*configbutleraiv1alpha3.ClusterProvider); ok { + return boom + } + return c.Get(ctx, key, obj, opts...) + }, + }) + + f.store.AddOrUpdateClusterWatchRule( + *cwaClusterWatchRule(), cwaTargetName, cwaTargetNS, "git", cwaTargetNS, "main", "clusters/prod") + + _, err := f.reconcile(ctx) + + require.Error(t, err, "a read failure must requeue, not silently pass or silently refuse") + require.ErrorIs(t, err, boom) + assert.Equal(t, []string{cwaRuleName}, f.compiledNames(), + "a transient read failure must NOT tear down a running rule") +} + +// --- mapper tests ----------------------------------------------------------------------------- + +// TestClusterProviderToClusterWatchRules proves a provider policy change requeues ClusterWatchRules +// and not only GitTargets. Without it a revocation converges only on the ~10m periodic reconcile, +// because the GitTarget's resulting status flip is dropped by GenerationChangedPredicate. +func TestClusterProviderToClusterWatchRules(t *testing.T) { + mirrored := cwaGitTarget() + elsewhere := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "other-mirror", Namespace: "team-b"}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + ClusterProviderRef: &configbutleraiv1alpha3.ClusterProviderReference{Name: "prod-us-1"}, + }, + } + + ruleFor := func(name, targetName, targetNS string) *configbutleraiv1alpha3.ClusterWatchRule { + r := cwaClusterWatchRule() + r.Name = name + r.Spec.TargetRef.Name = targetName + r.Spec.TargetRef.Namespace = targetNS + return r + } + + cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects( + mirrored, elsewhere, + ruleFor("rule-affected", cwaTargetName, cwaTargetNS), + ruleFor("rule-other-provider", "other-mirror", "team-b"), + ruleFor("rule-dangling", "no-such-target", cwaTargetNS), + ).Build() + r := &ClusterWatchRuleReconciler{Client: cl} + + reqs := r.clusterProviderToClusterWatchRules(context.Background(), cwaClusterProvider(cwaAdmitting())) + + names := []string{} + for _, req := range reqs { + assert.Empty(t, req.Namespace, "ClusterWatchRule is cluster-scoped: requests carry a name only") + names = append(names, req.Name) + } + assert.ElementsMatch(t, []string{"rule-affected"}, names, + "only rules whose target mirrors through THIS provider are requeued") +} + +// TestNamespaceToClusterWatchRules covers the selector half: a label change on a GitTarget's +// namespace can grant or revoke every ClusterWatchRule pointing at a target in it. +func TestNamespaceToClusterWatchRules(t *testing.T) { + inNS := cwaGitTarget() + otherNS := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "other-mirror", Namespace: "team-b"}, + } + + ruleFor := func(name, targetName, targetNS string) *configbutleraiv1alpha3.ClusterWatchRule { + r := cwaClusterWatchRule() + r.Name = name + r.Spec.TargetRef.Name = targetName + r.Spec.TargetRef.Namespace = targetNS + return r + } + + cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects( + inNS, otherNS, + ruleFor("rule-in-ns", cwaTargetName, cwaTargetNS), + ruleFor("rule-other-ns", "other-mirror", "team-b"), + ).Build() + r := &ClusterWatchRuleReconciler{Client: cl} + + reqs := r.namespaceToClusterWatchRules(context.Background(), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}) + + names := []string{} + for _, req := range reqs { + names = append(names, req.Name) + } + assert.ElementsMatch(t, []string{"rule-in-ns"}, names) +} + +// TestClusterWatchRulesTargeting_NoMatchesEnqueuesNothing keeps the mappers from degenerating into +// "re-enqueue everything", which would hide a broken match behind a passing convergence test. +func TestClusterWatchRulesTargeting_NoMatchesEnqueuesNothing(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects(cwaClusterWatchRule()).Build() + r := &ClusterWatchRuleReconciler{Client: cl} + + assert.Empty(t, r.clusterProviderToClusterWatchRules(context.Background(), + cwaClusterProvider(cwaAdmitting())), "no GitTargets mirror through this provider") + assert.Empty(t, r.namespaceToClusterWatchRules(context.Background(), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "empty-ns"}})) +} + +// TestRefusalReasonIsStableForOperators pins the reason string: it is a documented, greppable +// contract, so renaming it must break a test rather than silently break an operator's alert. +func TestRefusalReasonIsStableForOperators(t *testing.T) { + assert.Equal(t, "GitTargetNamespaceNotAuthorized", + ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized) + assert.Equal(t, "NamespaceNotAuthorized", authz.ReasonNamespaceNotAuthorized) + assert.Equal(t, "ClusterProviderNotFound", authz.ReasonClusterProviderNotFound) +} diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index 26c6fa1a..7f298b33 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -6,6 +6,8 @@ import ( "context" "fmt" + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -20,6 +22,7 @@ import ( ctrlreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/authz" "github.com/ConfigButler/gitops-reverser/internal/rulestore" "github.com/ConfigButler/gitops-reverser/internal/watch" ) @@ -35,6 +38,15 @@ const ( ClusterWatchRuleReasonReady = "Ready" ClusterWatchRuleReasonResourcesResolved = "Resolved" ClusterWatchRuleReasonUnresolvedResources = "UnresolvedResources" + + // ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized is the terminal reason when the + // referenced GitTarget's namespace is not admitted by that target's ClusterProvider — either + // because spec.allowedNamespaces excludes it or because the provider does not exist at all. + // + // One rule-side reason covers both provider-side causes on purpose: from the ClusterWatchRule's + // point of view the single fact that matters is that this rule may not compile against this + // target. The Message carries which of the two it was. + ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized = "GitTargetNamespaceNotAuthorized" ) // ClusterWatchRuleReconciler reconciles a ClusterWatchRule object. @@ -50,6 +62,8 @@ type ClusterWatchRuleReconciler struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterwatchrules/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch +// +kubebuilder:rbac:groups=configbutler.ai,resources=clusterproviders,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -168,6 +182,11 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( } r.setGitTargetReadyCondition(clusterRule, target) + // ClusterProvider namespace admission — see gateGitTargetAdmission. + if handled, result, err := r.gateGitTargetAdmission(ctx, clusterRule, target, log); handled { + return result, err + } + // Resolve GitProvider from target providerName := target.Spec.ProviderRef.Name providerNS := target.Namespace // Same as GitTarget @@ -223,6 +242,98 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( return r.setReadyAndUpdateStatusWithTarget(ctx, clusterRule) } +// gateGitTargetAdmission applies the ClusterProvider namespace admission to the GitTarget a +// ClusterWatchRule references. A ClusterWatchRule is cluster-scoped and its targetRef carries a +// REQUIRED namespace, so it may name a GitTarget in ANY namespace and widen that target's mirror +// scope cluster-wide. Compiling such a rule without re-applying the target's own provider +// admission would let it mirror through a credential whose allowedNamespaces never admitted that +// target — so the same decision the GitTarget reconciler makes is made here too, at the second +// call site that compiles rules. See internal/authz. +// +// It returns handled=false when the rule may proceed; handled=true means the reconcile is over and +// the caller must return the accompanying result and error unchanged. +func (r *ClusterWatchRuleReconciler) gateGitTargetAdmission( + ctx context.Context, + clusterRule *configbutleraiv1alpha3.ClusterWatchRule, + target configbutleraiv1alpha3.GitTarget, + log logr.Logger, +) (bool, ctrl.Result, error) { + admitted, err := authz.GitTargetAdmitted(ctx, r.Client, &target) + if err != nil { + // A transient apiserver failure must NOT tear down a running stream: leave the compiled + // rule in place and requeue with the error so the gate re-runs on real data. + log.Error(err, "Failed to evaluate ClusterProvider admission for referenced GitTarget", + "gitTargetName", target.Name, "gitTargetNamespace", target.Namespace) + return true, ctrl.Result{}, err + } + if admitted.Allowed { + return false, ctrl.Result{}, nil + } + + result, refuseErr := r.refuseUnauthorizedGitTarget(ctx, clusterRule, target, admitted, log) + return true, result, refuseErr +} + +// refuseUnauthorizedGitTarget is the denial half of the ClusterProvider admission gate. +// +// Order is the contract, not an implementation detail: the compiled rule is removed and the watch +// manager is replanned BEFORE any status is written. A gate that only writes a condition is not a +// gate — it leaves the stream running while announcing that it is not. Any test that asserts the +// terminal condition must therefore also be able to assert the rule is already gone. +// +// The refusal is terminal (Stalled=True, Reconciling=False) rather than a retry: nothing this +// controller does will change the verdict. Recovery arrives as an event — a ClusterProvider policy +// change or a Namespace label change — through the mappers registered in SetupWithManager. +func (r *ClusterWatchRuleReconciler) refuseUnauthorizedGitTarget( + ctx context.Context, + clusterRule *configbutleraiv1alpha3.ClusterWatchRule, + target configbutleraiv1alpha3.GitTarget, + decision authz.Decision, + log logr.Logger, +) (ctrl.Result, error) { + log.Info("Refusing ClusterWatchRule: referenced GitTarget's namespace is not admitted by its ClusterProvider", + "name", clusterRule.Name, + "gitTargetName", target.Name, + "gitTargetNamespace", target.Namespace, + "clusterProvider", target.SourceCluster(), + "reason", decision.Reason) + + // 1. Stop the data plane: drop any rule compiled by an earlier, admitted reconcile... + r.RuleStore.DeleteClusterWatchRule(types.NamespacedName{Name: clusterRule.Name}) + + // ...and replan so the watch manager tears down a stream this rule was keeping alive. + if r.WatchManager != nil { + if err := r.WatchManager.ReconcileForRuleChange(ctx); err != nil { + log.Error(err, "Failed to reconcile watch manager after refusing cluster rule", + "name", clusterRule.Name) + // Don't fail the reconciliation: the rule is already out of the store, so the next + // replan from any source converges. Publishing the refusal matters more than this. + } + } + + // 2. Only now publish the terminal status. + msg := fmt.Sprintf( + "ClusterWatchRule may not compile against GitTarget '%s/%s': %s", + target.Namespace, target.Name, decision.Message) + r.setTypedCondition( + clusterRule, + ConditionTypeGitTargetReady, + metav1.ConditionFalse, + ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized, + msg, + ) + r.setTypedCondition( + clusterRule, + ConditionTypeStreamsRunning, + metav1.ConditionFalse, + ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized, + "No streams: the referenced GitTarget is not authorized to use its ClusterProvider", + ) + r.setRuleStalled(clusterRule, ClusterWatchRuleReasonGitTargetNamespaceNotAuthorized, msg) + + return r.updateStatusAndRequeue(ctx, clusterRule) +} + // setReadyAndUpdateStatusWithTarget sets Ready with target message and updates status with retry. func (r *ClusterWatchRuleReconciler) setReadyAndUpdateStatusWithTarget( ctx context.Context, @@ -454,10 +565,108 @@ func (r *ClusterWatchRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { handler.EnqueueRequestsFromMapFunc(r.gitProviderToClusterWatchRules), builder.WithPredicates(predicate.GenerationChangedPredicate{}), ). + // React to a ClusterProvider's allowedNamespaces changing. Without this, REVOKING a + // namespace stops the GitTarget (which does watch ClusterProvider) but leaves this rule's + // compiled entry resident until the next periodic reconcile, so the admission gate would + // converge on a ~10m delay instead of on the event. The GitTarget's own status flip cannot + // carry it: that is a status-only update, which GenerationChangedPredicate above drops. + Watches( + &configbutleraiv1alpha3.ClusterProvider{}, + handler.EnqueueRequestsFromMapFunc(r.clusterProviderToClusterWatchRules), + builder.WithPredicates(clusterProviderReadyOrSpecChanged()), + ). + // React to a Namespace's LABELS changing: allowedNamespaces may admit by selector, so a + // label change on a GitTarget's namespace grants or revokes every ClusterWatchRule pointing + // at a target in it. LabelChangedPredicate ignores unrelated namespace churn. + Watches( + &corev1.Namespace{}, + handler.EnqueueRequestsFromMapFunc(r.namespaceToClusterWatchRules), + builder.WithPredicates(predicate.LabelChangedPredicate{}), + ). Named("clusterwatchrule"). Complete(r) } +// clusterProviderToClusterWatchRules maps a ClusterProvider policy change to every ClusterWatchRule +// whose referenced GitTarget mirrors through that provider, so an admission grant or revocation +// converges on the event rather than on the periodic reconcile. +func (r *ClusterWatchRuleReconciler) clusterProviderToClusterWatchRules( + ctx context.Context, + obj client.Object, +) []ctrlreconcile.Request { + var targets configbutleraiv1alpha3.GitTargetList + if err := r.List(ctx, &targets); err != nil { + logDependencyListError(ctx, err, "GitTargets", obj) + return nil + } + + affected := make(map[types.NamespacedName]struct{}, len(targets.Items)) + for i := range targets.Items { + t := &targets.Items[i] + if t.SourceCluster() != obj.GetName() { + continue + } + affected[types.NamespacedName{Name: t.Name, Namespace: t.Namespace}] = struct{}{} + } + + return r.clusterWatchRulesTargeting(ctx, affected, obj) +} + +// namespaceToClusterWatchRules maps a Namespace label change to every ClusterWatchRule whose +// referenced GitTarget lives in that namespace — the selector half of allowedNamespaces. +func (r *ClusterWatchRuleReconciler) namespaceToClusterWatchRules( + ctx context.Context, + obj client.Object, +) []ctrlreconcile.Request { + var targets configbutleraiv1alpha3.GitTargetList + if err := r.List(ctx, &targets, client.InNamespace(obj.GetName())); err != nil { + logDependencyListError(ctx, err, "GitTargets", obj) + return nil + } + + affected := make(map[types.NamespacedName]struct{}, len(targets.Items)) + for i := range targets.Items { + t := &targets.Items[i] + affected[types.NamespacedName{Name: t.Name, Namespace: t.Namespace}] = struct{}{} + } + + return r.clusterWatchRulesTargeting(ctx, affected, obj) +} + +// clusterWatchRulesTargeting returns a request for every ClusterWatchRule whose targetRef names one +// of the given GitTargets. Requests carry a name only — ClusterWatchRule is cluster-scoped. +func (r *ClusterWatchRuleReconciler) clusterWatchRulesTargeting( + ctx context.Context, + targets map[types.NamespacedName]struct{}, + obj client.Object, +) []ctrlreconcile.Request { + if len(targets) == 0 { + return nil + } + + var rules configbutleraiv1alpha3.ClusterWatchRuleList + if err := r.List(ctx, &rules); err != nil { + logDependencyListError(ctx, err, "ClusterWatchRules", obj) + return nil + } + + var requests []ctrlreconcile.Request + for i := range rules.Items { + rule := &rules.Items[i] + key := types.NamespacedName{ + Name: rule.Spec.TargetRef.Name, + Namespace: rule.Spec.TargetRef.Namespace, + } + if _, ok := targets[key]; !ok { + continue + } + requests = append(requests, ctrlreconcile.Request{ + NamespacedName: types.NamespacedName{Name: rule.Name}, + }) + } + return requests +} + // gitTargetToClusterWatchRules maps a GitTarget event to every ClusterWatchRule // whose targetRef matches it. ClusterWatchRule is cluster-scoped, so the lookup // is cluster-wide. diff --git a/internal/controller/gittarget_source_cluster.go b/internal/controller/gittarget_source_cluster.go index f6a33049..78dc1ca6 100644 --- a/internal/controller/gittarget_source_cluster.go +++ b/internal/controller/gittarget_source_cluster.go @@ -6,74 +6,45 @@ import ( "context" "fmt" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8stypes "k8s.io/apimachinery/pkg/types" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/authz" "github.com/ConfigButler/gitops-reverser/internal/watch" ) // GitTargetReasonNamespaceNotAuthorized is the Validated=False reason when a GitTarget's namespace -// is not admitted by its referenced ClusterProvider's spec.allowedNamespaces. This is the SINGLE -// enforcement point for that policy: it runs on every reconcile, so a policy tightened AFTER a -// GitTarget was created stops that target's watches too. -const GitTargetReasonNamespaceNotAuthorized = "NamespaceNotAuthorized" +// is not admitted by its referenced ClusterProvider's spec.allowedNamespaces. It runs on every +// reconcile, so a policy tightened AFTER a GitTarget was created stops that target's watches too. +const GitTargetReasonNamespaceNotAuthorized = authz.ReasonNamespaceNotAuthorized // GitTargetReasonClusterProviderNotFound is the Validated=False reason when a GitTarget's // referenced ClusterProvider does not exist. This is a HARD GATE: a GitTarget may mirror a source // cluster ONLY through an existing ClusterProvider, "default" included. The operator never creates // one, so a target whose provider was never declared is held NotReady and its data plane stopped // rather than mirroring on an implicit local identity. -const GitTargetReasonClusterProviderNotFound = "ClusterProviderNotFound" - -// checkSourceAuthorization is the reconcile-time source-cluster gate. It first REQUIRES the -// referenced ClusterProvider to exist — a missing provider ("default" included) is a hard NotReady -// gate, so a GitTarget can never mirror a source cluster the operator was not configured for, and -// local mirroring is never an implicit bypass of the authorization policy. -// Then it enforces the provider's namespace-access policy. This is the ONLY place that policy is -// enforced, and it is enforced on every reconcile rather than only at admission — so a policy -// tightened AFTER a GitTarget was created stops that target's watches too. Its caller runs it -// inside the Validated gate and returns BEFORE DeclareForGitTarget, so an unauthorized GitTarget -// never starts a watch and never writes to Git. It returns authorized=false with a legible reason -// in either case; a non-NotFound read error is returned as err so the reconcile requeues. +const GitTargetReasonClusterProviderNotFound = authz.ReasonClusterProviderNotFound + +// checkSourceAuthorization is the GitTarget reconciler's view of the shared source-cluster gate: +// the referenced ClusterProvider must exist, and it must admit the GitTarget's namespace. The +// decision itself lives in internal/authz because the ClusterWatchRule reconciler and the watch +// manager's bootstrap must reach the SAME verdict — see authz.GitTargetAdmitted. +// +// This caller runs it inside the Validated gate and returns BEFORE DeclareForGitTarget, so an +// unauthorized GitTarget never starts a watch and never writes to Git. It returns authorized=false +// with a legible reason on denial; a non-NotFound read error is returned as err so the reconcile +// requeues rather than tearing down a running data plane on a transient apiserver failure. func (r *GitTargetReconciler) checkSourceAuthorization( ctx context.Context, target *configbutleraiv1alpha3.GitTarget, ) (bool, string, string, error) { - providerName := target.SourceCluster() - var provider configbutleraiv1alpha3.ClusterProvider - if err := r.Get(ctx, k8stypes.NamespacedName{Name: providerName}, &provider); err != nil { - if apierrors.IsNotFound(err) { - return false, GitTargetReasonClusterProviderNotFound, fmt.Sprintf( - "referenced ClusterProvider %q was not found; a GitTarget may mirror a source cluster "+ - "only through an existing ClusterProvider. The operator never creates one: declare it "+ - "yourself, or let the chart render %q via clusterProvider.createDefault", - providerName, configbutleraiv1alpha3.DefaultClusterProviderName), nil - } - return false, "", "", fmt.Errorf("read ClusterProvider %q: %w", providerName, err) - } - - nsLabels := map[string]string{} - var ns corev1.Namespace - if err := r.Get(ctx, k8stypes.NamespacedName{Name: target.Namespace}, &ns); err != nil { - if !apierrors.IsNotFound(err) { - return false, "", "", fmt.Errorf("read namespace %q: %w", target.Namespace, err) - } - } else { - nsLabels = ns.Labels - } - - allowed, selErr := provider.AllowsNamespace(target.Namespace, nsLabels) - if selErr != nil { - return false, GitTargetReasonNamespaceNotAuthorized, fmt.Sprintf( - "ClusterProvider %q allowedNamespaces selector is invalid: %v", providerName, selErr), nil + decision, err := authz.GitTargetAdmitted(ctx, r.Client, target) + if err != nil { + return false, "", "", err } - if !allowed { - return false, GitTargetReasonNamespaceNotAuthorized, fmt.Sprintf( - "namespace %q is not permitted to reference ClusterProvider %q (spec.allowedNamespaces)", - target.Namespace, providerName), nil + if !decision.Allowed { + return false, decision.Reason, decision.Message, nil } return true, "", "", nil } diff --git a/internal/watch/bootstrap.go b/internal/watch/bootstrap.go index 366b82d5..f024ebe5 100644 --- a/internal/watch/bootstrap.go +++ b/internal/watch/bootstrap.go @@ -10,6 +10,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/authz" ) // bootstrapRuleStore loads existing WatchRule and ClusterWatchRule objects into the in-memory RuleStore @@ -79,6 +80,22 @@ func (m *Manager) bootstrapClusterWatchRule(ctx context.Context, rule configv1al return err } + // Re-apply the referenced GitTarget's ClusterProvider admission before seeding. Bootstrap runs + // BEFORE the first reconcile on every restart, so a gate the reconciler alone enforced would be + // bypassed for the whole startup window — long enough to compile a rule and plan a stream for a + // target the provider never admitted. A denial is not fatal to startup: the rule is simply left + // out of the store, and the reconciler's own gate re-decides (and can grant it) as soon as the + // controller's initial sync reaches this rule. + admitted, admitErr := authz.GitTargetAdmitted(ctx, m.Client, &target) + if admitErr != nil { + return fmt.Errorf("evaluating ClusterProvider admission for GitTarget %s/%s: %w", + target.Namespace, target.Name, admitErr) + } + if !admitted.Allowed { + return fmt.Errorf("ClusterWatchRule %q may not compile against GitTarget %s/%s: %s", + rule.Name, target.Namespace, target.Name, admitted.Message) + } + m.RuleStore.AddOrUpdateClusterWatchRule( rule, target.Name, target.Namespace, diff --git a/internal/watch/bootstrap_admission_test.go b/internal/watch/bootstrap_admission_test.go new file mode 100644 index 00000000..c2c39050 --- /dev/null +++ b/internal/watch/bootstrap_admission_test.go @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/rulestore" +) + +// Bootstrap runs BEFORE the first reconcile on every restart, so it is the second call site that +// must apply the ClusterProvider admission gate. A helper only the reconciler used would leave the +// whole startup window unguarded — long enough to compile a rule and plan a stream for a GitTarget +// the provider never admitted. + +const ( + bootRuleName = "mirror-everything" + bootTargetName = "prod-mirror" + bootTargetNS = "team-a" + bootProviderName = "prod-eu-1" +) + +func bootGitTarget() *configv1alpha3.GitTarget { + return &configv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: bootTargetName, Namespace: bootTargetNS}, + Spec: configv1alpha3.GitTargetSpec{ + ProviderRef: configv1alpha3.GitProviderReference{Name: "git"}, + ClusterProviderRef: &configv1alpha3.ClusterProviderReference{Name: bootProviderName}, + Branch: "main", + Path: "clusters/prod", + }, + } +} + +func bootGitProvider() *configv1alpha3.GitProvider { + return &configv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "git", Namespace: bootTargetNS}, + } +} + +func bootClusterProvider(policy *configv1alpha3.AllowedNamespaces) *configv1alpha3.ClusterProvider { + return &configv1alpha3.ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: bootProviderName}, + Spec: configv1alpha3.ClusterProviderSpec{AllowedNamespaces: policy}, + } +} + +func bootClusterWatchRule() *configv1alpha3.ClusterWatchRule { + return &configv1alpha3.ClusterWatchRule{ + ObjectMeta: metav1.ObjectMeta{Name: bootRuleName}, + Spec: configv1alpha3.ClusterWatchRuleSpec{ + TargetRef: configv1alpha3.NamespacedTargetReference{ + Kind: "GitTarget", Name: bootTargetName, Namespace: bootTargetNS, + }, + Rules: []configv1alpha3.ClusterResourceRule{{ + Scope: configv1alpha3.ResourceScopeNamespaced, + Resources: []string{"configmaps"}, + }}, + }, + } +} + +func bootManager(t *testing.T, objects ...client.Object) *Manager { + t.Helper() + return &Manager{ + Client: fake.NewClientBuilder().WithScheme(makeScheme(t)).WithObjects(objects...).Build(), + Log: logr.Discard(), + RuleStore: rulestore.NewStore(), + } +} + +func bootCompiledNames(m *Manager) []string { + names := []string{} + for _, r := range m.RuleStore.SnapshotClusterWatchRules() { + names = append(names, r.Source.Name) + } + return names +} + +// TestBootstrapClusterWatchRule_RefusesUnauthorizedGitTargetNamespace is the bootstrap half of the +// direct-refusal test: the rule must not be seeded at all. +func TestBootstrapClusterWatchRule_RefusesUnauthorizedGitTargetNamespace(t *testing.T) { + m := bootManager(t, + bootGitTarget(), bootGitProvider(), + bootClusterProvider(&configv1alpha3.AllowedNamespaces{Names: []string{"some-other-namespace"}}), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bootTargetNS}}, + ) + + err := m.bootstrapClusterWatchRule(context.Background(), *bootClusterWatchRule()) + + require.Error(t, err, "an unadmitted rule must be reported, not silently seeded") + assert.Contains(t, err.Error(), "may not compile against GitTarget") + assert.Empty(t, bootCompiledNames(m), "a refused rule must never reach the store") +} + +// TestBootstrapClusterWatchRule_RefusesMissingClusterProvider covers the hard-gate half: an +// undeclared provider is not an implicit allow at startup either. +func TestBootstrapClusterWatchRule_RefusesMissingClusterProvider(t *testing.T) { + m := bootManager(t, + bootGitTarget(), bootGitProvider(), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bootTargetNS}}, + ) + + err := m.bootstrapClusterWatchRule(context.Background(), *bootClusterWatchRule()) + + require.Error(t, err) + assert.Empty(t, bootCompiledNames(m)) +} + +// TestBootstrapClusterWatchRule_SeedsAdmittedRule is the regression guard: the admitted startup +// path is unchanged, and the compiled rule still carries its resolved GitTarget/GitProvider values. +func TestBootstrapClusterWatchRule_SeedsAdmittedRule(t *testing.T) { + m := bootManager(t, + bootGitTarget(), bootGitProvider(), + bootClusterProvider(&configv1alpha3.AllowedNamespaces{Names: []string{bootTargetNS}}), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bootTargetNS}}, + ) + + require.NoError(t, m.bootstrapClusterWatchRule(context.Background(), *bootClusterWatchRule())) + + compiled := m.RuleStore.SnapshotClusterWatchRules() + require.Len(t, compiled, 1) + assert.Equal(t, bootRuleName, compiled[0].Source.Name) + assert.Equal(t, bootTargetName, compiled[0].GitTargetRef) + assert.Equal(t, bootTargetNS, compiled[0].GitTargetNamespace) + assert.Equal(t, "main", compiled[0].Branch) + assert.Equal(t, "clusters/prod", compiled[0].Path) +} + +// TestBootstrapRuleStore_SkipsUnauthorizedRuleButStillReady proves the denial is contained: one +// refused rule must not abort startup or block the admitted rules beside it, and the store must +// still be marked ready — otherwise a single unauthorized rule would wedge the whole data plane. +func TestBootstrapRuleStore_SkipsUnauthorizedRuleButStillReady(t *testing.T) { + admittedTarget := &configv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "ok-mirror", Namespace: "team-ok"}, + Spec: configv1alpha3.GitTargetSpec{ + ProviderRef: configv1alpha3.GitProviderReference{Name: "git"}, + ClusterProviderRef: &configv1alpha3.ClusterProviderReference{Name: bootProviderName}, + Branch: "main", + Path: "clusters/ok", + }, + } + admittedRule := bootClusterWatchRule() + admittedRule.Name = "admitted-rule" + admittedRule.Spec.TargetRef.Name = "ok-mirror" + admittedRule.Spec.TargetRef.Namespace = "team-ok" + + m := bootManager(t, + bootGitTarget(), bootGitProvider(), bootClusterWatchRule(), + admittedTarget, + &configv1alpha3.GitProvider{ObjectMeta: metav1.ObjectMeta{Name: "git", Namespace: "team-ok"}}, + admittedRule, + // Admits team-ok only, so the team-a rule is refused. + bootClusterProvider(&configv1alpha3.AllowedNamespaces{Names: []string{"team-ok"}}), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bootTargetNS}}, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "team-ok"}}, + ) + + require.NoError(t, m.bootstrapRuleStore(context.Background(), logr.Discard()), + "one refused rule must not abort startup") + + assert.Equal(t, []string{"admitted-rule"}, bootCompiledNames(m), + "only the admitted rule may be seeded") + assert.True(t, m.RuleStore.IsReady(), + "the store must still be marked ready: a refused rule is a refusal, not a startup failure") +} diff --git a/internal/watch/manager_startup_test.go b/internal/watch/manager_startup_test.go index a86c86e9..c395bd8a 100644 --- a/internal/watch/manager_startup_test.go +++ b/internal/watch/manager_startup_test.go @@ -120,6 +120,19 @@ func TestManagerStart_MustSeedRuleStoreFromExistingClusterWatchRules(t *testing. }, } + // existingGitTarget omits clusterProviderRef, which resolves to the reserved "default" + // ClusterProvider. Bootstrap now applies that provider's namespace admission before seeding a + // ClusterWatchRule, so the fixture must ship the provider the way a real install does (an + // empty selector admits every namespace — the chart default). Without it the seed is correctly + // REFUSED, which is the behavior TestBootstrapClusterWatchRule_RefusesMissingClusterProvider + // covers; this test is about the admitted path. + defaultClusterProvider := &configv1alpha3.ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: configv1alpha3.DefaultClusterProviderName}, + Spec: configv1alpha3.ClusterProviderSpec{ + AllowedNamespaces: &configv1alpha3.AllowedNamespaces{Selector: &metav1.LabelSelector{}}, + }, + } + fakeClient := fake.NewClientBuilder(). WithScheme(scheme). WithObjects( @@ -127,6 +140,7 @@ func TestManagerStart_MustSeedRuleStoreFromExistingClusterWatchRules(t *testing. existingClusterWatchRule, existingGitTarget, existingGitProvider, + defaultClusterProvider, ). Build()