From a24cdf01ddbad06416ed728b203e8a22cdc4dba0 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Thu, 18 May 2023 16:04:25 +0100 Subject: [PATCH 01/17] feat: kubernetes Server Side Apply Proposal This purposely does not go into the implementation details, but as we understand it should be a relatively simple modification in place of where the current code sends the "apply" request to Kubernetes API Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 052-k8s-server-side-apply.md diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md new file mode 100644 index 00000000..623befec --- /dev/null +++ b/052-k8s-server-side-apply.md @@ -0,0 +1,35 @@ + + +# Kubernetes Server Side Apply + +Make the Topic Operator use [Kubernetes Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply). This will enable third parties (e.g. Kyverno, Kopf) to have their own labels/annotations on Strimzi owned Kubernetes Resources without Strimzi attempting to remove them. + +NOTE: Strimzi will still have ownership over the fields it wants. + +## Current situation + +As per [Issue 6938](https://github.com/strimzi/strimzi-kafka-operator/issues/6938) third party Kubernetes operators are having a negative interaction with the existing way Strimzi do Kubernetes applies to it's resources. +An example is [Kyverno](https://kyverno.io/) has a policy in a K8s cluster to add a label to all resources with a MutatingWebhookConfiguration, Strimzi detects this label as a resource diff and removes it as it doesn't own it / not included in [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/resource/StatefulSetDiff.java#LL28C34-L28C49). This triggers the MutatingWebhookConfiguration which re-applies the label and the loop continues. + +## Motivation + +Once this change is made (which is relatively simple to implement), it will allow third party tools within the cluster to interact with Strimzi owned resources without looping and Strimzi will be operating as if it never knew about them. + +## Proposal + +Provide an introduction to the proposal. Use sub sections to call out considerations, possible delivery mechanisms etc. +This proposal makes Kubernetes keep track of each field's "owner" and any changes submitted to those fields will be compared to the remote state before being applied. + +This would let the Kubernetes apiserver handle the three-way diff / updates etc, and allow other controllers / webhooks to touch fields / labels / annotations the operator doesn't care about. + +### Three-way diff + +The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of which fields the client previously applied, and indeed all clients (B) is stored in the `.metadata.managedFields`. And the incoming object (C) is supplied by the client. Kubernetes knows which fields were previously set by the client (in the case that the new object no longer specifies some, and they can be removed), and it knows about which client set all the other fields, and will keep them around instead of wiping out all the other labels etc. + +## Compatibility + +This proposal aims to be feature-flagged as not to affect existing use of Strimzi, but also allow people to "upgrade" at their own / controlled pace. + +## Rejected alternatives + +We considered adding a new ENV VAR which could take a list of Kubernetes resource paths to be ignored (or labels/annotations directly), but Kubernetes Server Side Apply is designed to do this job and considering it's within Kubernetes it seems wise to take the "out-of-the-box" option. From ae5434fa63ea6cfa2db33f446fafa39276e7c0e5 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Fri, 19 May 2023 14:12:24 +0100 Subject: [PATCH 02/17] modifications to readme, including pointers to code and responding to comments Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 623befec..3997e1c7 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -2,13 +2,13 @@ # Kubernetes Server Side Apply -Make the Topic Operator use [Kubernetes Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply). This will enable third parties (e.g. Kyverno, Kopf) to have their own labels/annotations on Strimzi owned Kubernetes Resources without Strimzi attempting to remove them. +Make the Operators use [Kubernetes Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply). This will enable third parties (e.g. Kyverno, Kopf) to have their own labels/annotations on Strimzi owned Kubernetes Resources without Strimzi attempting to remove them. NOTE: Strimzi will still have ownership over the fields it wants. ## Current situation -As per [Issue 6938](https://github.com/strimzi/strimzi-kafka-operator/issues/6938) third party Kubernetes operators are having a negative interaction with the existing way Strimzi do Kubernetes applies to it's resources. +As per [Issue 6938](https://github.com/strimzi/strimzi-kafka-operator/issues/6938) third party Kubernetes operators are having a negative interaction with the existing way Strimzi does Kubernetes applies. An example is [Kyverno](https://kyverno.io/) has a policy in a K8s cluster to add a label to all resources with a MutatingWebhookConfiguration, Strimzi detects this label as a resource diff and removes it as it doesn't own it / not included in [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/resource/StatefulSetDiff.java#LL28C34-L28C49). This triggers the MutatingWebhookConfiguration which re-applies the label and the loop continues. ## Motivation @@ -17,10 +17,37 @@ Once this change is made (which is relatively simple to implement), it will allo ## Proposal -Provide an introduction to the proposal. Use sub sections to call out considerations, possible delivery mechanisms etc. This proposal makes Kubernetes keep track of each field's "owner" and any changes submitted to those fields will be compared to the remote state before being applied. -This would let the Kubernetes apiserver handle the three-way diff / updates etc, and allow other controllers / webhooks to touch fields / labels / annotations the operator doesn't care about. +This would let the Kubernetes apiserver handle the three-way diff, updates etc, and allow other controllers, webhooks etc to touch fields, labels, annotations etc the operator doesn't care about. + +It appears as simple as modifying [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). +From: + +``` +....patch(PatchContext.of(PatchType.JSON)... +``` + +To: + +``` +....patch(PatchContext.of(PatchType.SERVER_SIDE_APPLY)... +``` + +According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). + +NOTE: The version of fabric8io being used is [6.5.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). + +It is likely that other parts of the codebase will need modifying in terms of label/annotation discovery so that they only check labels/annotations that are owned by themselves. +Unfortunately, as the person raising this proposal I am unaware of Java and precisely how the codebase works and seek input from others. + +Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply), it is open to discussion the method that Strimzi would use for this, if _force_ is chosen then it is implemented slightly differently, [like so](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply:~:text=If%20the%20resources,true).build()%2C%20service)%3B): + +``` +....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), service); +``` + +I suggest using the above _force_ method to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_, this obviously means if another party is _forcing_ the same field that they will create a loop of ownership but this would be rare as operators should only care about their own fields. ### Three-way diff From 05f5ddf159a96b698872aea91716e7245b3ad52d Mon Sep 17 00:00:00 2001 From: James Fuller Date: Fri, 19 May 2023 15:53:06 +0100 Subject: [PATCH 03/17] adding create function proposal and modifying reasoning Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 3997e1c7..7af0bcbd 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -21,33 +21,44 @@ This proposal makes Kubernetes keep track of each field's "owner" and any change This would let the Kubernetes apiserver handle the three-way diff, updates etc, and allow other controllers, webhooks etc to touch fields, labels, annotations etc the operator doesn't care about. +Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply), it is open to choose the method that Strimzi would use for this. + +[Kubernetes suggest](https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts:~:text=It%20is%20strongly%20recommended%20for%20controllers%20to%20always%20%22force%22%20conflicts%2C%20since%20they%20might%20not%20be%20able%20to%20resolve%20or%20act%20on%20these%20conflicts.) using the above _force_ method to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_, this obviously means if another party is _forcing_ the same field that they will create a loop of ownership but this would be rare as operators should only care about their own fields. + It appears as simple as modifying [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). From: ``` -....patch(PatchContext.of(PatchType.JSON)... +....patch(PatchContext.of(PatchType.JSON), desired); ``` To: ``` -....patch(PatchContext.of(PatchType.SERVER_SIDE_APPLY)... +....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); ``` -According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). +client.services().withName("name").patch(service); -NOTE: The version of fabric8io being used is [6.5.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). +And Kubernetes recommend it is used on CREATE also: +[From](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#L272): -It is likely that other parts of the codebase will need modifying in terms of label/annotation discovery so that they only check labels/annotations that are owned by themselves. -Unfortunately, as the person raising this proposal I am unaware of Java and precisely how the codebase works and seek input from others. +``` +ReconcileResult result = ReconcileResult.created(operation().inNamespace(namespace).resource(desired).create()); +``` -Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply), it is open to discussion the method that Strimzi would use for this, if _force_ is chosen then it is implemented slightly differently, [like so](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply:~:text=If%20the%20resources,true).build()%2C%20service)%3B): +To: ``` -....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), service); +ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); ``` -I suggest using the above _force_ method to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_, this obviously means if another party is _forcing_ the same field that they will create a loop of ownership but this would be rare as operators should only care about their own fields. +According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). + +NOTE: The version of fabric8io being used is [6.5.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). + +It is likely that other parts of the codebase will need modifying in terms of label/annotation discovery so that they only check labels/annotations that are owned by themselves. +Unfortunately, as the person raising this proposal I am unaware of Java and precisely how the codebase works and seek input from others. ### Three-way diff From 893d86c2d432a527e404a30991998055ffacb5f9 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Fri, 19 May 2023 16:22:01 +0100 Subject: [PATCH 04/17] adding in part about diffs and desired states Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 7af0bcbd..16496d9d 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -58,7 +58,11 @@ According to the [package being used](https://github.com/fabric8io/kubernetes-cl NOTE: The version of fabric8io being used is [6.5.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). It is likely that other parts of the codebase will need modifying in terms of label/annotation discovery so that they only check labels/annotations that are owned by themselves. -Unfortunately, as the person raising this proposal I am unaware of Java and precisely how the codebase works and seek input from others. + +The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only the parts of the resource that Strimzi owns as to not trigger a reconcile loop for externally owned resource paths. +This will allow the removal of `ignorableFields` throughout the codebase as paths are being selected now, rather than filtering out fields. + +It appears [this is a place](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ServiceOperator.java#L123) that the `desired` state imports existing external annotations, this would need to change to not include existing external annotations within the `desired` state as then Kubernetes would create an ownership conflict with Server Side Apply. ### Three-way diff From 11b0bc72c9cb0d3f1444342f09381ba65c3f80a0 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Tue, 13 Jun 2023 11:21:19 +0100 Subject: [PATCH 05/17] chore(docs): update proposal to comments Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 16496d9d..85537fa2 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -6,11 +6,19 @@ Make the Operators use [Kubernetes Server Side Apply](https://kubernetes.io/docs NOTE: Strimzi will still have ownership over the fields it wants. -## Current situation +## Current situation / cause of proposal As per [Issue 6938](https://github.com/strimzi/strimzi-kafka-operator/issues/6938) third party Kubernetes operators are having a negative interaction with the existing way Strimzi does Kubernetes applies. An example is [Kyverno](https://kyverno.io/) has a policy in a K8s cluster to add a label to all resources with a MutatingWebhookConfiguration, Strimzi detects this label as a resource diff and removes it as it doesn't own it / not included in [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/resource/StatefulSetDiff.java#LL28C34-L28C49). This triggers the MutatingWebhookConfiguration which re-applies the label and the loop continues. +The precise log we saw was: + +``` +2023-03-03 14:25:52 DEBUG ResourceDiff:38 - Reconciliation #178(timer) Kafka(kafka-backup-spike/kafka-backup-spike): StatefulSet kafka-backup-spike-zookeeper differs: {"op":"replace","path":"/metadata/annotations/policies.kyverno.io~1last-applied-patches","value":""} +``` + +This shows a diff found in the annotations of a `StatefulSet` (this was before StrimziPodSets) which generates the behaviour desribed above. + ## Motivation Once this change is made (which is relatively simple to implement), it will allow third party tools within the cluster to interact with Strimzi owned resources without looping and Strimzi will be operating as if it never knew about them. @@ -34,11 +42,13 @@ From: To: + ``` ....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); ``` -client.services().withName("name").patch(service); +Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), this would need updating too. +It would also need confirming that all resource creates/updates go through `serverSideApply`, not something that should be done in this proposal. And Kubernetes recommend it is used on CREATE also: [From](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#L272): @@ -55,15 +65,22 @@ ReconcileResult result = ReconcileResult.patched(operation().inNamespace(name According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). -NOTE: The version of fabric8io being used is [6.5.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). +NOTE: The version of fabric8io being used is [6.6.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). -It is likely that other parts of the codebase will need modifying in terms of label/annotation discovery so that they only check labels/annotations that are owned by themselves. +The operators will need modifying in terms of change discovery so that they only check fields that are owned by themselves, `metadata.managedFields`. The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only the parts of the resource that Strimzi owns as to not trigger a reconcile loop for externally owned resource paths. This will allow the removal of `ignorableFields` throughout the codebase as paths are being selected now, rather than filtering out fields. It appears [this is a place](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ServiceOperator.java#L123) that the `desired` state imports existing external annotations, this would need to change to not include existing external annotations within the `desired` state as then Kubernetes would create an ownership conflict with Server Side Apply. + +### Modifications to existing behaviours + +The only change to existing (non-Strimzi specific) behaviours is that Strimzi won't care about other fields it chooses not to care about. This does mean that another entity within the cluster can modify things without Strimzi's knowledge, but it shouldn't matter if the `metadata.managedFields` are chosen correctly. + +I do not know the codebase for the strimzi kafka operator so cannot comment on specific behaviours that might change, as this is just a proposal I suspect that is better placed in the eventual PR if this proposal gets accepted. + ### Three-way diff The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of which fields the client previously applied, and indeed all clients (B) is stored in the `.metadata.managedFields`. And the incoming object (C) is supplied by the client. Kubernetes knows which fields were previously set by the client (in the case that the new object no longer specifies some, and they can be removed), and it knows about which client set all the other fields, and will keep them around instead of wiping out all the other labels etc. @@ -71,6 +88,8 @@ The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of ## Compatibility This proposal aims to be feature-flagged as not to affect existing use of Strimzi, but also allow people to "upgrade" at their own / controlled pace. +A proposed name is `useServerSideApply`. +It should follow the same protocol as other feature gates, such as [`UseStrimziPodSets`](https://github.com/strimzi/strimzi-kafka-operator/blob/bcecad71b3676194ec7e17f245aef8d23556908a/CHANGELOG.md) (which was deployed in `0.28.0`, moved to beta in `0.30.0` and graduated in `0.35.0`). ## Rejected alternatives From 0c56f5c73e30b3dcf1f0a83f6247216d3efcde86 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Thu, 22 Jun 2023 15:43:00 +0100 Subject: [PATCH 06/17] updating server side apply proposal to comments Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 53 +++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 85537fa2..fbbcb64a 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -1,5 +1,3 @@ - - # Kubernetes Server Side Apply Make the Operators use [Kubernetes Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply). This will enable third parties (e.g. Kyverno, Kopf) to have their own labels/annotations on Strimzi owned Kubernetes Resources without Strimzi attempting to remove them. @@ -9,7 +7,9 @@ NOTE: Strimzi will still have ownership over the fields it wants. ## Current situation / cause of proposal As per [Issue 6938](https://github.com/strimzi/strimzi-kafka-operator/issues/6938) third party Kubernetes operators are having a negative interaction with the existing way Strimzi does Kubernetes applies. -An example is [Kyverno](https://kyverno.io/) has a policy in a K8s cluster to add a label to all resources with a MutatingWebhookConfiguration, Strimzi detects this label as a resource diff and removes it as it doesn't own it / not included in [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/resource/StatefulSetDiff.java#LL28C34-L28C49). This triggers the MutatingWebhookConfiguration which re-applies the label and the loop continues. +An example is [Kyverno](https://kyverno.io/) has a policy in a K8s cluster to add an annotation (`policies.kyverno.io/last-applied-patches:...`) to all resources with a MutatingWebhookConfiguration, Strimzi detects this label as a resource diff and removes it as it doesn't own it / not included in [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L26). This triggers the MutatingWebhookConfiguration which re-applies the label and the loop continues. + +Another use case could be a Kyverno policy that adds AWS IRSA annotations to a Service Account (specifically useful for a Kafka Sink/Source Connector), this would allow the annotation to be owned by Kyverno and therefore leave it in place without constantly trying to remove it. The precise log we saw was: @@ -21,7 +21,7 @@ This shows a diff found in the annotations of a `StatefulSet` (this was before S ## Motivation -Once this change is made (which is relatively simple to implement), it will allow third party tools within the cluster to interact with Strimzi owned resources without looping and Strimzi will be operating as if it never knew about them. +Once this change is made it will allow third party tools within the cluster to interact with Strimzi owned resources without looping and Strimzi will be operating as if it never knew about them. ## Proposal @@ -33,6 +33,15 @@ Conflicts can happen with Server Side Apply as described [here](https://github.c [Kubernetes suggest](https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts:~:text=It%20is%20strongly%20recommended%20for%20controllers%20to%20always%20%22force%22%20conflicts%2C%20since%20they%20might%20not%20be%20able%20to%20resolve%20or%20act%20on%20these%20conflicts.) using the above _force_ method to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_, this obviously means if another party is _forcing_ the same field that they will create a loop of ownership but this would be rare as operators should only care about their own fields. +[Kubernetes recommend](https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply:~:text=the%20object%20doesn%27t%20have%20to%20be%20read%20beforehand) omitting the `GET` call from the Kubernetes API, given that we only want to send the API our desired fields and leave it up to the API to solve. +The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per reconciliation as opposed to more. + +Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. If this occurs and is not a desired behaviour then we can stick to the Read -> edit -> apply method instead for those examples with a justification. + +There is more documentation within the [Merge Strategy][https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy] documentation in which it defines how we can change CRDs to merge how we want them to (StrimziPodSet, for example). For reference, there are two main kinds: `atomic` and `map`/`set`/`granular`; with `atomic` set, any change replaces the whole field/list and is recursive downwards; with the others the behaviour is more fine-tuned and is described on the above link in a table. + +### Code changes + It appears as simple as modifying [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). From: @@ -47,10 +56,13 @@ To: ....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); ``` +It has been suggested that the first pass of a Server-Side Apply would keep `force = false` and if a conflict were to arise then set `force = true`, this provides better visibility as to when these events may occur. +This seems like a logical approach and can be included in the proposal. + Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), this would need updating too. It would also need confirming that all resource creates/updates go through `serverSideApply`, not something that should be done in this proposal. -And Kubernetes recommend it is used on CREATE also: +And [Kubernetes](https://kubernetes.io/docs/reference/using-api/server-side-apply/#:~:text=through%20declarative%20configurations.-,Clients%20can%20create,-and%20modify%20their)/[Fabric8io](https://github.com/fabric8io/kubernetes-client/blob/master/doc/CHEATSHEET.md#server-side-apply:~:text=For%20any%20create%20or%20update) recommend it is used on CREATE also: [From](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#L272): ``` @@ -67,20 +79,41 @@ According to the [package being used](https://github.com/fabric8io/kubernetes-cl NOTE: The version of fabric8io being used is [6.6.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). -The operators will need modifying in terms of change discovery so that they only check fields that are owned by themselves, `metadata.managedFields`. - -The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only the parts of the resource that Strimzi owns as to not trigger a reconcile loop for externally owned resource paths. -This will allow the removal of `ignorableFields` throughout the codebase as paths are being selected now, rather than filtering out fields. +The operators will need modifying: +* The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only `metadata.managedFields` (with `strimzi-cluster-operator` as the manager) to compute whether a change is needed as to not trigger a reconcile loop for externally owned resource paths. +* This will allow the removal of `ignorableFields` throughout the codebase as paths are being selected now, rather than filtering out fields. It appears [this is a place](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ServiceOperator.java#L123) that the `desired` state imports existing external annotations, this would need to change to not include existing external annotations within the `desired` state as then Kubernetes would create an ownership conflict with Server Side Apply. +| current value of field | desired value of field | field owner | requires update? | change in behaviour | +| -------- | -------- | ------ | ---------------- | ------------------- | +| x | x | Strimzi | no | no | +| x | y | Strimzi | yes | no | +| x | not set | Strimzi | yes | no | +| x | not set | Not Strimzi | no | yes | +| not set | y | Strimzi | yes | no | +| not set | y | Not Strimzi | yes | no | + +The key thing to take from this table is that currently Strimzi is trying to make changes to ALL fields (minus short `ignorableFields` list), once Strimzi has knowledge of the fields it owns - we don't want it to try and change the fields it doesn't own. + +It would be wise to try and include this in as many operators as possible, but in some situations where it is decided against it can be overridden in the code and left as is. + ### Modifications to existing behaviours -The only change to existing (non-Strimzi specific) behaviours is that Strimzi won't care about other fields it chooses not to care about. This does mean that another entity within the cluster can modify things without Strimzi's knowledge, but it shouldn't matter if the `metadata.managedFields` are chosen correctly. +The only change to existing (non-Strimzi specific) behaviours is that `ResourceDiff` won't care about other fields it chooses not to care about. This does mean that another entity within the cluster can modify things without Strimzi's knowledge, but it shouldn't matter if the `metadata.managedFields` are chosen correctly. I do not know the codebase for the strimzi kafka operator so cannot comment on specific behaviours that might change, as this is just a proposal I suspect that is better placed in the eventual PR if this proposal gets accepted. +For users: + +* If you want to configure something for a single object (e.g. a single Kafka cluster), and it's in scope of the configuration, use that +* If you want to configure something that's not the currently configurable fields, you can use other mechanisms to do so (Kyverno, other controllers), and Strimzi won't interfere +* If you must configure something through some other method, and it's not directly being set in a different way by Strimzi, then Strimzi won't interfere and it'll "just work" (e.g. cattle annotations, pvc resizing annotations, metallb annotations. The user didn't make a choice about this other than the "I want to run Strimzi on this cluster that uses those technologies") +* If you (and again this may not be the direct user of Strimzi, but the platform/corporate/enterprise team enforcing policy on a wide scale) want to set certain fields, then again if it doesn't conflict, go for it. (e.g., The Datadog admission controller setting labels, Kyverno setting labels or annotations or automated topologySpreadConstraints) + +This does not add the possibility of conflicts, they already exist. This reduces the liklihood of reaching a conflict and has some mechanisms in place to avoid them once they are found, although it is still possible if 2 controllers are forcing ownership on the same field of the same resource. + ### Three-way diff The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of which fields the client previously applied, and indeed all clients (B) is stored in the `.metadata.managedFields`. And the incoming object (C) is supplied by the client. Kubernetes knows which fields were previously set by the client (in the case that the new object no longer specifies some, and they can be removed), and it knows about which client set all the other fields, and will keep them around instead of wiping out all the other labels etc. From 14c3fc6d610b27fe86dfce50ed2c0b109adc4ff2 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Tue, 27 Jun 2023 09:08:07 +0100 Subject: [PATCH 07/17] replying to more comments on the PR Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 43 +++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index fbbcb64a..ae65f6f5 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -29,12 +29,12 @@ This proposal makes Kubernetes keep track of each field's "owner" and any change This would let the Kubernetes apiserver handle the three-way diff, updates etc, and allow other controllers, webhooks etc to touch fields, labels, annotations etc the operator doesn't care about. -Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply), it is open to choose the method that Strimzi would use for this. +Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). The first pass of a Server-Side Apply would keep `force = false` and if a conflict were to arise then set `force = true`, this provides better visibility as to when these events may occur. [Kubernetes suggest](https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts:~:text=It%20is%20strongly%20recommended%20for%20controllers%20to%20always%20%22force%22%20conflicts%2C%20since%20they%20might%20not%20be%20able%20to%20resolve%20or%20act%20on%20these%20conflicts.) using the above _force_ method to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_, this obviously means if another party is _forcing_ the same field that they will create a loop of ownership but this would be rare as operators should only care about their own fields. [Kubernetes recommend](https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply:~:text=the%20object%20doesn%27t%20have%20to%20be%20read%20beforehand) omitting the `GET` call from the Kubernetes API, given that we only want to send the API our desired fields and leave it up to the API to solve. -The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per reconciliation as opposed to more. +The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per resource per reconciliation as opposed to more. Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. If this occurs and is not a desired behaviour then we can stick to the Read -> edit -> apply method instead for those examples with a justification. @@ -42,6 +42,15 @@ There is more documentation within the [Merge Strategy][https://kubernetes.io/do ### Code changes +We need to: +* Modify the PATCH/CREATE commands to server-side apply +* Remove the GET commands +* Change the ResourceDiff (and other similar functions) to only look at `strimzi-cluster-operator` owned resources (within `metadata.managedFields`) +* Update tests + +--- + +### **Modify the PATCH/CREATE commands to server-side apply** It appears as simple as modifying [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). From: @@ -56,9 +65,6 @@ To: ....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); ``` -It has been suggested that the first pass of a Server-Side Apply would keep `force = false` and if a conflict were to arise then set `force = true`, this provides better visibility as to when these events may occur. -This seems like a logical approach and can be included in the proposal. - Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), this would need updating too. It would also need confirming that all resource creates/updates go through `serverSideApply`, not something that should be done in this proposal. @@ -75,13 +81,18 @@ To: ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); ``` +Here `ReconcileResult` would be the `patched` type so would behave as such, code that relies on looking for `created` would need to be updated. + According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). -NOTE: The version of fabric8io being used is [6.6.1](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/pom.xml#L106) which [contains Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). +NOTE: The versions of fabric8io used by Strimzi [contain Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). + +--- +### **Change the ResourceDiff** The operators will need modifying: -* The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only `metadata.managedFields` (with `strimzi-cluster-operator` as the manager) to compute whether a change is needed as to not trigger a reconcile loop for externally owned resource paths. -* This will allow the removal of `ignorableFields` throughout the codebase as paths are being selected now, rather than filtering out fields. +* The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only `metadata.managedFields` (with `strimzi-cluster-operator` as the manager) to compare the current desired state and the new desired state, before sending it to the Kubernetes API. +* This will allow the removal of the `GET` method, as the operator should not care about other fields. It should only know the desired state, and ask Kubernetes API to apply that, letting Kubernetes handle the merge. It appears [this is a place](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ServiceOperator.java#L123) that the `desired` state imports existing external annotations, this would need to change to not include existing external annotations within the `desired` state as then Kubernetes would create an ownership conflict with Server Side Apply. @@ -98,19 +109,16 @@ The key thing to take from this table is that currently Strimzi is trying to mak It would be wise to try and include this in as many operators as possible, but in some situations where it is decided against it can be overridden in the code and left as is. - +--- ### Modifications to existing behaviours The only change to existing (non-Strimzi specific) behaviours is that `ResourceDiff` won't care about other fields it chooses not to care about. This does mean that another entity within the cluster can modify things without Strimzi's knowledge, but it shouldn't matter if the `metadata.managedFields` are chosen correctly. -I do not know the codebase for the strimzi kafka operator so cannot comment on specific behaviours that might change, as this is just a proposal I suspect that is better placed in the eventual PR if this proposal gets accepted. - For users: * If you want to configure something for a single object (e.g. a single Kafka cluster), and it's in scope of the configuration, use that * If you want to configure something that's not the currently configurable fields, you can use other mechanisms to do so (Kyverno, other controllers), and Strimzi won't interfere * If you must configure something through some other method, and it's not directly being set in a different way by Strimzi, then Strimzi won't interfere and it'll "just work" (e.g. cattle annotations, pvc resizing annotations, metallb annotations. The user didn't make a choice about this other than the "I want to run Strimzi on this cluster that uses those technologies") -* If you (and again this may not be the direct user of Strimzi, but the platform/corporate/enterprise team enforcing policy on a wide scale) want to set certain fields, then again if it doesn't conflict, go for it. (e.g., The Datadog admission controller setting labels, Kyverno setting labels or annotations or automated topologySpreadConstraints) This does not add the possibility of conflicts, they already exist. This reduces the liklihood of reaching a conflict and has some mechanisms in place to avoid them once they are found, although it is still possible if 2 controllers are forcing ownership on the same field of the same resource. @@ -122,8 +130,17 @@ The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of This proposal aims to be feature-flagged as not to affect existing use of Strimzi, but also allow people to "upgrade" at their own / controlled pace. A proposed name is `useServerSideApply`. -It should follow the same protocol as other feature gates, such as [`UseStrimziPodSets`](https://github.com/strimzi/strimzi-kafka-operator/blob/bcecad71b3676194ec7e17f245aef8d23556908a/CHANGELOG.md) (which was deployed in `0.28.0`, moved to beta in `0.30.0` and graduated in `0.35.0`). +It should follow the same protocol as other feature gates, such as [`UseStrimziPodSets`](https://github.com/strimzi/strimzi-kafka-operator/blob/bcecad71b3676194ec7e17f245aef8d23556908a/CHANGELOG.md) (which was deployed in `0.28.0`, moved to beta in `0.30.0` and graduated in `0.35.0`), the suggested protocol here is: +* alpha at `0.37.0` +* beta at `0.40.0` +* graduated at `0.43.0` + +### Mock server + +We have seen the [Fabric8 Mock Server](https://github.com/fabric8io/mockwebserver) in CRUD mode returns an error when using the `patch()` call (with server-side apply) when attempting to create a new resource, there is an existing issue [here](https://github.com/fabric8io/mockwebserver/issues/76) which would need fixing also. +The Mock Server could be put into Expectation Mode to allow us to override this behaviour, but it would require a lot more maintenance to upkeep. ## Rejected alternatives We considered adding a new ENV VAR which could take a list of Kubernetes resource paths to be ignored (or labels/annotations directly), but Kubernetes Server Side Apply is designed to do this job and considering it's within Kubernetes it seems wise to take the "out-of-the-box" option. +Another consequence of using ENV VARs would be the upkeep of both the existing way (hard-coded ignore paths) and the new way (server-side apply), this is a lot of overhead and we should just go with the one idea. From 522cb8885d457f4246520ae95c0c28df4941bc95 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Tue, 27 Jun 2023 09:16:26 +0100 Subject: [PATCH 08/17] updating to include info on choices, with recommendation Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 1 + 1 file changed, 1 insertion(+) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index ae65f6f5..41ffdca6 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -35,6 +35,7 @@ Conflicts can happen with Server Side Apply as described [here](https://github.c [Kubernetes recommend](https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply:~:text=the%20object%20doesn%27t%20have%20to%20be%20read%20beforehand) omitting the `GET` call from the Kubernetes API, given that we only want to send the API our desired fields and leave it up to the API to solve. The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per resource per reconciliation as opposed to more. +[Kuberenetes suggest](https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/#reconstructive-controllers) there are two ways to write controllers with Server Side Apply, the first being a modification to the existing "Read-Modify-Update" method that Strimzi is currently using, to compute the changes in a similar fashion but only on the fields Strimzi owns. The second (and recommended) way is that you reconstruct the whole resource for every reconciliation (of each resource) and Apply that, letting Kubernetes manage it. The advantages and disadvantages are discussed in the above link. Notably a NO OP Apply is not very taxing on the API and can reduce the number of calls by quite a lot. **Ideally Strimzi would do it the second way, that is we can remove all comparison logic and storing of state and all GET requests, and just rebuild the resource every time and Server Side Apply it.** Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. If this occurs and is not a desired behaviour then we can stick to the Read -> edit -> apply method instead for those examples with a justification. From 30c69ca6f98c198111cc0310d9a8aef4083c555e Mon Sep 17 00:00:00 2001 From: James Fuller Date: Thu, 29 Jun 2023 09:37:56 +0100 Subject: [PATCH 09/17] fixing markdown/language used Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 41ffdca6..98f1d512 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -39,7 +39,7 @@ The thing here is that by removing `GET`, we would always need to `UPDATE` with Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. If this occurs and is not a desired behaviour then we can stick to the Read -> edit -> apply method instead for those examples with a justification. -There is more documentation within the [Merge Strategy][https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy] documentation in which it defines how we can change CRDs to merge how we want them to (StrimziPodSet, for example). For reference, there are two main kinds: `atomic` and `map`/`set`/`granular`; with `atomic` set, any change replaces the whole field/list and is recursive downwards; with the others the behaviour is more fine-tuned and is described on the above link in a table. +There is more documentation within the [Merge Strategy](https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy) documentation in which it defines how we can change CRDs to merge how we want them to (StrimziPodSet, for example). For reference, there are two main kinds: `atomic` and `map`/`set`/`granular`; with `atomic` set, any change replaces the whole field/list and is recursive downwards; with the others the behaviour is more fine-tuned and is described on the above link in a table. ### Code changes @@ -52,7 +52,7 @@ We need to: --- ### **Modify the PATCH/CREATE commands to server-side apply** -It appears as simple as modifying [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). +Modify [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). From: ``` From 4e11611057af1a0c06b4583233d167063a9a2149 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Wed, 5 Jul 2023 11:12:23 +0100 Subject: [PATCH 10/17] chore(comments): addressing more comments from proposal Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 51 ++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 98f1d512..8c6cffc5 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -37,7 +37,7 @@ Conflicts can happen with Server Side Apply as described [here](https://github.c The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per resource per reconciliation as opposed to more. [Kuberenetes suggest](https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/#reconstructive-controllers) there are two ways to write controllers with Server Side Apply, the first being a modification to the existing "Read-Modify-Update" method that Strimzi is currently using, to compute the changes in a similar fashion but only on the fields Strimzi owns. The second (and recommended) way is that you reconstruct the whole resource for every reconciliation (of each resource) and Apply that, letting Kubernetes manage it. The advantages and disadvantages are discussed in the above link. Notably a NO OP Apply is not very taxing on the API and can reduce the number of calls by quite a lot. **Ideally Strimzi would do it the second way, that is we can remove all comparison logic and storing of state and all GET requests, and just rebuild the resource every time and Server Side Apply it.** -Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. If this occurs and is not a desired behaviour then we can stick to the Read -> edit -> apply method instead for those examples with a justification. +Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. There is more documentation within the [Merge Strategy](https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy) documentation in which it defines how we can change CRDs to merge how we want them to (StrimziPodSet, for example). For reference, there are two main kinds: `atomic` and `map`/`set`/`granular`; with `atomic` set, any change replaces the whole field/list and is recursive downwards; with the others the behaviour is more fine-tuned and is described on the above link in a table. @@ -46,7 +46,7 @@ There is more documentation within the [Merge Strategy](https://kubernetes.io/do We need to: * Modify the PATCH/CREATE commands to server-side apply * Remove the GET commands -* Change the ResourceDiff (and other similar functions) to only look at `strimzi-cluster-operator` owned resources (within `metadata.managedFields`) +* Remove the ResourceDiff (and other similar functions) * Update tests --- @@ -55,60 +55,67 @@ We need to: Modify [this line](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#LL263C88-L263C102). From: -``` +```java ....patch(PatchContext.of(PatchType.JSON), desired); ``` To: +```java +....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(), desired); ``` -....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); + +And to set without `Force`, as aforementioned for the first pass: + +```java +....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(false).build(), desired); ``` -Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), this would need updating too. -It would also need confirming that all resource creates/updates go through `serverSideApply`, not something that should be done in this proposal. +Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), this would need updating too. Every create/update call from the existing Operator should become a `serverSideApply` patch call. And [Kubernetes](https://kubernetes.io/docs/reference/using-api/server-side-apply/#:~:text=through%20declarative%20configurations.-,Clients%20can%20create,-and%20modify%20their)/[Fabric8io](https://github.com/fabric8io/kubernetes-client/blob/master/doc/CHEATSHEET.md#server-side-apply:~:text=For%20any%20create%20or%20update) recommend it is used on CREATE also: [From](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#L272): -``` +```java ReconcileResult result = ReconcileResult.created(operation().inNamespace(namespace).resource(desired).create()); ``` To: +```java +ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(), desired); ``` -ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withForce(true).build(), desired); + +And to set without `Force`, as aforementioned for the first pass: + +```java +ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(false).build(), desired); ``` -Here `ReconcileResult` would be the `patched` type so would behave as such, code that relies on looking for `created` would need to be updated. +Currently `ReconcileResult.patched` is used for `patch` and `ReconcileResult.created` is used for `create` operations, we can keep it this way - it seems only for logging purposes - or change it so that every call is `ReconcileResult.patched`, which would better represent what is actually happening within the code (there is no `create` called, as mentioned after the first example above). According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). NOTE: The versions of fabric8io used by Strimzi [contain Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). --- -### **Change the ResourceDiff** +### **Remove the ResourceDiff** The operators will need modifying: -* The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) will need to filter only `metadata.managedFields` (with `strimzi-cluster-operator` as the manager) to compare the current desired state and the new desired state, before sending it to the Kubernetes API. +* The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) is no longer needed, we can generate the whole resource (from Strimzi's perspective) every time and send it as a PATCH with Server-Side Apply. * This will allow the removal of the `GET` method, as the operator should not care about other fields. It should only know the desired state, and ask Kubernetes API to apply that, letting Kubernetes handle the merge. -It appears [this is a place](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ServiceOperator.java#L123) that the `desired` state imports existing external annotations, this would need to change to not include existing external annotations within the `desired` state as then Kubernetes would create an ownership conflict with Server Side Apply. - | current value of field | desired value of field | field owner | requires update? | change in behaviour | | -------- | -------- | ------ | ---------------- | ------------------- | -| x | x | Strimzi | no | no | +| x | x | Strimzi | no | yes | | x | y | Strimzi | yes | no | | x | not set | Strimzi | yes | no | | x | not set | Not Strimzi | no | yes | | not set | y | Strimzi | yes | no | | not set | y | Not Strimzi | yes | no | -The key thing to take from this table is that currently Strimzi is trying to make changes to ALL fields (minus short `ignorableFields` list), once Strimzi has knowledge of the fields it owns - we don't want it to try and change the fields it doesn't own. - -It would be wise to try and include this in as many operators as possible, but in some situations where it is decided against it can be overridden in the code and left as is. +The key thing to take from this table is that currently Strimzi is trying to make changes to ALL fields (minus short `ignorableFields` list), once Strimzi has knowledge of the fields it owns - we don't want it to try and change the fields it doesn't own. Also Strimzi will be sending a PATCH call for every resource every reconciliation loop, so where no fields have changed it will be a NO-OP with little resource usage --- ### Modifications to existing behaviours @@ -121,15 +128,21 @@ For users: * If you want to configure something that's not the currently configurable fields, you can use other mechanisms to do so (Kyverno, other controllers), and Strimzi won't interfere * If you must configure something through some other method, and it's not directly being set in a different way by Strimzi, then Strimzi won't interfere and it'll "just work" (e.g. cattle annotations, pvc resizing annotations, metallb annotations. The user didn't make a choice about this other than the "I want to run Strimzi on this cluster that uses those technologies") -This does not add the possibility of conflicts, they already exist. This reduces the liklihood of reaching a conflict and has some mechanisms in place to avoid them once they are found, although it is still possible if 2 controllers are forcing ownership on the same field of the same resource. +This does not add the possibility of conflicts, they already exist. This reduces the likelihood of reaching a conflict and has some mechanisms in place to avoid them once they are found, although it is still possible if 2 controllers are forcing ownership on the same field of the same resource. ### Three-way diff The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of which fields the client previously applied, and indeed all clients (B) is stored in the `.metadata.managedFields`. And the incoming object (C) is supplied by the client. Kubernetes knows which fields were previously set by the client (in the case that the new object no longer specifies some, and they can be removed), and it knows about which client set all the other fields, and will keep them around instead of wiping out all the other labels etc. +Kubernetes knows this information as it is set in the `patch` call, example: +```java +PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(); +``` + +Currently, if a field that Strimzi owns is unset by a user then Strimzi sets a default value. This behaviour will stay the same as the Operator can choose which fields to default if unset by a user when constructing the resource to send to Kubernetes API, this means that if `affinity` is unset then the Operator can set it before Applying and Kubernetes will know it is owned by Strimzi. ## Compatibility -This proposal aims to be feature-flagged as not to affect existing use of Strimzi, but also allow people to "upgrade" at their own / controlled pace. +This proposal aims to be feature-flagged to allow a gradual roll-out of this change to the users and give them time to adjust their own environments to it. A proposed name is `useServerSideApply`. It should follow the same protocol as other feature gates, such as [`UseStrimziPodSets`](https://github.com/strimzi/strimzi-kafka-operator/blob/bcecad71b3676194ec7e17f245aef8d23556908a/CHANGELOG.md) (which was deployed in `0.28.0`, moved to beta in `0.30.0` and graduated in `0.35.0`), the suggested protocol here is: * alpha at `0.37.0` From e03d622a06e1bd737e340b83334b3c85343c08a6 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Wed, 5 Jul 2023 11:30:55 +0100 Subject: [PATCH 11/17] chore(comments): updating small comment Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 8c6cffc5..d4c3163e 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -93,7 +93,7 @@ And to set without `Force`, as aforementioned for the first pass: ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(false).build(), desired); ``` -Currently `ReconcileResult.patched` is used for `patch` and `ReconcileResult.created` is used for `create` operations, we can keep it this way - it seems only for logging purposes - or change it so that every call is `ReconcileResult.patched`, which would better represent what is actually happening within the code (there is no `create` called, as mentioned after the first example above). +Currently `ReconcileResult.patched` is used for `patch` and `ReconcileResult.created` is used for `create` operations. We will no longer be making an initial GET request to determine the current resource, so will always be using a PATCH request and returning `ReconcileResult.patched`. There is no code specifically using the `ReconcileResult.Created` type to determine behaviour that we can see, it is only used in tests. According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). From 4f403c8a8d0de7cf141885e4562cff32efbb7262 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Thu, 13 Jul 2023 14:09:46 +0100 Subject: [PATCH 12/17] update to comments Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index d4c3163e..6745930a 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -35,7 +35,7 @@ Conflicts can happen with Server Side Apply as described [here](https://github.c [Kubernetes recommend](https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply:~:text=the%20object%20doesn%27t%20have%20to%20be%20read%20beforehand) omitting the `GET` call from the Kubernetes API, given that we only want to send the API our desired fields and leave it up to the API to solve. The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per resource per reconciliation as opposed to more. -[Kuberenetes suggest](https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/#reconstructive-controllers) there are two ways to write controllers with Server Side Apply, the first being a modification to the existing "Read-Modify-Update" method that Strimzi is currently using, to compute the changes in a similar fashion but only on the fields Strimzi owns. The second (and recommended) way is that you reconstruct the whole resource for every reconciliation (of each resource) and Apply that, letting Kubernetes manage it. The advantages and disadvantages are discussed in the above link. Notably a NO OP Apply is not very taxing on the API and can reduce the number of calls by quite a lot. **Ideally Strimzi would do it the second way, that is we can remove all comparison logic and storing of state and all GET requests, and just rebuild the resource every time and Server Side Apply it.** +[Kuberenetes suggest](https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/#reconstructive-controllers) there are two ways to write controllers with Server Side Apply, the first being a modification to the existing "Read-Modify-Update" method that Strimzi is currently using, to compute the changes in a similar fashion but only on the fields Strimzi owns. The second (and recommended) way is that you reconstruct the whole resource for every reconciliation (of each resource) and Apply that, letting Kubernetes manage it. The advantages and disadvantages are discussed in the above link. Notably a NO OP Apply is not very taxing on the API and can reduce the number of calls by quite a lot. **Strimzi will adopt the second way, that is we can remove all comparison logic and storing of state and all GET requests, and just rebuild the resource every time and Server Side Apply it.** Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. @@ -61,7 +61,6 @@ From: To: - ```java ....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(), desired); ``` @@ -102,7 +101,6 @@ NOTE: The versions of fabric8io used by Strimzi [contain Server Side Apply](http --- ### **Remove the ResourceDiff** -The operators will need modifying: * The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) is no longer needed, we can generate the whole resource (from Strimzi's perspective) every time and send it as a PATCH with Server-Side Apply. * This will allow the removal of the `GET` method, as the operator should not care about other fields. It should only know the desired state, and ask Kubernetes API to apply that, letting Kubernetes handle the merge. @@ -153,6 +151,7 @@ It should follow the same protocol as other feature gates, such as [`UseStrimziP We have seen the [Fabric8 Mock Server](https://github.com/fabric8io/mockwebserver) in CRUD mode returns an error when using the `patch()` call (with server-side apply) when attempting to create a new resource, there is an existing issue [here](https://github.com/fabric8io/mockwebserver/issues/76) which would need fixing also. The Mock Server could be put into Expectation Mode to allow us to override this behaviour, but it would require a lot more maintenance to upkeep. +The limitations will be addressed either in the Fabric8 Mock Server or by moving to another Mock Server as part of the server-side apply implementation. ## Rejected alternatives From 81be7a766c81a71b0741eb91764b4a85d79bfbac Mon Sep 17 00:00:00 2001 From: James Fuller Date: Fri, 14 Jul 2023 10:53:19 +0100 Subject: [PATCH 13/17] adds line about reconcileresult.noop Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 6745930a..71712e32 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -94,6 +94,8 @@ ReconcileResult result = ReconcileResult.patched(operation().inNamespace(name Currently `ReconcileResult.patched` is used for `patch` and `ReconcileResult.created` is used for `create` operations. We will no longer be making an initial GET request to determine the current resource, so will always be using a PATCH request and returning `ReconcileResult.patched`. There is no code specifically using the `ReconcileResult.Created` type to determine behaviour that we can see, it is only used in tests. +It seems there is some [usage](https://github.com/strimzi/strimzi-kafka-operator/blob/834bfc894475a3204029c2abd1426d4200ca3b22/cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaExporterReconciler.java#L191C21-L192C46) around `ReconcileResult.noop`, the implementation piece should also address this and make sure the logic continues to work. + According to the [package being used](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). NOTE: The versions of fabric8io used by Strimzi [contain Server Side Apply](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). From d4437656e566658b6eaffccaab712d910a6d6701 Mon Sep 17 00:00:00 2001 From: James Fuller Date: Mon, 17 Jul 2023 11:17:48 +0100 Subject: [PATCH 14/17] updates to comments, and restructures some code to make more sense Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 51 +++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 71712e32..b6edc1b0 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -7,21 +7,19 @@ NOTE: Strimzi will still have ownership over the fields it wants. ## Current situation / cause of proposal As per [Issue 6938](https://github.com/strimzi/strimzi-kafka-operator/issues/6938) third party Kubernetes operators are having a negative interaction with the existing way Strimzi does Kubernetes applies. -An example is [Kyverno](https://kyverno.io/) has a policy in a K8s cluster to add an annotation (`policies.kyverno.io/last-applied-patches:...`) to all resources with a MutatingWebhookConfiguration, Strimzi detects this label as a resource diff and removes it as it doesn't own it / not included in [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L26). This triggers the MutatingWebhookConfiguration which re-applies the label and the loop continues. +One example is [Kyverno](https://kyverno.io/), which has a policy to add an annotation (`policies.kyverno.io/last-applied-patches:...`) to all resources in a K8s cluster with a `MutatingWebhookConfiguration`. Strimzi detects this label as a resource diff and removes it as it doesn't own it or it is not included in the [hard-coded list of ignored labels](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L26). This, in turn, triggers the `MutatingWebhookConfiguration` which re-applies the label and the loop continues. -Another use case could be a Kyverno policy that adds AWS IRSA annotations to a Service Account (specifically useful for a Kafka Sink/Source Connector), this would allow the annotation to be owned by Kyverno and therefore leave it in place without constantly trying to remove it. +Another example could be a Kyverno policy that adds AWS IRSA annotations to a Service Account (specifically useful for a Kafka Sink/Source Connector). This would allow the annotation to be owned by Kyverno and therefore leave it in place without constantly trying to remove it. -The precise log we saw was: +The following log related to the behavior described shows a diff found in the annotations of a `StatefulSet` (before `StrimziPodSet` resources were introduced): ``` 2023-03-03 14:25:52 DEBUG ResourceDiff:38 - Reconciliation #178(timer) Kafka(kafka-backup-spike/kafka-backup-spike): StatefulSet kafka-backup-spike-zookeeper differs: {"op":"replace","path":"/metadata/annotations/policies.kyverno.io~1last-applied-patches","value":""} ``` -This shows a diff found in the annotations of a `StatefulSet` (this was before StrimziPodSets) which generates the behaviour desribed above. - ## Motivation -Once this change is made it will allow third party tools within the cluster to interact with Strimzi owned resources without looping and Strimzi will be operating as if it never knew about them. +Once this change is made it will allow third party tools within the cluster to interact with Strimzi owned resources without looping. Strimzi can continue to operate as if it never knew about them. ## Proposal @@ -29,17 +27,21 @@ This proposal makes Kubernetes keep track of each field's "owner" and any change This would let the Kubernetes apiserver handle the three-way diff, updates etc, and allow other controllers, webhooks etc to touch fields, labels, annotations etc the operator doesn't care about. -Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). The first pass of a Server-Side Apply would keep `force = false` and if a conflict were to arise then set `force = true`, this provides better visibility as to when these events may occur. +Conflicts can happen with Server Side Apply as described [here](https://github.com/fabric8io/kubernetes-client/blob/v6.5.1/doc/CHEATSHEET.md#server-side-apply). The first pass of a Server-Side Apply would keep `force = false` and if a conflict were to arise then set `force = true`, which provides better visibility as to when these events may occur. + +Kubernetes suggest [using the _force_ method](https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts:~:text=It%20is%20strongly%20recommended%20for%20controllers%20to%20always%20%22force%22%20conflicts%2C%20since%20they%20might%20not%20be%20able%20to%20resolve%20or%20act%20on%20these%20conflicts.) to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs: _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_. This obviously means if another party is _forcing_ the same field that they will create a loop of ownership, but this would be rare as operators should only care about their own fields. + +Kubernetes recommend [omitting the `GET` call from the Kubernetes API](https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply:~:text=the%20object%20doesn%27t%20have%20to%20be%20read%20beforehand), given that we only want to send the API our desired fields and leave it up to the API to solve. -[Kubernetes suggest](https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts:~:text=It%20is%20strongly%20recommended%20for%20controllers%20to%20always%20%22force%22%20conflicts%2C%20since%20they%20might%20not%20be%20able%20to%20resolve%20or%20act%20on%20these%20conflicts.) using the above _force_ method to make sure Strimzi have control over the parts they want. As described in the Kubernetes docs _"This forces the operation to succeed, changes the value of the field, and removes the field from all other managers' entries in managedFields"_, this obviously means if another party is _forcing_ the same field that they will create a loop of ownership but this would be rare as operators should only care about their own fields. +There are two ways to write controllers with Server Side Apply: +- Modification of the existing "Read-Modify-Update" method. This approach involves computing changes in a similar fashion to the current method used by Strimzi. However, the modifications are performed only on the fields owned by Strimzi. +- Reconstructing the entire resource for every reconciliation. This method is the recommended approach. It involves rebuilding the resource completely during each reconciliation of every resource and applying it through Kubernetes management. -[Kubernetes recommend](https://kubernetes.io/docs/reference/using-api/server-side-apply/#upgrading-from-client-side-apply-to-server-side-apply:~:text=the%20object%20doesn%27t%20have%20to%20be%20read%20beforehand) omitting the `GET` call from the Kubernetes API, given that we only want to send the API our desired fields and leave it up to the API to solve. -The thing here is that by removing `GET`, we would always need to `UPDATE` with the desired status and allow Kubernetes to resolve it. This would in theory be 1 API call made by Strimzi per resource per reconciliation as opposed to more. -[Kuberenetes suggest](https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/#reconstructive-controllers) there are two ways to write controllers with Server Side Apply, the first being a modification to the existing "Read-Modify-Update" method that Strimzi is currently using, to compute the changes in a similar fashion but only on the fields Strimzi owns. The second (and recommended) way is that you reconstruct the whole resource for every reconciliation (of each resource) and Apply that, letting Kubernetes manage it. The advantages and disadvantages are discussed in the above link. Notably a NO OP Apply is not very taxing on the API and can reduce the number of calls by quite a lot. **Strimzi will adopt the second way, that is we can remove all comparison logic and storing of state and all GET requests, and just rebuild the resource every time and Server Side Apply it.** +The advantages and disadvantages are discussed in the [Kubernetes blog post on Server Side Apply](https://kubernetes.io/blog/2022/10/20/advanced-server-side-apply/#reconstructive-controllers). Notably, performing a NO OP Apply is not very taxing on the API and can significantly reduce the number of calls. **Strimzi will adopt the second method**, which eliminates the need for comparison logic, storing of state, and GET requests. Instead, the resource will be rebuilt every time and applied using Server Side Apply. -Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), this explains how Server Side Apply will merge fields together, this will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. +Existing Resources (Pods/ConfigMaps/etc) contain a patch strategy in the [upstream docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/), which explains how Server Side Apply will merge fields together. This will be unique to every field and if no strategy is listed then it can be assumed the whole field is changed each time. -There is more documentation within the [Merge Strategy](https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy) documentation in which it defines how we can change CRDs to merge how we want them to (StrimziPodSet, for example). For reference, there are two main kinds: `atomic` and `map`/`set`/`granular`; with `atomic` set, any change replaces the whole field/list and is recursive downwards; with the others the behaviour is more fine-tuned and is described on the above link in a table. +[Kubernetes Merge Strategy](https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy) documentation defines how we can change CRDs to merge how we want them to (`StrimziPodSet`, for example). For reference, there are two main kinds: `atomic` and `map`/`set`/`granular`; with `atomic` set, any change replaces the whole field/list and is recursive downwards; with the others the behaviour is more fine-tuned. ### Code changes @@ -71,7 +73,7 @@ And to set without `Force`, as aforementioned for the first pass: ....patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(false).build(), desired); ``` -Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), this would need updating too. Every create/update call from the existing Operator should become a `serverSideApply` patch call. +Also, the `StrimziPodSetOperator` patches this method [here](https://github.com/strimzi/strimzi-kafka-operator/blob/main/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/StrimziPodSetOperator.java#L65), which would need updating too. Every create/update call from the existing Operator should become a `serverSideApply` patch call. And [Kubernetes](https://kubernetes.io/docs/reference/using-api/server-side-apply/#:~:text=through%20declarative%20configurations.-,Clients%20can%20create,-and%20modify%20their)/[Fabric8io](https://github.com/fabric8io/kubernetes-client/blob/master/doc/CHEATSHEET.md#server-side-apply:~:text=For%20any%20create%20or%20update) recommend it is used on CREATE also: [From](https://github.com/strimzi/strimzi-kafka-operator/blob/18d76bfabcfb9e91c71f9afda60b9dd880797f02/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/AbstractNamespacedResourceOperator.java#L272): @@ -80,16 +82,16 @@ And [Kubernetes](https://kubernetes.io/docs/reference/using-api/server-side-appl ReconcileResult result = ReconcileResult.created(operation().inNamespace(namespace).resource(desired).create()); ``` -To: +To set without `Force`, as aforementioned for the first pass: ```java -ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(), desired); +ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(false).build(), desired); ``` -And to set without `Force`, as aforementioned for the first pass: +To set with `Force`, if there is a conflict for a field and Strimzi should re-take ownership: ```java -ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(false).build(), desired); +ReconcileResult result = ReconcileResult.patched(operation().inNamespace(namespace).withName(name).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(), desired); ``` Currently `ReconcileResult.patched` is used for `patch` and `ReconcileResult.created` is used for `create` operations. We will no longer be making an initial GET request to determine the current resource, so will always be using a PATCH request and returning `ReconcileResult.patched`. There is no code specifically using the `ReconcileResult.Created` type to determine behaviour that we can see, it is only used in tests. @@ -106,6 +108,7 @@ NOTE: The versions of fabric8io used by Strimzi [contain Server Side Apply](http * The [`ResourceDiff`](https://github.com/strimzi/strimzi-kafka-operator/blob/c3522cf4b17004004a676854d37ba01bb9a44800/operator-common/src/main/java/io/strimzi/operator/common/operator/resource/ResourceDiff.java#L46-L78) is no longer needed, we can generate the whole resource (from Strimzi's perspective) every time and send it as a PATCH with Server-Side Apply. * This will allow the removal of the `GET` method, as the operator should not care about other fields. It should only know the desired state, and ask Kubernetes API to apply that, letting Kubernetes handle the merge. +The following table shows changes in existing Kubernetes behaviour, with examples: | current value of field | desired value of field | field owner | requires update? | change in behaviour | | -------- | -------- | ------ | ---------------- | ------------------- | | x | x | Strimzi | no | yes | @@ -115,7 +118,7 @@ NOTE: The versions of fabric8io used by Strimzi [contain Server Side Apply](http | not set | y | Strimzi | yes | no | | not set | y | Not Strimzi | yes | no | -The key thing to take from this table is that currently Strimzi is trying to make changes to ALL fields (minus short `ignorableFields` list), once Strimzi has knowledge of the fields it owns - we don't want it to try and change the fields it doesn't own. Also Strimzi will be sending a PATCH call for every resource every reconciliation loop, so where no fields have changed it will be a NO-OP with little resource usage +The key thing to take from this table is that currently Strimzi is trying to make changes to ALL fields (minus short `ignorableFields` list), once Strimzi has knowledge of the fields it owns - we don't want it to try and change the fields it doesn't own. Also Strimzi will be sending a PATCH call for every resource every reconciliation loop, so where no fields have changed it will be a NO-OP with little resource usage. --- ### Modifications to existing behaviours @@ -128,23 +131,23 @@ For users: * If you want to configure something that's not the currently configurable fields, you can use other mechanisms to do so (Kyverno, other controllers), and Strimzi won't interfere * If you must configure something through some other method, and it's not directly being set in a different way by Strimzi, then Strimzi won't interfere and it'll "just work" (e.g. cattle annotations, pvc resizing annotations, metallb annotations. The user didn't make a choice about this other than the "I want to run Strimzi on this cluster that uses those technologies") -This does not add the possibility of conflicts, they already exist. This reduces the likelihood of reaching a conflict and has some mechanisms in place to avoid them once they are found, although it is still possible if 2 controllers are forcing ownership on the same field of the same resource. +This does not add to the possibility of conflicts that already exist. But it does reduce the likelihood of reaching a conflict and has some mechanisms in place to avoid them once they are found. Conflicts are still possible if 2 controllers are forcing ownership on the same field of the same resource. ### Three-way diff The live object (A) is stored in the `.metadata`, `.spec` etc. The knowledge of which fields the client previously applied, and indeed all clients (B) is stored in the `.metadata.managedFields`. And the incoming object (C) is supplied by the client. Kubernetes knows which fields were previously set by the client (in the case that the new object no longer specifies some, and they can be removed), and it knows about which client set all the other fields, and will keep them around instead of wiping out all the other labels etc. -Kubernetes knows this information as it is set in the `patch` call, example: +Kubernetes knows this information as it is set in the `patch` call. For example: ```java PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("strimzi-cluster-operator").withForce(true).build(); ``` -Currently, if a field that Strimzi owns is unset by a user then Strimzi sets a default value. This behaviour will stay the same as the Operator can choose which fields to default if unset by a user when constructing the resource to send to Kubernetes API, this means that if `affinity` is unset then the Operator can set it before Applying and Kubernetes will know it is owned by Strimzi. +Currently, if a field that Strimzi owns is unset by a user then Strimzi sets a default value. This behaviour will stay the same as the Operator can choose which fields to default if unset by a user when constructing the resource to send to Kubernetes API, which means that if `affinity` is unset then the Operator can set it before Applying and Kubernetes will know it is owned by Strimzi. ## Compatibility This proposal aims to be feature-flagged to allow a gradual roll-out of this change to the users and give them time to adjust their own environments to it. A proposed name is `useServerSideApply`. -It should follow the same protocol as other feature gates, such as [`UseStrimziPodSets`](https://github.com/strimzi/strimzi-kafka-operator/blob/bcecad71b3676194ec7e17f245aef8d23556908a/CHANGELOG.md) (which was deployed in `0.28.0`, moved to beta in `0.30.0` and graduated in `0.35.0`), the suggested protocol here is: +It should follow the same protocol as other feature gates, such as [`UseStrimziPodSets`](https://github.com/strimzi/strimzi-kafka-operator/blob/bcecad71b3676194ec7e17f245aef8d23556908a/CHANGELOG.md) (which was deployed in `0.28.0`, moved to beta in `0.30.0` and graduated in `0.35.0`). The suggested protocol here is: * alpha at `0.37.0` * beta at `0.40.0` * graduated at `0.43.0` @@ -158,4 +161,4 @@ The limitations will be addressed either in the Fabric8 Mock Server or by moving ## Rejected alternatives We considered adding a new ENV VAR which could take a list of Kubernetes resource paths to be ignored (or labels/annotations directly), but Kubernetes Server Side Apply is designed to do this job and considering it's within Kubernetes it seems wise to take the "out-of-the-box" option. -Another consequence of using ENV VARs would be the upkeep of both the existing way (hard-coded ignore paths) and the new way (server-side apply), this is a lot of overhead and we should just go with the one idea. +Another consequence of using ENV VARs would be the overhead of using the existing approach (hard-coded ignore paths) and the new approach (server-side apply), rather than using a single approach. From 8b98b04682f4e7ed9677949e54345a920e1b9e3b Mon Sep 17 00:00:00 2001 From: James Fuller Date: Thu, 10 Aug 2023 14:54:58 +0100 Subject: [PATCH 15/17] chore: updating user guidelines with examples Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index b6edc1b0..45e6de27 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -127,9 +127,33 @@ The only change to existing (non-Strimzi specific) behaviours is that `ResourceD For users: -* If you want to configure something for a single object (e.g. a single Kafka cluster), and it's in scope of the configuration, use that -* If you want to configure something that's not the currently configurable fields, you can use other mechanisms to do so (Kyverno, other controllers), and Strimzi won't interfere -* If you must configure something through some other method, and it's not directly being set in a different way by Strimzi, then Strimzi won't interfere and it'll "just work" (e.g. cattle annotations, pvc resizing annotations, metallb annotations. The user didn't make a choice about this other than the "I want to run Strimzi on this cluster that uses those technologies") +* If you want to configure part of a Strimzi Custom Resource (CR) (e.g. `Kafka` CR `replicas`) and the field exists within the CR, then you should use that field +```yaml +apiVersion: kafka.strimzi.io/v1beta2 +kind: Kafka +metadata: + name: my-cluster + annotations: + custom-annotation: "..." # defined with use of CR, therefore "owned" by Strimzi +spec: + kafka: + replicas: 3 + ... +``` +* If you want to configure part of a Strimzi CR that is not within the configurable fields (e.g. `annotations` used by another operator), you can apply them in any manner you like and Strimzi won't interfere (unless you define them in the CR itself). +```yaml +apiVersion: kafka.strimzi.io/v1beta2 +kind: Kafka +metadata: + name: my-cluster + annotations: + policies.kyverno.io/last-applied-patches: "..." # applied from Kyverno operator, therefore ignored by Strimzi operator + custom-annotation: "..." # defined with use of CR, therefore "owned" by Strimzi +spec: + kafka: + replicas: 3 + ... +``` This does not add to the possibility of conflicts that already exist. But it does reduce the likelihood of reaching a conflict and has some mechanisms in place to avoid them once they are found. Conflicts are still possible if 2 controllers are forcing ownership on the same field of the same resource. From 1e9f04ce1a172c679adf7f4ca7699272b9b347be Mon Sep 17 00:00:00 2001 From: James Fuller Date: Thu, 10 Aug 2023 15:08:52 +0100 Subject: [PATCH 16/17] chore: updating user guidelines with examples Signed-off-by: James Fuller --- 052-k8s-server-side-apply.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/052-k8s-server-side-apply.md b/052-k8s-server-side-apply.md index 45e6de27..58ab1be7 100644 --- a/052-k8s-server-side-apply.md +++ b/052-k8s-server-side-apply.md @@ -127,32 +127,32 @@ The only change to existing (non-Strimzi specific) behaviours is that `ResourceD For users: -* If you want to configure part of a Strimzi Custom Resource (CR) (e.g. `Kafka` CR `replicas`) and the field exists within the CR, then you should use that field +* If you want to configure part of a Strimzi Custom Resource (CR) (e.g. Kafka CR replicas) and the field exists within the CR, then you should use that field ```yaml apiVersion: kafka.strimzi.io/v1beta2 kind: Kafka metadata: name: my-cluster - annotations: - custom-annotation: "..." # defined with use of CR, therefore "owned" by Strimzi spec: kafka: - replicas: 3 + template: + pod: + metadata: + annotations: + custom-annotation: "..." # defined with use of CR, therefore "owned" by Strimzi ... ``` -* If you want to configure part of a Strimzi CR that is not within the configurable fields (e.g. `annotations` used by another operator), you can apply them in any manner you like and Strimzi won't interfere (unless you define them in the CR itself). +* If you want to configure part of a Strimzi CR that is not within the configurable fields (e.g. annotations used by another operator) or has a dynamic value, you can apply them in any manner you like and Strimzi won't interfere (unless you define them in the CR itself). ```yaml -apiVersion: kafka.strimzi.io/v1beta2 -kind: Kafka +apiVersion: v1 +kind: Pod metadata: - name: my-cluster + name: my-cluster-kafka-0 annotations: policies.kyverno.io/last-applied-patches: "..." # applied from Kyverno operator, therefore ignored by Strimzi operator custom-annotation: "..." # defined with use of CR, therefore "owned" by Strimzi spec: - kafka: - replicas: 3 - ... + ... ``` This does not add to the possibility of conflicts that already exist. But it does reduce the likelihood of reaching a conflict and has some mechanisms in place to avoid them once they are found. Conflicts are still possible if 2 controllers are forcing ownership on the same field of the same resource. From 70377a993c612847d6895f6bd3a77877a4b2a582 Mon Sep 17 00:00:00 2001 From: Jakub Scholz Date: Thu, 10 Aug 2023 20:03:24 +0200 Subject: [PATCH 17/17] Update the index in README Signed-off-by: Jakub Scholz --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 4938213f..af158725 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,7 @@ This repository list of proposals for the Strimzi project. A template for new pr | # | Title | | :-: |:----------------------------------------------------------------------| +| 52 | [Kubernetes Server Side Apply](./052-k8s-server-side-apply.md) | | 51 | [Unidirectional Topic Operator](./051-unidirectional-topic-operator.md) | | 50 | [Kafka Node Pools](./050-Kafka-Node-Pools.md) | | 49 | [Prevent broker scale down if it contains partition replicas](./049-prevent-broker-scale-down-if-it-contains-partition-replicas.md) |