Apply metadata the same way as Kustomize did

[ANeumann82]

This fix applies metadata in the enhancer recursivly.

An Alternative would be to put Kustomize back in, or handling any embedded resources manually, i.e. code for StatefulSets, Deployments, etc.

Fixes #1302 #1303

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

[zmalik]

I was working on this issue also, as we talked in yesterday sync
these two lines worked for me and I was working on tests and error handling

unstructured.SetNestedStringMap(objUnstructured.Object, annotations, "spec", "template", "metadata", "annotations")
unstructured.SetNestedStringMap(objUnstructured.Object, labels, "spec", "template", "metadata", "labels")

[ANeumann82]

Yeah, that would work as well for StatefulSets and Deployments.

The recursively apply is more in Line with what Kustomize did, although your approach is less invasive. I really don't know which one is better.
The Problem with "SetNestedStringMap" is that it creates the path to the nested fields, if it does not exist. So for example in a pod.yaml it would create the "spec.template.metadata.labels" path, which might be an issue.
We could wrap that code in "NestedFieldNoCopy", which would allow us to check if the "spec.template.metadata" path exists before...

[zmalik]

SetNestedStringMap is calling to setNestedFieldNoCopy
I tested it for pod already and it works as expected.

[ANeumann82]

Yeah, but the code I see in setNestedFieldNoCopy creates missing objects in the tree, which is something we don't want.

[zmalik]

ok, I understand what you mean here. But I don't see why we would need to take special care of that when unmarshalling the map back to the object would ignore those fields

[ANeumann82]

Ok, I understand it now, at the end we convert it back from unstructured to the actual object type, which gets rid of any extra attributes.

Leaves the point of any other embedded resources. I don't know if there are any apart from StatefulSets and Deployments, to be honest.

[zmalik]

yes, correct. To see exactly what kustomize was doing-->
https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/commonlabels.go
https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/commonannotations.go

you can see they were adding far more labels then annotations, one example is spec/volumeClaimTemplates[]/metadata/labels with this current solution we will have both annotations and labels there. While with kustomize it would be just labels

[ANeumann82]

Yeah... I'm testing your approach now, seems recursively adding the labels/annotations fails the e2e test, something complains that only certain changes are allowed...

[nfnt]

2020-01-24T14:06:29.661746206Z stderr F 2020/01/24 14:06:29 PlanExecution: A transient error when executing task deploy.main.everything.app. Will retry. Deployment.apps "nginx-deployment" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"heritage":"kudo", "kudo.dev/instance":"first-operator", "kudo.dev/operator":"first-operator"}: `selector` does not match template `labels`

Looks like it's overwriting the existing labels which were app: nginx.

[zmalik]

@ANeumann82 we would be missing annotations for spec/jobTemplate/spec/template and spec/jobTemplate/spec/template/metadata/annotations for CronJob objects if somebody uses them with KUDO

and labels from more places like spec/volumeClaimTemplates[]/metadata/labels also
which KUDO 0.9.0 was doing with the help of kustomize

[ANeumann82]

@zmalik I've added the missing paths and added code to handle the slice parts of paths for volumeClaimTemplates. Added some more unit tests as well.

[kensipe]

this adds needed functionality and I have lots of comments / questions but nothing to stop the merge.

I would appreciate:

1. some restructuring / reordering
2. naming that better communicates intent

Future work should include "discovery" of labels as opposed to static paths

[kensipe]

wouldn't it be better to "discover" the label locations? this static approach works with existing kube objects... We could "discover" the labels (granted it is by naming convention).

 k explain job --recursive | grep -A 10 labels
      labels	<map[string]string>
      managedFields	<[]Object>
         apiVersion	<string>
         fieldsType	<string>
         fieldsV1	<map[string]>
         manager	<string>
         operation	<string>
         time	<string>
      name	<string>
      namespace	<string>
      ownerReferences	<[]Object>
--
            labels	<map[string]string>
            managedFields	<[]Object>
               apiVersion	<string>
               fieldsType	<string>
               fieldsV1	<map[string]>
               manager	<string>
               operation	<string>
               time	<string>
            name	<string>
            namespace	<string>
            ownerReferences	<[]Object>

[ANeumann82]

My first approach was just to walk the yaml tree recursively, looking for metadata and applying the labels/annotations there. The problem with that is, that there's a lot of metadata that can't be changed once the resource is created. I think that's why Kustomize has a list of static places as well.

[kensipe]

same comment as above for labels... we should "discover" annotations

k explain job --recursive | grep -A 10 annotat
      annotations	<map[string]string>
      clusterName	<string>
      creationTimestamp	<string>
      deletionGracePeriodSeconds	<integer>
      deletionTimestamp	<string>
      finalizers	<[]string>
      generateName	<string>
      generation	<integer>
      labels	<map[string]string>
      managedFields	<[]Object>
         apiVersion	<string>
--
            annotations	<map[string]string>
            clusterName	<string>
            creationTimestamp	<string>
            deletionGracePeriodSeconds	<integer>
            deletionTimestamp	<string>
            finalizers	<[]string>
            generateName	<string>
            generation	<integer>
            labels	<map[string]string>
            managedFields	<[]Object>
               apiVersion	<string>