diff --git a/pkg/monitor/azure/nsg/nsg.go b/pkg/monitor/azure/nsg/nsg.go index 4dc8b0ec34c..b2d4c66e26c 100644 --- 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 index f4a1dc3cc20..5e0dbad31ef 100644 --- 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)