From f4edca3cade8eae5d61b50bb31b0486384f7790b Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Wed, 6 May 2026 12:05:49 +0200 Subject: [PATCH 1/2] Introduce Policy custom resource --- charts/fleet-crd/templates/crds.yaml | 161 ++++++++ charts/fleet/templates/rbac_gitjob.yaml | 1 + charts/fleet/templates/rbac_helmops.yaml | 8 + e2e/assets/policy/gitrepo-with-sa.yaml | 11 + e2e/assets/policy/helmop-minimal.yaml | 19 + e2e/single-cluster/policy_test.go | 391 ++++++++++++++++++ .../gitops/reconciler/gitjob_controller.go | 9 + .../gitops/reconciler/gitjob_test.go | 7 +- .../gitops/reconciler/restrictions.go | 99 ++++- .../gitops/reconciler/restrictions_test.go | 101 ++++- .../helmops/reconciler/helmop_controller.go | 15 + .../reconciler/helmop_controller_test.go | 5 + .../helmops/reconciler/restrictions.go | 70 ++++ .../helmops/reconciler/restrictions_test.go | 196 +++++++++ .../policyrestrictions/policyrestrictions.go | 115 ++++++ .../reconciler/bundle_controller.go | 26 ++ .../reconciler/bundle_controller_test.go | 2 + .../controller/reconciler/bundle_policy.go | 66 +++ .../reconciler/bundle_policy_test.go | 173 ++++++++ .../fleet.cattle.io/v1alpha1/policy_types.go | 116 ++++++ .../v1alpha1/zz_generated.deepcopy.go | 127 ++++++ .../fleet.cattle.io/v1alpha1/interface.go | 5 + .../fleet.cattle.io/v1alpha1/policy.go | 39 ++ 23 files changed, 1738 insertions(+), 24 deletions(-) create mode 100644 e2e/assets/policy/gitrepo-with-sa.yaml create mode 100644 e2e/assets/policy/helmop-minimal.yaml create mode 100644 e2e/single-cluster/policy_test.go create mode 100644 internal/cmd/controller/helmops/reconciler/restrictions.go create mode 100644 internal/cmd/controller/helmops/reconciler/restrictions_test.go create mode 100644 internal/cmd/controller/policyrestrictions/policyrestrictions.go create mode 100644 internal/cmd/controller/reconciler/bundle_policy.go create mode 100644 internal/cmd/controller/reconciler/bundle_policy_test.go create mode 100644 pkg/apis/fleet.cattle.io/v1alpha1/policy_types.go create mode 100644 pkg/generated/controllers/fleet.cattle.io/v1alpha1/policy.go diff --git a/charts/fleet-crd/templates/crds.yaml b/charts/fleet-crd/templates/crds.yaml index f411f5c4b4..6a5994dd9a 100644 --- a/charts/fleet-crd/templates/crds.yaml +++ b/charts/fleet-crd/templates/crds.yaml @@ -10187,6 +10187,167 @@ spec: --- apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.21.0 + name: policies.fleet.cattle.io +spec: + group: fleet.cattle.io + names: + kind: Policy + listKind: PolicyList + plural: policies + singular: policy + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: "Policy restricts what GitRepo, HelmOp, and Bundle resources\ + \ in the same\nnamespace may do. Enforced at three points in the controller\ + \ stack:\n\n - GitRepo reconciler: validates and applies defaults before\ + \ producing a Bundle.\n - HelmOp reconciler: validates and applies defaults\ + \ before producing a Bundle.\n - Bundle reconciler: validates only (fail-only)\ + \ before producing BundleDeployments.\n\nTop-level fields are checked\ + \ by all three reconcilers.\nSub-object fields (gitRepo, helmOp) are only\ + \ read by their respective reconciler.\nDefault* fields inside sub-objects\ + \ are applied before top-level validators run.\n\nMultiple Policy objects\ + \ in the same namespace are aggregated with OR/union\nsemantics, sorted\ + \ by name for determinism." + properties: + allowedServiceAccounts: + description: 'AllowedServiceAccounts lists service accounts that may + be used. + + If non-empty, the ServiceAccount must appear in this list. + + When RequireServiceAccount is also true, an empty ServiceAccount is + + rejected regardless of this list.' + items: + type: string + nullable: true + type: array + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. + + Servers should convert recognized schemas to the latest internal value, + and + + may reject unrecognized values. + + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + gitRepo: + description: GitRepo contains restrictions and defaults applied only + by the GitRepo reconciler. + properties: + allowedClientSecretNames: + description: 'AllowedClientSecretNames lists client secret names + that GitRepo objects + + may reference.' + items: + type: string + nullable: true + type: array + allowedRepoPatterns: + description: 'AllowedRepoPatterns is a list of regex patterns restricting + the Repo + + field of GitRepo objects.' + items: + type: string + nullable: true + type: array + defaultClientSecretName: + description: 'DefaultClientSecretName is applied to GitRepo objects + whose + + ClientSecretName is empty.' + type: string + defaultServiceAccount: + description: 'DefaultServiceAccount is applied to GitRepo objects + whose ServiceAccount + + is empty, before the top-level RequireServiceAccount check runs.' + type: string + type: object + helmOp: + description: HelmOp contains restrictions and defaults applied only + by the HelmOp reconciler. + properties: + allowedChartPatterns: + description: 'AllowedChartPatterns is a list of regex patterns restricting + the + + spec.helm.chart field of HelmOp objects.' + items: + type: string + nullable: true + type: array + allowedHelmRepoPatterns: + description: 'AllowedHelmRepoPatterns is a list of regex patterns + restricting the + + spec.helm.repo field of HelmOp objects.' + items: + type: string + nullable: true + type: array + allowedHelmSecretNames: + description: 'AllowedHelmSecretNames lists credential secret names + that HelmOp objects + + may reference.' + items: + type: string + nullable: true + type: array + defaultHelmSecretName: + description: 'DefaultHelmSecretName is applied to HelmOp objects + whose HelmSecretName + + is empty.' + type: string + defaultServiceAccount: + description: 'DefaultServiceAccount is applied to HelmOp objects + whose ServiceAccount + + is empty, before the top-level RequireServiceAccount check runs.' + type: string + type: object + kind: + description: 'Kind is a string value representing the REST resource + this object represents. + + Servers may infer this from the endpoint the client submits requests + to. + + Cannot be updated. + + In CamelCase. + + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + requireServiceAccount: + description: 'RequireServiceAccount, when true, rejects any GitRepo, + HelmOp, or Bundle + + whose ServiceAccount is empty after any defaulting has been applied. + + Combine with AllowedServiceAccounts to also restrict which account + is used.' + type: boolean + type: object + served: true + storage: true +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition metadata: annotations: controller-gen.kubebuilder.io/version: v0.21.0 diff --git a/charts/fleet/templates/rbac_gitjob.yaml b/charts/fleet/templates/rbac_gitjob.yaml index dba8b13cfc..1fa1435829 100644 --- a/charts/fleet/templates/rbac_gitjob.yaml +++ b/charts/fleet/templates/rbac_gitjob.yaml @@ -41,6 +41,7 @@ rules: - "fleet.cattle.io" resources: - "gitreporestrictions" + - "policies" verbs: - list - get diff --git a/charts/fleet/templates/rbac_helmops.yaml b/charts/fleet/templates/rbac_helmops.yaml index 632a9ea323..38def620d4 100644 --- a/charts/fleet/templates/rbac_helmops.yaml +++ b/charts/fleet/templates/rbac_helmops.yaml @@ -25,6 +25,14 @@ rules: - "helmops/status" verbs: - "*" + - apiGroups: + - "fleet.cattle.io" + resources: + - "policies" + verbs: + - list + - get + - watch - apiGroups: - "fleet.cattle.io" resources: diff --git a/e2e/assets/policy/gitrepo-with-sa.yaml b/e2e/assets/policy/gitrepo-with-sa.yaml new file mode 100644 index 0000000000..44f46b9ba7 --- /dev/null +++ b/e2e/assets/policy/gitrepo-with-sa.yaml @@ -0,0 +1,11 @@ +kind: GitRepo +apiVersion: fleet.cattle.io/v1alpha1 +metadata: + name: {{ .Name }} +spec: + repo: https://github.com/rancher/fleet-test-data + branch: master + paths: + - simple + targetNamespace: {{ .TargetNamespace }} + serviceAccount: {{ .ServiceAccount }} diff --git a/e2e/assets/policy/helmop-minimal.yaml b/e2e/assets/policy/helmop-minimal.yaml new file mode 100644 index 0000000000..9ab45fbbfd --- /dev/null +++ b/e2e/assets/policy/helmop-minimal.yaml @@ -0,0 +1,19 @@ +apiVersion: fleet.cattle.io/v1alpha1 +kind: HelmOp +metadata: + name: {{ .Name }} + namespace: {{ .Namespace }} +spec: + helm: + repo: {{.Repo}} + chart: {{.Chart}} + version: "{{.Version}}" + namespace: {{ .Namespace }} + {{- if ne .ServiceAccount "" }} + serviceAccount: {{ .ServiceAccount }} + {{- end }} + {{- if ne .HelmSecretName "" }} + helmSecretName: {{ .HelmSecretName }} + {{- end }} + targets: + - clusterSelector: {} diff --git a/e2e/single-cluster/policy_test.go b/e2e/single-cluster/policy_test.go new file mode 100644 index 0000000000..ddf9ee084d --- /dev/null +++ b/e2e/single-cluster/policy_test.go @@ -0,0 +1,391 @@ +package singlecluster_test + +// Tests for Policy CRD enforcement across GitRepo and HelmOp reconcilers. +// Runs against the single-cluster (fleet-local) setup. +// +// All fleet resources (GitRepo, HelmOp, Policy, SA) are created in env.Namespace +// (fleet-local), the fleet workspace namespace that the controller watches. +// Each test gets its own targetNamespace for deployed workloads. +// +// Covered scenarios: +// 1. No Policy → GitRepo reconciles normally (Accepted=True). +// 2. Policy requireServiceAccount=true, GitRepo no SA → Accepted=False. +// 3. Policy requireServiceAccount=true, GitRepo SA set → Accepted=True. +// 4. Policy allowedServiceAccounts set, GitRepo uses unlisted SA → Accepted=False. +// 5. Policy gitRepo.defaultServiceAccount → SA injected before check → Accepted=True. +// 6. Policy requireServiceAccount=true, HelmOp no SA → Accepted=False. +// 7. Policy helmOp.allowedHelmSecretNames, HelmOp uses unlisted secret → Accepted=False. +// 8. Multiple Policies: OR semantics for requireServiceAccount. +// 9. Bundle: direct fleet-apply bypass is blocked by Policy. + +import ( + "context" + "math/rand" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/rancher/fleet/e2e/testenv" + "github.com/rancher/fleet/e2e/testenv/kubectl" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +var _ = Describe("Policy enforcement", Label("policy"), func() { + // k points at the fleet workspace namespace (fleet-local). + // All fleet resources (GitRepo, HelmOp, Policy, SA) live here. + // Each test also gets its own targetNamespace for deployed workloads. + var ( + k kubectl.Command + targetNamespace string + r = rand.New(rand.NewSource(GinkgoRandomSeed())) + ) + + BeforeEach(func() { + k = env.Kubectl.Namespace(env.Namespace) + targetNamespace = testenv.NewNamespaceName("policy-tgt", r) + + out, err := k.Create("ns", targetNamespace) + Expect(err).ToNot(HaveOccurred(), out) + }) + + AfterEach(func() { + _, _ = k.Delete("ns", targetNamespace, "--wait=false") + }) + + // createPolicy creates a Policy in the fleet workspace namespace. + createPolicy := func(name string, spec fleet.Policy) { + pol := &fleet.Policy{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: env.Namespace, + }, + RequireServiceAccount: spec.RequireServiceAccount, + AllowedServiceAccounts: spec.AllowedServiceAccounts, + GitRepo: spec.GitRepo, + HelmOp: spec.HelmOp, + } + Expect(clientUpstream.Create(context.Background(), pol)).To(Succeed()) + DeferCleanup(func() { + _ = clientUpstream.Delete(context.Background(), pol) + }) + } + + // createSA creates a ServiceAccount in the fleet workspace namespace. + createSA := func(name string) { + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: env.Namespace}, + } + Expect(clientUpstream.Create(context.Background(), sa)).To(Succeed()) + DeferCleanup(func() { + _ = clientUpstream.Delete(context.Background(), sa) + }) + } + + // gitRepoCondition polls the Accepted condition of a GitRepo in the fleet workspace. + gitRepoCondition := func(name string) func(g Gomega) string { + return func(g Gomega) string { + gr := &fleet.GitRepo{} + g.Expect(clientUpstream.Get(context.Background(), types.NamespacedName{ + Name: name, + Namespace: env.Namespace, + }, gr)).To(Succeed()) + for _, c := range gr.Status.Conditions { + if c.Type == "Accepted" { + return string(c.Status) + ":" + c.Message + } + } + return "no-condition" + } + } + + // helmOpCondition polls the Accepted condition of a HelmOp in the fleet workspace. + helmOpCondition := func(name string) func(g Gomega) string { + return func(g Gomega) string { + ho := &fleet.HelmOp{} + g.Expect(clientUpstream.Get(context.Background(), types.NamespacedName{ + Name: name, + Namespace: env.Namespace, + }, ho)).To(Succeed()) + for _, c := range ho.Status.Conditions { + if c.Type == fleet.HelmOpAcceptedCondition { + return string(c.Status) + ":" + c.Message + } + } + return "no-condition" + } + } + + // applyGitRepo creates a GitRepo (no SA) in the fleet workspace namespace. + applyGitRepo := func(name string) { + err := testenv.ApplyTemplate( + k, + testenv.AssetPath("gitrepo-template.yaml"), + testenv.GitRepoData{ + Name: name, + Branch: "master", + Paths: []string{"simple"}, + TargetNamespace: targetNamespace, + }, + ) + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + _, _ = k.Delete("gitrepo", name) + }) + } + + // applyGitRepoWithSA creates a GitRepo with a specific serviceAccount. + applyGitRepoWithSA := func(name, sa string) { + err := testenv.ApplyTemplate( + k, + testenv.AssetPath("policy/gitrepo-with-sa.yaml"), + map[string]string{ + "Name": name, + "TargetNamespace": targetNamespace, + "ServiceAccount": sa, + }, + ) + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + _, _ = k.Delete("gitrepo", name) + }) + } + + // ------------------------------------------------------------------------- + // GitRepo tests + // ------------------------------------------------------------------------- + + Describe("GitRepo reconciler", func() { + Context("when no Policy exists in the namespace", func() { + It("reconciles normally (Accepted=True)", func() { + name := testenv.NewNamespaceName("gr", r) + applyGitRepo(name) + + Eventually(gitRepoCondition(name), 30*time.Second, time.Second). + Should(HavePrefix("True:")) + }) + }) + + Context("when Policy requireServiceAccount=true", func() { + BeforeEach(func() { + createPolicy("policy-rsa-"+testenv.NewNamespaceName("", r), fleet.Policy{RequireServiceAccount: true}) + }) + + It("rejects GitRepo with no SA (Accepted=False)", func() { + name := testenv.NewNamespaceName("gr", r) + applyGitRepo(name) + + Eventually(gitRepoCondition(name), 30*time.Second, time.Second).Should(And( + HavePrefix("False:"), + ContainSubstring("serviceAccount"), + )) + }) + + It("accepts GitRepo with SA set (Accepted=True)", func() { + sa := testenv.NewNamespaceName("sa", r) + createSA(sa) + name := testenv.NewNamespaceName("gr", r) + applyGitRepoWithSA(name, sa) + + Eventually(gitRepoCondition(name), 30*time.Second, time.Second).Should(HavePrefix("True:")) + }) + }) + + Context("when Policy allowedServiceAccounts lists specific SAs", func() { + BeforeEach(func() { + createPolicy("policy-asa-"+testenv.NewNamespaceName("", r), fleet.Policy{ + AllowedServiceAccounts: []string{"approved-sa"}, + }) + }) + + It("rejects GitRepo using an unlisted SA (Accepted=False)", func() { + saName := testenv.NewNamespaceName("unapproved", r) + createSA(saName) + name := testenv.NewNamespaceName("gr", r) + applyGitRepoWithSA(name, saName) + + Eventually(gitRepoCondition(name), 30*time.Second, time.Second).Should(And( + HavePrefix("False:"), + ContainSubstring("disallowed serviceAccount"), + )) + }) + }) + + Context("when Policy gitRepo.defaultServiceAccount is set", func() { + var injectedSA string + + BeforeEach(func() { + injectedSA = testenv.NewNamespaceName("injected-sa", r) + createSA(injectedSA) + createPolicy("policy-dsa-"+testenv.NewNamespaceName("", r), fleet.Policy{ + RequireServiceAccount: true, + GitRepo: &fleet.GitRepoPolicySpec{ + DefaultServiceAccount: injectedSA, + }, + }) + }) + + It("injects the default SA and accepts the GitRepo (Accepted=True)", func() { + name := testenv.NewNamespaceName("gr", r) + // GitRepo created without an explicit SA. + applyGitRepo(name) + + // Accepted because the default SA was injected before the check. + Eventually(gitRepoCondition(name), 30*time.Second, time.Second).Should(HavePrefix("True:")) + + // The SA must have been written back onto the GitRepo spec. + gr := &fleet.GitRepo{} + Expect(clientUpstream.Get(context.Background(), types.NamespacedName{ + Name: name, + Namespace: env.Namespace, + }, gr)).To(Succeed()) + Expect(gr.Spec.ServiceAccount).To(Equal(injectedSA)) + }) + }) + }) + + // ------------------------------------------------------------------------- + // HelmOp tests — rejection cases + // These fire before any chart fetch and need no real registry. + // ------------------------------------------------------------------------- + + // createHelmOpForRejection creates a HelmOp that will be rejected by Policy + // before the reconciler ever tries to fetch the chart. + createHelmOpForRejection := func(name, sa, secretName string) { + err := testenv.ApplyTemplate( + k, + testenv.AssetPath("policy/helmop-minimal.yaml"), + map[string]string{ + "Name": name, + "Namespace": env.Namespace, + "ServiceAccount": sa, + "HelmSecretName": secretName, + // Placeholder values — Policy rejection fires before any chart fetch. + "Repo": "https://charts.example.com", + "Chart": "placeholder", + "Version": "0.1.0", + }, + ) + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + _, _ = k.Delete("helmop", name) + }) + } + + Describe("HelmOp reconciler — rejection cases", func() { + Context("when Policy requireServiceAccount=true", func() { + BeforeEach(func() { + createPolicy("policy-ho-rsa-"+testenv.NewNamespaceName("", r), fleet.Policy{RequireServiceAccount: true}) + }) + + It("rejects HelmOp with no SA (Accepted=False)", func() { + name := testenv.NewNamespaceName("ho", r) + createHelmOpForRejection(name, "", "") + + Eventually(helmOpCondition(name), 30*time.Second, time.Second).Should(And( + HavePrefix("False:"), + ContainSubstring("serviceAccount"), + )) + }) + }) + + Context("when Policy helmOp.allowedHelmSecretNames is set", func() { + BeforeEach(func() { + createPolicy("policy-ho-ahn-"+testenv.NewNamespaceName("", r), fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + AllowedHelmSecretNames: []string{"approved-secret"}, + }, + }) + }) + + It("rejects HelmOp referencing an unlisted secret (Accepted=False)", func() { + name := testenv.NewNamespaceName("ho", r) + createHelmOpForRejection(name, "", "unapproved-secret") + + Eventually(helmOpCondition(name), 30*time.Second, time.Second).Should(And( + HavePrefix("False:"), + ContainSubstring("disallowed helmSecretName"), + )) + }) + }) + }) + + // ------------------------------------------------------------------------- + // Multiple-policy OR semantics + // ------------------------------------------------------------------------- + + Describe("multiple Policies in a namespace", func() { + It("OR-s requireServiceAccount: one strict Policy is enough to enforce", func() { + createPolicy("policy-or-perm-"+testenv.NewNamespaceName("", r), fleet.Policy{RequireServiceAccount: false}) + createPolicy("policy-or-strict-"+testenv.NewNamespaceName("", r), fleet.Policy{RequireServiceAccount: true}) + + name := testenv.NewNamespaceName("gr", r) + applyGitRepo(name) + + Eventually(gitRepoCondition(name), 30*time.Second, time.Second).Should(And( + HavePrefix("False:"), + ContainSubstring("serviceAccount"), + )) + }) + }) + + // ------------------------------------------------------------------------- + // Bundle — direct fleet-apply bypass + // ------------------------------------------------------------------------- + + // bundleReadyCondition polls the Ready condition of a Bundle in the fleet workspace. + bundleReadyCondition := func(name string) func(g Gomega) string { + return func(g Gomega) string { + b := &fleet.Bundle{} + g.Expect(clientUpstream.Get(context.Background(), types.NamespacedName{ + Name: name, + Namespace: env.Namespace, + }, b)).To(Succeed()) + for _, c := range b.Status.Conditions { + if c.Type == string(fleet.Ready) { + return string(c.Status) + ":" + c.Message + } + } + return "no-condition" + } + } + + Describe("Bundle reconciler", func() { + Context("when Policy requireServiceAccount=true", func() { + BeforeEach(func() { + createPolicy("policy-bundle-rsa-"+testenv.NewNamespaceName("", r), fleet.Policy{RequireServiceAccount: true}) + }) + + It("rejects a directly-created Bundle with no SA (Ready=False)", func() { + name := testenv.NewNamespaceName("bundle", r) + bundle := &fleet.Bundle{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: env.Namespace, + }, + Spec: fleet.BundleSpec{ + Resources: []fleet.BundleResource{ + {Name: "configmap.yaml", Content: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test\n"}, + }, + Targets: []fleet.BundleTarget{ + {Name: "default", ClusterGroup: "default"}, + }, + }, + } + Expect(clientUpstream.Create(context.Background(), bundle)).To(Succeed()) + DeferCleanup(func() { + _ = clientUpstream.Delete(context.Background(), bundle) + }) + + Eventually(bundleReadyCondition(name), 30*time.Second, time.Second).Should(And( + HavePrefix("False:"), + ContainSubstring("serviceAccount"), + )) + }) + }) + }) +}) diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 98e2959c1e..505dfdc1fc 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -201,6 +201,7 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Restrictions / Overrides, gitrepo reconciler is responsible for setting error in status oldStatus := gitrepo.Status.DeepCopy() + oldSpec := gitrepo.Spec.DeepCopy() if err := AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo); err != nil { r.Recorder.Eventf( gitrepo, @@ -214,6 +215,14 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, updateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err) } + // Persist any spec defaults injected by AuthorizeAndAssignDefaults (e.g. defaultServiceAccount). + // The spec update bumps the generation and triggers a fresh reconcile. + if !reflect.DeepEqual(oldSpec, &gitrepo.Spec) { + if err := r.Update(ctx, gitrepo); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } if !gitrepo.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(gitrepo, finalize.GitRepoFinalizer) { diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go index dc3a6c859c..63ab0571ac 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go @@ -119,7 +119,12 @@ func TestReconcile_Error_WhenGitrepoRestrictionsAreNotMet(t *testing.T) { namespacedName := types.NamespacedName{Name: gitRepo.Name, Namespace: gitRepo.Namespace} mockClient := mocks.NewMockK8sClient(mockCtrl) mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( - func(ctx context.Context, restrictions *fleetv1.GitRepoRestrictionList, ns client.InNamespace) error { + func(ctx context.Context, obj client.ObjectList, ns client.InNamespace) error { + restrictions, ok := obj.(*fleetv1.GitRepoRestrictionList) + if !ok { + // PolicyList or other types — no policies in this test. + return nil + } // fill the restrictions with a couple of allowed namespaces. // As the gitrepo has no target namespace restrictions won't be met restriction := fleetv1.GitRepoRestriction{AllowedTargetNamespaces: []string{"ns1", "ns2"}} diff --git a/internal/cmd/controller/gitops/reconciler/restrictions.go b/internal/cmd/controller/gitops/reconciler/restrictions.go index f07519a17b..af4b54d739 100644 --- a/internal/cmd/controller/gitops/reconciler/restrictions.go +++ b/internal/cmd/controller/gitops/reconciler/restrictions.go @@ -8,60 +8,84 @@ import ( "slices" "sort" + apimeta "k8s.io/apimachinery/pkg/api/meta" + "github.com/rancher/fleet/internal/cmd/controller/labelselectors" + "github.com/rancher/fleet/internal/cmd/controller/policyrestrictions" fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" ) -// AuthorizeAndAssignDefaults applies restrictions and mutates the passed in -// GitRepo if it passes the restrictions +// AuthorizeAndAssignDefaults applies restrictions from both GitRepoRestriction and +// Policy objects in the same namespace, then mutates the GitRepo with resolved defaults. func AuthorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *fleet.GitRepo) error { restrictions := &fleet.GitRepoRestrictionList{} - err := c.List(ctx, restrictions, client.InNamespace(gitrepo.Namespace)) - if err != nil { + if err := c.List(ctx, restrictions, client.InNamespace(gitrepo.Namespace)); err != nil { + return err + } + + policies := &fleet.PolicyList{} + if err := c.List(ctx, policies, client.InNamespace(gitrepo.Namespace)); err != nil { + if apimeta.IsNoMatchError(err) { + return nil + } return err } - if len(restrictions.Items) == 0 { + if len(restrictions.Items) == 0 && len(policies.Items) == 0 { return nil } - restriction := aggregate(restrictions.Items) + grr := aggregate(restrictions.Items) + pol := policyrestrictions.Aggregate(policies.Items) - if len(restriction.AllowedTargetNamespaces) > 0 && gitrepo.Spec.TargetNamespace == "" { + // Merge defaults: GitRepoRestriction wins over Policy for first-non-empty. + defaultSA := firstNonEmpty(grr.DefaultServiceAccount, pol.GitDefaultServiceAccount) + defaultClientSecret := firstNonEmpty(grr.DefaultClientSecretName, pol.GitDefaultClientSecretName) + + // Union allow-lists from both sources. + allowedSAs := slices.Concat(grr.AllowedServiceAccounts, pol.AllowedServiceAccounts) + allowedClientSecrets := slices.Concat(grr.AllowedClientSecretNames, pol.GitAllowedClientSecretNames) + // Repo patterns are kept separate: GitRepoRestriction patterns are evaluated + // unanchored (backward-compatible), while Policy patterns are anchored (^(?:...)$). + // A repo is allowed if it passes either list. + allowedTargetNS := grr.AllowedTargetNamespaces + mergedSelector := grr.AllowedTargetNamespaceSelector + + if len(allowedTargetNS) > 0 && gitrepo.Spec.TargetNamespace == "" { return errors.New("empty targetNamespace denied, because allowedTargetNamespaces restriction is present") } - - if restriction.AllowedTargetNamespaceSelector != nil && gitrepo.Spec.TargetNamespace == "" { + if mergedSelector != nil && gitrepo.Spec.TargetNamespace == "" { return errors.New("empty targetNamespace denied, because allowedTargetNamespaceSelector restriction is present") } - targetNamespace, err := isAllowed(gitrepo.Spec.TargetNamespace, "", restriction.AllowedTargetNamespaces) + targetNamespace, err := isAllowed(gitrepo.Spec.TargetNamespace, "", allowedTargetNS) if err != nil { return fmt.Errorf("disallowed targetNamespace %s: %w", gitrepo.Spec.TargetNamespace, err) } - serviceAccount, err := isAllowed(gitrepo.Spec.ServiceAccount, - restriction.DefaultServiceAccount, - restriction.AllowedServiceAccounts) + serviceAccount, err := isAllowed(gitrepo.Spec.ServiceAccount, defaultSA, allowedSAs) if err != nil { return fmt.Errorf("disallowed serviceAccount %s: %w", gitrepo.Spec.ServiceAccount, err) } - repo, err := isAllowedByRegex(gitrepo.Spec.Repo, "", restriction.AllowedRepoPatterns) + repo, err := isAllowedByRepo(gitrepo.Spec.Repo, grr.AllowedRepoPatterns, pol.GitAllowedRepoPatterns) if err != nil { return fmt.Errorf("disallowed repo %s: %w", gitrepo.Spec.Repo, err) } - clientSecretName, err := isAllowed(gitrepo.Spec.ClientSecretName, - restriction.DefaultClientSecretName, - restriction.AllowedClientSecretNames) + clientSecretName, err := isAllowed(gitrepo.Spec.ClientSecretName, defaultClientSecret, allowedClientSecrets) if err != nil { return fmt.Errorf("disallowed clientSecretName %s: %w", gitrepo.Spec.ClientSecretName, err) } - // set the defaults back to the GitRepo + // Policy: RequireServiceAccount is checked after all defaulting. + if pol.RequireServiceAccount && serviceAccount == "" { + return errors.New("serviceAccount is required by Policy but is not set") + } + + // Write resolved values back to the GitRepo. gitrepo.Spec.TargetNamespace = targetNamespace gitrepo.Spec.ServiceAccount = serviceAccount gitrepo.Spec.Repo = repo @@ -70,6 +94,15 @@ func AuthorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *f return nil } +func firstNonEmpty(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" +} + func aggregate(restrictions []fleet.GitRepoRestriction) (result fleet.GitRepoRestriction) { sort.Slice(restrictions, func(i, j int) bool { return restrictions[i].Name < restrictions[j].Name @@ -128,3 +161,33 @@ func isAllowedByRegex(currentValue, defaultValue string, patterns []string) (str return currentValue, fmt.Errorf("%s not in allowed set %v", currentValue, patterns) } + +// isAllowedByRepo validates a repo URL against two separate allow-lists with different +// anchoring semantics. GitRepoRestriction patterns are evaluated unanchored (backward- +// compatible). Policy patterns are evaluated anchored via policyrestrictions.IsAllowedByRegex. +// An empty combined allow-list means no restriction. The repo passes if either non-empty +// list accepts it. +func isAllowedByRepo(repo string, grrPatterns, policyPatterns []string) (string, error) { + if len(grrPatterns) == 0 && len(policyPatterns) == 0 { + return repo, nil + } + var grrErr, polErr error + if len(grrPatterns) > 0 { + _, grrErr = isAllowedByRegex(repo, "", grrPatterns) + if grrErr == nil { + return repo, nil + } + } + if len(policyPatterns) > 0 { + _, polErr = policyrestrictions.IsAllowedByRegex(repo, "", policyPatterns) + if polErr == nil { + return repo, nil + } + } + // Both lists were non-empty and both rejected — report the Policy error if + // only Policy patterns were present, otherwise report the GRR error. + if grrErr != nil { + return repo, grrErr + } + return repo, polErr +} diff --git a/internal/cmd/controller/gitops/reconciler/restrictions_test.go b/internal/cmd/controller/gitops/reconciler/restrictions_test.go index 8113617fd4..d9abd7daa9 100644 --- a/internal/cmd/controller/gitops/reconciler/restrictions_test.go +++ b/internal/cmd/controller/gitops/reconciler/restrictions_test.go @@ -23,6 +23,7 @@ func TestAuthorizeAndAssignDefaults(t *testing.T) { name string inputGr fleet.GitRepo restrictions *fleet.GitRepoRestrictionList + policies *fleet.PolicyList restrictionsListErr error expectedGr fleet.GitRepo expectedErr string @@ -209,6 +210,91 @@ func TestAuthorizeAndAssignDefaults(t *testing.T) { }, }, }, + { + // Policy patterns are anchored: a pattern without .* must match the full URL. + name: "Policy repo pattern: reject URL that matches only as substring", + inputGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "https://evil.example.com/redirect?to=github.com/myorg/repo", + }, + }, + restrictions: &fleet.GitRepoRestrictionList{}, + policies: &fleet.PolicyList{Items: []fleet.Policy{ + {GitRepo: &fleet.GitRepoPolicySpec{ + AllowedRepoPatterns: []string{`github\.com/myorg/.*`}, + }}, + }}, + expectedGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "https://evil.example.com/redirect?to=github.com/myorg/repo", + }, + }, + expectedErr: "disallowed repo.*", + }, + { + // Policy patterns are anchored: the pattern must match the full URL. + name: "Policy repo pattern: accept URL that matches anchored pattern", + inputGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "https://github.com/myorg/myrepo", + }, + }, + restrictions: &fleet.GitRepoRestrictionList{}, + policies: &fleet.PolicyList{Items: []fleet.Policy{ + {GitRepo: &fleet.GitRepoPolicySpec{ + AllowedRepoPatterns: []string{`https://github\.com/myorg/.*`}, + }}, + }}, + expectedGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "https://github.com/myorg/myrepo", + }, + }, + }, + { + // GitRepoRestriction patterns remain unanchored for backward compat. + name: "GitRepoRestriction repo pattern: unanchored match still works", + inputGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "http://foo.bar/baz", + }, + }, + restrictions: &fleet.GitRepoRestrictionList{ + Items: []fleet.GitRepoRestriction{ + {AllowedRepoPatterns: []string{`foo\.bar`}}, + }, + }, + expectedGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "http://foo.bar/baz", + }, + }, + }, + { + // A URL rejected by the Policy list but accepted by a GRR pattern should pass. + name: "GRR allows repo even when Policy list would reject it", + inputGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "https://github.com/myorg/myrepo", + }, + }, + restrictions: &fleet.GitRepoRestrictionList{ + Items: []fleet.GitRepoRestriction{ + {AllowedRepoPatterns: []string{`.*github\.com.*`}}, + }, + }, + policies: &fleet.PolicyList{Items: []fleet.Policy{ + {GitRepo: &fleet.GitRepoPolicySpec{ + // Anchored pattern that does NOT match the full URL. + AllowedRepoPatterns: []string{`gitlab\.com/.*`}, + }}, + }}, + expectedGr: fleet.GitRepo{ + Spec: fleet.GitRepoSpec{ + Repo: "https://github.com/myorg/myrepo", + }, + }, + }, } for _, c := range cases { @@ -219,12 +305,17 @@ func TestAuthorizeAndAssignDefaults(t *testing.T) { client := mocks.NewMockK8sClient(mockCtrl) client.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( - func(_ context.Context, rl *fleet.GitRepoRestrictionList, ns crclient.InNamespace) error { - if c.restrictions != nil && len(c.restrictions.Items) > 0 { - rl.Items = c.restrictions.Items + func(_ context.Context, obj crclient.ObjectList, ns crclient.InNamespace) error { + if rl, ok := obj.(*fleet.GitRepoRestrictionList); ok { + if c.restrictions != nil && len(c.restrictions.Items) > 0 { + rl.Items = c.restrictions.Items + } + return c.restrictionsListErr } - - return c.restrictionsListErr + if pl, ok := obj.(*fleet.PolicyList); ok && c.policies != nil { + pl.Items = c.policies.Items + } + return nil }, ) diff --git a/internal/cmd/controller/helmops/reconciler/helmop_controller.go b/internal/cmd/controller/helmops/reconciler/helmop_controller.go index 59b6ef8724..2a2cc3fb71 100644 --- a/internal/cmd/controller/helmops/reconciler/helmop_controller.go +++ b/internal/cmd/controller/helmops/reconciler/helmop_controller.go @@ -39,6 +39,7 @@ import ( "github.com/rancher/fleet/pkg/cert" "github.com/rancher/fleet/pkg/durations" "github.com/rancher/fleet/pkg/sharding" + corev1 "k8s.io/api/core/v1" ) // HelmOpReconciler reconciles a HelmOp resource to create and apply bundles for helm charts @@ -132,6 +133,20 @@ func (r *HelmOpReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, updateErrorStatusHelm(ctx, r.Client, req.NamespacedName, helmop, err) } + // Policy restrictions: validate and apply defaults before producing the Bundle. + if err := AuthorizeAndAssignDefaults(ctx, r.Client, helmop); err != nil { + r.Recorder.Eventf( + helmop, + nil, + corev1.EventTypeWarning, + "FailedToApplyRestrictions", + "ApplyPolicyRestrictions", + "%v", + err, + ) + return ctrl.Result{}, updateErrorStatusHelm(ctx, r.Client, req.NamespacedName, helmop, err) + } + // Reconciling logger = logger.WithValues("generation", helmop.Generation, "chart", helmop.Spec.Helm.Chart) ctx = log.IntoContext(ctx, logger) diff --git a/internal/cmd/controller/helmops/reconciler/helmop_controller_test.go b/internal/cmd/controller/helmops/reconciler/helmop_controller_test.go index 63d07d79e4..d173b765ca 100644 --- a/internal/cmd/controller/helmops/reconciler/helmop_controller_test.go +++ b/internal/cmd/controller/helmops/reconciler/helmop_controller_test.go @@ -363,6 +363,8 @@ func TestReconcile_ErrorCreatingBundleIsShownInStatus(t *testing.T) { }, ) + client.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleet.PolicyList{}), gomock.Any()).Return(nil) + statusClient := mocks.NewMockStatusWriter(mockCtrl) client.EXPECT().Status().Return(statusClient).Times(1) statusClient.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do( @@ -541,6 +543,8 @@ func TestReconcile_ErrorCreatingBundleIfBundleWithSameNameExists(t *testing.T) { }, ) + client.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleet.PolicyList{}), gomock.Any()).Return(nil) + expectedErrorMsg := "non-helmops bundle already exists" statusClient := mocks.NewMockStatusWriter(mockCtrl) client.EXPECT().Status().Return(statusClient).Times(1) @@ -1332,6 +1336,7 @@ func (tm *typeMatcher) String() string { // NotFound for both. This avoids an overly broad matcher that would silently // absorb unrelated Secret Gets. func expectCABundleLookup(client *mocks.MockK8sClient) { + client.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleet.PolicyList{}), gomock.Any()).AnyTimes().Return(nil) client.EXPECT().Get( gomock.Any(), types.NamespacedName{Namespace: "cattle-system", Name: "tls-ca"}, diff --git a/internal/cmd/controller/helmops/reconciler/restrictions.go b/internal/cmd/controller/helmops/reconciler/restrictions.go new file mode 100644 index 0000000000..2db1537176 --- /dev/null +++ b/internal/cmd/controller/helmops/reconciler/restrictions.go @@ -0,0 +1,70 @@ +// Copyright (c) 2021-2025 SUSE LLC + +package reconciler + +import ( + "context" + "errors" + "fmt" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + + "github.com/rancher/fleet/internal/cmd/controller/policyrestrictions" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// AuthorizeAndAssignDefaults validates a HelmOp against all Policy objects in the +// same namespace and mutates the HelmOp with resolved defaults. +// It is a no-op when no Policy objects exist in the namespace. +func AuthorizeAndAssignDefaults(ctx context.Context, c client.Client, helmop *fleet.HelmOp) error { + policies := &fleet.PolicyList{} + if err := c.List(ctx, policies, client.InNamespace(helmop.Namespace)); err != nil { + if apimeta.IsNoMatchError(err) { + return nil + } + return err + } + + if len(policies.Items) == 0 { + return nil + } + + pol := policyrestrictions.Aggregate(policies.Items) + + // Apply HelmOp-specific defaults before running the top-level checks. + if helmop.Spec.ServiceAccount == "" { + helmop.Spec.ServiceAccount = pol.HelmDefaultServiceAccount + } + if helmop.Spec.HelmSecretName == "" { + helmop.Spec.HelmSecretName = pol.HelmDefaultHelmSecretName + } + + // Top-level: RequireServiceAccount. + if pol.RequireServiceAccount && helmop.Spec.ServiceAccount == "" { + return errors.New("serviceAccount is required by Policy but is not set") + } + + // Top-level: AllowedServiceAccounts. + if _, err := policyrestrictions.IsAllowed(helmop.Spec.ServiceAccount, "", pol.AllowedServiceAccounts); err != nil { + return fmt.Errorf("disallowed serviceAccount %s: %w", helmop.Spec.ServiceAccount, err) + } + + // HelmOp-specific: AllowedHelmSecretNames. + if _, err := policyrestrictions.IsAllowed(helmop.Spec.HelmSecretName, "", pol.HelmAllowedHelmSecretNames); err != nil { + return fmt.Errorf("disallowed helmSecretName %s: %w", helmop.Spec.HelmSecretName, err) + } + + // HelmOp-specific: AllowedHelmRepoPatterns and AllowedChartPatterns. + if helmop.Spec.Helm != nil { + if _, err := policyrestrictions.IsAllowedByRegex(helmop.Spec.Helm.Repo, "", pol.HelmAllowedRepoPatterns); err != nil { + return fmt.Errorf("disallowed helm repo %s: %w", helmop.Spec.Helm.Repo, err) + } + if _, err := policyrestrictions.IsAllowedByRegex(helmop.Spec.Helm.Chart, "", pol.HelmAllowedChartPatterns); err != nil { + return fmt.Errorf("disallowed helm chart %s: %w", helmop.Spec.Helm.Chart, err) + } + } + + return nil +} diff --git a/internal/cmd/controller/helmops/reconciler/restrictions_test.go b/internal/cmd/controller/helmops/reconciler/restrictions_test.go new file mode 100644 index 0000000000..d651504ecd --- /dev/null +++ b/internal/cmd/controller/helmops/reconciler/restrictions_test.go @@ -0,0 +1,196 @@ +package reconciler_test + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + crclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/rancher/fleet/internal/cmd/controller/helmops/reconciler" + "github.com/rancher/fleet/internal/mocks" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +func TestHelmOpAuthorizeAndAssignDefaults(t *testing.T) { + dummyErr := errors.New("list failed") + + helmopWithSA := func(sa string) fleet.HelmOp { + return fleet.HelmOp{Spec: fleet.HelmOpSpec{BundleSpec: fleet.BundleSpec{BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: sa}}}} + } + helmopWithRepo := func(repo string) fleet.HelmOp { + return fleet.HelmOp{Spec: fleet.HelmOpSpec{BundleSpec: fleet.BundleSpec{BundleDeploymentOptions: fleet.BundleDeploymentOptions{Helm: &fleet.HelmOptions{Repo: repo}}}}} + } + helmopWithChart := func(chart string) fleet.HelmOp { + return fleet.HelmOp{Spec: fleet.HelmOpSpec{BundleSpec: fleet.BundleSpec{BundleDeploymentOptions: fleet.BundleDeploymentOptions{Helm: &fleet.HelmOptions{Chart: chart}}}}} + } + helmopWithSecret := func(secret string) fleet.HelmOp { + return fleet.HelmOp{Spec: fleet.HelmOpSpec{HelmSecretName: secret}} + } + + policy := func(spec fleet.Policy) *fleet.PolicyList { + return &fleet.PolicyList{Items: []fleet.Policy{spec}} + } + + cases := []struct { + name string + input fleet.HelmOp + policies *fleet.PolicyList + listErr error + expected fleet.HelmOp + expectedErr string + }{ + { + name: "fail when listing policies errors", + input: fleet.HelmOp{}, + listErr: dummyErr, + expected: fleet.HelmOp{}, + expectedErr: "list failed", + }, + { + name: "no-op when no policies exist", + input: helmopWithSA("any-sa"), + policies: &fleet.PolicyList{}, + expected: helmopWithSA("any-sa"), + }, + { + name: "require SA: reject when SA is empty", + input: fleet.HelmOp{}, + policies: policy(fleet.Policy{ + RequireServiceAccount: true, + }), + expected: fleet.HelmOp{}, + expectedErr: "serviceAccount is required", + }, + { + name: "require SA: inject default SA from helmOp sub-object, then pass", + input: fleet.HelmOp{}, + policies: policy(fleet.Policy{ + RequireServiceAccount: true, + HelmOp: &fleet.HelmOpPolicySpec{ + DefaultServiceAccount: "injected-sa", + }, + }), + expected: helmopWithSA("injected-sa"), + }, + { + name: "allowedServiceAccounts: reject unlisted SA", + input: helmopWithSA("bad-sa"), + policies: policy(fleet.Policy{ + AllowedServiceAccounts: []string{"good-sa"}, + }), + expected: helmopWithSA("bad-sa"), + expectedErr: "disallowed serviceAccount.*", + }, + { + name: "allowedServiceAccounts: accept listed SA", + input: helmopWithSA("good-sa"), + policies: policy(fleet.Policy{ + RequireServiceAccount: true, + AllowedServiceAccounts: []string{"good-sa"}, + }), + expected: helmopWithSA("good-sa"), + }, + { + name: "allowedHelmSecretNames: inject default and accept", + input: fleet.HelmOp{}, + policies: policy(fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + DefaultHelmSecretName: "default-secret", + AllowedHelmSecretNames: []string{"default-secret"}, + }, + }), + expected: helmopWithSecret("default-secret"), + }, + { + name: "allowedHelmSecretNames: reject unlisted secret", + input: helmopWithSecret("bad-secret"), + policies: policy(fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + AllowedHelmSecretNames: []string{"good-secret"}, + }, + }), + expected: helmopWithSecret("bad-secret"), + expectedErr: "disallowed helmSecretName.*", + }, + { + name: "allowedHelmRepoPatterns: reject non-matching repo", + input: helmopWithRepo("https://evil.example.com/charts"), + policies: policy(fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + AllowedHelmRepoPatterns: []string{"^https://charts\\.tenant-a\\.example\\.com/.*"}, + }, + }), + expected: helmopWithRepo("https://evil.example.com/charts"), + expectedErr: "disallowed helm repo.*", + }, + { + name: "allowedHelmRepoPatterns: accept matching repo", + input: helmopWithRepo("https://charts.tenant-a.example.com/stable"), + policies: policy(fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + AllowedHelmRepoPatterns: []string{"^https://charts\\.tenant-a\\.example\\.com/.*"}, + }, + }), + expected: helmopWithRepo("https://charts.tenant-a.example.com/stable"), + }, + { + name: "allowedChartPatterns: reject non-matching chart", + input: helmopWithChart("malicious-chart"), + policies: policy(fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + AllowedChartPatterns: []string{"^tenant-a-.*"}, + }, + }), + expected: helmopWithChart("malicious-chart"), + expectedErr: "disallowed helm chart.*", + }, + { + name: "allowedChartPatterns: accept matching chart", + input: helmopWithChart("tenant-a-app"), + policies: policy(fleet.Policy{ + HelmOp: &fleet.HelmOpPolicySpec{ + AllowedChartPatterns: []string{"^tenant-a-.*"}, + }, + }), + expected: helmopWithChart("tenant-a-app"), + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockClient := mocks.NewMockK8sClient(mockCtrl) + mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( + func(_ context.Context, obj crclient.ObjectList, _ crclient.InNamespace) error { + if pl, ok := obj.(*fleet.PolicyList); ok { + if c.listErr != nil { + return c.listErr + } + if c.policies != nil { + pl.Items = c.policies.Items + } + } + return nil + }, + ) + + input := c.input.DeepCopy() + err := reconciler.AuthorizeAndAssignDefaults(context.TODO(), mockClient, input) + + if c.expectedErr != "" { + require.Error(t, err) + assert.Regexp(t, c.expectedErr, err.Error()) + } else { + require.NoError(t, err) + } + + assert.Equal(t, c.expected, *input) + }) + } +} diff --git a/internal/cmd/controller/policyrestrictions/policyrestrictions.go b/internal/cmd/controller/policyrestrictions/policyrestrictions.go new file mode 100644 index 0000000000..b3b098afb7 --- /dev/null +++ b/internal/cmd/controller/policyrestrictions/policyrestrictions.go @@ -0,0 +1,115 @@ +// Package policyrestrictions provides shared aggregation helpers for Fleet Policy enforcement. +// It is used by the GitRepo, HelmOp, and Bundle reconcilers. +package policyrestrictions + +import ( + "fmt" + "regexp" + "slices" + "sort" + + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +// Merged is the aggregated result of one or more Policy objects in a namespace. +type Merged struct { + RequireServiceAccount bool + AllowedServiceAccounts []string + + // GitRepo-specific + GitDefaultServiceAccount string + GitDefaultClientSecretName string + GitAllowedClientSecretNames []string + GitAllowedRepoPatterns []string + + // HelmOp-specific + HelmDefaultServiceAccount string + HelmDefaultHelmSecretName string + HelmAllowedHelmSecretNames []string + HelmAllowedRepoPatterns []string + HelmAllowedChartPatterns []string +} + +// Aggregate merges a slice of Policy objects into a single Merged value. +// Policies are processed in name order for determinism. +// Boolean fields use OR semantics; list fields are unioned; string defaults use first-non-empty. +func Aggregate(policies []fleet.Policy) Merged { + sort.Slice(policies, func(i, j int) bool { + return policies[i].Name < policies[j].Name + }) + + var m Merged + for _, p := range policies { + if p.RequireServiceAccount { + m.RequireServiceAccount = true + } + m.AllowedServiceAccounts = append(m.AllowedServiceAccounts, p.AllowedServiceAccounts...) + + if p.GitRepo != nil { + if m.GitDefaultServiceAccount == "" { + m.GitDefaultServiceAccount = p.GitRepo.DefaultServiceAccount + } + if m.GitDefaultClientSecretName == "" { + m.GitDefaultClientSecretName = p.GitRepo.DefaultClientSecretName + } + m.GitAllowedClientSecretNames = append(m.GitAllowedClientSecretNames, p.GitRepo.AllowedClientSecretNames...) + m.GitAllowedRepoPatterns = append(m.GitAllowedRepoPatterns, p.GitRepo.AllowedRepoPatterns...) + } + + if p.HelmOp != nil { + if m.HelmDefaultServiceAccount == "" { + m.HelmDefaultServiceAccount = p.HelmOp.DefaultServiceAccount + } + if m.HelmDefaultHelmSecretName == "" { + m.HelmDefaultHelmSecretName = p.HelmOp.DefaultHelmSecretName + } + m.HelmAllowedHelmSecretNames = append(m.HelmAllowedHelmSecretNames, p.HelmOp.AllowedHelmSecretNames...) + m.HelmAllowedRepoPatterns = append(m.HelmAllowedRepoPatterns, p.HelmOp.AllowedHelmRepoPatterns...) + m.HelmAllowedChartPatterns = append(m.HelmAllowedChartPatterns, p.HelmOp.AllowedChartPatterns...) + } + } + return m +} + +// IsAllowed validates currentValue against an optional allowedValues list, applying defaultValue +// when currentValue is empty. +// Returns (resolved value, nil) on success, or (currentValue, error) when the value is disallowed. +func IsAllowed(currentValue, defaultValue string, allowedValues []string) (string, error) { + if currentValue == "" { + return defaultValue, nil + } + if len(allowedValues) == 0 { + return currentValue, nil + } + if slices.Contains(allowedValues, currentValue) { + return currentValue, nil + } + return currentValue, fmt.Errorf("%s not in allowed set %v", currentValue, allowedValues) +} + +// IsAllowedByRegex validates currentValue against a list of regex patterns, applying defaultValue +// when currentValue is empty. +// Patterns may also match verbatim (for compatibility with plain-string allow-lists). +// Returns (resolved value, nil) on success, or (currentValue, error) when no pattern matches. +func IsAllowedByRegex(currentValue, defaultValue string, patterns []string) (string, error) { + if currentValue == "" { + return defaultValue, nil + } + if len(patterns) == 0 { + return currentValue, nil + } + for _, pattern := range patterns { + // Allow verbatim match for compatibility with plain-string allow-lists. + if pattern == currentValue { + return currentValue, nil + } + p, err := regexp.Compile("^(?:" + pattern + ")$") + if err != nil { + return currentValue, fmt.Errorf("policy failed to compile regex %q: %w", pattern, err) + } + if p.MatchString(currentValue) { + return currentValue, nil + } + } + return currentValue, fmt.Errorf("%s not in allowed set %v", currentValue, patterns) +} diff --git a/internal/cmd/controller/reconciler/bundle_controller.go b/internal/cmd/controller/reconciler/bundle_controller.go index b495cc9691..a1b0ea8c4d 100644 --- a/internal/cmd/controller/reconciler/bundle_controller.go +++ b/internal/cmd/controller/reconciler/bundle_controller.go @@ -194,6 +194,13 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return r.computeResult(ctx, logger, bundleOrig, bundle, "failed to remove display name label", err) } + // Policy restrictions: validate the Bundle before producing any BundleDeployments. + // This closes the direct fleet-apply bypass path: a Bundle that violates policy is + // never deployed regardless of how it was created. + if err := r.authorizeBundle(ctx, bundle); err != nil { + return ctrl.Result{}, r.updateErrorStatus(ctx, bundleOrig, bundle, err) + } + logger.V(1).Info( "Reconciling bundle, checking targets, calculating changes, building objects", "generation", @@ -770,6 +777,25 @@ func (r *BundleReconciler) handleContentAccessSecrets(ctx context.Context, bundl return nil } +// authorizeBundle validates the Bundle against Policy objects and records a +// warning event if a violation is found. Returns a non-nil error on violation. +func (r *BundleReconciler) authorizeBundle(ctx context.Context, bundle *fleet.Bundle) error { + if err := AuthorizeBundle(ctx, r.Client, bundle); err != nil { + r.Recorder.Eventf( + bundle, + nil, + corev1.EventTypeWarning, + "PolicyViolation", + "ApplyPolicyRestrictions", + "Bundle in namespace %s violates Policy: %v", + bundle.Namespace, + err, + ) + return err + } + return nil +} + // updateErrorStatus sets the Ready condition in the bundle status and tries to update the resource. // Setting that condition makes the error message visible in the Rancher UI. // Upon successful update of the status, updateErrorStatus returns a TerminalError, preventing requeues. diff --git a/internal/cmd/controller/reconciler/bundle_controller_test.go b/internal/cmd/controller/reconciler/bundle_controller_test.go index 061e2a5fbd..4af2d95cad 100644 --- a/internal/cmd/controller/reconciler/bundle_controller_test.go +++ b/internal/cmd/controller/reconciler/bundle_controller_test.go @@ -1236,6 +1236,8 @@ func expectGetWithFinalizer(mockCli *mocks.MockK8sClient, bundle fleetv1.Bundle) return nil }, ) + // AuthorizeBundle lists Policies in the namespace; return empty list so no policy is enforced. + mockCli.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleetv1.PolicyList{}), gomock.Any()).Return(nil) } func TestReconcile_DownstreamResourcesGeneration_Increment(t *testing.T) { diff --git a/internal/cmd/controller/reconciler/bundle_policy.go b/internal/cmd/controller/reconciler/bundle_policy.go new file mode 100644 index 0000000000..4b424bb067 --- /dev/null +++ b/internal/cmd/controller/reconciler/bundle_policy.go @@ -0,0 +1,66 @@ +// Copyright (c) 2021-2025 SUSE LLC + +package reconciler + +import ( + "context" + "errors" + "fmt" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + + "github.com/rancher/fleet/internal/cmd/controller/policyrestrictions" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// AuthorizeBundle validates a Bundle against all Policy objects in the same +// namespace. It never mutates the Bundle. Returns a non-nil error when the +// Bundle violates any policy, which will be surfaced as a status condition. +// It is a no-op when no Policy objects exist in the namespace. +func AuthorizeBundle(ctx context.Context, c client.Client, bundle *fleet.Bundle) error { + policies := &fleet.PolicyList{} + if err := c.List(ctx, policies, client.InNamespace(bundle.Namespace)); err != nil { + if apimeta.IsNoMatchError(err) { + return nil + } + return err + } + + if len(policies.Items) == 0 { + return nil + } + + pol := policyrestrictions.Aggregate(policies.Items) + + // No defaulting at the Bundle level — there is no backstop. + + // RequireServiceAccount: the top-level ServiceAccount is the fallback for + // every target that does not set its own, so it must always be present when + // the policy requires one. + if pol.RequireServiceAccount && bundle.Spec.ServiceAccount == "" { + return errors.New("serviceAccount is required by Policy but is not set on the Bundle") + } + + // AllowedServiceAccounts — check the top-level default. + if _, err := policyrestrictions.IsAllowed(bundle.Spec.ServiceAccount, "", pol.AllowedServiceAccounts); err != nil { + return fmt.Errorf("disallowed serviceAccount %s: %w", bundle.Spec.ServiceAccount, err) + } + + // AllowedServiceAccounts — check per-target overrides. + // A target that sets its own ServiceAccount bypasses the top-level default, + // so each non-empty target SA must also be validated independently. + for _, target := range bundle.Spec.Targets { + sa := target.ServiceAccount + if sa == "" { + // Inherits the top-level SA which was already validated above. + continue + } + if _, err := policyrestrictions.IsAllowed(sa, "", pol.AllowedServiceAccounts); err != nil { + return fmt.Errorf("disallowed serviceAccount %s in target %s: %w", sa, target.Name, err) + } + } + + return nil +} diff --git a/internal/cmd/controller/reconciler/bundle_policy_test.go b/internal/cmd/controller/reconciler/bundle_policy_test.go new file mode 100644 index 0000000000..361ac66c88 --- /dev/null +++ b/internal/cmd/controller/reconciler/bundle_policy_test.go @@ -0,0 +1,173 @@ +package reconciler_test + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + crclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/rancher/fleet/internal/cmd/controller/reconciler" + "github.com/rancher/fleet/internal/mocks" + fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" +) + +func TestAuthorizeBundle(t *testing.T) { + dummyErr := errors.New("list failed") + + bundleWithSA := func(sa string) fleet.Bundle { + return fleet.Bundle{Spec: fleet.BundleSpec{BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: sa}}} + } + policy := func(p fleet.Policy) *fleet.PolicyList { + return &fleet.PolicyList{Items: []fleet.Policy{p}} + } + + cases := []struct { + name string + input fleet.Bundle + policies *fleet.PolicyList + listErr error + expectedErr string + }{ + { + name: "fail when listing policies errors", + input: fleet.Bundle{}, + listErr: dummyErr, + expectedErr: "list failed", + }, + { + name: "no-op when no policies exist", + input: bundleWithSA("any-sa"), + policies: &fleet.PolicyList{}, + }, + { + name: "require SA: reject when SA is empty", + input: fleet.Bundle{}, + policies: policy(fleet.Policy{RequireServiceAccount: true}), + expectedErr: "serviceAccount is required", + }, + { + name: "require SA: accept when SA is set", + input: bundleWithSA("tenant-sa"), + policies: policy(fleet.Policy{RequireServiceAccount: true}), + }, + { + name: "allowedServiceAccounts: reject unlisted SA", + input: bundleWithSA("bad-sa"), + policies: policy(fleet.Policy{AllowedServiceAccounts: []string{"good-sa"}}), + expectedErr: "disallowed serviceAccount.*", + }, + { + name: "allowedServiceAccounts: accept listed SA", + input: bundleWithSA("good-sa"), + policies: policy(fleet.Policy{ + RequireServiceAccount: true, + AllowedServiceAccounts: []string{"good-sa"}, + }), + }, + { + name: "allowedServiceAccounts: no enforcement when SA is empty and requireServiceAccount is false", + // SA is empty and requireServiceAccount is false — omitting an SA + // entirely is allowed even when an allowlist is set. + input: fleet.Bundle{}, + policies: policy(fleet.Policy{AllowedServiceAccounts: []string{"good-sa"}}), + }, + { + name: "bundle is never mutated: no SA injection at bundle level", + input: fleet.Bundle{}, + // Even if a GitRepo sub-object has a default SA, the Bundle reconciler + // never applies defaults — it only rejects. + policies: policy(fleet.Policy{ + RequireServiceAccount: false, + GitRepo: &fleet.GitRepoPolicySpec{DefaultServiceAccount: "injected-sa"}, + }), + }, + { + name: "multiple policies: requireServiceAccount is OR-ed", + input: fleet.Bundle{}, + policies: &fleet.PolicyList{Items: []fleet.Policy{ + {RequireServiceAccount: false}, + {RequireServiceAccount: true}, + }}, + expectedErr: "serviceAccount is required", + }, + { + name: "multiple policies: allowedServiceAccounts is union", + input: bundleWithSA("sa-b"), + policies: &fleet.PolicyList{Items: []fleet.Policy{ + {AllowedServiceAccounts: []string{"sa-a"}}, + {AllowedServiceAccounts: []string{"sa-b"}}, + }}, + }, + { + name: "per-target SA: reject unlisted SA in target override", + input: fleet.Bundle{Spec: fleet.BundleSpec{ + BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: "good-sa"}, + Targets: []fleet.BundleTarget{ + {Name: "prod", BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: "evil-sa"}}, + }, + }}, + policies: policy(fleet.Policy{AllowedServiceAccounts: []string{"good-sa"}}), + expectedErr: `disallowed serviceAccount.*target prod`, + }, + { + name: "per-target SA: accept listed SA in target override", + input: fleet.Bundle{Spec: fleet.BundleSpec{ + BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: "good-sa"}, + Targets: []fleet.BundleTarget{ + {Name: "prod", BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: "other-good-sa"}}, + }, + }}, + policies: policy(fleet.Policy{AllowedServiceAccounts: []string{"good-sa", "other-good-sa"}}), + }, + { + name: "per-target SA: empty target SA inherits top-level, no double-rejection", + input: fleet.Bundle{Spec: fleet.BundleSpec{ + BundleDeploymentOptions: fleet.BundleDeploymentOptions{ServiceAccount: "good-sa"}, + Targets: []fleet.BundleTarget{ + {Name: "dev"}, + }, + }}, + policies: policy(fleet.Policy{AllowedServiceAccounts: []string{"good-sa"}}), + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + mockClient := mocks.NewMockK8sClient(mockCtrl) + mockClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn( + func(_ context.Context, obj crclient.ObjectList, _ crclient.InNamespace) error { + if pl, ok := obj.(*fleet.PolicyList); ok { + if c.listErr != nil { + return c.listErr + } + if c.policies != nil { + pl.Items = c.policies.Items + } + } + return nil + }, + ) + + // Capture original to verify no mutation occurred. + original := c.input.DeepCopy() + err := reconciler.AuthorizeBundle(context.TODO(), mockClient, &c.input) + + if c.expectedErr != "" { + require.Error(t, err) + assert.Regexp(t, c.expectedErr, err.Error()) + } else { + require.NoError(t, err) + } + + // AuthorizeBundle must never mutate the Bundle. + assert.Equal(t, *original, c.input, "AuthorizeBundle must not mutate the Bundle") + }) + } +} diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/policy_types.go b/pkg/apis/fleet.cattle.io/v1alpha1/policy_types.go new file mode 100644 index 0000000000..20c31f9bb2 --- /dev/null +++ b/pkg/apis/fleet.cattle.io/v1alpha1/policy_types.go @@ -0,0 +1,116 @@ +package v1alpha1 + +import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + +func init() { + InternalSchemeBuilder.Register(&Policy{}, &PolicyList{}) +} + +// +genclient +// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object +// +kubebuilder:object:root=true + +// Policy restricts what GitRepo, HelmOp, and Bundle resources in the same +// namespace may do. Enforced at three points in the controller stack: +// +// - GitRepo reconciler: validates and applies defaults before producing a Bundle. +// - HelmOp reconciler: validates and applies defaults before producing a Bundle. +// - Bundle reconciler: validates only (fail-only) before producing BundleDeployments. +// +// Top-level fields are checked by all three reconcilers. +// Sub-object fields (gitRepo, helmOp) are only read by their respective reconciler. +// Default* fields inside sub-objects are applied before top-level validators run. +// +// Multiple Policy objects in the same namespace are aggregated with OR/union +// semantics, sorted by name for determinism. +type Policy struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + // RequireServiceAccount, when true, rejects any GitRepo, HelmOp, or Bundle + // whose ServiceAccount is empty after any defaulting has been applied. + // Combine with AllowedServiceAccounts to also restrict which account is used. + // +optional + RequireServiceAccount bool `json:"requireServiceAccount,omitempty"` + + // AllowedServiceAccounts lists service accounts that may be used. + // If non-empty, the ServiceAccount must appear in this list. + // When RequireServiceAccount is also true, an empty ServiceAccount is + // rejected regardless of this list. + // +optional + // +nullable + AllowedServiceAccounts []string `json:"allowedServiceAccounts,omitempty"` + + // GitRepo contains restrictions and defaults applied only by the GitRepo reconciler. + // +optional + GitRepo *GitRepoPolicySpec `json:"gitRepo,omitempty"` + + // HelmOp contains restrictions and defaults applied only by the HelmOp reconciler. + // +optional + HelmOp *HelmOpPolicySpec `json:"helmOp,omitempty"` +} + +// GitRepoPolicySpec holds GitRepo-specific defaults and source restrictions. +type GitRepoPolicySpec struct { + // DefaultServiceAccount is applied to GitRepo objects whose ServiceAccount + // is empty, before the top-level RequireServiceAccount check runs. + // +optional + DefaultServiceAccount string `json:"defaultServiceAccount,omitempty"` + + // DefaultClientSecretName is applied to GitRepo objects whose + // ClientSecretName is empty. + // +optional + DefaultClientSecretName string `json:"defaultClientSecretName,omitempty"` + + // AllowedClientSecretNames lists client secret names that GitRepo objects + // may reference. + // +optional + // +nullable + AllowedClientSecretNames []string `json:"allowedClientSecretNames,omitempty"` + + // AllowedRepoPatterns is a list of regex patterns restricting the Repo + // field of GitRepo objects. + // +optional + // +nullable + AllowedRepoPatterns []string `json:"allowedRepoPatterns,omitempty"` +} + +// HelmOpPolicySpec holds HelmOp-specific defaults and source restrictions. +type HelmOpPolicySpec struct { + // DefaultServiceAccount is applied to HelmOp objects whose ServiceAccount + // is empty, before the top-level RequireServiceAccount check runs. + // +optional + DefaultServiceAccount string `json:"defaultServiceAccount,omitempty"` + + // DefaultHelmSecretName is applied to HelmOp objects whose HelmSecretName + // is empty. + // +optional + DefaultHelmSecretName string `json:"defaultHelmSecretName,omitempty"` + + // AllowedHelmSecretNames lists credential secret names that HelmOp objects + // may reference. + // +optional + // +nullable + AllowedHelmSecretNames []string `json:"allowedHelmSecretNames,omitempty"` + + // AllowedHelmRepoPatterns is a list of regex patterns restricting the + // spec.helm.repo field of HelmOp objects. + // +optional + // +nullable + AllowedHelmRepoPatterns []string `json:"allowedHelmRepoPatterns,omitempty"` + + // AllowedChartPatterns is a list of regex patterns restricting the + // spec.helm.chart field of HelmOp objects. + // +optional + // +nullable + AllowedChartPatterns []string `json:"allowedChartPatterns,omitempty"` +} + +// +kubebuilder:object:root=true + +// PolicyList contains a list of Policy. +type PolicyList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []Policy `json:"items"` +} diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go index 490dbd87e2..da3316d99f 100644 --- a/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/fleet.cattle.io/v1alpha1/zz_generated.deepcopy.go @@ -1529,6 +1529,31 @@ func (in *GitRepoList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GitRepoPolicySpec) DeepCopyInto(out *GitRepoPolicySpec) { + *out = *in + if in.AllowedClientSecretNames != nil { + in, out := &in.AllowedClientSecretNames, &out.AllowedClientSecretNames + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.AllowedRepoPatterns != nil { + in, out := &in.AllowedRepoPatterns, &out.AllowedRepoPatterns + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitRepoPolicySpec. +func (in *GitRepoPolicySpec) DeepCopy() *GitRepoPolicySpec { + if in == nil { + return nil + } + out := new(GitRepoPolicySpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitRepoRestriction) DeepCopyInto(out *GitRepoRestriction) { *out = *in @@ -1770,6 +1795,36 @@ func (in *HelmOpList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *HelmOpPolicySpec) DeepCopyInto(out *HelmOpPolicySpec) { + *out = *in + if in.AllowedHelmSecretNames != nil { + in, out := &in.AllowedHelmSecretNames, &out.AllowedHelmSecretNames + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.AllowedHelmRepoPatterns != nil { + in, out := &in.AllowedHelmRepoPatterns, &out.AllowedHelmRepoPatterns + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.AllowedChartPatterns != nil { + in, out := &in.AllowedChartPatterns, &out.AllowedChartPatterns + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HelmOpPolicySpec. +func (in *HelmOpPolicySpec) DeepCopy() *HelmOpPolicySpec { + if in == nil { + return nil + } + out := new(HelmOpPolicySpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HelmOpSpec) DeepCopyInto(out *HelmOpSpec) { *out = *in @@ -2254,6 +2309,78 @@ func (in *PodDisruptionBudgetSpec) DeepCopy() *PodDisruptionBudgetSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Policy) DeepCopyInto(out *Policy) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + if in.AllowedServiceAccounts != nil { + in, out := &in.AllowedServiceAccounts, &out.AllowedServiceAccounts + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.GitRepo != nil { + in, out := &in.GitRepo, &out.GitRepo + *out = new(GitRepoPolicySpec) + (*in).DeepCopyInto(*out) + } + if in.HelmOp != nil { + in, out := &in.HelmOp, &out.HelmOp + *out = new(HelmOpPolicySpec) + (*in).DeepCopyInto(*out) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Policy. +func (in *Policy) DeepCopy() *Policy { + if in == nil { + return nil + } + out := new(Policy) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *Policy) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PolicyList) DeepCopyInto(out *PolicyList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]Policy, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PolicyList. +func (in *PolicyList) DeepCopy() *PolicyList { + if in == nil { + return nil + } + out := new(PolicyList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *PolicyList) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PriorityClassSpec) DeepCopyInto(out *PriorityClassSpec) { *out = *in diff --git a/pkg/generated/controllers/fleet.cattle.io/v1alpha1/interface.go b/pkg/generated/controllers/fleet.cattle.io/v1alpha1/interface.go index 0e09f1d92c..83e9c8d2fb 100644 --- a/pkg/generated/controllers/fleet.cattle.io/v1alpha1/interface.go +++ b/pkg/generated/controllers/fleet.cattle.io/v1alpha1/interface.go @@ -43,6 +43,7 @@ type Interface interface { GitRepoRestriction() GitRepoRestrictionController HelmOp() HelmOpController ImageScan() ImageScanController + Policy() PolicyController Schedule() ScheduleController } @@ -104,6 +105,10 @@ func (v *version) ImageScan() ImageScanController { return generic.NewController[*v1alpha1.ImageScan, *v1alpha1.ImageScanList](schema.GroupVersionKind{Group: "fleet.cattle.io", Version: "v1alpha1", Kind: "ImageScan"}, "imagescans", true, v.controllerFactory) } +func (v *version) Policy() PolicyController { + return generic.NewController[*v1alpha1.Policy, *v1alpha1.PolicyList](schema.GroupVersionKind{Group: "fleet.cattle.io", Version: "v1alpha1", Kind: "Policy"}, "policies", true, v.controllerFactory) +} + func (v *version) Schedule() ScheduleController { return generic.NewController[*v1alpha1.Schedule, *v1alpha1.ScheduleList](schema.GroupVersionKind{Group: "fleet.cattle.io", Version: "v1alpha1", Kind: "Schedule"}, "schedules", true, v.controllerFactory) } diff --git a/pkg/generated/controllers/fleet.cattle.io/v1alpha1/policy.go b/pkg/generated/controllers/fleet.cattle.io/v1alpha1/policy.go new file mode 100644 index 0000000000..8a29baf00b --- /dev/null +++ b/pkg/generated/controllers/fleet.cattle.io/v1alpha1/policy.go @@ -0,0 +1,39 @@ +/* +Copyright (c) 2020 - 2026 SUSE LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Code generated by main. DO NOT EDIT. + +package v1alpha1 + +import ( + v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" + "github.com/rancher/wrangler/v3/pkg/generic" +) + +// PolicyController interface for managing Policy resources. +type PolicyController interface { + generic.ControllerInterface[*v1alpha1.Policy, *v1alpha1.PolicyList] +} + +// PolicyClient interface for managing Policy resources in Kubernetes. +type PolicyClient interface { + generic.ClientInterface[*v1alpha1.Policy, *v1alpha1.PolicyList] +} + +// PolicyCache interface for retrieving Policy resources in memory. +type PolicyCache interface { + generic.CacheInterface[*v1alpha1.Policy] +} From 3b84e2ef0d28a0ec5ce9cb3e1427cc92ca13ff12 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Wed, 27 May 2026 15:04:11 +0200 Subject: [PATCH 2/2] Address Policy code review feedback - Clone policies slice in Aggregate before sorting to avoid mutating the caller's backing array - Align event reason to PolicyViolation across GitRepo, HelmOp, and Bundle reconcilers - Use AnyTimes() on Policy List mock expectation so tests that reconcile multiple passes compose correctly --- internal/cmd/controller/gitops/reconciler/gitjob_controller.go | 2 +- internal/cmd/controller/gitops/reconciler/gitjob_test.go | 2 +- internal/cmd/controller/helmops/reconciler/helmop_controller.go | 2 +- .../cmd/controller/policyrestrictions/policyrestrictions.go | 1 + internal/cmd/controller/reconciler/bundle_controller_test.go | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 505dfdc1fc..20f59eb8c1 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go @@ -207,7 +207,7 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr gitrepo, nil, corev1.EventTypeWarning, - "FailedToApplyRestrictions", + "PolicyViolation", "ApplyGitRepoRestrictions", "%v", err, diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_test.go b/internal/cmd/controller/gitops/reconciler/gitjob_test.go index 63ab0571ac..9a5ce4ad86 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_test.go +++ b/internal/cmd/controller/gitops/reconciler/gitjob_test.go @@ -158,7 +158,7 @@ func TestReconcile_Error_WhenGitrepoRestrictionsAreNotMet(t *testing.T) { &gitRepoMatcher{gitRepo}, nil, corev1.EventTypeWarning, - "FailedToApplyRestrictions", + "PolicyViolation", "ApplyGitRepoRestrictions", "%v", errorMatcher{"empty targetNamespace denied, because allowedTargetNamespaces restriction is present"}, diff --git a/internal/cmd/controller/helmops/reconciler/helmop_controller.go b/internal/cmd/controller/helmops/reconciler/helmop_controller.go index 2a2cc3fb71..8a183ea58d 100644 --- a/internal/cmd/controller/helmops/reconciler/helmop_controller.go +++ b/internal/cmd/controller/helmops/reconciler/helmop_controller.go @@ -139,7 +139,7 @@ func (r *HelmOpReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr helmop, nil, corev1.EventTypeWarning, - "FailedToApplyRestrictions", + "PolicyViolation", "ApplyPolicyRestrictions", "%v", err, diff --git a/internal/cmd/controller/policyrestrictions/policyrestrictions.go b/internal/cmd/controller/policyrestrictions/policyrestrictions.go index b3b098afb7..1810cedacf 100644 --- a/internal/cmd/controller/policyrestrictions/policyrestrictions.go +++ b/internal/cmd/controller/policyrestrictions/policyrestrictions.go @@ -34,6 +34,7 @@ type Merged struct { // Policies are processed in name order for determinism. // Boolean fields use OR semantics; list fields are unioned; string defaults use first-non-empty. func Aggregate(policies []fleet.Policy) Merged { + policies = slices.Clone(policies) sort.Slice(policies, func(i, j int) bool { return policies[i].Name < policies[j].Name }) diff --git a/internal/cmd/controller/reconciler/bundle_controller_test.go b/internal/cmd/controller/reconciler/bundle_controller_test.go index 4af2d95cad..65a02cd123 100644 --- a/internal/cmd/controller/reconciler/bundle_controller_test.go +++ b/internal/cmd/controller/reconciler/bundle_controller_test.go @@ -1237,7 +1237,7 @@ func expectGetWithFinalizer(mockCli *mocks.MockK8sClient, bundle fleetv1.Bundle) }, ) // AuthorizeBundle lists Policies in the namespace; return empty list so no policy is enforced. - mockCli.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleetv1.PolicyList{}), gomock.Any()).Return(nil) + mockCli.EXPECT().List(gomock.Any(), gomock.AssignableToTypeOf(&fleetv1.PolicyList{}), gomock.Any()).Return(nil).AnyTimes() } func TestReconcile_DownstreamResourcesGeneration_Increment(t *testing.T) {