Repository: rancher/fleet
Pull request #3056 (2024-11-07 16:49:57)

Apply defaults from gitrepo restrictions
Refers to https://github.com/rancher/fleet/issues/3077

The [gitreporestriction defaulting feature](https://fleet.rancher.io/namespaces#restricting-gitrepos) seems broken in 0.10 and 0.11.

This PR prevents a returned value from staying unused which, incidentally, made our linter complain.


- [x] add tests
- [x] add issue for 0.11.1, 0.10.next


Patch:
diff --git a/go.mod b/go.mod
index f3b1c1fe..f42dfb9f 100644
--- a/go.mod
+++ b/go.mod
@@ -27,7 +27,7 @@ require (
 	github.com/gorilla/mux v1.8.1
 	github.com/hashicorp/go-getter v1.7.6
 	github.com/jpillora/backoff v1.0.0
-	github.com/onsi/ginkgo/v2 v2.20.2
+	github.com/onsi/ginkgo/v2 v2.21.0
 	github.com/onsi/gomega v1.35.1
 	github.com/opencontainers/go-digest v1.0.0
 	github.com/opencontainers/image-spec v1.1.0
@@ -136,7 +136,7 @@ require (
 	github.com/google/btree v1.1.2 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
-	github.com/google/pprof v0.0.0-20240910150728-a0b0bb1d4134 // indirect
+	github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db // indirect
 	github.com/google/s2a-go v0.1.7 // indirect
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
 	github.com/google/uuid v1.6.0 // indirect
diff --git a/go.sum b/go.sum
index 773e794a..182f78c8 100644
--- a/go.sum
+++ b/go.sum
@@ -500,8 +500,8 @@ github.com/google/pprof v0.0.0-20210226084205-cbba55b83ad5/go.mod h1:kpwsk12EmLe
 github.com/google/pprof v0.0.0-20210601050228-01bbb1931b22/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
 github.com/google/pprof v0.0.0-20210609004039-a478d1d731e9/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
 github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
-github.com/google/pprof v0.0.0-20240910150728-a0b0bb1d4134 h1:c5FlPPgxOn7kJz3VoPLkQYQXGBS3EklQ4Zfi57uOuqQ=
-github.com/google/pprof v0.0.0-20240910150728-a0b0bb1d4134/go.mod h1:vavhavw2zAxS5dIdcRluK6cSGGPlZynqzFM8NdvU144=
+github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db h1:097atOisP2aRj7vFgYQBbFN4U4JNXUNYpxael3UzMyo=
+github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db/go.mod h1:vavhavw2zAxS5dIdcRluK6cSGGPlZynqzFM8NdvU144=
 github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
 github.com/google/s2a-go v0.1.7 h1:60BLSyTrOV4/haCDW4zb1guZItoSq8foHCXrAnjBo/o=
 github.com/google/s2a-go v0.1.7/go.mod h1:50CgR4k1jNlWBu4UfS4AcfhVe1r6pdZPygJ3R8F0Qdw=
@@ -685,8 +685,8 @@ github.com/nxadm/tail v1.4.8 h1:nPr65rt6Y5JFSKQO7qToXr7pePgD6Gwiw05lkbyAQTE=
 github.com/nxadm/tail v1.4.8/go.mod h1:+ncqLTQzXmGhMZNUePPaPqPvBxHAIsmXswZKocGu+AU=
 github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE=
 github.com/onsi/ginkgo v1.16.5/go.mod h1:+E8gABHa3K6zRBolWtd+ROzc/U5bkGt0FwiG042wbpU=
-github.com/onsi/ginkgo/v2 v2.20.2 h1:7NVCeyIWROIAheY21RLS+3j2bb52W0W82tkberYytp4=
-github.com/onsi/ginkgo/v2 v2.20.2/go.mod h1:K9gyxPIlb+aIvnZ8bd9Ak+YP18w3APlR+5coaZoE2ag=
+github.com/onsi/ginkgo/v2 v2.21.0 h1:7rg/4f3rB88pb5obDgNZrNHrQ4e6WpjonchcpuBRnZM=
+github.com/onsi/ginkgo/v2 v2.21.0/go.mod h1:7Du3c42kxCUegi0IImZ1wUQzMBVecgIHjR1C+NkhLQo=
 github.com/onsi/gomega v1.35.1 h1:Cwbd75ZBPxFSuZ6T+rN/WCb/gOc6YgFBXLlZLhC7Ds4=
 github.com/onsi/gomega v1.35.1/go.mod h1:PvZbdDc8J6XJEpDK4HCuRBm8a6Fzp9/DmhC9C7yFlog=
 github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go
index f8360eb2..7b991879 100644
--- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go
+++ b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go
@@ -134,8 +134,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()
-	_, err := authorizeAndAssignDefaults(ctx, r.Client, gitrepo)
-	if err != nil {
+	if err := AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo); err != nil {
 		r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToApplyRestrictions", err.Error())
 		return ctrl.Result{}, updateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err)
 	}
diff --git a/internal/cmd/controller/gitops/reconciler/restrictions.go b/internal/cmd/controller/gitops/reconciler/restrictions.go
index 28410bd6..acbfbf0a 100644
--- a/internal/cmd/controller/gitops/reconciler/restrictions.go
+++ b/internal/cmd/controller/gitops/reconciler/restrictions.go
@@ -11,50 +11,56 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// authorizeAndAssignDefaults applies restrictions and returns a new GitRepo if it passes the restrictions
-func authorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *fleet.GitRepo) (*fleet.GitRepo, error) {
+// AuthorizeAndAssignDefaults applies restrictions and mutates the passed in
+// GitRepo if it passes the restrictions
+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 {
-		return nil, err
+		return err
 	}
 
 	if len(restrictions.Items) == 0 {
-		return gitrepo, nil
+		return nil
 	}
 
 	restriction := aggregate(restrictions.Items)
-	gitrepo = gitrepo.DeepCopy()
 
 	if len(restriction.AllowedTargetNamespaces) > 0 && gitrepo.Spec.TargetNamespace == "" {
-		return nil, fmt.Errorf("empty targetNamespace denied, because allowedTargetNamespaces restriction is present")
+		return fmt.Errorf("empty targetNamespace denied, because allowedTargetNamespaces restriction is present")
 	}
 
-	gitrepo.Spec.TargetNamespace, err = isAllowed(gitrepo.Spec.TargetNamespace, "", restriction.AllowedTargetNamespaces)
+	targetNamespace, err := isAllowed(gitrepo.Spec.TargetNamespace, "", restriction.AllowedTargetNamespaces)
 	if err != nil {
-		return nil, fmt.Errorf("disallowed targetNamespace %s: %w", gitrepo.Spec.TargetNamespace, err)
+		return fmt.Errorf("disallowed targetNamespace %s: %w", gitrepo.Spec.TargetNamespace, err)
 	}
 
-	gitrepo.Spec.ServiceAccount, err = isAllowed(gitrepo.Spec.ServiceAccount,
+	serviceAccount, err := isAllowed(gitrepo.Spec.ServiceAccount,
 		restriction.DefaultServiceAccount,
 		restriction.AllowedServiceAccounts)
 	if err != nil {
-		return nil, fmt.Errorf("disallowed serviceAccount %s: %w", gitrepo.Spec.ServiceAccount, err)
+		return fmt.Errorf("disallowed serviceAccount %s: %w", gitrepo.Spec.ServiceAccount, err)
 	}
 
-	gitrepo.Spec.Repo, err = isAllowedByRegex(gitrepo.Spec.Repo, "", restriction.AllowedRepoPatterns)
+	repo, err := isAllowedByRegex(gitrepo.Spec.Repo, "", restriction.AllowedRepoPatterns)
 	if err != nil {
-		return nil, fmt.Errorf("disallowed repo %s: %w", gitrepo.Spec.ServiceAccount, err)
+		return fmt.Errorf("disallowed repo %s: %w", gitrepo.Spec.Repo, err)
 	}
 
-	gitrepo.Spec.ClientSecretName, err = isAllowed(gitrepo.Spec.ClientSecretName,
+	clientSecretName, err := isAllowed(gitrepo.Spec.ClientSecretName,
 		restriction.DefaultClientSecretName,
 		restriction.AllowedClientSecretNames)
 	if err != nil {
-		return nil, fmt.Errorf("disallowed clientSecretName %s: %w", gitrepo.Spec.ServiceAccount, err)
+		return fmt.Errorf("disallowed clientSecretName %s: %w", gitrepo.Spec.ClientSecretName, err)
 	}
 
-	return gitrepo, nil
+	// set the defaults back to the GitRepo
+	gitrepo.Spec.TargetNamespace = targetNamespace
+	gitrepo.Spec.ServiceAccount = serviceAccount
+	gitrepo.Spec.Repo = repo
+	gitrepo.Spec.ClientSecretName = clientSecretName
+
+	return nil
 }
 
 func aggregate(restrictions []fleet.GitRepoRestriction) (result fleet.GitRepoRestriction) {
diff --git a/internal/cmd/controller/gitops/reconciler/status_controller.go b/internal/cmd/controller/gitops/reconciler/status_controller.go
index e93acf98..15d1bca8 100644
--- a/internal/cmd/controller/gitops/reconciler/status_controller.go
+++ b/internal/cmd/controller/gitops/reconciler/status_controller.go
@@ -73,8 +73,7 @@ func (r *StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}
 
 	// Restrictions / Overrides, gitrepo reconciler is responsible for setting error in status
-	_, err := authorizeAndAssignDefaults(ctx, r.Client, gitrepo)
-	if err != nil {
+	if err := AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo); err != nil {
 		// the gitjob_controller will handle the error
 		return ctrl.Result{}, nil
 	}
@@ -94,7 +93,7 @@ func (r *StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	logger.V(1).Info("Reconciling GitRepo status")
 
 	bdList := &v1alpha1.BundleDeploymentList{}
-	err = r.List(ctx, bdList, client.MatchingLabels{
+	err := r.List(ctx, bdList, client.MatchingLabels{
 		v1alpha1.RepoLabel:            gitrepo.Name,
 		v1alpha1.BundleNamespaceLabel: gitrepo.Namespace,
 	})


Test patch:
diff --git a/internal/cmd/controller/gitops/reconciler/restrictions_test.go b/internal/cmd/controller/gitops/reconciler/restrictions_test.go
new file mode 100644
index 00000000..7d6bc19f
--- /dev/null
+++ b/internal/cmd/controller/gitops/reconciler/restrictions_test.go
@@ -0,0 +1,249 @@
+//go:generate mockgen --build_flags=--mod=mod -destination=../../../../mocks/client_mock.go -package=mocks sigs.k8s.io/controller-runtime/pkg/client Client
+
+package reconciler_test
+
+import (
+	"context"
+	"errors"
+	"reflect"
+	"regexp"
+	"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/gitops/reconciler"
+	"github.com/rancher/fleet/internal/mocks"
+	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+	fleetv1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+)
+
+func TestAuthorizeAndAssignDefaults(t *testing.T) {
+	dummyErrMsg := "something happened"
+	dummyErr := errors.New(dummyErrMsg)
+	cases := []struct {
+		name                string
+		inputGr             fleet.GitRepo
+		restrictions        *fleet.GitRepoRestrictionList
+		restrictionsListErr error
+		expectedGr          fleet.GitRepo
+		expectedErr         string
+	}{
+		{
+			name:                "fail when listing GitRepo restrictions errors",
+			inputGr:             fleet.GitRepo{},
+			restrictionsListErr: dummyErr,
+			expectedGr:          fleet.GitRepo{},
+			expectedErr:         dummyErrMsg,
+		},
+		{
+			name:         "pass when list of GitRepo restrictions is empty",
+			inputGr:      fleet.GitRepo{},
+			restrictions: &fleet.GitRepoRestrictionList{},
+			expectedGr:   fleet.GitRepo{},
+		},
+		{
+			name:    "deny empty targetNamespace when allowedTargetNamespaces restriction present",
+			inputGr: fleet.GitRepo{}, // no target ns provided
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedTargetNamespaces: []string{"foo"},
+					},
+				},
+			},
+			expectedGr:  fleet.GitRepo{},
+			expectedErr: "empty targetNamespace denied.*allowedTargetNamespaces restriction is present",
+		},
+		{
+			name: "deny disallowed targetNamespace",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					TargetNamespace: "not-foo",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedTargetNamespaces: []string{"foo"},
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					TargetNamespace: "not-foo",
+				},
+			},
+			expectedErr: "disallowed targetNamespace.*",
+		},
+		{
+			name: "deny disallowed service account",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ServiceAccount: "not-foo",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedServiceAccounts: []string{"foo"},
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ServiceAccount: "not-foo",
+				},
+			},
+			expectedErr: "disallowed serviceAccount.*",
+		},
+		{
+			name: "deny disallowed repo pattern",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "bar",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedRepoPatterns: []string{"baz"},
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "bar",
+				},
+			},
+			expectedErr: "disallowed repo.*",
+		},
+		{
+			name: "deny disallowed client secret name",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ClientSecretName: "not-foo",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedClientSecretNames: []string{"foo"},
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ClientSecretName: "not-foo",
+				},
+			},
+			expectedErr: "disallowed clientSecretName.*",
+		},
+		{
+			name: "pass when no restrictions nor disallowed values exist",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "http://foo.bar/baz",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "http://foo.bar/baz",
+				},
+			},
+		},
+		{
+			name: "pass when restrictions exist and the GitRepo matches allowed values",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ClientSecretName: "csn",
+					Repo:             "http://foo.bar/baz",
+					ServiceAccount:   "sacc",
+					TargetNamespace:  "tns",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedClientSecretNames: []string{"csn"},
+						AllowedRepoPatterns:      []string{".*foo.bar.*"},
+						AllowedServiceAccounts:   []string{"sacc"},
+						AllowedTargetNamespaces:  []string{"tns"},
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ClientSecretName: "csn",
+					Repo:             "http://foo.bar/baz",
+					ServiceAccount:   "sacc",
+					TargetNamespace:  "tns",
+				},
+			},
+		},
+		{
+			name: "pass and mutate repo with defaults when restrictions exist and the GitRepo matches allowed values",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo:            "http://foo.bar/baz",
+					TargetNamespace: "tns",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedClientSecretNames: []string{"csn"},
+						AllowedRepoPatterns:      []string{".*foo.bar.*"},
+						AllowedServiceAccounts:   []string{"sacc"},
+						AllowedTargetNamespaces:  []string{"tns"},
+						DefaultClientSecretName:  "dcsn",
+						DefaultServiceAccount:    "dsacc",
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					ClientSecretName: "dcsn",
+					Repo:             "http://foo.bar/baz",
+					ServiceAccount:   "dsacc",
+					TargetNamespace:  "tns",
+				},
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+
+			client := mocks.NewMockClient(mockCtrl)
+
+			client.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(
+				func(_ context.Context, rl *fleetv1.GitRepoRestrictionList, ns crclient.InNamespace) error {
+					if c.restrictions != nil && len(c.restrictions.Items) > 0 {
+						rl.Items = c.restrictions.Items
+					}
+
+					return c.restrictionsListErr
+				},
+			)
+
+			err := reconciler.AuthorizeAndAssignDefaults(context.TODO(), client, &c.inputGr)
+
+			if len(c.expectedErr) > 0 {
+				require.NotNil(t, err)
+				assert.Regexp(t, regexp.MustCompile(c.expectedErr), err.Error())
+			} else {
+				assert.Nil(t, err)
+			}
+
+			if !reflect.DeepEqual(c.inputGr, c.expectedGr) {
+				t.Errorf("Expected res %v, got %v", c.expectedGr, c.inputGr)
+			}
+		})
+	}
+}