Use the status subresource for setting labels and annotations

Changes to metadata via status subresources are not restricted for the basic kubernetes types. This means we can set the status/annotations/labels with only the status subresource permissions. Upstream note: kubernetes/kubernetes#92022 (comment) Signed-off-by: Patryk Diak <[email protected]>
kyrtapz · Aug 22, 2023 · 511f635 · 511f635
1 parent 18f8966
commit 511f635
Show file tree

Hide file tree

Showing 13 changed files with 31 additions and 61 deletions.
diff --git a/go-controller/pkg/clustermanager/node/node_allocator.go b/go-controller/pkg/clustermanager/node/node_allocator.go
@@ -313,7 +313,9 @@ func (na *NodeAllocator) updateNodeNetworkAnnotationsWithRetry(nodeName string,
 			return fmt.Errorf("failed to update node %q network id annotation %d for network %s",
 				node.Name, networkId, networkName)
 		}
-		return na.kube.UpdateNode(cnode)
+		// It is possible to update the node annotations using status subresource
+		// because changes to metadata via status subresource are not restricted for nodes.
+		return na.kube.UpdateNodeStatus(cnode)
 	})
 	if resultErr != nil {
 		return fmt.Errorf("failed to update node %s annotation", nodeName)

diff --git a/go-controller/pkg/clustermanager/pod/allocator_test.go b/go-controller/pkg/clustermanager/pod/allocator_test.go
@@ -408,7 +408,7 @@ func TestPodAllocator_reconcileForNAD(t *testing.T) {
 			podListerMock.On("Pods", mock.AnythingOfType("string")).Return(podNamespaceLister)
 
 			var allocated bool
-			kubeMock.On("UpdatePod", mock.AnythingOfType(fmt.Sprintf("%T", &corev1.Pod{}))).Run(
+			kubeMock.On("UpdatePodStatus", mock.AnythingOfType(fmt.Sprintf("%T", &corev1.Pod{}))).Run(
 				func(args mock.Arguments) {
 					allocated = true
 				},

diff --git a/go-controller/pkg/cni/cni_dpu_test.go b/go-controller/pkg/cni/cni_dpu_test.go
@@ -70,7 +70,7 @@ var _ = Describe("cni_dpu tests", func() {
 			cpod := pod.DeepCopy()
 			cpod.Annotations, err = util.MarshalPodDPUConnDetails(cpod.Annotations, &dpuCd, ovntypes.DefaultNetworkName)
 			Expect(err).ToNot(HaveOccurred())
-			fakeKubeInterface.On("UpdatePod", cpod).Return(nil)
+			fakeKubeInterface.On("UpdatePodStatus", cpod).Return(nil)
 			err = pr.addDPUConnectionDetailsAnnot(&fakeKubeInterface, &podLister, "")
 			Expect(err).ToNot(HaveOccurred())
 
@@ -124,7 +124,7 @@ var _ = Describe("cni_dpu tests", func() {
 			cpod := pod.DeepCopy()
 			cpod.Annotations, err = util.MarshalPodDPUConnDetails(cpod.Annotations, &dpuCd, ovntypes.DefaultNetworkName)
 			Expect(err).ToNot(HaveOccurred())
-			fakeKubeInterface.On("UpdatePod", cpod).Return(fmt.Errorf("failed to set annotation"))
+			fakeKubeInterface.On("UpdatePodStatus", cpod).Return(fmt.Errorf("failed to set annotation"))
 			err = pr.addDPUConnectionDetailsAnnot(&fakeKubeInterface, &podLister, "")
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("failed to set annotation"))

diff --git a/go-controller/pkg/kube/kube.go b/go-controller/pkg/kube/kube.go
@@ -50,9 +50,8 @@ type Interface interface {
 	RemoveTaintFromNode(nodeName string, taint *kapi.Taint) error
 	SetLabelsOnNode(nodeName string, labels map[string]interface{}) error
 	PatchNode(old, new *kapi.Node) error
-	UpdateNode(node *kapi.Node) error
 	UpdateNodeStatus(node *kapi.Node) error
-	UpdatePod(pod *kapi.Pod) error
+	UpdatePodStatus(pod *kapi.Pod) error
 	GetAnnotationsOnPod(namespace, name string) (map[string]string, error)
 	GetNodes() (*kapi.NodeList, error)
 	GetNamespaces(labelSelector metav1.LabelSelector) (*kapi.NamespaceList, error)
@@ -100,7 +99,7 @@ func (k *Kube) SetAnnotationsOnPod(namespace, podName string, annotations map[st
 		return err
 	}
 
-	_, err = k.KClient.CoreV1().Pods(namespace).Patch(context.TODO(), podName, types.MergePatchType, patchData, metav1.PatchOptions{})
+	_, err = k.KClient.CoreV1().Pods(namespace).Patch(context.TODO(), podName, types.MergePatchType, patchData, metav1.PatchOptions{}, "status")
 	if err != nil {
 		klog.Errorf("Error in setting annotation on pod %s: %v", podDesc, err)
 	}
@@ -126,7 +125,7 @@ func (k *Kube) SetAnnotationsOnNode(nodeName string, annotations map[string]inte
 		return err
 	}
 
-	_, err = k.KClient.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.MergePatchType, patchData, metav1.PatchOptions{})
+	_, err = k.KClient.CoreV1().Nodes().PatchStatus(context.TODO(), nodeName, patchData)
 	if err != nil {
 		klog.Errorf("Error in setting annotation on node %s: %v", nodeName, err)
 	}
@@ -152,7 +151,7 @@ func (k *Kube) SetAnnotationsOnNamespace(namespaceName string, annotations map[s
 		return err
 	}
 
-	_, err = k.KClient.CoreV1().Namespaces().Patch(context.TODO(), namespaceName, types.MergePatchType, patchData, metav1.PatchOptions{})
+	_, err = k.KClient.CoreV1().Namespaces().Patch(context.TODO(), namespaceName, types.MergePatchType, patchData, metav1.PatchOptions{}, "status")
 	if err != nil {
 		klog.Errorf("Error in setting annotation on namespace %s: %v", namespaceName, err)
 	}
@@ -179,7 +178,7 @@ func (k *Kube) SetAnnotationsOnService(namespace, name string, annotations map[s
 		return err
 	}
 
-	_, err = k.KClient.CoreV1().Services(namespace).Patch(context.TODO(), name, types.MergePatchType, patchData, metav1.PatchOptions{})
+	_, err = k.KClient.CoreV1().Services(namespace).Patch(context.TODO(), name, types.MergePatchType, patchData, metav1.PatchOptions{}, "status")
 	if err != nil {
 		klog.Errorf("Error in setting annotation on service %s: %v", serviceDesc, err)
 	}
@@ -265,7 +264,7 @@ func (k *Kube) SetLabelsOnNode(nodeName string, labels map[string]interface{}) e
 		return err
 	}
 
-	_, err = k.KClient.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.MergePatchType, patchData, metav1.PatchOptions{})
+	_, err = k.KClient.CoreV1().Nodes().PatchStatus(context.TODO(), nodeName, patchData)
 	return err
 }
 
@@ -297,12 +296,6 @@ func (k *Kube) PatchNode(old, new *kapi.Node) error {
 	return nil
 }
 
-// UpdateNode updates node with provided node data
-func (k *Kube) UpdateNode(node *kapi.Node) error {
-	_, err := k.KClient.CoreV1().Nodes().Update(context.TODO(), node, metav1.UpdateOptions{})
-	return err
-}
-
 // UpdateNodeStatus takes the node object and sets the provided update status
 func (k *Kube) UpdateNodeStatus(node *kapi.Node) error {
 	klog.Infof("Updating status on node %s", node.Name)
@@ -313,10 +306,10 @@ func (k *Kube) UpdateNodeStatus(node *kapi.Node) error {
 	return err
 }
 
-// UpdatePod update pod with provided pod data
-func (k *Kube) UpdatePod(pod *kapi.Pod) error {
+// UpdatePodStatus update pod with provided pod data, limited to .Status and .ObjectMeta fields
+func (k *Kube) UpdatePodStatus(pod *kapi.Pod) error {
 	klog.Infof("Updating pod %s/%s", pod.Namespace, pod.Name)
-	_, err := k.KClient.CoreV1().Pods(pod.Namespace).Update(context.TODO(), pod, metav1.UpdateOptions{})
+	_, err := k.KClient.CoreV1().Pods(pod.Namespace).UpdateStatus(context.TODO(), pod, metav1.UpdateOptions{})
 	return err
 }
 

diff --git a/go-controller/pkg/kube/mocks/Interface.go b/go-controller/pkg/kube/mocks/Interface.go
diff --git a/go-controller/pkg/kube/mocks/InterfaceOVN.go b/go-controller/pkg/kube/mocks/InterfaceOVN.go
diff --git a/go-controller/pkg/kubevirt/pod.go b/go-controller/pkg/kubevirt/pod.go
@@ -114,7 +114,7 @@ func EnsurePodAnnotationForVM(watchFactory *factory.WatchFactory, kube *kube.Kub
 				return err
 			}
 		}
-		return kube.UpdatePod(modifiedPod)
+		return kube.UpdatePodStatus(modifiedPod)
 	})
 	if resultErr != nil {
 		return nil, fmt.Errorf("failed to update labels and annotations on pod %s/%s: %v", pod.Namespace, pod.Name, resultErr)

diff --git a/go-controller/pkg/node/base_node_network_controller_dpu_test.go b/go-controller/pkg/node/base_node_network_controller_dpu_test.go
@@ -339,7 +339,7 @@ var _ = Describe("Node DPU tests", func() {
 				podInformer.On("Lister").Return(&podLister)
 				podLister.On("Pods", mock.AnythingOfType("string")).Return(&podNamespaceLister)
 				podNamespaceLister.On("Get", mock.AnythingOfType("string")).Return(&pod, nil)
-				kubeMock.On("UpdatePod", cpod).Return(nil)
+				kubeMock.On("UpdatePodStatus", cpod).Return(nil)
 
 				err = dnnc.addRepPort(&pod, &scd, ifInfo, clientset)
 				Expect(err).ToNot(HaveOccurred())
@@ -368,7 +368,7 @@ var _ = Describe("Node DPU tests", func() {
 				podInformer.On("Lister").Return(&podLister)
 				podLister.On("Pods", mock.AnythingOfType("string")).Return(&podNamespaceLister)
 				podNamespaceLister.On("Get", mock.AnythingOfType("string")).Return(&pod, nil)
-				kubeMock.On("UpdatePod", cpod).Return(fmt.Errorf("failed to set pod annotations"))
+				kubeMock.On("UpdatePodStatus", cpod).Return(fmt.Errorf("failed to set pod annotations"))
 
 				err = dnnc.addRepPort(&pod, &scd, ifInfo, clientset)
 				Expect(err).To(HaveOccurred())

diff --git a/go-controller/pkg/ovn/base_network_controller.go b/go-controller/pkg/ovn/base_network_controller.go
@@ -428,7 +428,9 @@ func (cnci *CommonNetworkControllerInfo) UpdateNodeAnnotationWithRetry(nodeName
 		for k, v := range nodeAnnotations {
 			cnode.Annotations[k] = v
 		}
-		return cnci.kube.UpdateNode(cnode)
+		// It is possible to update the node annotations using status subresource
+		// because changes to metadata via status subresource are not restricted for nodes.
+		return cnci.kube.UpdateNodeStatus(cnode)
 	})
 	if resultErr != nil {
 		return fmt.Errorf("failed to update node %s annotation", nodeName)

diff --git a/go-controller/pkg/ovn/controller/services/node_tracker.go b/go-controller/pkg/ovn/controller/services/node_tracker.go
@@ -217,7 +217,7 @@ func (nt *nodeTracker) removeNode(nodeName string) {
 	delete(nt.nodes, nodeName)
 }
 
-// UpdateNode is called when a node's gateway router / switch / IPs have changed
+// updateNode is called when a node's gateway router / switch / IPs have changed
 // The switch exists when the HostSubnet annotation is set.
 // The gateway router will exist sometime after the L3Gateway annotation is set.
 func (nt *nodeTracker) updateNode(node *v1.Node) {

diff --git a/go-controller/pkg/ovn/topology_version.go b/go-controller/pkg/ovn/topology_version.go
@@ -61,10 +61,9 @@ func (oc *DefaultNetworkController) cleanTopologyAnnotation() error {
 				return err
 			}
 			if _, ok := node.Annotations[anno]; ok {
-				newNode := node.DeepCopy()
-				delete(newNode.Annotations, anno)
 				klog.Infof("Deleting topology annotation from node %s", node.Name)
-				return oc.kube.PatchNode(node, newNode)
+				// Setting the annotation value to nil removes it
+				return oc.kube.SetAnnotationsOnNode(node.Name, map[string]interface{}{anno: nil})
 			}
 			return nil
 		})

diff --git a/go-controller/pkg/util/pod.go b/go-controller/pkg/util/pod.go
@@ -44,7 +44,9 @@ func UpdatePodWithRetryOrRollback(podLister listers.PodLister, kube kube.Interfa
 		}
 
 		updated = true
-		err = kube.UpdatePod(pod)
+		// It is possible to update the pod annotations using status subresource
+		// because changes to metadata via status subresource are not restricted pods.
+		err = kube.UpdatePodStatus(pod)
 		if err != nil && rollback != nil {
 			rollback()
 		}

diff --git a/go-controller/pkg/util/pod_test.go b/go-controller/pkg/util/pod_test.go
@@ -93,9 +93,9 @@ func TestUpdatePodWithAllocationOrRollback(t *testing.T) {
 			}
 
 			if tt.updatePodErr {
-				kubeMock.On("UpdatePod", pod).Return(errors.New("Update pod error"))
+				kubeMock.On("UpdatePodStatus", pod).Return(errors.New("Update pod error"))
 			} else if tt.expectUpdate {
-				kubeMock.On("UpdatePod", pod).Return(nil)
+				kubeMock.On("UpdatePodStatus", pod).Return(nil)
 			}
 
 			err := UpdatePodWithRetryOrRollback(podListerMock, kubeMock, &v1.Pod{}, allocate)