Merge pull request #748 from jakobmoellerdev/applyset-label-annotation-fix

fix(applyset): make sure labels and annotations are applied

Author: k8s-ci-robot

pkg/applyset/applyset.go

@@ -26,13 +26,13 @@ import (
 	"crypto/sha256"
 	"encoding/base64"
 	"fmt"
-	"reflect"
 	"sort"
 	"strings"
 	"sync"
 
 	"github.com/go-logr/logr"
 	"golang.org/x/sync/errgroup"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -374,13 +374,21 @@ func (a *applySet) Add(ctx context.Context, obj ApplyableObject) (*unstructured.
 }
 
 // ID is the label value that we are using to identify this applyset.
-// Format: base64(sha256(<name>.<namespace>.<kind>.<apiVersion>)), using the URL safe encoding of RFC4648.
 func (a *applySet) ID() string {
+	return ID(a.parent)
+}
+
+// ID computes an apply set identifier for a given parent object.
+// Format: base64(sha256(<name>.<namespace>.<kind>.<apiVersion>)), using the URL safe encoding of RFC4648.
+func ID(parent interface {
+	metav1.Object
+	schema.ObjectKind
+}) string {
 	unencoded := strings.Join([]string{
-		a.parent.GetName(),
-		a.parent.GetNamespace(),
-		a.parent.GroupVersionKind().Kind,
-		a.parent.GroupVersionKind().Group,
+		parent.GetName(),
+		parent.GetNamespace(),
+		parent.GroupVersionKind().Kind,
+		parent.GroupVersionKind().Group,
 	}, ApplySetIDPartDelimiter)
 	hashed := sha256.Sum256([]byte(unencoded))
 	b64 := base64.RawURLEncoding.EncodeToString(hashed[:])
@@ -425,55 +433,29 @@ func (a *applySet) updateParentLabelsAndAnnotations(
 
 	// Generate and append the desired labels to the parent labels
 	desiredLabels := a.desiredParentLabels()
-	labels := a.parent.GetLabels()
-	if labels == nil {
-		labels = make(map[string]string)
-	}
-	for k, v := range desiredLabels {
-		labels[k] = v
-	}
-
 	// Get the desired annotations and append them to the parent
 	desiredAnnotations, returnNamespaces, returnGKs := a.desiredParentAnnotations(mode == updateToSuperset)
-	annotations := a.parent.GetAnnotations()
-	if annotations == nil {
-		annotations = make(map[string]string)
-	}
-	for k, v := range desiredAnnotations {
-		annotations[k] = v
-	}
 
 	options := metav1.ApplyOptions{
 		FieldManager: a.fieldManager + "-parent-labeller",
 		Force:        false,
 	}
 
-	// Convert labels to map[string]interface{} for the unstructured object
-	labelsMap := make(map[string]interface{})
-	for k, v := range labels {
-		labelsMap[k] = v
-	}
-
-	// Convert annotations to map[string]interface{} for the unstructured object
-	annotationsMap := make(map[string]interface{})
-	for k, v := range annotations {
-		annotationsMap[k] = v
-	}
-
 	parentPatch := &unstructured.Unstructured{}
 	parentPatch.SetUnstructuredContent(map[string]interface{}{
 		"apiVersion": a.parent.APIVersion,
 		"kind":       a.parent.Kind,
 		"metadata": map[string]interface{}{
-			"name":        a.parent.GetName(),
-			"namespace":   a.parent.GetNamespace(),
-			"labels":      labelsMap,
-			"annotations": annotationsMap,
+			"name":      a.parent.GetName(),
+			"namespace": a.parent.GetNamespace(),
 		},
 	})
+	parentPatch.SetAnnotations(desiredAnnotations)
+	parentPatch.SetLabels(desiredLabels)
 	// update parent in the cluster.
-	if !reflect.DeepEqual(original.GetLabels(), parentPatch.GetLabels()) ||
-		!reflect.DeepEqual(original.GetAnnotations(), parentPatch.GetAnnotations()) {
+
+	if !equality.Semantic.DeepEqual(original.GetLabels(), parentPatch.GetLabels()) ||
+		!equality.Semantic.DeepEqual(original.GetAnnotations(), parentPatch.GetAnnotations()) {
 		if _, err := a.parentClient.Apply(ctx, a.parent.GetName(), parentPatch, options); err != nil {
 			return nil, nil, fmt.Errorf("error updating parent %w", err)
 		}

pkg/applyset/applyset_test.go

@@ -17,6 +17,7 @@ package applyset
 import (
 	"context"
 	"encoding/json"
+	"strings"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -200,14 +201,16 @@ func TestApplySet_Apply(t *testing.T) {
 	_, err := aset.Add(context.Background(), configMap("test-cm", "default"))
 	assert.NoError(t, err)
 
+	expected := []string{"test-cm"}
+
 	dynamicClient.PrependReactor("patch", "configmaps", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
 		var appliedCM unstructured.Unstructured
 		patchAction := action.(k8stesting.PatchAction)
 		assert.Equal(t, types.ApplyPatchType, patchAction.GetPatchType())
 		err = json.Unmarshal(patchAction.GetPatch(), &appliedCM)
 		assert.NoError(t, err)
 		assert.NotNil(t, appliedCM)
-		assert.Equal(t, "test-cm", appliedCM.GetName())
+		assert.Contains(t, expected, appliedCM.GetName())
 		assert.Contains(t, appliedCM.GetLabels(), ApplysetPartOfLabel)
 		// The fake client needs to return the object that was applied.
 		return true, &appliedCM, nil
@@ -236,6 +239,70 @@ func TestApplySet_Apply(t *testing.T) {
 	assert.NoError(t, result.Errors())
 	assert.Equal(t, 1, result.DesiredCount)
 	assert.Len(t, result.AppliedObjects, 1)
+
+	t.Run("reapply with new object", func(t *testing.T) {
+		_, err := aset.Add(context.Background(), configMap("test-cm-2", "default"))
+		assert.NoError(t, err)
+		expected = append(expected, "test-cm-2")
+
+		result, err := aset.Apply(context.Background(), false)
+		assert.NoError(t, err)
+		assert.NoError(t, result.Errors())
+		assert.Equal(t, 2, result.DesiredCount)
+		assert.Len(t, result.AppliedObjects, 2)
+	})
+
+}
+
+func TestApplySet_UpdatesParentAnnotationsWithMultipleGroupKinds(t *testing.T) {
+	parent := parentObj(secretGVK, "parent-secret")
+
+	aset, dynamicClient := newTestApplySet(t, parent)
+
+	// Add two different GKs
+	_, err := aset.Add(context.Background(), configMap("cm-1", "default"))
+	assert.NoError(t, err)
+	_, err = aset.Add(context.Background(), foo("foo-1", "default"))
+	assert.NoError(t, err)
+
+	// Reactor for both GKs
+	dynamicClient.PrependReactor("patch", "configmaps", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		var obj unstructured.Unstructured
+		_ = json.Unmarshal(action.(k8stesting.PatchAction).GetPatch(), &obj)
+		return true, &obj, nil
+	})
+	dynamicClient.PrependReactor("patch", "foos", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		var obj unstructured.Unstructured
+		_ = json.Unmarshal(action.(k8stesting.PatchAction).GetPatch(), &obj)
+		return true, &obj, nil
+	})
+
+	// Capture and validate the parent update
+	dynamicClient.PrependReactor("patch", "secrets", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		patchAction := action.(k8stesting.PatchAction)
+		assert.Equal(t, types.ApplyPatchType, patchAction.GetPatchType())
+
+		var parentPatch unstructured.Unstructured
+		err := json.Unmarshal(patchAction.GetPatch(), &parentPatch)
+		assert.NoError(t, err)
+
+		ann := parentPatch.GetAnnotations()
+		assert.NotNil(t, ann)
+		assert.Contains(t, ann, ApplySetGKsAnnotation)
+
+		// Expect both GKs (order not guaranteed)
+		gks := ann[ApplySetGKsAnnotation]
+		assert.True(t, strings.Contains(gks, "ConfigMap"))
+		assert.True(t, strings.Contains(gks, "Foo"))
+
+		return true, &parentPatch, nil
+	})
+
+	result, err := aset.Apply(context.Background(), false)
+	assert.NoError(t, err)
+	assert.NoError(t, result.Errors())
+	assert.Equal(t, 2, result.DesiredCount)
+	assert.Len(t, result.AppliedObjects, 2)
 }
 
 func TestApplySet_Prune(t *testing.T) {
@@ -516,7 +583,7 @@ func TestApplySet_PruneOldNamespace(t *testing.T) {
 		assert.Contains(t, parentPatch.GetLabels(), ApplySetParentIDLabel)
 		assert.Contains(t, parentPatch.GetAnnotations(), ApplySetAdditionalNamespacesAnnotation)
 		assert.Contains(t, parentPatch.GetAnnotations(), ApplySetGKsAnnotation)
-		//assert.Equal(t, parentPatch.GetAnnotations()[ApplySetAdditionalNamespacesAnnotation], "ns1,ns2,ns3,oldns1")
+		// assert.Equal(t, parentPatch.GetAnnotations()[ApplySetAdditionalNamespacesAnnotation], "ns1,ns2,ns3,oldns1")
 		assert.Equal(t, parentPatch.GetAnnotations()[ApplySetGKsAnnotation], "ConfigMap,Foo.foo.bar")
 
 		return true, &parentPatch, nil
@@ -587,7 +654,7 @@ func TestApplySet_PruneOldGVKs(t *testing.T) {
 		assert.Contains(t, parentPatch.GetAnnotations(), ApplySetAdditionalNamespacesAnnotation)
 		assert.Contains(t, parentPatch.GetAnnotations(), ApplySetGKsAnnotation)
 		assert.Equal(t, parentPatch.GetAnnotations()[ApplySetAdditionalNamespacesAnnotation], "ns1,ns2")
-		//assert.Equal(t, parentPatch.GetAnnotations()[ApplySetGKsAnnotation], "ConfigMap,Foo.foo.bar")
+		// assert.Equal(t, parentPatch.GetAnnotations()[ApplySetGKsAnnotation], "ConfigMap,Foo.foo.bar")
 
 		return true, &parentPatch, nil
 	})

test/integration/suites/core/annotation_label_test.go

@@ -0,0 +1,161 @@
+// Copyright 2025 The Kubernetes Authors.
+//
+// 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 core_test
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/kubernetes-sigs/kro/pkg/applyset"
+	"github.com/kubernetes-sigs/kro/pkg/metadata"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/rand"
+
+	krov1alpha1 "github.com/kubernetes-sigs/kro/api/v1alpha1"
+	"github.com/kubernetes-sigs/kro/pkg/testutil/generator"
+)
+
+var _ = Describe("Labels and Annotations", func() {
+	var (
+		namespace string
+	)
+
+	BeforeEach(func(ctx SpecContext) {
+		namespace = fmt.Sprintf("test-%s", rand.String(5))
+		// Create namespace
+		Expect(env.Client.Create(ctx, &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: namespace,
+			},
+		})).To(Succeed())
+	})
+
+	AfterEach(func(ctx SpecContext) {
+		Expect(env.Client.Delete(ctx, &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: namespace,
+			},
+		})).To(Succeed())
+	})
+
+	It("should have correct conditions when ResourceGraphDefinition is created", func(ctx SpecContext) {
+		rgd := generator.NewResourceGraphDefinition("test-apply",
+			generator.WithSchema(
+				"TestApply", "v1alpha1",
+				map[string]interface{}{
+					"field1": "string",
+				},
+				nil,
+			),
+			generator.WithResource("res1", map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name": "${schema.spec.field1}",
+				},
+			}, nil, nil),
+		)
+
+		Expect(env.Client.Create(ctx, rgd)).To(Succeed())
+		DeferCleanup(func(ctx SpecContext) {
+			Expect(env.Client.Delete(ctx, rgd)).To(Succeed())
+		})
+
+		Eventually(func(g Gomega, ctx SpecContext) {
+			err := env.Client.Get(ctx, types.NamespacedName{
+				Name: rgd.Name,
+			}, rgd)
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(rgd.Status.State).To(Equal(krov1alpha1.ResourceGraphDefinitionStateActive))
+		}, 10*time.Second, time.Second).WithContext(ctx).Should(Succeed())
+
+		apiVersion, kind := schema.GroupVersionKind{
+			Group:   rgd.Spec.Schema.Group,
+			Version: rgd.Spec.Schema.APIVersion,
+			Kind:    rgd.Spec.Schema.Kind,
+		}.ToAPIVersionAndKind()
+
+		// Create instance
+		instance := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": apiVersion,
+				"kind":       kind,
+				"metadata": map[string]interface{}{
+					"name":      "instance",
+					"namespace": namespace,
+				},
+				"spec": map[string]interface{}{
+					"field1": "foobar",
+				},
+			},
+		}
+		Expect(env.Client.Create(ctx, instance)).To(Succeed())
+		DeferCleanup(func(ctx SpecContext) {
+			Expect(env.Client.Delete(ctx, instance)).To(Succeed())
+		})
+
+		Eventually(func(g Gomega, ctx SpecContext) {
+			err := env.Client.Get(ctx, types.NamespacedName{
+				Name:      instance.GetName(),
+				Namespace: namespace,
+			}, instance)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			g.Expect(instance.GetAnnotations()).To(SatisfyAll(
+				Not(BeNil()),
+				HaveKeyWithValue(applyset.ApplySetGKsAnnotation, "ConfigMap"),
+				HaveKeyWithValue(applyset.ApplySetToolingAnnotation, "kro/devel"),
+				HaveKeyWithValue(applyset.ApplySetAdditionalNamespacesAnnotation, namespace),
+			), "instance should have group kind for owned resource")
+
+			g.Expect(instance.GetLabels()).To(SatisfyAll(
+				Not(BeNil()),
+				HaveKeyWithValue(applyset.ApplySetParentIDLabel, applyset.ID(instance)),
+			), "instance should be assigned as apply set parent")
+
+			g.Expect(instance.GetLabels()).To(SatisfyAll(
+				HaveKeyWithValue(metadata.KROVersionLabel, "devel"),
+				HaveKeyWithValue(metadata.OwnedLabel, "true"),
+				HaveKeyWithValue(metadata.ResourceGraphDefinitionIDLabel, string(rgd.GetUID())),
+				HaveKeyWithValue(metadata.ResourceGraphDefinitionNameLabel, rgd.GetName()),
+			), "default kro labels should also be present")
+
+		}, 10*time.Second, time.Second).WithContext(ctx).Should(Succeed())
+
+		cfgMap := &corev1.ConfigMap{}
+		err := env.Client.Get(ctx, types.NamespacedName{
+			Name:      "foobar",
+			Namespace: namespace,
+		}, cfgMap)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfgMap.GetLabels()).To(SatisfyAll(
+			HaveKeyWithValue(applyset.ApplysetPartOfLabel, applyset.ID(instance)),
+			HaveKeyWithValue(metadata.InstanceNamespaceLabel, instance.GetNamespace()),
+			HaveKeyWithValue(metadata.InstanceLabel, instance.GetName()),
+			HaveKeyWithValue(metadata.InstanceIDLabel, string(instance.GetUID())),
+			HaveKeyWithValue(metadata.KROVersionLabel, "devel"),
+			HaveKeyWithValue(metadata.OwnedLabel, "true"),
+			HaveKeyWithValue(metadata.ResourceGraphDefinitionIDLabel, string(rgd.GetUID())),
+			HaveKeyWithValue(metadata.ResourceGraphDefinitionNameLabel, rgd.GetName()),
+		), "config map should be created as part of apply set managed by instance created through rgd")
+	})
+
+})