[v0.10] Fix apply defaults from gitrepo restriction

e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml

@@ -7,4 +7,4 @@ spec:
   repo: https://github.com/rancher/fleet-test-data
   branch: master
   paths:
-    - target-customization-namespace-labels
+    - target-customization-namespace-labels/with-default-values

e2e/single-cluster/targetCustomization_test.go

@@ -85,14 +85,14 @@ var _ = Describe("Helm deploy options", func() {
 						return "nil"
 					}
 
-					bundleDeploymentNamespacesLabels, _ := clusterK.Get(
+					bundleDeploymentNamespacesAnnotations, _ := clusterK.Get(
 						"bundledeployments",
 						bundleDeploymentName,
 						"-o",
 						"jsonpath={.spec.options.namespaceAnnotations}",
 					)
-					return bundleDeploymentNamespacesLabels
-				}).Should(Equal(`{"foo":"bar","this.is/a":"test"}`))
+					return bundleDeploymentNamespacesAnnotations
+				}).Should(Equal(`{"foo":"bar","this.is/another":"test"}`))
 
 			})
 		})

internal/cmd/agent/deployer/deployer.go

@@ -264,7 +264,6 @@ func deployErrToStatus(err error, status fleet.BundleDeploymentStatus) (bool, fl
 			"(YAML parse error)|" + // YAML is broken in source files
 			"(Forbidden: updates to [0-9A-Za-z]+ spec for fields other than [0-9A-Za-z ']+ are forbidden)|" + // trying to update fields that cannot be updated
 			"(Forbidden: spec is immutable after creation)|" + // trying to modify immutable spec
-			"(cannot patch.*is invalid)|" + // trying to apply an invalid patch (eg. undoing a port edit on a Service)
 			"(chart requires kubeVersion: [0-9A-Za-z\\.\\-<>=]+ which is incompatible with Kubernetes)", // trying to deploy to incompatible Kubernetes
 	)
 	if re.MatchString(msg) {

internal/cmd/controller/agentmanagement/controllers/cluster/import.go

@@ -292,7 +292,7 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust
 				TTL: &metav1.Duration{Duration: durations.ClusterImportTokenTTL},
 			},
 		})
-		logrus.Debugf("Failed to create ClusterRegistrationToken for cluster %s/%s: %v (requeueing)", cluster.Namespace, cluster.Name, err)
+		logrus.Debugf("Failed to create ClusterRegistrationToken for cluster %s/%s: %v (requeuing)", cluster.Namespace, cluster.Name, err)
 		i.clusters.EnqueueAfter(cluster.Namespace, cluster.Name, durations.TokenClusterEnqueueDelay)
 		return status, nil
 	}

internal/cmd/controller/gitops/reconciler/gitjob_controller.go

@@ -150,8 +150,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)
 	}

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) {

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)
+			}
+		})
+	}
+}

internal/cmd/controller/reconciler/bundledeployment_controller.go

@@ -109,7 +109,7 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req
 
 	err = r.Status().Update(ctx, bd)
 	if err != nil {
-		logger.V(1).Error(err, "Reconcile failed update to bundle deployment status, requeueing", "status", bd.Status)
+		logger.V(1).Error(err, "Reconcile failed update to bundle deployment status, requeuing", "status", bd.Status)
 		return ctrl.Result{}, err
 	}