From 35a46c720e8b18327d861003fb0a3734a3f5ac58 Mon Sep 17 00:00:00 2001 From: Mario Manno Date: Thu, 14 Nov 2024 14:01:38 +0100 Subject: [PATCH 1/4] Apply defaults from gitrepo restrictions (#3056) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Apply defaults from gitrepo restrictions * Add unit tests covering defaults auth and assign This covers a few happy and error cases with unit tests, uncovering a few typos in error messages in the process. --------- Co-authored-by: Corentin Néau --- .../gitops/reconciler/gitjob_controller.go | 3 +- .../gitops/reconciler/restrictions.go | 36 +-- .../gitops/reconciler/restrictions_test.go | 249 ++++++++++++++++++ 3 files changed, 271 insertions(+), 17 deletions(-) create mode 100644 internal/cmd/controller/gitops/reconciler/restrictions_test.go diff --git a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go b/internal/cmd/controller/gitops/reconciler/gitjob_controller.go index 10873506f0..0b8f9353b5 100644 --- a/internal/cmd/controller/gitops/reconciler/gitjob_controller.go +++ b/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) } 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) + } + }) + } +} From 0f7456cf3001b515a01468fed613b2070accd9c3 Mon Sep 17 00:00:00 2001 From: Mario Manno Date: Fri, 15 Nov 2024 13:56:28 +0100 Subject: [PATCH 2/4] Revert "Skip install loop for invalid patches (#3040)" This reverts commit 02e4067cd33b876ca3ec5e15766ba7002cdefb41. --- internal/cmd/agent/deployer/deployer.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/cmd/agent/deployer/deployer.go b/internal/cmd/agent/deployer/deployer.go index 8cb3bfc919..f1044acd83 100644 --- a/internal/cmd/agent/deployer/deployer.go +++ b/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) { From 99137fb8a113d979c8b26f87be6b4fbacec11100 Mon Sep 17 00:00:00 2001 From: Mario Manno Date: Fri, 15 Nov 2024 13:59:12 +0100 Subject: [PATCH 3/4] Fix CI on release branch --- .../single-cluster/namespaceLabels_targetCustomization.yaml | 2 +- .../controller/agentmanagement/controllers/cluster/import.go | 2 +- .../cmd/controller/reconciler/bundledeployment_controller.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml b/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml index 337746ae93..779a616ec5 100644 --- a/e2e/assets/single-cluster/namespaceLabels_targetCustomization.yaml +++ b/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 diff --git a/internal/cmd/controller/agentmanagement/controllers/cluster/import.go b/internal/cmd/controller/agentmanagement/controllers/cluster/import.go index 1df456a652..b6e3dc5277 100644 --- a/internal/cmd/controller/agentmanagement/controllers/cluster/import.go +++ b/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 } diff --git a/internal/cmd/controller/reconciler/bundledeployment_controller.go b/internal/cmd/controller/reconciler/bundledeployment_controller.go index 966de38d51..d0a5f91344 100644 --- a/internal/cmd/controller/reconciler/bundledeployment_controller.go +++ b/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 } From cc28a1e84d341fcf71d3b3639483c48bd4798c62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Fri, 15 Nov 2024 14:35:39 +0100 Subject: [PATCH 4/4] Fix expectation on namespace annotations This reflects a recent change in `rancher/fleet-test-data` where we use distinct values for labels vs annotations. --- e2e/single-cluster/targetCustomization_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/single-cluster/targetCustomization_test.go b/e2e/single-cluster/targetCustomization_test.go index 651563e06a..9737f6ad1c 100644 --- a/e2e/single-cluster/targetCustomization_test.go +++ b/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"}`)) }) })