fix: overlay should check NNC label (#1707)

* fix: overlay should check NNC label

* Handle more Overlay CNI edge cases

- GetSubnet will retry on errors.
- Subnet CIDR can be read from address prefix or address prefixes.
- Extra unit tests.

---------

Co-authored-by: Sean Jeffrey <seanjeffrey@microsoft.com>

Author: tyler-lloyd

pkg/azure/client.go

@@ -296,10 +296,15 @@ func (az *azClient) ApplyRouteTable(subnetID string, routeTableID string) error
 	return nil
 }
 
-func (az *azClient) GetSubnet(subnetID string) (n.Subnet, error) {
-	_, subnetResourceGroup, subnetVnetName, subnetName := ParseSubResourceID(subnetID)
-	subnet, err := az.subnetsClient.Get(az.ctx, string(subnetResourceGroup), string(subnetVnetName), string(subnetName), "")
-	return subnet, err
+func (az *azClient) GetSubnet(subnetID string) (subnet n.Subnet, err error) {
+	_ = utils.Retry(retryCount, retryPause,
+		func() (utils.Retriable, error) {
+			_, subnetResourceGroup, subnetVnetName, subnetName := ParseSubResourceID(subnetID)
+			subnet, err = az.subnetsClient.Get(az.ctx, string(subnetResourceGroup), string(subnetVnetName), string(subnetName), "")
+			return utils.Retriable(true), err
+		})
+
+	return
 }
 
 // DeployGatewayWithVnet creates Application Gateway within the specifid VNet. Implements AzClient interface.

pkg/cni/overlay.go

@@ -9,7 +9,7 @@ import (
 	overlayextensionconfig_v1alpha1 "github.com/Azure/azure-container-networking/crd/overlayextensionconfig/api/v1alpha1"
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	meta "k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/api/meta"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -22,6 +22,9 @@ const (
 )
 
 const (
+	// PodNetworkTypeLabel is the name of the label on NNCs to tell what mode the network is in.
+	PodNetworkTypeLabel = "kubernetes.azure.com/podnetwork-type"
+
 	// OverlayExtensionConfigName is the name of the overlay extension config resource
 	OverlayExtensionConfigName = "agic-overlay-extension-config"
 
@@ -52,7 +55,16 @@ func (r *Reconciler) reconcileOverlayCniIfNeeded(ctx context.Context, subnetID s
 		return errors.Wrap(err, "failed to get subnet")
 	}
 
-	subnetCIDR := *subnet.AddressPrefix
+	var subnetCIDR string
+	if subnet.AddressPrefix != nil {
+		subnetCIDR = *subnet.AddressPrefix
+	} else if subnet.AddressPrefixes != nil && len(*subnet.AddressPrefixes) > 0 {
+		subnetCIDR = (*subnet.AddressPrefixes)[0]
+	} else {
+		return errors.New("subnet does not have an address prefix(es)")
+	}
+
+	klog.Infof("Using subnet prefix %q", subnetCIDR)
 	err = r.reconcileOverlayExtensionConfig(ctx, subnetCIDR)
 	if err != nil {
 		return errors.Wrap(err, "failed to reconcile overlay resources")
@@ -72,7 +84,13 @@ func (r *Reconciler) isClusterOverlayCNI(ctx context.Context) (bool, error) {
 		return false, errors.Wrap(err, "failed to list node network configs")
 	}
 
-	return len(nodeNetworkConfigs.Items) > 0, nil
+	// if any NNCs are overlay then this cluster is using CNI Overlay
+	for _, nnc := range nodeNetworkConfigs.Items {
+		if val, ok := nnc.Labels[PodNetworkTypeLabel]; ok && val == "overlay" {
+			return true, nil
+		}
+	}
+	return false, nil
 }
 
 func (r *Reconciler) reconcileOverlayExtensionConfig(ctx context.Context, subnetCIDR string) error {

pkg/cni/overlay_test.go

@@ -92,12 +92,36 @@ var _ = Describe("Overlay CNI", func() {
 		}, appGw, agicPod, namespace, false)
 	})
 
+	Context("NodeNetworkConfigList does not have the overlay label", func() {
+		It("returns nil error", func() {
+			// Create NodeNetworkConfigList so that cluster is not considered as overlay CNI
+			config := &nodenetworkconfig_v1alpha.NodeNetworkConfig{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-node-network-config",
+					Namespace: namespace,
+					Labels: map[string]string{
+						cni.PodNetworkTypeLabel: "not overlay",
+					},
+				},
+				Spec: nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
+			}
+			Expect(k8sClient.Create(ctx, config)).To(BeNil())
+			Expect(reconciler.Reconcile(ctx)).To(BeNil())
+		})
+	})
+
 	Context("Handle Overlay CNI cluster", func() {
 		BeforeEach(func() {
 			// Create NodeNetworkConfig so that cluster is considered as overlay CNI
 			config := &nodenetworkconfig_v1alpha.NodeNetworkConfig{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-node-network-config", Namespace: namespace},
-				Spec:       nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-node-network-config",
+					Namespace: namespace,
+					Labels: map[string]string{
+						cni.PodNetworkTypeLabel: "overlay",
+					},
+				},
+				Spec: nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
 			}
 			Expect(k8sClient.Create(ctx, config)).To(BeNil())
 
@@ -149,6 +173,38 @@ var _ = Describe("Overlay CNI", func() {
 			Expect(err.Error()).To(ContainSubstring("failed to get subnet"))
 		})
 
+		It("should return error if subnet does not have an address prefix or address prefixes", func() {
+			azClient.GetSubnetFunc = func(subnetID string) (n.Subnet, error) {
+				return n.Subnet{
+					SubnetPropertiesFormat: &n.SubnetPropertiesFormat{
+						AddressPrefix:   nil,
+						AddressPrefixes: &[]string{},
+					}}, nil
+			}
+
+			err := reconciler.Reconcile(ctx)
+			Expect(err).To(Not(BeNil()))
+			Expect(err.Error()).To(ContainSubstring("subnet does not have an address prefix(es)"))
+		})
+
+		It("should not return error if subnet has an entry in address prefixes", func() {
+			azClient.GetSubnetFunc = func(subnetID string) (n.Subnet, error) {
+				return n.Subnet{
+					SubnetPropertiesFormat: &n.SubnetPropertiesFormat{
+						AddressPrefix:   nil,
+						AddressPrefixes: &[]string{"CIDR1"},
+					}}, nil
+			}
+
+			err := reconciler.Reconcile(ctx)
+			Expect(err).To(Not(HaveOccurred()))
+
+			var config overlayextensionconfig_v1alpha1.OverlayExtensionConfig
+			Expect(k8sClient.Get(ctx, ctrl_client.ObjectKey{Name: cni.OverlayExtensionConfigName, Namespace: namespace}, &config)).To(BeNil())
+			Expect(config.Labels).To(HaveLen(1))
+			Expect(config.Spec.ExtensionIPRange).To(Equal("CIDR1"))
+		})
+
 		It("should delete and recreate OEC if subnet CIDR changes", func() {
 			// Create OverlayExtensionConfig
 			Expect(k8sClient.Create(ctx, &overlayextensionconfig_v1alpha1.OverlayExtensionConfig{
@@ -197,8 +253,10 @@ var _ = Describe("Overlay CNI", func() {
 
 			// Create NodeNetworkConfig so that cluster is considered as overlay CNI
 			Expect(k8sClient.Create(ctx, &nodenetworkconfig_v1alpha.NodeNetworkConfig{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-node-network-config", Namespace: namespace},
-				Spec:       nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
+				ObjectMeta: metav1.ObjectMeta{Name: "test-node-network-config", Namespace: namespace, Labels: map[string]string{
+					cni.PodNetworkTypeLabel: "overlay",
+				}},
+				Spec: nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
 			})).To(BeNil())
 
 			Expect(reconciler.Reconcile(ctx)).To(BeNil())
@@ -216,8 +274,10 @@ var _ = Describe("Overlay CNI", func() {
 
 			// Create NodeNetworkConfig so that cluster is considered as overlay CNI
 			Expect(k8sClient.Create(ctx, &nodenetworkconfig_v1alpha.NodeNetworkConfig{
-				ObjectMeta: metav1.ObjectMeta{Name: "test-node-network-config", Namespace: namespace},
-				Spec:       nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
+				ObjectMeta: metav1.ObjectMeta{Name: "test-node-network-config", Namespace: namespace, Labels: map[string]string{
+					cni.PodNetworkTypeLabel: "overlay",
+				}},
+				Spec: nodenetworkconfig_v1alpha.NodeNetworkConfigSpec{},
 			})).To(BeNil())
 
 			go func() {