fix: Deletion of cluster labels don't work (#48)

Signed-off-by: Kasprzak, Mikolaj <[email protected]>
Co-authored-by: Daniecki, Jozef <[email protected]>

Author: MikolajKasprzak

internal/k8s/client.go

@@ -240,28 +240,32 @@ func (cli *Client) DeleteNamespace(ctx context.Context, namespace string) error
 	return cli.Dyn.Resource(namespaceRes).Delete(ctx, namespace, deleteOptions)
 }
 
-// CreateClusterLabels creates new labels on the cluster object in the given namespace
-func (cli *Client) CreateClusterLabels(ctx context.Context, namespace string, clusterName string, newLabels map[string]string) error {
-	if newLabels == nil {
+// SetClusterLabels overrides the labels of the cluster object in the given namespace
+func (cli *Client) SetClusterLabels(ctx context.Context, namespace string, clusterName string, newUserLabels map[string]string) error {
+	if newUserLabels == nil {
 		return nil
 	}
 
-	return createLabels(ctx, cli, namespace, clusterResourceSchema, clusterName, newLabels)
+	return modifyLabels(ctx, cli, namespace, clusterResourceSchema, clusterName, func(cluster *unstructured.Unstructured) {
+		cluster.SetLabels(labels.Merge(labels.SystemLabels(cluster.GetLabels()), newUserLabels))
+	})
 }
 
-// CreateTemplateLabels creates new labels on the template object in the given namespace
-func (cli *Client) CreateTemplateLabels(ctx context.Context, namespace string, templateName string, newLabels map[string]string) error {
+// AddTemplateLabels appends new labels on the template object in the given namespace
+func (cli *Client) AddTemplateLabels(ctx context.Context, namespace string, templateName string, newLabels map[string]string) error {
 	if newLabels == nil {
 		return nil
 	}
 
-	return createLabels(ctx, cli, namespace, templateResourceSchema, templateName, newLabels)
+	return modifyLabels(ctx, cli, namespace, templateResourceSchema, templateName, func(template *unstructured.Unstructured) {
+		template.SetLabels(labels.Merge(template.GetLabels(), newLabels))
+	})
 }
 
-// createLabels creates new labels on the resource object in the given namespace
+// modifyLabels modifies the labels of the given resource in the given namespace
 // It retries on transient "the object has been modified" error, which is expected when the cluster object was updated by another process after we fetched it
 // It returns an error if the operation fails after all retries
-func createLabels(ctx context.Context, cli *Client, namespace string, resourceSchema schema.GroupVersionResource, resourceName string, newLabels map[string]string) error {
+func modifyLabels(ctx context.Context, cli *Client, namespace string, resourceSchema schema.GroupVersionResource, resourceName string, op func(*unstructured.Unstructured)) error {
 	transientError := func(err error) bool {
 		tryAgainErrPattern := "the object has been modified; please apply your changes to the latest version and try again"
 		return strings.Contains(err.Error(), tryAgainErrPattern)
@@ -272,7 +276,7 @@ func createLabels(ctx context.Context, cli *Client, namespace string, resourceSc
 		if err != nil {
 			return backoff.Permanent(err)
 		}
-		resource.SetLabels(labels.Merge(resource.GetLabels(), newLabels))
+		op(resource)
 		if _, err = cli.Dyn.Resource(resourceSchema).Namespace(namespace).Update(ctx, resource, metav1.UpdateOptions{}); err != nil {
 			if transientError(err) {
 				return err // retry on transient error

internal/labels/labels.go

@@ -20,7 +20,7 @@ const (
 )
 
 var (
-	systemPrefixes  = []string{PlatformPrefix, capiDomainLabelKey, capiTopologyLabelKey, PrometheusMetricsUrlLabelKey}
+	systemPrefixes  = []string{PlatformPrefix, capiDomainLabelKey, capiTopologyLabelKey, PrometheusMetricsUrlLabelKey, TrustedComputeLabelKey}
 	labelKeyRegex   = regexp.MustCompile(`^(([A-Za-z0-9][-A-Za-z0-9_.]{0,250})?[A-Za-z0-9]\/)?([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9]$`)
 	labelValueRegex = regexp.MustCompile(`^([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9]?$`)
 )
@@ -29,10 +29,8 @@ func OverrideSystemPrefixes(prefixes []string) {
 	systemPrefixes = prefixes
 }
 
-// Filter returns new map with only user defined labels
-func Filter(clusterLabels map[string]string) map[string]string {
-	f := map[string]string{}
-
+// UserLabels returns new map with only user defined labels
+func UserLabels(clusterLabels map[string]string) map[string]string {
 	keep := func(s string) bool {
 		for _, p := range systemPrefixes {
 			if strings.HasPrefix(s, p) {
@@ -42,7 +40,27 @@ func Filter(clusterLabels map[string]string) map[string]string {
 		return true
 	}
 
-	for key, value := range clusterLabels {
+	return filter(clusterLabels, keep)
+}
+
+// SystemLabels returns new map with only system defined labels
+func SystemLabels(clusterLabels map[string]string) map[string]string {
+	keep := func(s string) bool {
+		for _, p := range systemPrefixes {
+			if strings.HasPrefix(s, p) {
+				return true
+			}
+		}
+		return false
+	}
+
+	return filter(clusterLabels, keep)
+}
+
+func filter(labels map[string]string, keep func(string) bool) map[string]string {
+	f := map[string]string{}
+
+	for key, value := range labels {
 		if !keep(key) {
 			continue
 		}

internal/labels/labels_test.go

@@ -10,7 +10,7 @@ import (
 	"github.com/open-edge-platform/cluster-manager/v2/internal/labels"
 )
 
-func TestFilter(t *testing.T) {
+func TestUserLabels(t *testing.T) {
 	clusterLabels := map[string]string{
 		"edge-orchestrator.intel.com/cluster-id":  "cluster-479656a6",
 		"edge-orchestrator.intel.com/clustername": "scale-0a9c889f-97a4-5a27-aa57-e82300a2b19c",
@@ -25,6 +25,7 @@ func TestFilter(t *testing.T) {
 		"default":                                 "true",
 		"edge-orchestrator.intel.com/users-label": "user-value",
 		"topology.cluster.x-k8s.io/owned":         "",
+		"trusted-compute-compatible":              "true",
 	}
 
 	want := map[string]string{
@@ -35,7 +36,43 @@ func TestFilter(t *testing.T) {
 		"default":     "true",
 	}
 
-	got := labels.Filter(clusterLabels)
+	got := labels.UserLabels(clusterLabels)
+	if diff := cmp.Diff(want, got); diff != "" {
+		t.Errorf("filter mismatch (-want +got):\n%s", diff)
+	}
+}
+
+func TestSystemLabels(t *testing.T) {
+	clusterLabels := map[string]string{
+		"edge-orchestrator.intel.com/cluster-id":  "cluster-479656a6",
+		"edge-orchestrator.intel.com/clustername": "scale-0a9c889f-97a4-5a27-aa57-e82300a2b19c",
+		"edge-orchestrator.intel.com/project-id":  "0ba9d4dc-c4c2-4904-b526-57ace6e76922",
+		"edge-orchestrator.intel.com/template":    "scaletest-v0.0.2",
+		"clustername":                             "scale-0a9c889f-97a4-5a27-aa57-e82300a2b19c",
+		"cpumanager":                              "true",
+		"prometheusMetricsURL":                    "metrics-node.scale.espd.infra-host.com",
+		"test-app":                                "enabled",
+		"tests":                                   "scale",
+		"cluster.x-k8s.io/cluster-name":           "demo-cluster",
+		"default":                                 "true",
+		"edge-orchestrator.intel.com/users-label": "user-value",
+		"topology.cluster.x-k8s.io/owned":         "",
+		"trusted-compute-compatible":              "false",
+	}
+
+	want := map[string]string{
+		"edge-orchestrator.intel.com/cluster-id":  "cluster-479656a6",
+		"edge-orchestrator.intel.com/clustername": "scale-0a9c889f-97a4-5a27-aa57-e82300a2b19c",
+		"edge-orchestrator.intel.com/project-id":  "0ba9d4dc-c4c2-4904-b526-57ace6e76922",
+		"edge-orchestrator.intel.com/template":    "scaletest-v0.0.2",
+		"prometheusMetricsURL":                    "metrics-node.scale.espd.infra-host.com",
+		"cluster.x-k8s.io/cluster-name":           "demo-cluster",
+		"edge-orchestrator.intel.com/users-label": "user-value",
+		"topology.cluster.x-k8s.io/owned":         "",
+		"trusted-compute-compatible":              "false",
+	}
+
+	got := labels.SystemLabels(clusterLabels)
 	if diff := cmp.Diff(want, got); diff != "" {
 		t.Errorf("filter mismatch (-want +got):\n%s", diff)
 	}

internal/multitenancy/multitenancy.go

@@ -170,7 +170,7 @@ func (tdm *TenancyDatamodel) setupProject(ctx context.Context, project *nexus.Ru
 	}
 
 	labels := map[string]string{labels.DefaultLabelKey: labels.DefaultLabelVal}
-	if err = tdm.k8s.CreateTemplateLabels(ctx, projectId, defaultTemplateName, labels); err != nil {
+	if err = tdm.k8s.AddTemplateLabels(ctx, projectId, defaultTemplateName, labels); err != nil {
 		return fmt.Errorf("failed to label default template: %w", err)
 	}
 

internal/rest/getv2clusters.go

@@ -125,7 +125,7 @@ func (s *Server) convertClusters(ctx context.Context, namespace string, unstruct
 		// get machines associated with the cluster
 		machines := getClusterMachines(allMachines, capiCluster.Name)
 
-		labels := labels.Filter(capiCluster.Labels)
+		labels := labels.UserLabels(capiCluster.Labels)
 		unstructuredLabels := convert.MapStringToAny(labels)
 
 		lp, errs := getClusterLifecyclePhase(&capiCluster)

internal/rest/getv2clustersclusterdetail.go

@@ -30,7 +30,7 @@ func (s *Server) GetV2ClustersNodeIdClusterdetail(ctx context.Context, request a
 			},
 		}, nil
 	}
-	labels := labels.Filter(convert.MapAnyToString(*clusterDetails.Labels))
+	labels := labels.UserLabels(convert.MapAnyToString(*clusterDetails.Labels))
 	unstructuredLabels := convert.MapStringToAny(labels)
 	clusterDetails.Labels = &unstructuredLabels
 	return api.GetV2ClustersNodeIdClusterdetail200JSONResponse(clusterDetails), nil

internal/rest/getv2clustersname.go

@@ -97,7 +97,7 @@ func (s *Server) getCluster(ctx context.Context, activeProjectID, name string) (
 		slog.Error("failed to fetch machines for cluster", "cluster", capiCluster.Name, "error", err)
 	}
 
-	labels := labels.Filter(capiCluster.Labels)
+	labels := labels.UserLabels(capiCluster.Labels)
 	unstrucutreLabels := convert.MapStringToAny(labels)
 
 	nodes, err := cluster.Nodes(ctx, cli, capiCluster)

internal/rest/putv2clusterlabels.go

@@ -38,10 +38,10 @@ func (s *Server) PutV2ClustersNameLabels(ctx context.Context, request api.PutV2C
 		}, nil
 	}
 
-	newLabels := *request.Body.Labels
-	if !labels.Valid(newLabels) {
+	newUserLabels := *request.Body.Labels
+	if !labels.Valid(newUserLabels) {
 		errMsg := "invalid cluster label keys"
-		slog.Warn(errMsg, "labels", newLabels)
+		slog.Warn(errMsg, "labels", newUserLabels)
 		return api.PutV2ClustersNameLabels400JSONResponse{
 			N400BadRequestJSONResponse: api.N400BadRequestJSONResponse{
 				Message: &errMsg,
@@ -56,7 +56,7 @@ func (s *Server) PutV2ClustersNameLabels(ctx context.Context, request api.PutV2C
 		return api.PutV2ClustersNameLabels500JSONResponse{N500InternalServerErrorJSONResponse: api.N500InternalServerErrorJSONResponse{Message: &message}}, nil
 	}
 
-	err = cli.CreateClusterLabels(ctx, activeProjectID, clusterName, newLabels)
+	err = cli.SetClusterLabels(ctx, activeProjectID, clusterName, newUserLabels)
 
 	switch {
 	case errors.IsBadRequest(err):
@@ -73,6 +73,6 @@ func (s *Server) PutV2ClustersNameLabels(ctx context.Context, request api.PutV2C
 		return api.PutV2ClustersNameLabels500JSONResponse{N500InternalServerErrorJSONResponse: api.N500InternalServerErrorJSONResponse{Message: &message}}, nil
 	}
 
-	slog.Info("Cluster labels updated", "namespace", activeProjectID, "name", request.Name, "labels", newLabels)
+	slog.Info("Cluster labels updated", "namespace", activeProjectID, "name", request.Name, "labels", newUserLabels)
 	return api.PutV2ClustersNameLabels200Response{}, nil
 }

test/service/service_suite_test.go

@@ -203,7 +203,7 @@ var _ = Describe("Cluster create/delete flow", Ordered, func() {
 			params := api.PutV2ClustersNameLabelsParams{}
 			params.Activeprojectid = testTenantID
 			body := api.PutV2ClustersNameLabelsJSONRequestBody{
-				Labels: &map[string]string{"app": "wordpress"},
+				Labels: &map[string]string{"app": "wordpress", "default-extension": "baseline"},
 			}
 			resp, err := cli.PutV2ClustersNameLabelsWithResponse(context.Background(), clusterName, &params, body)
 			Expect(err).ToNot(HaveOccurred())
@@ -218,10 +218,9 @@ var _ = Describe("Cluster create/delete flow", Ordered, func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(resp.StatusCode()).To(Equal(200))
 			Expect(*resp.JSON200.Name).To(Equal(clusterName))
-			Expect(*resp.JSON200.Labels).To(HaveLen(3))
+			Expect(*resp.JSON200.Labels).To(HaveLen(2))
 			Expect(*resp.JSON200.Labels).To(HaveKeyWithValue("app", "wordpress"))
 			Expect(*resp.JSON200.Labels).To(HaveKeyWithValue("default-extension", "baseline"))
-			Expect(*resp.JSON200.Labels).To(HaveKeyWithValue("trusted-compute-compatible", "false"))
 			Expect(*resp.JSON200.Nodes).To(HaveLen(1))
 			nodes := *resp.JSON200.Nodes
 			Expect(*nodes[0].Role).To(Equal("all"))
@@ -234,13 +233,35 @@ var _ = Describe("Cluster create/delete flow", Ordered, func() {
 			err = containsLabels(testTenantID.String(), clusterName, []string{
 				"app:wordpress",
 				"default-extension:baseline",
-				"trusted-compute-compatible:false",
 				fmt.Sprintf("edge-orchestrator.intel.com/clustername:%v", clusterName),
 				fmt.Sprintf("edge-orchestrator.intel.com/project-id:%v", testTenantID.String()),
 			})
 			Expect(err).ToNot(HaveOccurred())
 		})
 
+		It("Should delete label", func() {
+			params := api.PutV2ClustersNameLabelsParams{}
+			params.Activeprojectid = testTenantID
+			body := api.PutV2ClustersNameLabelsJSONRequestBody{
+				Labels: &map[string]string{"default-extension": "baseline"},
+			}
+			resp, err := cli.PutV2ClustersNameLabelsWithResponse(context.Background(), clusterName, &params, body)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(resp.StatusCode()).To(Equal(200))
+
+		})
+
+		It("Deleted label should be missing in label list", func() {
+			params := api.GetV2ClustersNameParams{}
+			params.Activeprojectid = testTenantID
+			resp, err := cli.GetV2ClustersNameWithResponse(context.Background(), clusterName, &params)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(resp.StatusCode()).To(Equal(200))
+			Expect(*resp.JSON200.Name).To(Equal(clusterName))
+			Expect(*resp.JSON200.Labels).To(HaveLen(1))
+			Expect(*resp.JSON200.Labels).To(HaveKeyWithValue("default-extension", "baseline"))
+		})
+
 		It("Should fail to delete cluster template if cluster is running", func() {
 			params := api.DeleteV2TemplatesNameVersionParams{}
 			params.Activeprojectid = testTenantID