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[bot]]

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[bot]]

@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[bot]]

@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[bot]]

@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[bot]]

@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?

[metacoma]

knative-operator 1.14.0 still have this issue how to reproduce:

1. install knative-operator
2. apply namespace-ke.yaml with the following content

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

3. then apply knative-eventing resource

apiVersion: operator.knative.dev/v1beta1
kind: KnativeEventing
metadata:
  name: knative-eventing

and as a result

$ kubectl get ns knative-eventing -o yaml  | grep -i istio  | wc -l
0

[houshengbo]

Knative Operator bundles the manifests of knative serving for each official release. The knative-serving namespace is included in the knative serving release, so operator will apply the yamls, containing the namespace knative-serving.

It will work to use kubectl create ns ... to create the namespace, before installing the the knativeserving CR, but we cannot guarantee that no occurence of kubectl apply ... happens after the namespace was created at the beginning.

I can give an option to configure both labels and annotations for the namespace with the KnativeServing and KnativeEventing CRs. For example:

apiVersion: operator.knative.dev/v1beta1
kind: KnativeEventing
metadata:
  name: knative-eventing
  namespace: knative-eventing
spec:
  version: "1.15"
  namespaces:
    - name: <name-of-namespace>
      labels:
        key1: values1
        key2: value2
      annotations:
        key1: values1
        key2: value2

to add or configure the key-value pairs. How does it sound?

[sunsided]

This is still messing with ArgoCD and Istio:

It also purges automatically added annotations.