Fix getHostNamespaceAddressesForNode error wrapping

Incorrect wrapping of errors in getHostNamespaceAddressesForNode
prevented callers to assert the type of error returned. In this specific
case the caller delIPFromHostNetworkNamespaceAddrSet could not appropriately
check that the error returned was AnnotationNotSetError and was failing
a deletion flow that should be noop instead.

This was triggered by [1] changes that cause the deletion flow to depend on
annotations set by different controllers instead of annotations set
atomically by a single controller, making it possible that some
annotations were set but not others and opening up a new error handling
flow that wasn't possible before. The scenario is a node that is added
and deleted quickly enough for some controller in cluster manager to be
able to annotate the node and some not.

[1] ovn-kubernetes/ovn-kubernetes#5396

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Author: jcaamano

go-controller/pkg/ovn/master_test.go

@@ -1566,45 +1566,62 @@ var _ = ginkgo.Describe("Default network controller operations", func() {
 		gomega.Expect(err).NotTo(gomega.HaveOccurred())
 	})
 
-	ginkgo.It("doesn't retry deleting a node that doesn't have a node-subnet annotation", func() {
-		app.Action = func(ctx *cli.Context) error {
-			_, err := config.InitConfig(ctx, nil, nil)
+	ginkgo.DescribeTable("doesn't retry deleting a node that is missing annotation",
+		func(node *corev1.Node) {
+			app.Action = func(ctx *cli.Context) error {
+				_, err := config.InitConfig(ctx, nil, nil)
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				startFakeController(oc, wg)
+				ginkgo.By("create new node with no annotation defined and ensure there's a retry entry")
+				_, err = kubeFakeClient.CoreV1().Nodes().Create(context.TODO(), node, metav1.CreateOptions{})
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				retry.CheckRetryObjectMultipleFieldsEventually(
+					node.Name,
+					oc.retryNodes,
+					gomega.BeNil(),             // oldObj should be nil
+					gomega.Not(gomega.BeNil()), // newObj should not be nil
+				)
+				keyExists := true
+				retry.CheckRetryObjectEventually(node.Name, keyExists, oc.retryNodes)
+				ginkgo.By("delete node and check that there are no retries for the deleted node")
+				err = kubeFakeClient.CoreV1().Nodes().Delete(context.TODO(), node.Name, metav1.DeleteOptions{})
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				retry.CheckRetryObjectEventually(node.Name, !keyExists, oc.retryNodes)
+				ginkgo.By("ensure failures for sync host network address or adding nodes are cleared")
+				gomega.Eventually(func() bool {
+					_, foundSyncHostNetAddrSetFailed := oc.syncHostNetAddrSetFailed.Load(node.Name)
+					_, foundAddNodeFailed := oc.addNodeFailed.Load(node.Name)
+					return foundSyncHostNetAddrSetFailed || foundAddNodeFailed
+				}).Should(gomega.BeFalse())
+				return nil
+			}
+			err := app.Run([]string{
+				app.Name,
+			})
 			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			startFakeController(oc, wg)
-			newNode := &corev1.Node{
+
+		},
+		ginkgo.Entry("k8s.ovn.org/node-subnets",
+			&corev1.Node{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:        "newNode",
-					Annotations: map[string]string{},
+					Name: "newNode",
+					Annotations: map[string]string{
+						"k8s.ovn.org/node-id": "2",
+					},
 				},
-			}
-			ginkgo.By("create new node with no host-subnet annotation defined and ensure theres a retry entry")
-			_, err = kubeFakeClient.CoreV1().Nodes().Create(context.TODO(), newNode, metav1.CreateOptions{})
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			retry.CheckRetryObjectMultipleFieldsEventually(
-				newNode.Name,
-				oc.retryNodes,
-				gomega.BeNil(),             // oldObj should be nil
-				gomega.Not(gomega.BeNil()), // newObj should not be nil
-			)
-			keyExists := true
-			retry.CheckRetryObjectEventually(newNode.Name, keyExists, oc.retryNodes)
-			ginkgo.By("delete node and check that there are no retries for the deleted node")
-			err = kubeFakeClient.CoreV1().Nodes().Delete(context.TODO(), newNode.Name, metav1.DeleteOptions{})
-			gomega.Expect(err).NotTo(gomega.HaveOccurred())
-			retry.CheckRetryObjectEventually(newNode.Name, !keyExists, oc.retryNodes)
-			ginkgo.By("ensure failures for sync host network address or adding nodes are cleared")
-			gomega.Eventually(func() bool {
-				_, foundSyncHostNetAddrSetFailed := oc.syncHostNetAddrSetFailed.Load(newNode.Name)
-				_, foundAddNodeFailed := oc.addNodeFailed.Load(newNode.Name)
-				return foundSyncHostNetAddrSetFailed || foundAddNodeFailed
-			}).Should(gomega.BeFalse())
-			return nil
-		}
-		err := app.Run([]string{
-			app.Name,
-		})
-		gomega.Expect(err).NotTo(gomega.HaveOccurred())
-	})
+			},
+		),
+		ginkgo.Entry("k8s.ovn.org/node-id",
+			&corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "newNode",
+					Annotations: map[string]string{
+						"k8s.ovn.org/node-subnets": "{\"default\": [\"10.130.0.0/23\", \"fd01:0:0:2::/64\"]}",
+					},
+				},
+			},
+		),
+	)
 
 	ginkgo.It("delete a partially constructed node", func() {
 		app.Action = func(ctx *cli.Context) error {

go-controller/pkg/ovn/namespace.go

@@ -376,16 +376,17 @@ func (oc *DefaultNetworkController) getHostNamespaceAddressesForNode(node *corev
 	}
 	// for shared gateway mode we will use LRP IPs to SNAT host network traffic
 	// so add these to the address set.
-	lrpIPs, err := udn.GetGWRouterIPs(node, oc.GetNetInfo())
-	if err != nil {
-		if util.IsAnnotationNotSetError(err) {
-			// FIXME(tssurya): This is present for backwards compatibility
-			// Remove me a few months from now
-			var err1 error
-			lrpIPs, err1 = util.ParseNodeGatewayRouterLRPAddrs(node)
-			if err1 != nil {
-				return nil, fmt.Errorf("failed to get join switch port IP address for node %s: %v/%v", node.Name, err, err1)
-			}
+	lrpIPs, gwIPsErr := udn.GetGWRouterIPs(node, oc.GetNetInfo())
+	if gwIPsErr != nil {
+		if !util.IsAnnotationNotSetError(gwIPsErr) {
+			return nil, gwIPsErr
+		}
+		// FIXME(tssurya): This is present for backwards compatibility
+		// Remove me a few months from now
+		var lrpAddrsErr error
+		lrpIPs, lrpAddrsErr = util.ParseNodeGatewayRouterLRPAddrs(node)
+		if lrpAddrsErr != nil {
+			return nil, fmt.Errorf("failed to fallback to annotations after error %q: %w", gwIPsErr, lrpAddrsErr)
 		}
 	}