Repository: Azure/ARO-RP
Pull request #2493 (2022-10-27 10:34:08)

validate permissions on NAT gateways
### Which issue this PR addresses:

We should update our azure-cli command module to include adding the network contributor on the NAT gateway (if one exists). Right now we're only adding the vnet and route tables.

### What this PR does / why we need it:

This PR is related to server side changes for Nat Gateway validation.
Client Side Implementation: [https://github.com/Azure/ARO-RP/pull/2394](https://github.com/Azure/ARO-RP/pull/2394)

### Test plan for issue:
Below two test cases added:
_TestValidateNatGatewaysPermissions_
_TestGetNatGatewayID_


### Is there any documentation that needs to be updated for this PR?

<!--
- If yes and the docs are in GitHub, include doc updates in the PR.
- If yes and the docs are not in GitHub (i.e. ADO wiki), include a link to the
  docs.
- If no, explain why (e.g. "tech debt cleanup, N/A").
-->


facchettos: /azp run ci 
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/pkg/api/error.go b/pkg/api/error.go
index c5d5ab7a0..2f1cda416 100644
--- a/pkg/api/error.go
+++ b/pkg/api/error.go
@@ -81,6 +81,7 @@ const (
 	CloudErrorCodeUnsupportedMediaType               = "UnsupportedMediaType"
 	CloudErrorCodeInvalidLinkedVNet                  = "InvalidLinkedVNet"
 	CloudErrorCodeInvalidLinkedRouteTable            = "InvalidLinkedRouteTable"
+	CloudErrorCodeInvalidLinkedNatGateway            = "InvalidLinkedNatGateway"
 	CloudErrorCodeInvalidLinkedDiskEncryptionSet     = "InvalidLinkedDiskEncryptionSet"
 	CloudErrorCodeNotFound                           = "NotFound"
 	CloudErrorCodeForbidden                          = "Forbidden"
diff --git a/pkg/api/validate/dynamic/dynamic.go b/pkg/api/validate/dynamic/dynamic.go
index a7b80144d..29208fd15 100644
--- a/pkg/api/validate/dynamic/dynamic.go
+++ b/pkg/api/validate/dynamic/dynamic.go
@@ -150,6 +150,12 @@ func (dv *dynamic) ValidateVnet(ctx context.Context, location string, subnets []
 		}
 	}
 
+	for _, s := range subnets {
+		err := dv.validateNatGatewayPermissions(ctx, s)
+		if err != nil {
+			return err
+		}
+	}
 	return dv.validateCIDRRanges(ctx, subnets, additionalCIDRs...)
 }
 
@@ -229,6 +235,59 @@ func (dv *dynamic) validateRouteTablePermissions(ctx context.Context, s Subnet)
 	return err
 }
 
+// validateNatGatewayPermissions will validate permissions on provided subnet
+func (dv *dynamic) validateNatGatewayPermissions(ctx context.Context, s Subnet) error {
+	dv.log.Printf("validateNatGatewayPermissions")
+
+	vnetID, _, err := subnet.Split(s.ID)
+	if err != nil {
+		return err
+	}
+
+	vnetr, err := azure.ParseResourceID(vnetID)
+	if err != nil {
+		return err
+	}
+
+	vnet, err := dv.virtualNetworks.Get(ctx, vnetr.ResourceGroup, vnetr.ResourceName, "")
+	if err != nil {
+		return err
+	}
+
+	ngID, err := getNatGatewayID(&vnet, s.ID)
+	if err != nil {
+		return err
+	}
+
+	if ngID == "" { // empty nat gateway
+		return nil
+	}
+
+	ngr, err := azure.ParseResourceID(ngID)
+	if err != nil {
+		return err
+	}
+
+	errCode := api.CloudErrorCodeInvalidResourceProviderPermissions
+	if dv.authorizerType == AuthorizerClusterServicePrincipal {
+		errCode = api.CloudErrorCodeInvalidServicePrincipalPermissions
+	}
+
+	err = dv.validateActions(ctx, &ngr, []string{
+		"Microsoft.Network/natGateways/join/action",
+		"Microsoft.Network/natGateways/read",
+		"Microsoft.Network/natGateways/write",
+	})
+	if err == wait.ErrWaitTimeout {
+		return api.NewCloudError(http.StatusBadRequest, errCode, "", "The %s service principal does not have Network Contributor permission on nat gateway '%s'.", dv.authorizerType, ngID)
+	}
+	if detailedErr, ok := err.(autorest.DetailedError); ok &&
+		detailedErr.StatusCode == http.StatusNotFound {
+		return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedNatGateway, "", "The nat gateway '%s' could not be found.", ngID)
+	}
+	return err
+}
+
 func (dv *dynamic) validateActions(ctx context.Context, r *azure.Resource, actions []string) error {
 	timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Minute)
 	defer cancel()
@@ -421,6 +480,19 @@ func getRouteTableID(vnet *mgmtnetwork.VirtualNetwork, subnetID string) (string,
 	return *s.RouteTable.ID, nil
 }
 
+func getNatGatewayID(vnet *mgmtnetwork.VirtualNetwork, subnetID string) (string, error) {
+	s, err := findSubnet(vnet, subnetID)
+	if err != nil {
+		return "", err
+	}
+
+	if s == nil || s.NatGateway == nil {
+		return "", nil
+	}
+
+	return *s.NatGateway.ID, nil
+}
+
 func findSubnet(vnet *mgmtnetwork.VirtualNetwork, subnetID string) (*mgmtnetwork.Subnet, error) {
 	if vnet.Subnets != nil {
 		for _, s := range *vnet.Subnets {


Test patch:
diff --git a/pkg/api/validate/dynamic/dynamic_test.go b/pkg/api/validate/dynamic/dynamic_test.go
index f5310d53b..785dad804 100644
--- a/pkg/api/validate/dynamic/dynamic_test.go
+++ b/pkg/api/validate/dynamic/dynamic_test.go
@@ -34,6 +34,8 @@ var (
 	masterSubnetPath  = "properties.masterProfile.subnetId"
 	masterRtID        = resourceGroupID + "/providers/Microsoft.Network/routeTables/masterRt"
 	workerRtID        = resourceGroupID + "/providers/Microsoft.Network/routeTables/workerRt"
+	masterNgID        = resourceGroupID + "/providers/Microsoft.Network/natGateways/masterNg"
+	workerNgID        = resourceGroupID + "/providers/Microsoft.Network/natGateways/workerNg"
 	masterNSGv1       = resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-controlplane-nsg"
 	workerNSGv1       = resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-node-nsg"
 )
@@ -194,6 +196,63 @@ func TestGetRouteTableID(t *testing.T) {
 	}
 }
 
+func TestGetNatGatewayID(t *testing.T) {
+	for _, tt := range []struct {
+		name       string
+		modifyVnet func(*mgmtnetwork.VirtualNetwork)
+		wantErr    string
+	}{
+		{
+			name: "pass",
+		},
+		{
+			name: "pass: no nat gateway",
+			modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) {
+				(*vnet.Subnets)[0].NatGateway = nil
+			},
+		},
+		{
+			name: "fail: can't find subnet",
+			modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) {
+				vnet.Subnets = nil
+			},
+			wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + masterSubnet + "' could not be found.",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			vnet := &mgmtnetwork.VirtualNetwork{
+				ID: &vnetID,
+				VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{
+					Subnets: &[]mgmtnetwork.Subnet{
+						{
+							ID: &masterSubnet,
+							SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+								NatGateway: &mgmtnetwork.SubResource{
+									ID: &masterNgID,
+								},
+							},
+						},
+					},
+				},
+			}
+
+			if tt.modifyVnet != nil {
+				tt.modifyVnet(vnet)
+			}
+
+			// purposefully hardcoding path to "" so it is not needed in the wantErr message
+			_, err := getNatGatewayID(vnet, masterSubnet)
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Error(fmt.Errorf("\n%s\n !=\n%s", err.Error(), tt.wantErr))
+			}
+		})
+	}
+}
+
 func TestValidateRouteTablesPermissions(t *testing.T) {
 	ctx := context.Background()
 
@@ -356,6 +415,168 @@ func TestValidateRouteTablesPermissions(t *testing.T) {
 	}
 }
 
+func TestValidateNatGatewaysPermissions(t *testing.T) {
+	ctx := context.Background()
+
+	for _, tt := range []struct {
+		name            string
+		subnet          Subnet
+		permissionMocks func(*mock_authorization.MockPermissionsClient, context.CancelFunc)
+		vnetMocks       func(*mock_network.MockVirtualNetworksClient, mgmtnetwork.VirtualNetwork)
+		wantErr         string
+	}{
+		{
+			name:   "fail: failed to get vnet",
+			subnet: Subnet{ID: masterSubnet},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					Return(vnet, errors.New("failed to get vnet"))
+			},
+			wantErr: "failed to get vnet",
+		},
+		{
+			name:   "fail: master subnet doesn't exist",
+			subnet: Subnet{ID: masterSubnet},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				vnet.Subnets = nil
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					Return(vnet, nil)
+			},
+			wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + masterSubnet + "' could not be found.",
+		},
+		{
+			name:   "fail: worker subnet ID doesn't exist",
+			subnet: Subnet{ID: workerSubnet},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				(*vnet.Subnets)[1].ID = to.StringPtr("not valid")
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					Return(vnet, nil)
+			},
+			wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + workerSubnet + "' could not be found.",
+		},
+		{
+			name:   "fail: permissions don't exist",
+			subnet: Subnet{ID: workerSubnet},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					Return(vnet, nil)
+			},
+			permissionMocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel context.CancelFunc) {
+				permissionsClient.EXPECT().
+					ListForResource(gomock.Any(), resourceGroupName, "Microsoft.Network", "", "natGateways", gomock.Any()).
+					Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) {
+						cancel()
+					}).
+					Return(
+						[]mgmtauthorization.Permission{
+							{
+								Actions:    &[]string{},
+								NotActions: &[]string{},
+							},
+						},
+						nil,
+					)
+			},
+			wantErr: "400: InvalidServicePrincipalPermissions: : The cluster service principal does not have Network Contributor permission on nat gateway '" + workerNgID + "'.",
+		},
+		{
+			name:   "pass",
+			subnet: Subnet{ID: masterSubnet},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					Return(vnet, nil)
+			},
+			permissionMocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel context.CancelFunc) {
+				permissionsClient.EXPECT().
+					ListForResource(gomock.Any(), resourceGroupName, "Microsoft.Network", "", "natGateways", gomock.Any()).
+					AnyTimes().
+					Return([]mgmtauthorization.Permission{
+						{
+							Actions: &[]string{
+								"Microsoft.Network/natGateways/join/action",
+								"Microsoft.Network/natGateways/read",
+								"Microsoft.Network/natGateways/write",
+							},
+							NotActions: &[]string{},
+						},
+					}, nil)
+			},
+		},
+		{
+			name:   "pass: no nat gateway to check",
+			subnet: Subnet{ID: masterSubnet},
+			vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) {
+				(*vnet.Subnets)[0].NatGateway = nil
+				(*vnet.Subnets)[1].NatGateway = nil
+				vnetClient.EXPECT().
+					Get(gomock.Any(), resourceGroupName, vnetName, "").
+					Return(vnet, nil)
+			},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			ctx, cancel := context.WithCancel(ctx)
+			defer cancel()
+
+			permissionsClient := mock_authorization.NewMockPermissionsClient(controller)
+			vnetClient := mock_network.NewMockVirtualNetworksClient(controller)
+
+			vnet := &mgmtnetwork.VirtualNetwork{
+				ID: &vnetID,
+				VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{
+					Subnets: &[]mgmtnetwork.Subnet{
+						{
+							ID: &masterSubnet,
+							SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+								NatGateway: &mgmtnetwork.SubResource{
+									ID: &masterNgID,
+								},
+							},
+						},
+						{
+							ID: &workerSubnet,
+							SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{
+								NatGateway: &mgmtnetwork.SubResource{
+									ID: &workerNgID,
+								},
+							},
+						},
+					},
+				},
+			}
+
+			dv := &dynamic{
+				authorizerType:  AuthorizerClusterServicePrincipal,
+				log:             logrus.NewEntry(logrus.StandardLogger()),
+				permissions:     permissionsClient,
+				virtualNetworks: vnetClient,
+			}
+
+			if tt.permissionMocks != nil {
+				tt.permissionMocks(permissionsClient, cancel)
+			}
+
+			if tt.vnetMocks != nil {
+				tt.vnetMocks(vnetClient, *vnet)
+			}
+
+			err := dv.validateNatGatewayPermissions(ctx, tt.subnet)
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Error(fmt.Errorf("\n%s\n !=\n%s", err.Error(), tt.wantErr))
+			}
+		})
+	}
+}
+
 func TestValidateCIDRRanges(t *testing.T) {
 	ctx := context.Background()
 
diff --git a/test/e2e/setup.go b/test/e2e/setup.go
index 7ed299fc1..69a983c28 100644
--- a/test/e2e/setup.go
+++ b/test/e2e/setup.go
@@ -405,10 +405,6 @@ func setup(ctx context.Context) error {
 	return nil
 }
 
-func tearDown(ctx context.Context) error {
-	return tearDownSelenium(context.Background())
-}
-
 var _ = BeforeSuite(func() {
 	log.Info("BeforeSuite")
 
@@ -423,7 +419,7 @@ var _ = BeforeSuite(func() {
 var _ = AfterSuite(func() {
 	log.Info("AfterSuite")
 
-	if err := tearDown(context.Background()); err != nil {
+	if err := tearDownSelenium(context.Background()); err != nil {
 		log.Printf(err.Error())
 	}
 })