Knative Installation Wipes Namespace Labels (Should Merge)

[awgneo]

Describe the bug
When installing Knative (serving) into a fresh namespace with labels defined, they are wiped out with the Knative release version number when the labels should be merged.

apiVersion: v1
kind: Namespace
metadata:
 name: knative-serving
 labels:
  istio-injection: enabled

becomes

apiVersion: v1
kind: Namespace
metadata:
 name: knative-serving
 labels:
  serving.knative.dev/release=v0.18.0

Expected behavior
The labels of the namespace should be merged.

apiVersion: v1
kind: Namespace
metadata:
 name: knative-serving
 labels:
  istio-injection: enabled
  serving.knative.dev/release=v0.18.0

To Reproduce
Create a namespace with labels and install a Knative operator instance.

Knative release version
v0.18.0 - v0.19.1

Additional context
None

[houshengbo]

@jcrossley3 Could you take a look?

[jcrossley3]

@awgneo how was the original Namespace created? In particular, did it have a last-applied-configuration annotation? If your label was in that annotation, it will be deleted per https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#merging-changes-to-map-fields

[alxndr42]

The namespace was created with kubectl apply. Is using kubectl create a safe workaround for this problem?

I also wonder if the Operator could add its label in a less destructive way, even if kubectl apply was used.

[houshengbo]

Knative operator supports customized manifests, which means you can configure your knative yamls into ANYTHING, fitting your platform.
In the 0.18 and 0.19, we only support a complete replacement of all yamls with the field spec.manifests, but in the coming new release, we are able to support customized yamls appending to the existing manifests by the field spec.additionalManifests.
You can leverage this coming feature for your need to customize your resource.

[houshengbo]

I used the manifestival lib to try creating the namespace knative-serving first with the label istio-injection":"enabled and the annotation knative.dev/example-checksum:1d830d9e. Then updating the namespace without the label istio-injection: enabled.

Here is the output when merging:

The unstructured.Unstructured to be applied is

&{map[apiVersion:v1 kind:Namespace metadata:map[labels:map[serving.knative.dev/release:v0.19.0] name:knative-serving]]}

The current unstructured.Unstructured in the cluster is

&{map[apiVersion:v1 kind:Namespace metadata:map[annotations:map[knative.dev/example-checksum:1d830d9e kubectl.kubernetes.io/last-applied-configuration:{"apiVersion":"v1","kind":"Namespace","metadata":{"annotations":{"knative.dev/example-checksum":"1d830d9e"},"labels":{"istio-injection":"enabled","serving.knative.dev/release":"v0.19.0"},"name":"knative-serving"}}

The original unstructured.Unstructured in last-applied-configuration is:

{"apiVersion":"v1","kind":"Namespace","metadata":{"annotations":{"knative.dev/example-checksum":"1d830d9e"},"labels":{"istio-injection":"enabled","serving.knative.dev/release":"v0.19.0"},"name":"knative-serving"}

After the update, the namespace is

apiVersion: v1
kind: Namespace
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Namespace","metadata":{"labels":{"serving.knative.dev/release":"v0.19.0"},"name":"knative-serving"}}
  creationTimestamp: "2020-12-16T21:16:13Z"
  labels:
    serving.knative.dev/release: v0.19.0

The existing labels and annotations are gone.

[alxndr42]

We're using Knative Operator 0.18, so I tested creating the namespace with kubectl create and the istio-injection label, then installing Knative Serving. This did not cause the istio-injection label to disappear, so it seems like a viable workaround.

(I also took a quick look at spec.manifests, but it doesn't seem to support file: URLs (unsupported protocol scheme \"file\"), which would be convenient. Unless I'm misunderstanding what this attribute does.)

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[maxbog]

/reopen

I'm having trouble understanding how this is the intended behavior.
This is the basic scenario when installing knative using kubectl apply.

IMO, the workaround with kubectl create is not a viable one, because it changes the approach of defining namspace from declarative to imperative.

Given the scenario from the bug - namespace definition with a label: how do I make sure that the operator does not remove the label set by kubectl apply?

[knative-prow]

@maxbog: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

I'm having trouble understanding how this is the intended behavior.
This is the basic scenario when installing knative using kubectl apply.

IMO, the workaround with kubectl create is not a viable one, because it changes the approach of defining namspace from declarative to imperative.

Given the scenario from the bug - namespace definition with a label: how do I make sure that the operator does not remove the label set by kubectl apply?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[eloyekunle]

/reopen

This issue is still not resolved.

[knative-prow]

@eloyekunle: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

This issue is still not resolved.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sugarman402]

/reopen

[knative-prow]

@sugarman402: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sugarman402]

/remove-lifecycle stale

[sugarman402]

The issue is still there, it would be nice to include some annotation which we can add to the namespace and in result the operator-webhook won't touch the existing labels on the namespace.

[pierDipi]

/reopen
/remove-lifecycle stale
/triage accepted

[knative-prow]

@pierDipi: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle stale
/triage accepted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dprotaso]

@houshengbo if the operator is installing stuff into a namespace should it remove that namespace from the manifests? Thus not overwritting it?