Merge pull request #5584 from alimaazamat/immutable-empty-CIDRBlocks-bug

Only update the subnet spec if status ASO CIDRs not empty

Author: k8s-ci-robot

azure/services/subnets/subnets.go

@@ -56,8 +56,11 @@ func postCreateOrUpdateResourceHook(_ context.Context, scope SubnetScope, subnet
 	}
 
 	name := subnet.AzureName()
-	scope.UpdateSubnetID(name, ptr.Deref(subnet.Status.Id, ""))
-	scope.UpdateSubnetCIDRs(name, converters.GetSubnetAddresses(*subnet))
+	statusASOCIDRs := converters.GetSubnetAddresses(*subnet)
+	if len(statusASOCIDRs) != 0 {
+		scope.UpdateSubnetID(name, ptr.Deref(subnet.Status.Id, ""))
+		scope.UpdateSubnetCIDRs(name, statusASOCIDRs)
+	}
 
 	return nil
 }

azure/services/subnets/subnets_test.go

@@ -55,4 +55,36 @@ func TestPostCreateOrUpdateResourceHook(t *testing.T) {
 		}
 		g.Expect(postCreateOrUpdateResourceHook(context.Background(), scope, subnet, nil)).To(Succeed())
 	})
+
+	t.Run("correctly handles empty and non-empty ASO Status CIDRBlocks", func(t *testing.T) {
+		g := NewGomegaWithT(t)
+		mockCtrl := gomock.NewController(t)
+		scope := mock_subnets.NewMockSubnetScope(mockCtrl)
+
+		emptyCIDRSubnet := &asonetworkv1.VirtualNetworksSubnet{
+			Spec: asonetworkv1.VirtualNetworksSubnet_Spec{
+				AzureName: "empty-cidr-status-subnet",
+			},
+			Status: asonetworkv1.VirtualNetworksSubnet_STATUS{
+				Id:              ptr.To("id-empty"),
+				AddressPrefixes: []string{},
+			},
+		}
+		scope.EXPECT().UpdateSubnetID("empty-cidr-status-subnet", "id-empty").Times(0)
+		scope.EXPECT().UpdateSubnetCIDRs("empty-cidr-status-subnet", []string{}).Times(0)
+		g.Expect(postCreateOrUpdateResourceHook(context.Background(), scope, emptyCIDRSubnet, nil)).To(Succeed())
+
+		nonEmptyCIDRSubnet := &asonetworkv1.VirtualNetworksSubnet{
+			Spec: asonetworkv1.VirtualNetworksSubnet_Spec{
+				AzureName: "nonempty-cidr-status-subnet",
+			},
+			Status: asonetworkv1.VirtualNetworksSubnet_STATUS{
+				Id:              ptr.To("id-nonempty"),
+				AddressPrefixes: []string{"cidr"},
+			},
+		}
+		scope.EXPECT().UpdateSubnetID("nonempty-cidr-status-subnet", "id-nonempty").Times(1)
+		scope.EXPECT().UpdateSubnetCIDRs("nonempty-cidr-status-subnet", []string{"cidr"}).Times(1)
+		g.Expect(postCreateOrUpdateResourceHook(context.Background(), scope, nonEmptyCIDRSubnet, nil)).To(Succeed())
+	})
 }

azure/services/virtualnetworks/virtualnetworks.go

@@ -71,7 +71,10 @@ func postCreateOrUpdateResourceHook(ctx context.Context, scope VNetScope, existi
 		return errors.Wrap(err, "failed to list subnets")
 	}
 	for _, subnet := range subnets.Items {
-		scope.UpdateSubnetCIDRs(subnet.AzureName(), converters.GetSubnetAddresses(subnet))
+		statusASOCIDRs := converters.GetSubnetAddresses(subnet)
+		if len(statusASOCIDRs) != 0 {
+			scope.UpdateSubnetCIDRs(subnet.AzureName(), statusASOCIDRs)
+		}
 	}
 	// Only update the vnet's CIDRBlocks when we also updated subnets' since the vnet is created before
 	// subnets to prevent an updated vnet CIDR from invalidating subnet CIDRs that were defaulted and do not

azure/services/virtualnetworks/virtualnetworks_test.go

@@ -109,4 +109,67 @@ func TestPostCreateOrUpdateResourceHook(t *testing.T) {
 		g.Expect(vnet.Tags).To(Equal(infrav1.Tags{"actual": "tags"}))
 		g.Expect(vnet.CIDRBlocks).To(Equal([]string{"cidr"}))
 	})
+
+	t.Run("correctly handles empty and non-empty ASO Status CIDRBlocks", func(t *testing.T) {
+		g := NewGomegaWithT(t)
+		mockCtrl := gomock.NewController(t)
+		scope := mock_virtualnetworks.NewMockVNetScope(mockCtrl)
+
+		existing := &asonetworkv1.VirtualNetwork{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "vnet",
+			},
+			Status: asonetworkv1.VirtualNetwork_STATUS{
+				Id:   ptr.To("id"),
+				Tags: map[string]string{"actual": "tags"},
+				AddressSpace: &asonetworkv1.AddressSpace_STATUS{
+					AddressPrefixes: []string{"cidr"},
+				},
+			},
+		}
+
+		vnet := &infrav1.VnetSpec{}
+		scope.EXPECT().Vnet().Return(vnet)
+
+		subnets := []client.Object{
+			&asonetworkv1.VirtualNetworksSubnet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "empty-cidr-status-subnet",
+					Labels: map[string]string{
+						labels.OwnerNameLabel: existing.Name,
+					},
+				},
+				Spec: asonetworkv1.VirtualNetworksSubnet_Spec{
+					AzureName: "empty-cidr-status-subnet",
+				},
+				Status: asonetworkv1.VirtualNetworksSubnet_STATUS{
+					AddressPrefixes: []string{},
+				},
+			},
+			&asonetworkv1.VirtualNetworksSubnet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "nonempty-cidr-status-subnet",
+					Labels: map[string]string{
+						labels.OwnerNameLabel: existing.Name,
+					},
+				},
+				Spec: asonetworkv1.VirtualNetworksSubnet_Spec{
+					AzureName: "nonempty-cidr-status-subnet",
+				},
+				Status: asonetworkv1.VirtualNetworksSubnet_STATUS{
+					AddressPrefixes: []string{"cidr"},
+				},
+			},
+		}
+		s := runtime.NewScheme()
+		g.Expect(asonetworkv1.AddToScheme(s)).To(Succeed())
+		c := fakeclient.NewClientBuilder().
+			WithScheme(s).
+			WithObjects(subnets...).
+			Build()
+		scope.EXPECT().GetClient().Return(c)
+		scope.EXPECT().UpdateSubnetCIDRs("empty-cidr-status-subnet", []string{}).Times(0)
+		scope.EXPECT().UpdateSubnetCIDRs("nonempty-cidr-status-subnet", []string{"cidr"}).Times(1)
+		g.Expect(postCreateOrUpdateResourceHook(context.Background(), scope, existing, nil)).To(Succeed())
+	})
 }