diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 2b88bf9d17..423870f2fa 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 28410bd6a9..acbfbf0ab7 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/restrictions_test.go b/internal/cmd/controller/gitops/reconciler/restrictions_test.go new file mode 100644 index 0000000000..7d6bc19fa7 --- /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) + } + }) + } +} diff --git a/internal/cmd/controller/gitops/reconciler/status_controller.go b/internal/cmd/controller/gitops/reconciler/status_controller.go index e93acf9876..15d1bca839 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, })