Improve NSG monitoring by retrieving subnet only once (#3324)

* Improve NSG monitoring by retrieving just once * Skip wp profiles if count is 0
Azure · Jan 11, 2024 · aee18fe · aee18fe
1 parent 5068e9d
commit aee18fe
Show file tree

Hide file tree

Showing 2 changed files with 108 additions and 13 deletions.
diff --git a/pkg/monitor/azure/nsg/nsg.go b/pkg/monitor/azure/nsg/nsg.go
@@ -109,8 +109,10 @@ func (n *NSGMonitor) toSubnetConfig(ctx context.Context, subnetID string) (subne
 	return subnetNSGConfig{prefixes, subnet.Properties.NetworkSecurityGroup}, nil
 }
 
+// Monitor checks the custom NSGs customers attach to their ARO subnets
 func (n *NSGMonitor) Monitor(ctx context.Context) []error {
 	defer n.wg.Done()
+
 	masterSubnet, err := n.toSubnetConfig(ctx, n.oc.Properties.MasterProfile.SubnetID)
 	if err != nil {
 		// FP has no access to the subnet
@@ -121,12 +123,21 @@ func (n *NSGMonitor) Monitor(ctx context.Context) []error {
 	workerProfiles, _ := api.GetEnrichedWorkerProfiles(n.oc.Properties)
 	workerSubnets := make([]subnetNSGConfig, 0, len(workerProfiles))
 	workerPrefixes := make([]netip.Prefix, 0, len(workerProfiles))
+	subnetSet := map[string]struct{}{}
 	for _, wp := range workerProfiles {
 		// Customer can configure a machineset with an invalid subnet.
 		// In such case, the subnetID will be empty.
-		if len(wp.SubnetID) == 0 {
+		//
+		// We do not need to consider profiles with 0 machines.
+		if len(wp.SubnetID) == 0 || wp.Count == 0 {
+			continue
+		}
+
+		// Many profiles can have the same subnet ID.  To minimize the possibility of throttling, we only get it once.
+		if _, ok := subnetSet[wp.SubnetID]; ok {
 			continue
 		}
+		subnetSet[wp.SubnetID] = struct{}{}
 
 		s, err := n.toSubnetConfig(ctx, wp.SubnetID)
 		if err != nil {

diff --git a/pkg/monitor/azure/nsg/nsg_test.go b/pkg/monitor/azure/nsg/nsg_test.go
@@ -80,7 +80,20 @@ var (
 	ocClusterName = "testing"
 	ocID          = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenShift/OpenShiftClusters/%s", subscriptionID, resourcegroupName, ocClusterName)
 	ocLocation    = "eastus"
-	oc            = api.OpenShiftCluster{
+
+	nsg1Name = "NSG-1"
+	nsg1ID   = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourcegroupName, nsg1Name)
+	nsg2Name = "NSG-2"
+	nsg2ID   = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourcegroupName, nsg2Name)
+
+	nsgAllow    = armnetwork.SecurityRuleAccessAllow
+	nsgDeny     = armnetwork.SecurityRuleAccessDeny
+	nsgInbound  = armnetwork.SecurityRuleDirectionInbound
+	nsgOutbound = armnetwork.SecurityRuleDirectionOutbound
+)
+
+func ocFactory() api.OpenShiftCluster {
+	return api.OpenShiftCluster{
 		ID:       ocID,
 		Location: ocLocation,
 		Properties: api.OpenShiftClusterProperties{
@@ -90,27 +103,20 @@ var (
 			WorkerProfiles: []api.WorkerProfile{
 				{
 					SubnetID: workerSubnet1ID,
+					Count:    1,
 				},
 				{
 					SubnetID: "", // This should still work. Customers can create a faulty MachineSet.
+					Count:    1,
 				},
 				{
 					SubnetID: workerSubnet2ID,
+					Count:    1,
 				},
 			},
 		},
 	}
-
-	nsg1Name = "NSG-1"
-	nsg1ID   = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourcegroupName, nsg1Name)
-	nsg2Name = "NSG-2"
-	nsg2ID   = fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkSecurityGroups/%s", subscriptionID, resourcegroupName, nsg2Name)
-
-	nsgAllow    = armnetwork.SecurityRuleAccessAllow
-	nsgDeny     = armnetwork.SecurityRuleAccessDeny
-	nsgInbound  = armnetwork.SecurityRuleDirectionInbound
-	nsgOutbound = armnetwork.SecurityRuleDirectionOutbound
-)
+}
 
 func createBaseSubnets() (armnetwork.SubnetsClientGetResponse, armnetwork.SubnetsClientGetResponse, armnetwork.SubnetsClientGetResponse) {
 	resp := make([]armnetwork.SubnetsClientGetResponse, 0, 3)
@@ -173,6 +179,7 @@ func TestMonitor(t *testing.T) {
 		name        string
 		mockSubnet  func(*mock_armnetwork.MockSubnetsClient)
 		mockEmitter func(*mock_metrics.MockEmitter)
+		modOC       func(*api.OpenShiftCluster)
 		wantErr     string
 	}{
 		{
@@ -241,6 +248,79 @@ func TestMonitor(t *testing.T) {
 				gomock.InOrder(_1, _2, _3)
 			},
 		},
+		{
+			name: "pass - no rules, 3 workerprofiles have the same subnetID, subnet should be retrieved once",
+			mockSubnet: func(mock *mock_armnetwork.MockSubnetsClient) {
+				masterSubnet, workerSubnet, _ := createBaseSubnets()
+				nsg := armnetwork.SecurityGroup{
+					ID: &nsg1ID,
+					Properties: &armnetwork.SecurityGroupPropertiesFormat{
+						SecurityRules: []*armnetwork.SecurityRule{},
+					},
+				}
+				masterSubnet.Properties.NetworkSecurityGroup = &nsg
+				workerSubnet.Properties.NetworkSecurityGroup = &nsg
+
+				_1 := mock.EXPECT().
+					Get(ctx, resourcegroupName, vNetName, masterSubnetName, options).
+					Return(masterSubnet, nil)
+
+				_2 := mock.EXPECT().
+					Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, options).
+					Return(workerSubnet, nil)
+				gomock.InOrder(_1, _2)
+			},
+			modOC: func(oc *api.OpenShiftCluster) {
+				oc.Properties.WorkerProfiles = []api.WorkerProfile{
+					{
+						SubnetID: workerSubnet1ID,
+						Count:    1,
+					},
+					{
+						SubnetID: workerSubnet1ID,
+						Count:    1,
+					},
+					{
+						SubnetID: workerSubnet1ID,
+						Count:    1,
+					},
+				}
+			},
+		}, {
+			name: "pass - no rules, 0 count profiles are not checked",
+			mockSubnet: func(mock *mock_armnetwork.MockSubnetsClient) {
+				masterSubnet, workerSubnet, _ := createBaseSubnets()
+				nsg := armnetwork.SecurityGroup{
+					ID: &nsg1ID,
+					Properties: &armnetwork.SecurityGroupPropertiesFormat{
+						SecurityRules: []*armnetwork.SecurityRule{},
+					},
+				}
+				masterSubnet.Properties.NetworkSecurityGroup = &nsg
+				workerSubnet.Properties.NetworkSecurityGroup = &nsg
+
+				_1 := mock.EXPECT().
+					Get(ctx, resourcegroupName, vNetName, masterSubnetName, options).
+					Return(masterSubnet, nil)
+
+				_2 := mock.EXPECT().
+					Get(ctx, resourcegroupName, vNetName, workerSubnet1Name, options).
+					Return(workerSubnet, nil)
+				gomock.InOrder(_1, _2)
+			},
+			modOC: func(oc *api.OpenShiftCluster) {
+				oc.Properties.WorkerProfiles = []api.WorkerProfile{
+					{
+						SubnetID: workerSubnet1ID,
+						Count:    1,
+					},
+					{
+						SubnetID: "NOT BEING CHECKED",
+						Count:    0,
+					},
+				}
+			},
+		},
 		{
 			name: "pass - no rules",
 			mockSubnet: func(mock *mock_armnetwork.MockSubnetsClient) {
@@ -449,6 +529,10 @@ func TestMonitor(t *testing.T) {
 			if tt.mockEmitter != nil {
 				tt.mockEmitter(emitter)
 			}
+			oc := ocFactory()
+			if tt.modOC != nil {
+				tt.modOC(&oc)
+			}
 
 			var wg sync.WaitGroup
 			n := NewNSGMonitor(logrus.NewEntry(logrus.New()), &oc, subscriptionID, subnetClient, emitter, &wg)