Apply labels and annotations only in the correct paths (#1499)

Apply labels and annotations only in the correct paths depending on the GVK

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

pkg/engine/renderer/consts.go

@@ -0,0 +1,66 @@
+package renderer
+
+import (
+	"strings"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+type ResourcePath struct {
+	Group   string
+	Version string
+	Kind    string
+	path    string
+}
+
+func (lp *ResourcePath) pathFields() []string {
+	return strings.Split(lp.path, "/")
+}
+
+func (lp *ResourcePath) matches(gvk schema.GroupVersionKind) bool {
+	if lp.Group != "" && lp.Group != gvk.Group {
+		return false
+	}
+	if lp.Version != "" && lp.Version != gvk.Version {
+		return false
+	}
+	if lp.Kind != "" && lp.Kind != gvk.Kind {
+		return false
+	}
+	return true
+}
+
+var (
+	// CommonLabelPaths is a list of locations in an object type where labels should be added.
+	// Taken from here: https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/commonlabels.go
+	CommonLabelPaths = []ResourcePath{
+		{Group: "", Kind: "", path: "metadata/labels"},
+		{Group: "apps", Kind: "StatefulSet", path: "spec/template/metadata/labels"},
+		{Group: "apps", Kind: "StatefulSet", path: "spec/volumeClaimTemplates[]/metadata/labels"},
+		{Group: "apps", Kind: "Deployment", path: "spec/template/metadata/labels"},
+		{Group: "", Kind: "ReplicaSet", path: "spec/template/metadata/labels"},
+		{Group: "", Kind: "ReplicationController", path: "spec/template/metadata/labels"},
+		{Group: "", Kind: "DaemonSet", path: "spec/template/metadata/labels"},
+		{Group: "batch", Kind: "Job", path: "spec/template/metadata/labels"},
+		{Group: "batch", Kind: "CronJob", path: "spec/template/metadata/labels"},
+		{Group: "batch", Kind: "CronJob", path: "spec/jobTemplate/spec/template/metadata/labels"},
+	}
+
+	// CommonAnnotationPaths is a list of locations for annotations to add in all resources
+	CommonAnnotationPaths = []ResourcePath{
+		{Group: "", Kind: "", path: "metadata/annotations"},
+	}
+
+	// TemplateAnnotationPaths is a list of locations specific to objects where annotations should be added in
+	// templates. Taken from here https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/commonannotations.go
+	TemplateAnnotationPaths = []ResourcePath{
+		{Group: "", Kind: "ReplicationController", path: "spec/template/metadata/annotations"},
+		{Group: "apps", Kind: "StatefulSet", path: "spec/template/metadata/annotations"},
+		{Group: "apps", Kind: "Deployment", path: "spec/template/metadata/annotations"},
+		{Group: "", Kind: "ReplicaSet", path: "spec/template/metadata/annotations"},
+		{Group: "", Kind: "DaemonSet", path: "spec/template/metadata/annotations"},
+		{Group: "batch", Kind: "Job", path: "spec/template/metadata/annotations"},
+		{Group: "batch", Kind: "CronJob", path: "spec/template/metadata/annotations"},
+		{Group: "batch", Kind: "CronJob", path: "spec/jobTemplate/spec/template/metadata/annotations"},
+	}
+)

pkg/engine/renderer/dependencies.go

@@ -227,20 +227,16 @@ func calculateResourceDependencies(obj *unstructured.Unstructured) (resourceDepe
 
 // setTemplateHash adds the given hash in the pod template spec of the obj
 func setTemplateHash(obj *unstructured.Unstructured, hashStr string) error {
-	// Again, we don't know the actual type of the object, so we just add the annotation in
-	// possible paths and rely on the conversion to typed objects later to clear invalid locations
-
-	annotationPaths := [][]string{
-		{"spec", "template", "metadata", "annotations"},
-		{"spec", "jobTemplate", "spec", "template", "metadata", "annotations"},
-	}
-
 	fieldsToAdd := map[string]string{
 		kudo.DependenciesHashAnnotation: hashStr,
 	}
-	for _, path := range annotationPaths {
-		if err := addMapValues(obj.Object, fieldsToAdd, path...); err != nil {
-			return err
+
+	gvk := obj.GroupVersionKind()
+	for _, lp := range TemplateAnnotationPaths {
+		if lp.matches(gvk) {
+			if err := addMapValues(obj.UnstructuredContent(), fieldsToAdd, lp.pathFields()...); err != nil {
+				return err
+			}
 		}
 	}
 

pkg/engine/renderer/dependencies_test.go

@@ -107,6 +107,9 @@ func TestSetDependenciesHash(t *testing.T) {
 			assert.NilError(t, err)
 			assert.DeepEqual(t, pod("somename", "namespace"), p)
 		}},
+		{name: "no change in unstructured CRD", obj: unstructuredCrd("crd", "namespace"), assert: func(us *unstructured.Unstructured) {
+			assert.DeepEqual(t, unstructuredCrd("crd", "namespace"), us)
+		}},
 	}
 	for _, tt := range tests {
 		tt := tt

pkg/engine/renderer/enhancer.go

@@ -47,15 +47,15 @@ func (de *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 				return nil, fmt.Errorf("%wconverting to unstructured failed: %v", engine.ErrFatalExecution, err)
 			}
 
-			if err = addLabels(unstructMap, metadata); err != nil {
+			objUnstructured := &unstructured.Unstructured{Object: unstructMap}
+
+			if err = addLabels(objUnstructured, metadata); err != nil {
 				return nil, fmt.Errorf("%wadding labels on parsed object: %v", engine.ErrFatalExecution, err)
 			}
-			if err = addAnnotations(unstructMap, metadata); err != nil {
+			if err = addAnnotations(objUnstructured, metadata); err != nil {
 				return nil, fmt.Errorf("%wadding annotations on parsed object %s: %v", engine.ErrFatalExecution, obj.GetObjectKind(), err)
 			}
 
-			objUnstructured := &unstructured.Unstructured{Object: unstructMap}
-
 			isNamespaced, err := resource.IsNamespacedObject(obj, de.Discovery)
 			if err != nil {
 				return nil, fmt.Errorf("failed to determine if object %s is namespaced: %v", obj.GetObjectKind(), err)
@@ -72,19 +72,6 @@ func (de *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 				}
 			}
 
-			// We convert to the typed version here and back to clean additional labels and annotations that were
-			// added. This needs to be done as otherwise the dependency calculator calculates hashes based on the added
-			// fields which are later missing if the resource is fetched from the api-server
-			err = runtime.DefaultUnstructuredConverter.FromUnstructured(objUnstructured.UnstructuredContent(), obj)
-			if err != nil {
-				return nil, fmt.Errorf("%wconverting from unstructured failed: %v", engine.ErrFatalExecution, err)
-			}
-			unstructMap, err = runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
-			if err != nil {
-				return nil, fmt.Errorf("%wconverting to unstructured failed: %v", engine.ErrFatalExecution, err)
-			}
-			objUnstructured = &unstructured.Unstructured{Object: unstructMap}
-
 			unstructuredObjs = append(unstructuredObjs, objUnstructured)
 		}
 	}
@@ -95,8 +82,6 @@ func (de *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 
 	// This is pretty important, if we don't convert it to the actual type everything will be Unstructured.
 	// We depend on types later on in the processing e.g. when evaluating health.
-	// Additionally, as we add annotations and labels to all possible paths, this step gets rid of anything
-	// that doesn't belong to the specific object type.
 	objs, err := de.convertToTyped(unstructuredObjs)
 	if err != nil {
 		return nil, fmt.Errorf("failed to convert objects to typed: %v", err)
@@ -141,68 +126,54 @@ func (de *DefaultEnhancer) convertToTyped(unstructuredObjs []*unstructured.Unstr
 	return objs, nil
 }
 
-func addLabels(obj map[string]interface{}, metadata Metadata) error {
-	// List of paths for labels from here:
-	// https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/commonlabels.go
-
-	// Labels are added to top-level resources as well as pod template specs. All labels here do not change
-	// over the livetime of the instance and can not trigger unwanted restarts of the pods
-	labelPaths := [][]string{
-		{"metadata", "labels"},
-		{"spec", "template", "metadata", "labels"},
-		{"spec", "volumeClaimTemplates[]", "metadata", "labels"},
-		{"spec", "jobTemplate", "metadata", "labels"},
-		{"spec", "jobTemplate", "spec", "template", "metadata", "labels"},
-	}
-
+func addLabels(obj *unstructured.Unstructured, metadata Metadata) error {
 	fieldsToAdd := map[string]string{
 		kudo.HeritageLabel: "kudo",
 		kudo.OperatorLabel: metadata.OperatorName,
 		kudo.InstanceLabel: metadata.InstanceName,
 	}
 
-	for _, path := range labelPaths {
-		if err := addMapValues(obj, fieldsToAdd, path...); err != nil {
-			return err
+	gvk := obj.GroupVersionKind()
+	for _, lp := range CommonLabelPaths {
+		if lp.matches(gvk) {
+			if err := addMapValues(obj.UnstructuredContent(), fieldsToAdd, lp.pathFields()...); err != nil {
+				return err
+			}
 		}
 	}
 
 	return nil
 }
 
-func addAnnotations(obj map[string]interface{}, metadata Metadata) error {
-	// List of paths for annotations from here:
-	// https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/commonannotations.go
-
-	// For all pod template specs, we only add the operator version. It is pretty stable
+func addAnnotations(obj *unstructured.Unstructured, metadata Metadata) error {
+	// For all pod template specs, we only add the operator version annotation. It is pretty stable
 	// and shouldn't change often, therefore not trigger an unwanted restart of the created pod
-	annotationPaths := [][]string{
-		{"metadata", "annotations"},
-		{"spec", "template", "metadata", "annotations"},
-		{"spec", "jobTemplate", "metadata", "annotations"},
-		{"spec", "jobTemplate", "spec", "template", "metadata", "annotations"},
-	}
-
 	fieldsToAdd := map[string]string{
 		kudo.OperatorVersionAnnotation: metadata.OperatorVersion,
 	}
-
-	for _, path := range annotationPaths {
-		if err := addMapValues(obj, fieldsToAdd, path...); err != nil {
-			return err
+	gvk := obj.GroupVersionKind()
+	for _, lp := range TemplateAnnotationPaths {
+		if lp.matches(gvk) {
+			if err := addMapValues(obj.UnstructuredContent(), fieldsToAdd, lp.pathFields()...); err != nil {
+				return err
+			}
 		}
 	}
 
-	// The plan, phase and step annotations are only added to the top level resources, not and pod template specs, as
+	// The plan, phase and step annotations are only added to the top level resources, not any pod template specs, as
 	// that may lead to unwanted restarts of the pods
 	topLevelFieldsToAdd := map[string]string{
 		kudo.PlanAnnotation:    metadata.PlanName,
 		kudo.PhaseAnnotation:   metadata.PhaseName,
 		kudo.StepAnnotation:    metadata.StepName,
 		kudo.PlanUIDAnnotation: string(metadata.PlanUID),
 	}
-	if err := addMapValues(obj, topLevelFieldsToAdd, "metadata", "annotations"); err != nil {
-		return err
+	for _, lp := range CommonAnnotationPaths {
+		if lp.matches(gvk) {
+			if err := addMapValues(obj.UnstructuredContent(), topLevelFieldsToAdd, lp.pathFields()...); err != nil {
+				return err
+			}
+		}
 	}
 
 	return nil

pkg/engine/renderer/enhancer_test.go

@@ -112,13 +112,21 @@ func TestEnhancerApply_noAdditionalMetadata(t *testing.T) {
 
 	tpls := map[string]string{
 		"pod": resourceAsString(pod("pod", "default")),
+		"crd": resourceAsString(unstructuredCrd("crd", "default")),
 	}
 
 	meta := metadata()
 
+	crdType := &metav1.APIResourceList{
+		GroupVersion: "install.istio.io/v1alpha1",
+		APIResources: []metav1.APIResource{
+			{Name: "istiooperator", Namespaced: true, Kind: "IstioOperator"},
+		},
+	}
+
 	e := &DefaultEnhancer{
 		Scheme:    utils.Scheme(),
-		Discovery: fake.CachedDiscoveryClient(),
+		Discovery: fake.CustomCachedDiscoveryClient(crdType),
 	}
 
 	objs, err := e.Apply(tpls, meta)
@@ -132,10 +140,15 @@ func TestEnhancerApply_noAdditionalMetadata(t *testing.T) {
 			t.Errorf("failed to parse object to unstructured: %s", err)
 		}
 
-		f, ok, _ := unstructured.NestedFieldNoCopy(unstructMap, "spec", "template")
+		// Make sure the top-level metadata is there
+		uo := unstructured.Unstructured{Object: unstructMap}
+		assert.Equal(t, string(meta.PlanUID), uo.GetAnnotations()[kudo.PlanUIDAnnotation])
+		assert.Equal(t, "kudo", uo.GetLabels()[kudo.HeritageLabel])
 
+		// But no nested fields
+		f, ok, _ := unstructured.NestedFieldNoCopy(unstructMap, "spec", "template")
 		assert.Nil(t, f)
-		assert.False(t, ok, "Pod struct contains template field")
+		assert.False(t, ok, "%s struct contains template field", o.GetObjectKind())
 	}
 }
 func TestEnhancerApply_dependencyHash_noDependencies(t *testing.T) {
@@ -271,7 +284,7 @@ func owner() *corev1.Pod {
 	}
 }
 
-func resourceAsString(resource metav1.Object) string {
+func resourceAsString(resource runtime.Object) string {
 	bytes, _ := yaml.Marshal(resource)
 	return string(bytes)
 }
@@ -394,3 +407,17 @@ func pod(name string, namespace string) *corev1.Pod {
 	}
 	return pod
 }
+
+func unstructuredCrd(name string, namespace string) runtime.Object {
+	data := `apiVersion: install.istio.io/v1alpha1
+kind: IstioOperator
+metadata:
+  namespace: ` + namespace + `
+  name: ` + name + `
+spec:
+  profile: default`
+
+	parsed, _ := YamlToObject(data)
+
+	return parsed[0]
+}