Fix NSG e2e error

pkg/validate/dynamic/dynamic.go

@@ -38,6 +38,7 @@ var (
 	errMsgOriginalNSGNotAttached            = "The provided subnet '%s' is invalid: must have network security group '%s' attached."
 	errMsgNSGNotAttached                    = "The provided subnet '%s' is invalid: must have a network security group attached."
 	errMsgNSGNotProperlyAttached            = "When the enable-preconfigured-nsg option is specified, both the master and worker subnets should have network security groups (NSG) attached to them before starting the cluster installation."
+	errMsgSPHasNoRequiredPermissionsOnNSG   = "The %s service principal (Application ID: %s) does not have Network Contributor role on network security group '%s'. This is required when the enable-preconfigured-nsg option is specified."
 	errMsgSubnetNotFound                    = "The provided subnet '%s' could not be found."
 	errMsgSubnetNotInSucceededState         = "The provided subnet '%s' is not in a Succeeded state"
 	errMsgSubnetInvalidSize                 = "The provided subnet '%s' is invalid: must be /27 or larger."
@@ -74,6 +75,7 @@ type Dynamic interface {
 	ValidateDiskEncryptionSets(ctx context.Context, oc *api.OpenShiftCluster) error
 	ValidateEncryptionAtHost(ctx context.Context, oc *api.OpenShiftCluster) error
 	ValidateLoadBalancerProfile(ctx context.Context, oc *api.OpenShiftCluster) error
+	ValidatePreConfiguredNSGs(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error
 }
 
 type dynamic struct {
@@ -804,6 +806,65 @@ func validateSubnetSize(s Subnet, address string) error {
 	return nil
 }
 
+func (dv *dynamic) ValidatePreConfiguredNSGs(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error {
+	dv.log.Print("ValidatePreConfiguredNSGs")
+
+	if oc.Properties.NetworkProfile.PreconfiguredNSG != api.PreconfiguredNSGEnabled {
+		return nil // exit early
+	}
+
+	subnetByID, err := dv.createSubnetMapByID(ctx, subnets)
+	if err != nil {
+		return err
+	}
+
+	for _, s := range subnetByID {
+		nsgID := s.NetworkSecurityGroup.ID
+		if nsgID == nil || *nsgID == "" {
+			return api.NewCloudError(
+				http.StatusBadRequest,
+				api.CloudErrorCodeNotFound,
+				"",
+				errMsgNSGNotProperlyAttached,
+			)
+		}
+
+		if err := dv.validateNSGPermissions(ctx, *nsgID); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (dv *dynamic) validateNSGPermissions(ctx context.Context, nsgID string) error {
+	nsg, err := azure.ParseResourceID(nsgID)
+	if err != nil {
+		return err
+	}
+
+	err = dv.validateActions(ctx, &nsg, []string{
+		"Microsoft.Network/networkSecurityGroups/join/action",
+	})
+
+	if err == wait.ErrWaitTimeout {
+		errCode := api.CloudErrorCodeInvalidResourceProviderPermissions
+		if dv.authorizerType == AuthorizerClusterServicePrincipal {
+			errCode = api.CloudErrorCodeInvalidServicePrincipalPermissions
+		}
+		return api.NewCloudError(
+			http.StatusBadRequest,
+			errCode,
+			"",
+			errMsgSPHasNoRequiredPermissionsOnNSG,
+			dv.authorizerType,
+			dv.appID,
+			nsgID,
+		)
+	}
+
+	return err
+}
+
 func isTheSameNSG(found, inDB string) bool {
 	return strings.EqualFold(found, inDB)
 }

pkg/validate/dynamic/dynamic_test.go

@@ -39,6 +39,7 @@ var (
 	masterSubnet      = vnetID + "/subnet/masterSubnet"
 	workerSubnet      = vnetID + "/subnet/workerSubnet"
 	masterSubnetPath  = "properties.masterProfile.subnetId"
+	workerSubnetPath  = "properties.workerProfile.subnetId"
 	masterRtID        = resourceGroupID + "/providers/Microsoft.Network/routeTables/masterRt"
 	workerRtID        = resourceGroupID + "/providers/Microsoft.Network/routeTables/workerRt"
 	masterNgID        = resourceGroupID + "/providers/Microsoft.Network/natGateways/masterNg"
@@ -1126,6 +1127,7 @@ var (
 func mockTokenCredential(tokenCred *mock_azcore.MockTokenCredential) {
 	tokenCred.EXPECT().
 		GetToken(gomock.Any(), gomock.Any()).
+		AnyTimes().
 		Return(mockAccessToken, nil)
 }
 
@@ -1729,6 +1731,173 @@ func TestCheckBYONsg(t *testing.T) {
 	}
 }
 
+var (
+	canJoinNSG = remotepdp.AuthorizationDecisionResponse{
+		Value: []remotepdp.AuthorizationDecision{
+			{
+				ActionId:       "Microsoft.Network/networkSecurityGroups/join/action",
+				AccessDecision: remotepdp.Allowed},
+		},
+	}
+
+	cannotJoinNSG = remotepdp.AuthorizationDecisionResponse{
+		Value: []remotepdp.AuthorizationDecision{
+			{
+				ActionId:       "Microsoft.Network/networkSecurityGroups/join/action",
+				AccessDecision: remotepdp.NotAllowed},
+		},
+	}
+)
+
+func TestValidatePreconfiguredNSGPermissions(t *testing.T) {
+	ctx := context.Background()
+	for _, tt := range []struct {
+		name             string
+		modifyOC         func(*api.OpenShiftCluster)
+		checkAccessMocks func(context.CancelFunc, *mock_remotepdp.MockRemotePDPClient, *mock_azcore.MockTokenCredential)
+		vnetMocks        func(*mock_network.MockVirtualNetworksClient, mgmtnetwork.VirtualNetwork)
+		wantErr          string
+	}{
+		{
+			name: "pass: skip when preconfiguredNSG is not enabled",
+			modifyOC: func(oc *api.OpenShiftCluster) {
+				oc.Properties.NetworkProfile.PreconfiguredNSG = api.PreconfiguredNSGDisabled
+			},
+		},
+		{
+			name: "fail: sp doesn't have the permission on all NSGs",
+			modifyOC: func(oc *api.OpenShiftCluster) {
+				oc.Properties.NetworkProfile.PreconfiguredNSG = api.PreconfiguredNSGEnabled
+			},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					AnyTimes().
+					Return(vnet, nil)
+			},
+			checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) {
+				mockTokenCredential(tokenCred)
+				pdpClient.EXPECT().CheckAccess(gomock.Any(), gomock.Any()).
+					DoAndReturn(func(_ context.Context, authReq remotepdp.AuthorizationRequest) (*remotepdp.AuthorizationDecisionResponse, error) {
+						cancel() // wait.PollImmediateUntil will always be invoked at least once
+						switch authReq.Resource.Id {
+						case masterNSGv1:
+							return &canJoinNSG, nil
+						case workerNSGv1:
+							return &cannotJoinNSG, nil
+						}
+						return &cannotJoinNSG, nil
+					},
+					).AnyTimes()
+			},
+			wantErr: "400: InvalidServicePrincipalPermissions: : The cluster service principal (Application ID: fff51942-b1f9-4119-9453-aaa922259eb7) does not have Network Contributor role on network security group '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/networkSecurityGroups/aro-node-nsg'. This is required when the enable-preconfigured-nsg option is specified.",
+		},
+		{
+			name: "pass: sp has the required permission on the NSG",
+			modifyOC: func(oc *api.OpenShiftCluster) {
+				oc.Properties.NetworkProfile.PreconfiguredNSG = api.PreconfiguredNSGEnabled
+			},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					AnyTimes().
+					Return(vnet, nil)
+			},
+			checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) {
+				mockTokenCredential(tokenCred)
+				pdpClient.EXPECT().CheckAccess(gomock.Any(), gomock.Any()).
+					Do(func(_, _ interface{}) {
+						cancel()
+					}).
+					Return(&canJoinNSG, nil).
+					AnyTimes()
+			},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			ctx, cancel := context.WithCancel(ctx)
+			defer cancel()
+
+			oc := &api.OpenShiftCluster{
+				Properties: api.OpenShiftClusterProperties{
+					ClusterProfile: api.ClusterProfile{
+						ResourceGroupID: resourceGroupID,
+					},
+				},
+			}
+			vnet := mgmtnetwork.VirtualNetwork{
+				ID: &vnetID,
+				VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{
+					Subnets: &[]mgmtnetwork.Subnet{
+						{
+							ID: &masterSubnet,
+							SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+								AddressPrefix: to.StringPtr("10.0.0.0/24"),
+								NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{
+									ID: &masterNSGv1,
+								},
+								ProvisioningState: mgmtnetwork.Succeeded,
+							},
+						},
+						{
+							ID: &workerSubnet,
+							SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+								AddressPrefix: to.StringPtr("10.0.1.0/24"),
+								NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{
+									ID: &workerNSGv1,
+								},
+								ProvisioningState: mgmtnetwork.Succeeded,
+							},
+						},
+					},
+				},
+			}
+
+			if tt.modifyOC != nil {
+				tt.modifyOC(oc)
+			}
+
+			vnetClient := mock_network.NewMockVirtualNetworksClient(controller)
+
+			if tt.vnetMocks != nil {
+				tt.vnetMocks(vnetClient, vnet)
+			}
+
+			tokenCred := mock_azcore.NewMockTokenCredential(controller)
+			pdpClient := mock_remotepdp.NewMockRemotePDPClient(controller)
+
+			if tt.checkAccessMocks != nil {
+				tt.checkAccessMocks(cancel, pdpClient, tokenCred)
+			}
+
+			dv := &dynamic{
+				azEnv:                      &azureclient.PublicCloud,
+				appID:                      "fff51942-b1f9-4119-9453-aaa922259eb7",
+				authorizerType:             AuthorizerClusterServicePrincipal,
+				virtualNetworks:            vnetClient,
+				pdpClient:                  pdpClient,
+				checkAccessSubjectInfoCred: tokenCred,
+				log:                        logrus.NewEntry(logrus.StandardLogger()),
+			}
+
+			subnets := []Subnet{
+				{ID: masterSubnet,
+					Path: masterSubnetPath},
+				{
+					ID:   workerSubnet,
+					Path: workerSubnetPath,
+				},
+			}
+
+			err := dv.ValidatePreConfiguredNSGs(ctx, oc, subnets)
+			utilerror.AssertErrorMessage(t, err, tt.wantErr)
+		})
+	}
+}
+
 func TestValidateSubnetSize(t *testing.T) {
 	subnetId := "id"
 	subnetPath := "path"

pkg/validate/openshiftcluster_validatedynamic.go

@@ -172,35 +172,6 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 		)
 	}
 
-	// FP validation
-	fpDynamic := dynamic.NewValidator(
-		dv.log,
-		dv.env,
-		dv.env.Environment(),
-		dv.subscriptionDoc.ID,
-		dv.fpAuthorizer,
-		dv.env.FPClientID(),
-		dynamic.AuthorizerFirstParty,
-		fpClientCred,
-		pdpClient,
-	)
-
-	err = fpDynamic.ValidateVnet(
-		ctx,
-		dv.oc.Location,
-		subnets,
-		dv.oc.Properties.NetworkProfile.PodCIDR,
-		dv.oc.Properties.NetworkProfile.ServiceCIDR,
-	)
-	if err != nil {
-		return err
-	}
-
-	err = fpDynamic.ValidateDiskEncryptionSets(ctx, dv.oc)
-	if err != nil {
-		return err
-	}
-
 	tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID
 	options := dv.env.Environment().ClientSecretCredentialOptions()
 	spTokenCredential, err := azidentity.NewClientSecretCredential(
@@ -265,5 +236,44 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 		return err
 	}
 
+	err = spDynamic.ValidatePreConfiguredNSGs(ctx, dv.oc, subnets)
+	if err != nil {
+		return err
+	}
+
+	// FP validation
+	fpDynamic := dynamic.NewValidator(
+		dv.log,
+		dv.env,
+		dv.env.Environment(),
+		dv.subscriptionDoc.ID,
+		dv.fpAuthorizer,
+		dv.env.FPClientID(),
+		dynamic.AuthorizerFirstParty,
+		fpClientCred,
+		pdpClient,
+	)
+
+	err = fpDynamic.ValidateVnet(
+		ctx,
+		dv.oc.Location,
+		subnets,
+		dv.oc.Properties.NetworkProfile.PodCIDR,
+		dv.oc.Properties.NetworkProfile.ServiceCIDR,
+	)
+	if err != nil {
+		return err
+	}
+
+	err = fpDynamic.ValidateDiskEncryptionSets(ctx, dv.oc)
+	if err != nil {
+		return err
+	}
+
+	err = fpDynamic.ValidatePreConfiguredNSGs(ctx, dv.oc, subnets)
+	if err != nil {
+		return err
+	}
+
 	return nil
 }

test/e2e/operator.go

@@ -342,6 +342,15 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 	}
 
 	BeforeEach(func(ctx context.Context) {
+		// TODO remove this when GA
+		By("checking if preconfiguredNSG is enabled")
+		co, err := clients.AROClusters.AroV1alpha1().Clusters().Get(ctx, "cluster", metav1.GetOptions{})
+		Expect(err).NotTo(HaveOccurred())
+		if co.Spec.OperatorFlags["aro.azuresubnets.nsg.managed"] == "false" {
+			Skip("preconfiguredNSG is enabled, skipping test")
+		}
+		By("preconfiguredNSG is disabled")
+
 		gatherNetworkInfo(ctx)
 		createE2ENSG(ctx)
 	})