From 8a49f96818f67c06b738a76ce7ca612c765782c4 Mon Sep 17 00:00:00 2001 From: nont Date: Thu, 20 Jul 2023 18:45:25 -0700 Subject: [PATCH 1/3] Remove the NSG validation for e2e tests --- pkg/validate/dynamic/dynamic.go | 26 +---- pkg/validate/dynamic/dynamic_test.go | 146 --------------------------- 2 files changed, 1 insertion(+), 171 deletions(-) diff --git a/pkg/validate/dynamic/dynamic.go b/pkg/validate/dynamic/dynamic.go index c2e14ee964f..38a7049994f 100644 --- a/pkg/validate/dynamic/dynamic.go +++ b/pkg/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/validate/dynamic/dynamic_test.go b/pkg/validate/dynamic/dynamic_test.go index 8a1cabcf3d1..f78a133d548 100644 --- a/pkg/validate/dynamic/dynamic_test.go +++ b/pkg/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) - }) - } } From 590553f30bfaa31a27cb555c8d0b819048d62a9f Mon Sep 17 00:00:00 2001 From: Daniel Bizau Date: Fri, 21 Jul 2023 14:36:06 +0200 Subject: [PATCH 2/3] Revert "Merge pull request #3049 from Azure/remove_nsg_checks_temp" This reverts commit 407ce68b3134194405384fe9fff575aa79c6800a, reversing changes made to 9efa064ab04c5404d8d1a123aa2dc087b5edf232. --- pkg/validate/dynamic/dynamic.go | 26 ++++- pkg/validate/dynamic/dynamic_test.go | 146 +++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 1 deletion(-) diff --git a/pkg/validate/dynamic/dynamic.go b/pkg/validate/dynamic/dynamic.go index 38a7049994f..c2e14ee964f 100644 --- a/pkg/validate/dynamic/dynamic.go +++ b/pkg/validate/dynamic/dynamic.go @@ -785,7 +785,31 @@ 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") - dv.log.Print("PreconfiguredNSG flag is: ", oc.Properties.NetworkProfile.PreconfiguredNSG) + + 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 } diff --git a/pkg/validate/dynamic/dynamic_test.go b/pkg/validate/dynamic/dynamic_test.go index f78a133d548..8a1cabcf3d1 100644 --- a/pkg/validate/dynamic/dynamic_test.go +++ b/pkg/validate/dynamic/dynamic_test.go @@ -1749,4 +1749,150 @@ 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) + }) + } } From 9c93de9ffb263172588186994cb07d26d91b3c2b Mon Sep 17 00:00:00 2001 From: Daniel Bizau Date: Fri, 21 Jul 2023 14:37:40 +0200 Subject: [PATCH 3/3] Revert "Add nsg enhancement permission validation for FP/Cluster SPs (#2969)" This reverts commit f87f19847904bdbb2c2aff868f649f69c4357046. --- pkg/util/mocks/dynamic/dynamic.go | 14 -- pkg/validate/dynamic/dynamic.go | 61 ------- pkg/validate/dynamic/dynamic_test.go | 169 ------------------ .../openshiftcluster_validatedynamic.go | 10 -- 4 files changed, 254 deletions(-) diff --git a/pkg/util/mocks/dynamic/dynamic.go b/pkg/util/mocks/dynamic/dynamic.go index 10e3917291c..d45d6bde183 100644 --- a/pkg/util/mocks/dynamic/dynamic.go +++ b/pkg/util/mocks/dynamic/dynamic.go @@ -103,20 +103,6 @@ func (mr *MockDynamicMockRecorder) ValidateEncryptionAtHost(ctx, oc interface{}) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateEncryptionAtHost", reflect.TypeOf((*MockDynamic)(nil).ValidateEncryptionAtHost), ctx, oc) } -// ValidatePreConfiguredNSGs mocks base method. -func (m *MockDynamic) ValidatePreConfiguredNSGs(ctx context.Context, oc *api.OpenShiftCluster, subnets []dynamic.Subnet) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ValidatePreConfiguredNSGs", ctx, oc, subnets) - ret0, _ := ret[0].(error) - return ret0 -} - -// ValidatePreConfiguredNSGs indicates an expected call of ValidatePreConfiguredNSGs. -func (mr *MockDynamicMockRecorder) ValidatePreConfiguredNSGs(ctx, oc, subnets interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidatePreConfiguredNSGs", reflect.TypeOf((*MockDynamic)(nil).ValidatePreConfiguredNSGs), ctx, oc, subnets) -} - // ValidateServicePrincipal mocks base method. func (m *MockDynamic) ValidateServicePrincipal(ctx context.Context, spTokenCredential azcore.TokenCredential) error { m.ctrl.T.Helper() diff --git a/pkg/validate/dynamic/dynamic.go b/pkg/validate/dynamic/dynamic.go index c2e14ee964f..11c1e461363 100644 --- a/pkg/validate/dynamic/dynamic.go +++ b/pkg/validate/dynamic/dynamic.go @@ -38,7 +38,6 @@ 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." @@ -72,7 +71,6 @@ type Dynamic interface { ValidateSubnets(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error ValidateDiskEncryptionSets(ctx context.Context, oc *api.OpenShiftCluster) error ValidateEncryptionAtHost(ctx context.Context, oc *api.OpenShiftCluster) error - ValidatePreConfiguredNSGs(ctx context.Context, oc *api.OpenShiftCluster, subnets []Subnet) error } type dynamic struct { @@ -783,65 +781,6 @@ func (dv *dynamic) ValidateSubnets(ctx context.Context, oc *api.OpenShiftCluster 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) } diff --git a/pkg/validate/dynamic/dynamic_test.go b/pkg/validate/dynamic/dynamic_test.go index 8a1cabcf3d1..1e69c6c86a1 100644 --- a/pkg/validate/dynamic/dynamic_test.go +++ b/pkg/validate/dynamic/dynamic_test.go @@ -38,7 +38,6 @@ 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,7 +1125,6 @@ var ( func mockTokenCredential(tokenCred *mock_azcore.MockTokenCredential) { tokenCred.EXPECT(). GetToken(gomock.Any(), gomock.Any()). - AnyTimes(). Return(mockAccessToken, nil) } @@ -1729,170 +1727,3 @@ 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) - }) - } -} diff --git a/pkg/validate/openshiftcluster_validatedynamic.go b/pkg/validate/openshiftcluster_validatedynamic.go index f0cfe178a91..3ddb261f819 100644 --- a/pkg/validate/openshiftcluster_validatedynamic.go +++ b/pkg/validate/openshiftcluster_validatedynamic.go @@ -199,11 +199,6 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { return err } - err = fpDynamic.ValidatePreConfiguredNSGs(ctx, dv.oc, subnets) - if err != nil { - return err - } - tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID options := dv.env.Environment().ClientSecretCredentialOptions() spTokenCredential, err := azidentity.NewClientSecretCredential( @@ -263,10 +258,5 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { return err } - err = spDynamic.ValidatePreConfiguredNSGs(ctx, dv.oc, subnets) - if err != nil { - return err - } - return nil }