Ensure that UDN updates update NAD annotations correctly

With b05875b we introduced copying the
UDN annotations to the NAD, except for internal annotations
(k8s.ovn.org). However, this patch missed doing this on update events.

Signed-off-by: Tim Rozet <[email protected]>

Author: trozet

go-controller/pkg/clustermanager/userdefinednetwork/controller_helper.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 
 	netv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 
@@ -76,12 +77,24 @@ func (c *Controller) updateNAD(obj client.Object, namespace string) (*netv1.Netw
 		return nil, fmt.Errorf("foreign NetworkAttachmentDefinition with the desired name already exist [%s/%s]", nadCopy.Namespace, nadCopy.Name)
 	}
 
-	if reflect.DeepEqual(nadCopy.Spec.Config, desiredNAD.Spec.Config) && reflect.DeepEqual(nadCopy.ObjectMeta.Labels, desiredNAD.ObjectMeta.Labels) {
+	// NAD update path, need to merge internal (k8s.ovn.org) current annotations with desired
+	for k, v := range nadCopy.Annotations {
+		if strings.HasPrefix(k, types.OvnK8sPrefix) {
+			if desiredNAD.Annotations == nil {
+				desiredNAD.Annotations = make(map[string]string)
+			}
+			desiredNAD.Annotations[k] = v
+		}
+	}
+
+	if reflect.DeepEqual(nadCopy.Spec.Config, desiredNAD.Spec.Config) && reflect.DeepEqual(nadCopy.ObjectMeta.Labels, desiredNAD.ObjectMeta.Labels) &&
+		reflect.DeepEqual(desiredNAD.Annotations, nadCopy.Annotations) {
 		return nadCopy, nil
 	}
 
 	nadCopy.Spec.Config = desiredNAD.Spec.Config
 	nadCopy.ObjectMeta.Labels = desiredNAD.ObjectMeta.Labels
+	nadCopy.Annotations = desiredNAD.Annotations
 	updatedNAD, err := c.nadClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(nadCopy.Namespace).Update(context.Background(), nadCopy, metav1.UpdateOptions{})
 	if err != nil {
 		return nil, fmt.Errorf("failed to update NetworkAttachmentDefinition: %w", err)

go-controller/pkg/clustermanager/userdefinednetwork/controller_test.go

@@ -445,6 +445,63 @@ var _ = Describe("User Defined Network Controller", func() {
 				}
 			})
 
+			It("should update NAD annotations and preserve internal OVNK annotations on UDN update", func() {
+				testNamespaces := []string{"red", "blue"}
+				var objs []runtime.Object
+				for _, nsName := range testNamespaces {
+					objs = append(objs, testNamespace(nsName))
+				}
+				cudn := testClusterUDN("test", testNamespaces...)
+				cudn.Spec.Network = udnv1.NetworkSpec{Topology: udnv1.NetworkTopologyLayer2, Layer2: &udnv1.Layer2Config{
+					Subnets: udnv1.DualStackCIDRs{"10.10.10.0/24"},
+				}}
+				cudn.Annotations = map[string]string{"foo": "bar"}
+
+				objs = append(objs, cudn)
+				networkName := ovntypes.CUDNPrefix + cudn.Name
+				expectedNsNADs := map[string]*netv1.NetworkAttachmentDefinition{}
+				for _, nsName := range testNamespaces {
+					nad := testClusterUdnNAD(cudn.Name, nsName)
+					nadName := nsName + "/" + cudn.Name
+					nad.Spec.Config = `{"cniVersion":"1.0.0","name":"` + networkName + `","netAttachDefName":"` + nadName + `","role":"","subnets":"10.10.10.0/24","topology":"layer2","type":"ovn-k8s-cni-overlay"}`
+					nad.Annotations = map[string]string{
+						"foo":                             "bar",
+						ovntypes.OvnNetworkNameAnnotation: networkName,
+						ovntypes.OvnNetworkIDAnnotation:   "6",
+					}
+					expectedNsNADs[nsName] = nad.DeepCopy()
+					objs = append(objs, nad)
+				}
+
+				c = newTestController(template.RenderNetAttachDefManifest, objs...)
+				Expect(c.Run()).To(Succeed())
+
+				By("updating CUDN with a new annotation")
+				cudn, err := cs.UserDefinedNetworkClient.K8sV1().ClusterUserDefinedNetworks().Get(context.Background(), cudn.Name, metav1.GetOptions{})
+				Expect(err).NotTo(HaveOccurred())
+				updatedCUDN := cudn.DeepCopy()
+				updatedCUDN.Annotations = map[string]string{"foo2": "bar2"}
+				_, err = cs.UserDefinedNetworkClient.K8sV1().ClusterUserDefinedNetworks().Update(context.Background(), updatedCUDN, metav1.UpdateOptions{})
+				Expect(err).NotTo(HaveOccurred())
+
+				for testNamespace, expectedNAD := range expectedNsNADs {
+					expectedNAD.Annotations = map[string]string{
+						"foo2":                            "bar2",
+						ovntypes.OvnNetworkNameAnnotation: networkName,
+						ovntypes.OvnNetworkIDAnnotation:   "6",
+					}
+
+					Eventually(func(g Gomega) {
+						actualNAD, err := cs.NetworkAttchDefClient.K8sCniCncfIoV1().
+							NetworkAttachmentDefinitions(testNamespace).
+							Get(context.Background(), cudn.Name, metav1.GetOptions{})
+						g.Expect(err).NotTo(HaveOccurred())
+						g.Expect(actualNAD).To(Equal(expectedNAD), "NAD should exist, have updated "+
+							"annotations, and preserve internal annotations")
+					}).Should(Succeed())
+				}
+			})
+
 			When("CR exist, and few connected & disconnected namespaces", func() {
 				const (
 					cudnName       = "global-network"