diff --git a/pkg/api/validate/dynamic/dynamic.go b/pkg/api/validate/dynamic/dynamic.go index c2e14ee964f..38a7049994f 100644 --- a/pkg/api/validate/dynamic/dynamic.go +++ b/pkg/api/validate/dynamic/dynamic.go @@ -785,31 +785,7 @@ func (dv *dynamic) ValidateSubnets(ctx context.Context, oc *api.OpenShiftCluster 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 - } - } + dv.log.Print("PreconfiguredNSG flag is: ", oc.Properties.NetworkProfile.PreconfiguredNSG) return nil } diff --git a/pkg/api/validate/dynamic/dynamic_test.go b/pkg/api/validate/dynamic/dynamic_test.go index 8a1cabcf3d1..f78a133d548 100644 --- a/pkg/api/validate/dynamic/dynamic_test.go +++ b/pkg/api/validate/dynamic/dynamic_test.go @@ -1749,150 +1749,4 @@ var ( ) 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) - }) - } }