From 1f491517d85ffbc98efc6c794d1580e904728075 Mon Sep 17 00:00:00 2001 From: mgianluc Date: Sun, 17 Nov 2024 16:29:53 +0100 Subject: [PATCH] (bug) when deploymentType is Local, validate namespace A Profile using deploymentType: Local, can only deploy resources in the same namespace. Previously, Sveltos used the following format for the OwnerReferences name for resources it deployed due to a Profile: ``` Profile.namespace/Profile.name ``` However, a Profile with this exact name doesn't exist within the management cluster. As a result, the Kubernetes controller would immediately remove the deployed resource. This behavior occurred because: ``` A namespaced owner must exist in the same namespace as the dependent. If it does not, the owner reference is treated as absent, and the dependent is subject to deletion once all owners are verified absent ``` Given this PR ensures that a Profile only creates resources in the management cluster in the same namespace, now only Profile.name is set in the OwnerReference.name --- controllers/handlers_utils.go | 27 +++-- controllers/utils.go | 22 ---- test/fv/profile_local_test.go | 221 ++++++++++++++++++++++++++++++++++ 3 files changed, 241 insertions(+), 29 deletions(-) create mode 100644 test/fv/profile_local_test.go diff --git a/controllers/handlers_utils.go b/controllers/handlers_utils.go index b2f640ff..862047a8 100644 --- a/controllers/handlers_utils.go +++ b/controllers/handlers_utils.go @@ -358,6 +358,21 @@ func adjustNamespace(policy *unstructured.Unstructured, destConfig *rest.Config) return nil } +// isResourceNamespaceValid validates the resource namespace. +// A Profile, when deploying resources locally, i.e, to the management cluster, can +// only deploy resources in the same namespace +func isResourceNamespaceValid(profile client.Object, policy *unstructured.Unstructured, + deployingToMgmtCluster bool) bool { + + if profile.GetObjectKind().GroupVersionKind().Kind == configv1beta1.ProfileKind { + if deployingToMgmtCluster && policy.GetNamespace() != profile.GetNamespace() { + return false + } + } + + return true +} + // deployUnstructured deploys referencedUnstructured objects. // Returns an error if one occurred. Otherwise it returns a slice containing the name of // the policies deployed in the form of kind.group:namespace:name for namespaced policies @@ -373,9 +388,6 @@ func deployUnstructured(ctx context.Context, deployingToMgmtCluster bool, destCo if err != nil { return nil, err } - if profile.GetObjectKind().GroupVersionKind().Kind == configv1beta1.ProfileKind { - profile.SetName(profileNameToOwnerReferenceName(profile)) - } patches, err := initiatePatches(ctx, clusterSummary, "patch", mgmtResources, logger) if err != nil { @@ -400,6 +412,10 @@ func deployUnstructured(ctx context.Context, deployingToMgmtCluster bool, destCo return nil, err } + if !isResourceNamespaceValid(profile, policy, deployingToMgmtCluster) { + return nil, fmt.Errorf("profile can only deploy resource in same namespace in the management cluster") + } + logger.V(logs.LogDebug).Info(fmt.Sprintf("deploying resource %s %s/%s (deploy to management cluster: %v)", policy.GetKind(), policy.GetNamespace(), policy.GetName(), deployingToMgmtCluster)) @@ -517,7 +533,7 @@ func requeueAllOldOwners(ctx context.Context, profileOwners []corev1.ObjectRefer profileName = types.NamespacedName{Name: profileOwners[i].Name} case configv1beta1.ProfileKind: profileKind = configv1beta1.ProfileKind - profileName = *getProfileNameFromOwnerReferenceName(profileOwners[i].Name) + profileName = types.NamespacedName{Namespace: clusterSummary.Namespace, Name: profileOwners[i].Name} default: continue } @@ -1066,9 +1082,6 @@ func undeployStaleResources(ctx context.Context, isMgmtCluster bool, if err != nil { return nil, err } - if profile.GetObjectKind().GroupVersionKind().Kind == configv1beta1.ProfileKind { - profile.SetName(profileNameToOwnerReferenceName(profile)) - } undeployed := make([]configv1beta1.ResourceReport, 0) diff --git a/controllers/utils.go b/controllers/utils.go index d288efd8..e4fbc3a7 100644 --- a/controllers/utils.go +++ b/controllers/utils.go @@ -393,28 +393,6 @@ func parseMapFromString(data string) (map[string]string, error) { return result, nil } -// Sveltos deployment in managed clusters relies on OwnerReferences to track the responsible profile. -// However, a limitation arises with namespaced Profiles. -// Kubernetes OwnerReferences lack a namespace field, assuming owners reside in the same namespace. -// For Profile resources (namespaced), Sveltos dynamically modifies the owner name to incorporate both -// namespace and name for proper identification. -func profileNameToOwnerReferenceName(profile client.Object) string { - if profile.GetObjectKind().GroupVersionKind().Kind == configv1beta1.ProfileKind { - return fmt.Sprintf("%s/%s", profile.GetNamespace(), profile.GetName()) - } - - return profile.GetName() -} - -func getProfileNameFromOwnerReferenceName(profileName string) *types.NamespacedName { - result := strings.Split(profileName, "/") - if len(result) == 1 { - // resources deployed by Sveltos before release v0.30.0 did not have profile namespace/name - return &types.NamespacedName{Name: profileName} - } - return &types.NamespacedName{Namespace: result[0], Name: result[1]} -} - // Function to remove duplicates from a slice func unique[T comparable](input []T) []T { seen := make(map[T]bool) diff --git a/test/fv/profile_local_test.go b/test/fv/profile_local_test.go new file mode 100644 index 00000000..b84ceecc --- /dev/null +++ b/test/fv/profile_local_test.go @@ -0,0 +1,221 @@ +/* +Copyright 2024. projectsveltos.io. All rights reserved. + +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. +*/ + +package fv_test + +import ( + "context" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + + configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" + "github.com/projectsveltos/addon-controller/controllers" + libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" +) + +var _ = Describe("Profile with Deployment Local", func() { + const ( + namePrefix = "profile-local" + + sa = `apiVersion: v1 +kind: ServiceAccount +metadata: + name: %s + namespace: %s + labels: + name: fv` + ) + + It("deploymentType Local only allows resources to be created in the same namespace", Label("FV", "EXTENDED"), func() { + By("Grant addon-controller permission to create/delete ServiceAccount in the management cluster") + clusterRole := &rbacv1.ClusterRole{} + Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: "addon-controller-role-extra"}, + clusterRole)).To(Succeed()) + clusterRole.Rules = append(clusterRole.Rules, + rbacv1.PolicyRule{ + Verbs: []string{"*"}, + APIGroups: []string{""}, + Resources: []string{"serviceaccounts"}, + }) + Expect(k8sClient.Update(context.TODO(), clusterRole)).To(Succeed()) + + Byf("Create a Profile matching Cluster %s/%s", + kindWorkloadCluster.Namespace, kindWorkloadCluster.Name) + + profile := getProfile(defaultNamespace, namePrefix, map[string]string{key: value}) + profile.Spec.SyncMode = configv1beta1.SyncModeContinuous + + Expect(k8sClient.Create(context.TODO(), profile)).To(Succeed()) + + verifyProfileMatches(profile) + + verifyClusterSummary(controllers.ProfileLabelName, + profile.Name, &profile.Spec, + kindWorkloadCluster.Namespace, kindWorkloadCluster.Name) + + cmName := randomString() + Byf("Create a configMap with a sa %s/%s", defaultNamespace, cmName) + // This ConfigMap contains an invalid ServiceAccount. It is invalid + // because Profile will reference this ConfigMap and use DeploymentType: Local + // but the ServiceAccount is in a namespace different than profile and that is + // not allowed + invalidConfigMap := createConfigMapWithPolicy(defaultNamespace, randomString(), + fmt.Sprintf(sa, randomString(), randomString())) + Expect(k8sClient.Create(context.TODO(), invalidConfigMap)).To(Succeed()) + + currentConfigMap := &corev1.ConfigMap{} + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: invalidConfigMap.Namespace, Name: invalidConfigMap.Name}, + currentConfigMap)).To(Succeed()) + + Byf("Update Profile %s to reference ConfigMap %s/%s", + profile.Name, invalidConfigMap.Namespace, invalidConfigMap.Name) + + currentProfile := &configv1beta1.Profile{} + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: profile.Namespace, Name: profile.Name}, + currentProfile)).To(Succeed()) + currentProfile.Spec.PolicyRefs = []configv1beta1.PolicyRef{ + { + Kind: string(libsveltosv1beta1.ConfigMapReferencedResourceKind), + Namespace: invalidConfigMap.Namespace, + Name: invalidConfigMap.Name, + DeploymentType: configv1beta1.DeploymentTypeLocal, + }, + } + Expect(k8sClient.Update(context.TODO(), currentProfile)).To(Succeed()) + + clusterSummary := verifyClusterSummary(controllers.ProfileLabelName, + currentProfile.Name, ¤tProfile.Spec, + kindWorkloadCluster.Namespace, kindWorkloadCluster.Name) + + Byf("Verifying ClusterSummary %s/%s reports an error", clusterSummary.Namespace, clusterSummary.Name) + Eventually(func() bool { + errorMsg := "profile can only deploy resource in same namespace in the management cluster" + currentClusterSummary := &configv1beta1.ClusterSummary{} + err := k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary) + if err != nil { + return false + } + + for i := range currentClusterSummary.Status.FeatureSummaries { + if currentClusterSummary.Status.FeatureSummaries[i].FeatureID == configv1beta1.FeatureResources { + if currentClusterSummary.Status.FeatureSummaries[i].Status != configv1beta1.FeatureStatusProvisioned { + if currentClusterSummary.Status.FeatureSummaries[i].FailureMessage != nil && + *currentClusterSummary.Status.FeatureSummaries[i].FailureMessage == errorMsg { + return true + } + } + } + } + + return false + }, timeout, pollingInterval).Should(BeTrue()) + + cmName = randomString() + saName := randomString() + Byf("Create a configMap with a sa %s/%s", defaultNamespace, cmName) + // This ConfigMap contains a valid ServiceAccount. It is valid + // because Profile will reference this ConfigMap and use DeploymentType: Local + // and the ServiceAccount is in the Profile namespace + validConfigMap := createConfigMapWithPolicy(defaultNamespace, randomString(), + fmt.Sprintf(sa, saName, defaultNamespace)) + Expect(k8sClient.Create(context.TODO(), validConfigMap)).To(Succeed()) + + Byf("Update Profile %s to reference ConfigMap %s/%s", + profile.Name, validConfigMap.Namespace, validConfigMap.Name) + + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: profile.Namespace, Name: profile.Name}, + currentProfile)).To(Succeed()) + currentProfile.Spec.PolicyRefs = []configv1beta1.PolicyRef{ + { + Kind: string(libsveltosv1beta1.ConfigMapReferencedResourceKind), + Namespace: validConfigMap.Namespace, + Name: validConfigMap.Name, + DeploymentType: configv1beta1.DeploymentTypeLocal, + }, + } + Expect(k8sClient.Update(context.TODO(), currentProfile)).To(Succeed()) + + clusterSummary = verifyClusterSummary(controllers.ProfileLabelName, + currentProfile.Name, ¤tProfile.Spec, + kindWorkloadCluster.Namespace, kindWorkloadCluster.Name) + + Byf("Verifying ClusterSummary %s/%s is Provisioned", clusterSummary.Namespace, clusterSummary.Name) + Eventually(func() bool { + currentClusterSummary := &configv1beta1.ClusterSummary{} + err := k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary) + if err != nil { + return false + } + + for i := range currentClusterSummary.Status.FeatureSummaries { + if currentClusterSummary.Status.FeatureSummaries[i].FeatureID == configv1beta1.FeatureResources { + if currentClusterSummary.Status.FeatureSummaries[i].Status == configv1beta1.FeatureStatusProvisioned { + return true + } + } + } + + return false + }, timeout, pollingInterval).Should(BeTrue()) + + Byf("Verifying ServiceAccount %s/%s is present", defaultNamespace, saName) + serviceAccount := &corev1.ServiceAccount{} + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: defaultNamespace, Name: saName}, serviceAccount)).To(Succeed()) + + deleteProfile(profile) + + Byf("Verifying ServiceAccount is gone from the management cluster") + Eventually(func() bool { + currentServiceAccount := &corev1.ServiceAccount{} + err := k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: defaultNamespace, Name: saName}, + currentServiceAccount) + if err == nil { + return !currentServiceAccount.DeletionTimestamp.IsZero() + } + return err != nil && + apierrors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue()) + + Byf("Deleting ConfigMap %s/%s", invalidConfigMap.Namespace, invalidConfigMap.Name) + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: invalidConfigMap.Namespace, Name: invalidConfigMap.Name}, + currentConfigMap)).To(Succeed()) + Expect(k8sClient.Delete(context.TODO(), currentConfigMap)) + + Byf("Deleting ConfigMap %s/%s", validConfigMap.Namespace, validConfigMap.Name) + Expect(k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: validConfigMap.Namespace, Name: validConfigMap.Name}, + currentConfigMap)).To(Succeed()) + Expect(k8sClient.Delete(context.TODO(), currentConfigMap)) + + }) +})