diff --git a/pkg/api/validate/cache.go b/pkg/api/validate/cache.go new file mode 100644 index 00000000000..afa904ae5c5 --- /dev/null +++ b/pkg/api/validate/cache.go @@ -0,0 +1,48 @@ +package validate + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + + mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" +) + +type virtualNetworksGetClient interface { + Get(context.Context, string, string, string) (mgmtnetwork.VirtualNetwork, error) +} + +type virtualNetworksCacheKey struct { + resourceGroupName string + virtualNetworkName string + expand string +} + +type virtualNetworksCache struct { + c virtualNetworksGetClient + m map[virtualNetworksCacheKey]mgmtnetwork.VirtualNetwork +} + +func (vnc *virtualNetworksCache) Get(ctx context.Context, resourceGroupName string, virtualNetworkName string, expand string) (mgmtnetwork.VirtualNetwork, error) { + if _, ok := vnc.m[virtualNetworksCacheKey{resourceGroupName, virtualNetworkName, expand}]; !ok { + vnet, err := vnc.c.Get(ctx, resourceGroupName, virtualNetworkName, expand) + if err != nil { + return vnet, err + } + + vnc.m[virtualNetworksCacheKey{resourceGroupName, virtualNetworkName, expand}] = vnet + } + + return vnc.m[virtualNetworksCacheKey{resourceGroupName, virtualNetworkName, expand}], nil +} + +// newVirtualNetworksCache returns a new virtualNetworksCache. It knows nothing +// about updates and is not thread-safe, but it does retry in the face of +// errors. +func newVirtualNetworksCache(c virtualNetworksGetClient) virtualNetworksGetClient { + return &virtualNetworksCache{ + c: c, + m: map[virtualNetworksCacheKey]mgmtnetwork.VirtualNetwork{}, + } +} diff --git a/pkg/api/validate/dynamic.go b/pkg/api/validate/dynamic.go new file mode 100644 index 00000000000..2a4965317ba --- /dev/null +++ b/pkg/api/validate/dynamic.go @@ -0,0 +1,228 @@ +package validate + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "fmt" + "net/http" + "sort" + "strings" + "time" + + mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" + "github.com/Azure/go-autorest/autorest" + "github.com/Azure/go-autorest/autorest/azure" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/util/wait" + + "github.com/Azure/ARO-RP/pkg/api" + "github.com/Azure/ARO-RP/pkg/env" + "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/authorization" + "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network" + utilpermissions "github.com/Azure/ARO-RP/pkg/util/permissions" + "github.com/Azure/ARO-RP/pkg/util/refreshable" + "github.com/Azure/ARO-RP/pkg/util/steps" + "github.com/Azure/ARO-RP/pkg/util/subnet" +) + +// SlimDynamic validate in the operator context. +type SlimDynamic interface { + ValidateVnetPermissions(ctx context.Context) error + ValidateRouteTablesPermissions(ctx context.Context) error + ValidateVnetDns(ctx context.Context) error + // etc + // does Quota code go in here too? +} + +type dynamic struct { + log *logrus.Entry + vnetr *azure.Resource + masterSubnetID string + workerSubnetIDs []string + + code string + typ string + + permissions authorization.PermissionsClient + virtualNetworks virtualNetworksGetClient +} + +func NewValidator(log *logrus.Entry, env env.Interface, masterSubnetID string, workerSubnetIDs []string, subscriptionID string, authorizer refreshable.Authorizer, code string, typ string) (*dynamic, error) { + vnetID, _, err := subnet.Split(masterSubnetID) + if err != nil { + return nil, err + } + + vnetr, err := azure.ParseResourceID(vnetID) + if err != nil { + return nil, err + } + + return &dynamic{ + log: log, + vnetr: &vnetr, + masterSubnetID: masterSubnetID, + workerSubnetIDs: workerSubnetIDs, + + code: code, + typ: typ, + + permissions: authorization.NewPermissionsClient(env.Environment(), subscriptionID, authorizer), + virtualNetworks: newVirtualNetworksCache(network.NewVirtualNetworksClient(env.Environment(), subscriptionID, authorizer)), + }, nil +} + +func (dv *dynamic) ValidateVnetPermissions(ctx context.Context) error { + dv.log.Printf("ValidateVnetPermissions (%s)", dv.typ) + + err := dv.validateActions(ctx, dv.vnetr, []string{ + "Microsoft.Network/virtualNetworks/join/action", + "Microsoft.Network/virtualNetworks/read", + "Microsoft.Network/virtualNetworks/write", + "Microsoft.Network/virtualNetworks/subnets/join/action", + "Microsoft.Network/virtualNetworks/subnets/read", + "Microsoft.Network/virtualNetworks/subnets/write", + }) + + if err == wait.ErrWaitTimeout { + return api.NewCloudError(http.StatusBadRequest, dv.code, "", "The %s does not have Network Contributor permission on vnet '%s'.", dv.typ, dv.vnetr) + } + if detailedErr, ok := err.(autorest.DetailedError); ok && + detailedErr.StatusCode == http.StatusNotFound { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The vnet '%s' could not be found.", dv.vnetr) + } + return err +} + +func (dv *dynamic) ValidateRouteTablesPermissions(ctx context.Context) error { + vnet, err := dv.virtualNetworks.Get(ctx, dv.vnetr.ResourceGroup, dv.vnetr.ResourceName, "") + if err != nil { + return err + } + + m := map[string]string{} + + rtID, err := getRouteTableID(&vnet, "properties.masterProfile.subnetId", dv.masterSubnetID) + if err != nil { + return err + } + + if rtID != "" { + m[strings.ToLower(rtID)] = "properties.masterProfile.subnetId" + } + + for i, s := range dv.workerSubnetIDs { + path := fmt.Sprintf("properties.workerProfiles[%d].subnetId", i) + + rtID, err := getRouteTableID(&vnet, path, s) + if err != nil { + return err + } + + if _, ok := m[strings.ToLower(rtID)]; ok || rtID == "" { + continue + } + m[strings.ToLower(rtID)] = path + } + + rts := make([]string, 0, len(m)) + for rt := range m { + rts = append(rts, rt) + } + + sort.Slice(rts, func(i, j int) bool { return strings.Compare(m[rts[i]], m[rts[j]]) < 0 }) + + for _, rt := range rts { + err := dv.validateRouteTablePermissions(ctx, rt, m[rt]) + if err != nil { + return err + } + } + + return nil +} + +func (dv *dynamic) validateRouteTablePermissions(ctx context.Context, rtID string, path string) error { + dv.log.Printf("validateRouteTablePermissions(%s, %s)", dv.typ, path) + + rtr, err := azure.ParseResourceID(rtID) + if err != nil { + return err + } + + err = dv.validateActions(ctx, &rtr, []string{ + "Microsoft.Network/routeTables/join/action", + "Microsoft.Network/routeTables/read", + "Microsoft.Network/routeTables/write", + }) + if err == wait.ErrWaitTimeout { + return api.NewCloudError(http.StatusBadRequest, dv.code, "", "The %s does not have Network Contributor permission on route table '%s'.", dv.typ, rtID) + } + if detailedErr, ok := err.(autorest.DetailedError); ok && + detailedErr.StatusCode == http.StatusNotFound { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedRouteTable, "", "The route table '%s' could not be found.", rtID) + } + return err +} + +func (dv *dynamic) ValidateVnetDNS(ctx context.Context) error { + dv.log.Print("validateVnetDns") + + vnet, err := dv.virtualNetworks.Get(ctx, dv.vnetr.ResourceGroup, dv.vnetr.ResourceName, "") + if err != nil { + return err + } + + if vnet.DhcpOptions != nil && + vnet.DhcpOptions.DNSServers != nil && + len(*vnet.DhcpOptions.DNSServers) > 0 { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The provided vnet '%s' is invalid: custom DNS servers are not supported.", *vnet.ID) + } + + return nil +} + +func (dv *dynamic) validateActions(ctx context.Context, r *azure.Resource, actions []string) error { + timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + + return wait.PollImmediateUntil(10*time.Second, func() (bool, error) { + permissions, err := dv.permissions.ListForResource(ctx, r.ResourceGroup, r.Provider, "", r.ResourceType, r.ResourceName) + + if detailedErr, ok := err.(autorest.DetailedError); ok && + detailedErr.StatusCode == http.StatusForbidden { + return false, steps.ErrWantRefresh + } + if err != nil { + return false, err + } + + for _, action := range actions { + ok, err := utilpermissions.CanDoAction(permissions, action) + if !ok || err != nil { + // TODO(jminter): I don't understand if there are genuinely + // cases where CanDoAction can return false then true shortly + // after. I'm a little skeptical; if it can't happen we can + // simplify this code. We should add a metric on this. + return false, err + } + } + + return true, nil + }, timeoutCtx.Done()) +} + +func getRouteTableID(vnet *mgmtnetwork.VirtualNetwork, path, subnetID string) (string, error) { + s := findSubnet(vnet, subnetID) + if s == nil { + return "", api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The subnet '%s' could not be found.", subnetID) + } + + if s.RouteTable == nil { + return "", nil + } + + return *s.RouteTable.ID, nil +} diff --git a/pkg/api/validate/dynamic_test.go b/pkg/api/validate/dynamic_test.go new file mode 100644 index 00000000000..5489a0b9200 --- /dev/null +++ b/pkg/api/validate/dynamic_test.go @@ -0,0 +1,529 @@ +package validate + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "errors" + "net/http" + "strings" + "testing" + + mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" + mgmtauthorization "github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-09-01-preview/authorization" + "github.com/Azure/go-autorest/autorest" + "github.com/Azure/go-autorest/autorest/azure" + "github.com/Azure/go-autorest/autorest/to" + "github.com/golang/mock/gomock" + "github.com/sirupsen/logrus" + + mock_authorization "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/authorization" + mock_network "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/network" +) + +func TestValidateVnetPermissions(t *testing.T) { + ctx := context.Background() + + resourceGroupName := "testGroup" + vnetName := "testVnet" + subscriptionID := "0000000-0000-0000-0000-000000000000" + vnetID := "/subscriptions/" + subscriptionID + "/resourceGroups/" + resourceGroupName + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + resourceType := "virtualNetworks" + resourceProvider := "Microsoft.Network" + + controller := gomock.NewController(t) + defer controller.Finish() + + for _, tt := range []struct { + name string + mocks func(*mock_authorization.MockPermissionsClient, func()) + wantErr string + }{ + { + name: "pass", + mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), resourceGroupName, resourceProvider, "", resourceType, vnetName). + Return([]mgmtauthorization.Permission{ + { + Actions: &[]string{ + "Microsoft.Network/virtualNetworks/join/action", + "Microsoft.Network/virtualNetworks/read", + "Microsoft.Network/virtualNetworks/write", + "Microsoft.Network/virtualNetworks/subnets/join/action", + "Microsoft.Network/virtualNetworks/subnets/read", + "Microsoft.Network/virtualNetworks/subnets/write", + }, + NotActions: &[]string{}, + }, + }, nil) + }, + }, + { + name: "fail: missing permissions", + mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), resourceGroupName, resourceProvider, "", resourceType, vnetName). + Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { + cancel() + }). + Return( + []mgmtauthorization.Permission{ + { + Actions: &[]string{}, + NotActions: &[]string{}, + }, + }, + nil, + ) + }, + wantErr: "400: InvalidResourceProviderPermissions: : The resource provider does not have Network Contributor permission on vnet '" + vnetID + "'.", + }, + { + name: "fail: not found", + mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), resourceGroupName, resourceProvider, "", resourceType, vnetName). + Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { + cancel() + }). + Return( + nil, + autorest.DetailedError{ + StatusCode: http.StatusNotFound, + }, + ) + }, + wantErr: "400: InvalidLinkedVNet: : The vnet '" + vnetID + "' could not be found.", + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + permissionsClient := mock_authorization.NewMockPermissionsClient(controller) + tt.mocks(permissionsClient, cancel) + + dv := &dynamic{ + log: logrus.NewEntry(logrus.StandardLogger()), + permissions: permissionsClient, + vnetr: &azure.Resource{ + ResourceGroup: resourceGroupName, + ResourceType: resourceType, + Provider: resourceProvider, + ResourceName: vnetName, + SubscriptionID: subscriptionID, + }, + code: "InvalidResourceProviderPermissions", + typ: "resource provider", + } + + err := dv.ValidateVnetPermissions(ctx) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } + }) + } +} + +func TestGetRouteTableID(t *testing.T) { + resourceGroupID := "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" + vnetID := resourceGroupID + "/providers/Microsoft.Network/virtualNetworks/testVnet" + genericSubnet := vnetID + "/subnet/genericSubnet" + routeTableID := resourceGroupID + "/providers/Microsoft.Network/routeTables/testRouteTable" + + for _, tt := range []struct { + name string + modifyVnet func(*mgmtnetwork.VirtualNetwork) + wantErr string + }{ + { + name: "pass", + }, + { + name: "pass: no route table", + modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { + (*vnet.Subnets)[0].RouteTable = nil + }, + }, + { + name: "fail: can't find subnet", + modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { + vnet.Subnets = nil + }, + wantErr: "400: InvalidLinkedVNet: : The subnet '" + genericSubnet + "' could not be found.", + }, + } { + vnet := &mgmtnetwork.VirtualNetwork{ + ID: &vnetID, + VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{ + Subnets: &[]mgmtnetwork.Subnet{ + { + ID: &genericSubnet, + SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ + RouteTable: &mgmtnetwork.RouteTable{ + ID: &routeTableID, + }, + }, + }, + }, + }, + } + + if tt.modifyVnet != nil { + tt.modifyVnet(vnet) + } + + // purposefully hardcoding path to "" so it is not needed in the wantErr message + _, err := getRouteTableID(vnet, "", genericSubnet) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } + } +} + +func TestValidateVnetDNS(t *testing.T) { + ctx := context.Background() + + controller := gomock.NewController(t) + defer controller.Finish() + + resourceGroupName := "testGroup" + vnetName := "testVnet" + vnetID := "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/" + resourceGroupName + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + + for _, tt := range []struct { + name string + vnetMocks func(*mock_network.MockVirtualNetworksClient, mgmtnetwork.VirtualNetwork) + wantErr string + }{ + { + name: "pass", + vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) { + vnetClient.EXPECT(). + Get(gomock.Any(), resourceGroupName, vnetName, ""). + Return(vnet, nil) + }, + }, + { + name: "fail: dhcp options set", + vnetMocks: func(vnetClient *mock_network.MockVirtualNetworksClient, vnet mgmtnetwork.VirtualNetwork) { + vnet.DhcpOptions = &mgmtnetwork.DhcpOptions{ + DNSServers: &[]string{ + "8.8.8.8", + }, + } + vnetClient.EXPECT(). + Get(gomock.Any(), resourceGroupName, vnetName, ""). + Return(vnet, nil) + }, + wantErr: "400: InvalidLinkedVNet: : The provided vnet '" + vnetID + "' is invalid: custom DNS servers are not supported.", + }, + { + name: "fail: failed to get vnet", + 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", + }, + } { + vnet := mgmtnetwork.VirtualNetwork{ + ID: to.StringPtr(vnetID), + VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{ + DhcpOptions: nil, + }, + } + + vnetClient := mock_network.NewMockVirtualNetworksClient(controller) + tt.vnetMocks(vnetClient, vnet) + + dv := &dynamic{ + log: logrus.NewEntry(logrus.StandardLogger()), + virtualNetworks: vnetClient, + vnetr: &azure.Resource{ + ResourceGroup: resourceGroupName, + ResourceName: vnetName, + }, + } + + err := dv.ValidateVnetDNS(ctx) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } + } +} + +func TestValidateRouteTablePermissions(t *testing.T) { + ctx := context.Background() + + resourceGroupName := "testGroup" + resourceGroupID := "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/" + resourceGroupName + routeTableName := "testRouteTable" + routeTableID := resourceGroupID + "/providers/Microsoft.Network/routeTables/" + routeTableName + + controller := gomock.NewController(t) + defer controller.Finish() + + for _, tt := range []struct { + name string + rtID string + mocks func(*mock_authorization.MockPermissionsClient, func()) + wantErr string + }{ + { + name: "pass", + rtID: routeTableID, + mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), resourceGroupName, "Microsoft.Network", "", "routeTables", routeTableName). + Return([]mgmtauthorization.Permission{ + { + Actions: &[]string{ + "Microsoft.Network/routeTables/join/action", + "Microsoft.Network/routeTables/read", + "Microsoft.Network/routeTables/write", + }, + NotActions: &[]string{}, + }, + }, nil) + }, + }, + { + name: "fail: cannot parse resource id", + rtID: "invalid_route_table_id", + wantErr: "parsing failed for invalid_route_table_id. Invalid resource Id format", + }, + { + name: "fail: missing permissions", + rtID: routeTableID, + mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), resourceGroupName, "Microsoft.Network", "", "routeTables", routeTableName). + Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { + cancel() + }). + Return([]mgmtauthorization.Permission{ + { + Actions: &[]string{}, + NotActions: &[]string{}, + }, + }, nil) + }, + wantErr: "400: InvalidResourceProviderPermissions: : The resource provider does not have Network Contributor permission on route table '" + routeTableID + "'.", + }, + { + name: "fail: not found", + rtID: routeTableID, + mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), resourceGroupName, "Microsoft.Network", "", "routeTables", routeTableName). + Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { + cancel() + }). + Return( + nil, + autorest.DetailedError{ + StatusCode: http.StatusNotFound, + }, + ) + }, + wantErr: "400: InvalidLinkedRouteTable: : The route table '" + routeTableID + "' could not be found.", + }, + } { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + permissionsClient := mock_authorization.NewMockPermissionsClient(controller) + if tt.mocks != nil { + tt.mocks(permissionsClient, cancel) + } + + dv := &dynamic{ + log: logrus.NewEntry(logrus.StandardLogger()), + permissions: permissionsClient, + code: "InvalidResourceProviderPermissions", + typ: "resource provider", + } + + // purposefully hardcoding path to "" so it is not needed in the wantErr message + err := dv.validateRouteTablePermissions(ctx, tt.rtID, "") + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } + } +} + +func TestValidateRouteTablesPermissions(t *testing.T) { + ctx := context.Background() + + subscriptionID := "0000000-0000-0000-0000-000000000000" + resourceGroupName := "testGroup" + resourceGroupID := "/subscriptions/" + subscriptionID + "/resourceGroups/" + resourceGroupName + vnetName := "testVnet" + vnetID := resourceGroupID + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + masterSubnet := vnetID + "/subnet/masterSubnet" + workerSubnet := vnetID + "/subnet/workerSubnet" + masterRtID := resourceGroupID + "/providers/Microsoft.Network/routeTables/masterRt" + workerRtID := resourceGroupID + "/providers/Microsoft.Network/routeTables/workerRt" + + controller := gomock.NewController(t) + defer controller.Finish() + + for _, tt := range []struct { + name string + permissionMocks func(*mock_authorization.MockPermissionsClient, func()) + vnetMocks func(*mock_network.MockVirtualNetworksClient, mgmtnetwork.VirtualNetwork) + wantErr string + }{ + { + name: "fail: failed to get vnet", + 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", + 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: properties.masterProfile.subnetId: The subnet '" + masterSubnet + "' could not be found.", + }, + { + name: "fail: worker subnet ID doesn't exist", + 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: properties.workerProfiles[0].subnetId: The subnet '" + workerSubnet + "' could not be found.", + }, + { + name: "fail: permissions don't exist", + 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 func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), strings.ToLower(resourceGroupName), strings.ToLower("Microsoft.Network"), "", strings.ToLower("routeTables"), gomock.Any()). + Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { + cancel() + }). + Return( + []mgmtauthorization.Permission{ + { + Actions: &[]string{}, + NotActions: &[]string{}, + }, + }, + nil, + ) + }, + wantErr: "400: InvalidResourceProviderPermissions: : The resource provider does not have Network Contributor permission on route table '" + strings.ToLower(masterRtID) + "'.", + }, + { + name: "pass", + 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 func()) { + permissionsClient.EXPECT(). + ListForResource(gomock.Any(), strings.ToLower(resourceGroupName), strings.ToLower("Microsoft.Network"), "", strings.ToLower("routeTables"), gomock.Any()). + AnyTimes(). + Return([]mgmtauthorization.Permission{ + { + Actions: &[]string{ + "Microsoft.Network/routeTables/join/action", + "Microsoft.Network/routeTables/read", + "Microsoft.Network/routeTables/write", + }, + NotActions: &[]string{}, + }, + }, nil) + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + 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{ + RouteTable: &mgmtnetwork.RouteTable{ + ID: &masterRtID, + }, + }, + }, + { + ID: &workerSubnet, + SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ + RouteTable: &mgmtnetwork.RouteTable{ + ID: &workerRtID, + }, + }, + }, + }, + }, + } + + dv := &dynamic{ + log: logrus.NewEntry(logrus.StandardLogger()), + permissions: permissionsClient, + virtualNetworks: vnetClient, + + vnetr: &azure.Resource{ + ResourceGroup: resourceGroupName, + ResourceName: vnetName, + SubscriptionID: subscriptionID, + Provider: "Microsoft.Network", + ResourceType: "virtualNetworks", + }, + + masterSubnetID: masterSubnet, + workerSubnetIDs: []string{workerSubnet}, + + code: "InvalidResourceProviderPermissions", + typ: "resource provider", + } + + if tt.permissionMocks != nil { + tt.permissionMocks(permissionsClient, cancel) + } + + if tt.vnetMocks != nil { + tt.vnetMocks(vnetClient, *vnet) + } + + err := dv.ValidateRouteTablesPermissions(ctx) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } + }) + } +} diff --git a/pkg/api/validate/openshiftcluster_validatedynamic.go b/pkg/api/validate/openshiftcluster_validatedynamic.go index 559062d3af8..13961adbb72 100644 --- a/pkg/api/validate/openshiftcluster_validatedynamic.go +++ b/pkg/api/validate/openshiftcluster_validatedynamic.go @@ -8,185 +8,127 @@ import ( "fmt" "net" "net/http" - "strconv" "strings" - "time" mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features" - "github.com/Azure/go-autorest/autorest" - "github.com/Azure/go-autorest/autorest/azure" "github.com/apparentlymart/go-cidr/cidr" jwt "github.com/form3tech-oss/jwt-go" "github.com/sirupsen/logrus" - "k8s.io/apimachinery/pkg/util/wait" "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/env" - arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" "github.com/Azure/ARO-RP/pkg/util/aad" - "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/authorization" "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/features" - "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network" - utilpermissions "github.com/Azure/ARO-RP/pkg/util/permissions" "github.com/Azure/ARO-RP/pkg/util/refreshable" "github.com/Azure/ARO-RP/pkg/util/subnet" ) -// OpenShiftClusterFullDynamicValidator is the dynamic validator interface -// for the RP execution contexts -type OpenShiftClusterFullDynamicValidator interface { +// OpenShiftClusterDynamicValidator is the dynamic validator interface +type OpenShiftClusterDynamicValidator interface { Dynamic(context.Context) error } -// OpenShiftClusterSlimDynamicValidator is the dynamic validator interface -// for the cluster execution context -type OpenShiftClusterSlimDynamicValidator interface { - Dynamic(context.Context) error -} - -// NewOpenShiftClusterFullDynamicValidator creates a new OpenShiftClusterFullDynamicValidator -func NewOpenShiftClusterFullDynamicValidator(log *logrus.Entry, env env.Interface, oc *api.OpenShiftCluster, subscriptionDoc *api.SubscriptionDocument, fpAuthorizer refreshable.Authorizer) OpenShiftClusterFullDynamicValidator { - return &openShiftClusterFullDynamicValidator{ +// NewOpenShiftClusterDynamicValidator creates a new OpenShiftClusterDynamicValidator +func NewOpenShiftClusterDynamicValidator(log *logrus.Entry, env env.Interface, oc *api.OpenShiftCluster, subscriptionDoc *api.SubscriptionDocument, fpAuthorizer refreshable.Authorizer) OpenShiftClusterDynamicValidator { + return &openShiftClusterDynamicValidator{ + log: log, env: env, oc: oc, subscriptionDoc: subscriptionDoc, fpAuthorizer: fpAuthorizer, - - log: log, + providers: features.NewProvidersClient(env.Environment(), subscriptionDoc.ID, fpAuthorizer), } } -// NewOpenShiftClusterSlimDynamicValidator creates a new OpenShiftClusterSlimDynamicValidator -func NewOpenShiftClusterSlimDynamicValidator(log *logrus.Entry, clientID string, clientSecret api.SecureString, subscriptionID string, tenantID string, clusterSpec *arov1alpha1.ClusterSpec) OpenShiftClusterSlimDynamicValidator { - spp := &api.ServicePrincipalProfile{ - ClientID: clientID, - ClientSecret: clientSecret, - } - return &openShiftClusterSlimDynamicValidator{ - log: log, - spp: spp, - tenantID: tenantID, - subscriptionID: subscriptionID, - clusterSpec: clusterSpec, - } +type azureClaim struct { + Roles []string `json:"roles,omitempty"` +} + +func (*azureClaim) Valid() error { + return fmt.Errorf("unimplemented") } -// openShiftClusterFullDynamicValidator is dynamic validator used inside RP context -// and can operate within cluster and firstParty service principals. -// It includes openShiftClusterSlimDynamicValidatora -type openShiftClusterFullDynamicValidator struct { +type openShiftClusterDynamicValidator struct { + log *logrus.Entry env env.Interface oc *api.OpenShiftCluster subscriptionDoc *api.SubscriptionDocument fpAuthorizer refreshable.Authorizer - - log *logrus.Entry + providers features.ProvidersClient } -// openShiftClusterSlimDynamicValidator is dynamic validator used inside clusters -// context and can operate ONLY with cluster service principal scope -type openShiftClusterSlimDynamicValidator struct { - log *logrus.Entry - - spp *api.ServicePrincipalProfile - clusterSpec *arov1alpha1.ClusterSpec - tenantID string - subscriptionID string - environmentName string -} - -// Dynamic validates an OpenShift cluster in the context of cluster -func (dv *openShiftClusterSlimDynamicValidator) Dynamic(ctx context.Context) error { - azureEnv, err := azure.EnvironmentFromName(dv.environmentName) - if err != nil { - return err - } - spAuthorizer, err := validateServicePrincipalProfile(ctx, dv.log, dv.spp, dv.tenantID, azureEnv.ResourceManagerEndpoint) - if err != nil { - return err - } - - spPermissions := authorization.NewPermissionsClient(&azureEnv, dv.subscriptionID, spAuthorizer) +// Dynamic validates an OpenShift cluster +func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { + // Get all subnets + mSubnetID := dv.oc.Properties.MasterProfile.SubnetID + wSubnetIDs := []string{} - vnetr, err := azure.ParseResourceID(dv.clusterSpec.VNetID) - if err != nil { - return err + for _, s := range dv.oc.Properties.WorkerProfiles { + wSubnetIDs = append(wSubnetIDs, s.SubnetID) } - err = validateVnetPermissions(ctx, dv.log, spAuthorizer, spPermissions, dv.clusterSpec.VNetID, &vnetr, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal") + // FP validation + fpDynamic, err := NewValidator(dv.log, dv.env, mSubnetID, wSubnetIDs, dv.subscriptionDoc.ID, dv.fpAuthorizer, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider") if err != nil { return err } - return nil -} - -// Dynamic validates an OpenShift cluster -func (dv *openShiftClusterFullDynamicValidator) Dynamic(ctx context.Context) error { - // TODO: Dynamic() should work on an enriched oc (in update), and it - // currently doesn't. One sticking point is handling subnet overlap - // calculations. - - r, err := azure.ParseResourceID(dv.oc.ID) + err = fpDynamic.ValidateVnetPermissions(ctx) if err != nil { return err } - fpPermissions := authorization.NewPermissionsClient(dv.env.Environment(), dv.subscriptionDoc.ID, dv.fpAuthorizer) - spAuthorizer, err := validateServicePrincipalProfile(ctx, dv.log, &dv.oc.Properties.ServicePrincipalProfile, dv.subscriptionDoc.Subscription.Properties.TenantID, dv.env.Environment().ResourceManagerEndpoint) + err = fpDynamic.ValidateRouteTablesPermissions(ctx) if err != nil { return err } - spPermissions := authorization.NewPermissionsClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer) - spProviders := features.NewProvidersClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer) - spVirtualNetworks := network.NewVirtualNetworksClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer) - - vnetID, _, err := subnet.Split(dv.oc.Properties.MasterProfile.SubnetID) + // SP validation + spAuthorizer, err := validateServicePrincipalProfile(ctx, dv.log, dv.env, dv.oc, dv.subscriptionDoc) if err != nil { return err } - vnetr, err := azure.ParseResourceID(vnetID) + spDynamic, err := NewValidator(dv.log, dv.env, mSubnetID, wSubnetIDs, dv.subscriptionDoc.ID, spAuthorizer, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal") if err != nil { return err } - err = validateVnetPermissions(ctx, dv.log, spAuthorizer, spPermissions, vnetID, &vnetr, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal") + err = spDynamic.ValidateVnetPermissions(ctx) if err != nil { return err } - err = validateVnetPermissions(ctx, dv.log, dv.fpAuthorizer, fpPermissions, vnetID, &vnetr, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider") + err = spDynamic.ValidateRouteTablesPermissions(ctx) if err != nil { return err } - // Get after validating permissions - vnet, err := spVirtualNetworks.Get(ctx, vnetr.ResourceGroup, vnetr.ResourceName, "") + // Additional checks - use any dynamic because they both have the correct permissions + err = spDynamic.ValidateVnetDNS(ctx) if err != nil { return err } - err = validateRouteTablePermissions(ctx, dv.log, dv.oc, spAuthorizer, spPermissions, &vnet, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal") + vnet, err := spDynamic.virtualNetworks.Get(ctx, spDynamic.vnetr.ResourceGroup, spDynamic.vnetr.ResourceName, "") if err != nil { return err } - err = validateRouteTablePermissions(ctx, dv.log, dv.oc, dv.fpAuthorizer, fpPermissions, &vnet, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider") + err = dv.validateCIDRRanges(ctx, &vnet) if err != nil { return err } - err = validateVnet(ctx, dv.log, dv.oc, &vnet) + err = dv.validateVnetLocation(ctx, &vnet) if err != nil { return err } - err = validateProviders(ctx, dv.log, spProviders) + err = dv.validateProviders(ctx) if err != nil { return err } @@ -194,129 +136,77 @@ func (dv *openShiftClusterFullDynamicValidator) Dynamic(ctx context.Context) err return nil } -type azureClaim struct { - Roles []string `json:"roles,omitempty"` -} - -func (*azureClaim) Valid() error { - return fmt.Errorf("unimplemented") -} - -func validateServicePrincipalProfile(ctx context.Context, log *logrus.Entry, spp *api.ServicePrincipalProfile, tenantID, resource string) (refreshable.Authorizer, error) { - log.Print("validateServicePrincipalProfile") - - token, err := aad.GetToken(ctx, log, spp, tenantID, resource) - if err != nil { - return nil, err - } - - p := &jwt.Parser{} - c := &azureClaim{} - _, _, err = p.ParseUnverified(token.OAuthToken(), c) - if err != nil { - return nil, err - } +func (dv *openShiftClusterDynamicValidator) validateVnetLocation(ctx context.Context, vnet *mgmtnetwork.VirtualNetwork) error { + dv.log.Print("validateVnetLocation") - for _, role := range c.Roles { - if role == "Application.ReadWrite.OwnedBy" { - return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalCredentials, "properties.servicePrincipalProfile", "The provided service principal must not have the Application.ReadWrite.OwnedBy permission.") - } + if !strings.EqualFold(*vnet.Location, dv.oc.Location) { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The vnet location '%s' must match the cluster location '%s'.", *vnet.Location, dv.oc.Location) } - return refreshable.NewAuthorizer(token), nil + return nil } -func validateVnetPermissions(ctx context.Context, log *logrus.Entry, authorizer refreshable.Authorizer, client authorization.PermissionsClient, vnetID string, vnetr *azure.Resource, code, typ string) error { - log.Printf("validateVnetPermissions (%s)", typ) - - err := validateActions(ctx, log, vnetr, []string{ - "Microsoft.Network/virtualNetworks/join/action", - "Microsoft.Network/virtualNetworks/read", - "Microsoft.Network/virtualNetworks/write", - "Microsoft.Network/virtualNetworks/subnets/join/action", - "Microsoft.Network/virtualNetworks/subnets/read", - "Microsoft.Network/virtualNetworks/subnets/write", - }, authorizer, client) - if err == wait.ErrWaitTimeout { - return api.NewCloudError(http.StatusBadRequest, code, "", "The %s does not have Network Contributor permission on vnet '%s'.", typ, vnetID) - } - if detailedErr, ok := err.(autorest.DetailedError); ok && - detailedErr.StatusCode == http.StatusNotFound { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The vnet '%s' could not be found.", vnetID) - } - return err -} +func (dv *openShiftClusterDynamicValidator) validateCIDRRanges(ctx context.Context, vnet *mgmtnetwork.VirtualNetwork) error { + dv.log.Print("validateCIDRRanges") -func validateRouteTablePermissions(ctx context.Context, log *logrus.Entry, oc *api.OpenShiftCluster, authorizer refreshable.Authorizer, client authorization.PermissionsClient, vnet *mgmtnetwork.VirtualNetwork, code, typ string) error { - err := validateRouteTablePermissionsSubnet(ctx, log, authorizer, client, vnet, oc.Properties.MasterProfile.SubnetID, "properties.masterProfile.subnetId", code, typ) - if err != nil { - return err - } + var subnets []string + var CIDRArray []*net.IPNet - for i, s := range oc.Properties.WorkerProfiles { - err := validateRouteTablePermissionsSubnet(ctx, log, authorizer, client, vnet, s.SubnetID, "properties.workerProfiles["+strconv.Itoa(i)+"].subnetId", code, typ) - if err != nil { - return err + // unique names of subnets from all node pools + for i, subnet := range dv.oc.Properties.WorkerProfiles { + exists := false + for _, s := range subnets { + if strings.EqualFold(strings.ToLower(subnet.SubnetID), strings.ToLower(s)) { + exists = true + break + } } - } - return nil -} -func validateRouteTablePermissionsSubnet(ctx context.Context, log *logrus.Entry, authorizer refreshable.Authorizer, client authorization.PermissionsClient, vnet *mgmtnetwork.VirtualNetwork, subnetID, path, code, typ string) error { - log.Printf("validateRouteTablePermissionsSubnet(%s, %s)", typ, path) + if !exists { + subnets = append(subnets, subnet.SubnetID) + path := fmt.Sprintf("properties.workerProfiles[%d].subnetId", i) + c, err := dv.validateSubnet(ctx, vnet, path, subnet.SubnetID) + if err != nil { + return err + } - var s *mgmtnetwork.Subnet - for _, ss := range *vnet.Subnets { - if strings.EqualFold(*ss.ID, subnetID) { - s = &ss - break + CIDRArray = append(CIDRArray, c) } } - if s == nil { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The subnet '%s' could not be found.", subnetID) - } - if s.RouteTable == nil { - return nil + masterCIDR, err := dv.validateSubnet(ctx, vnet, "properties.MasterProfile.subnetId", dv.oc.Properties.MasterProfile.SubnetID) + if err != nil { + return err } - - rtr, err := azure.ParseResourceID(*s.RouteTable.ID) + _, podCIDR, err := net.ParseCIDR(dv.oc.Properties.NetworkProfile.PodCIDR) if err != nil { return err } - err = validateActions(ctx, log, &rtr, []string{ - "Microsoft.Network/routeTables/join/action", - "Microsoft.Network/routeTables/read", - "Microsoft.Network/routeTables/write", - }, authorizer, client) - if err == wait.ErrWaitTimeout { - return api.NewCloudError(http.StatusBadRequest, code, "", "The %s does not have Network Contributor permission on route table '%s'.", typ, *s.RouteTable.ID) + _, serviceCIDR, err := net.ParseCIDR(dv.oc.Properties.NetworkProfile.ServiceCIDR) + if err != nil { + return err } - if detailedErr, ok := err.(autorest.DetailedError); ok && - detailedErr.StatusCode == http.StatusNotFound { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedRouteTable, "", "The route table '%s' could not be found.", *s.RouteTable.ID) + + CIDRArray = append(CIDRArray, masterCIDR, podCIDR, serviceCIDR) + + err = cidr.VerifyNoOverlap(CIDRArray, &net.IPNet{IP: net.IPv4zero, Mask: net.IPMask(net.IPv4zero)}) + if err != nil { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The provided CIDRs must not overlap: '%s'.", err) } - return err + + return nil } -func validateSubnet(ctx context.Context, log *logrus.Entry, oc *api.OpenShiftCluster, vnet *mgmtnetwork.VirtualNetwork, path, subnetID string) (*net.IPNet, error) { - log.Printf("validateSubnet (%s)", path) +func (dv *openShiftClusterDynamicValidator) validateSubnet(ctx context.Context, vnet *mgmtnetwork.VirtualNetwork, path, subnetID string) (*net.IPNet, error) { + dv.log.Printf("validateSubnet (%s)", path) - var s *mgmtnetwork.Subnet - if vnet.Subnets != nil { - for _, ss := range *vnet.Subnets { - if strings.EqualFold(*ss.ID, subnetID) { - s = &ss - break - } - } - } + s := findSubnet(vnet, subnetID) if s == nil { return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The provided subnet '%s' could not be found.", subnetID) } - if strings.EqualFold(oc.Properties.MasterProfile.SubnetID, subnetID) { + if strings.EqualFold(dv.oc.Properties.MasterProfile.SubnetID, subnetID) { if s.PrivateLinkServiceNetworkPolicies == nil || !strings.EqualFold(*s.PrivateLinkServiceNetworkPolicies, "Disabled") { return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The provided subnet '%s' is invalid: must have privateLinkServiceNetworkPolicies disabled.", subnetID) @@ -337,14 +227,14 @@ func validateSubnet(ctx context.Context, log *logrus.Entry, oc *api.OpenShiftClu return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The provided subnet '%s' is invalid: must have Microsoft.ContainerRegistry serviceEndpoint.", subnetID) } - if oc.Properties.ProvisioningState == api.ProvisioningStateCreating { + if dv.oc.Properties.ProvisioningState == api.ProvisioningStateCreating { if s.SubnetPropertiesFormat != nil && s.SubnetPropertiesFormat.NetworkSecurityGroup != nil { return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The provided subnet '%s' is invalid: must not have a network security group attached.", subnetID) } } else { - nsgID, err := subnet.NetworkSecurityGroupID(oc, *s.ID) + nsgID, err := subnet.NetworkSecurityGroupID(dv.oc, *s.ID) if err != nil { return nil, err } @@ -360,81 +250,19 @@ func validateSubnet(ctx context.Context, log *logrus.Entry, oc *api.OpenShiftClu if err != nil { return nil, err } - { - ones, _ := net.Mask.Size() - if ones > 27 { - return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The provided subnet '%s' is invalid: must be /27 or larger.", subnetID) - } - } - - return net, nil -} -// validateVnet checks that the vnet does not have custom dns servers set -// and the subnets do not overlap with cluster pod/service CIDR blocks -func validateVnet(ctx context.Context, log *logrus.Entry, oc *api.OpenShiftCluster, vnet *mgmtnetwork.VirtualNetwork) error { - log.Print("validateVnet") - var err error - - if !strings.EqualFold(*vnet.Location, oc.Location) { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The vnet location '%s' must match the cluster location '%s'.", *vnet.Location, oc.Location) + ones, _ := net.Mask.Size() + if ones > 27 { + return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, path, "The provided subnet '%s' is invalid: must be /27 or larger.", subnetID) } - // unique names of subnets from all node pools - var subnets []string - var CIDRArray []*net.IPNet - for i, subnet := range oc.Properties.WorkerProfiles { - exists := false - for _, s := range subnets { - if strings.EqualFold(strings.ToLower(subnet.SubnetID), strings.ToLower(s)) { - exists = true - break - } - } - if !exists { - subnets = append(subnets, subnet.SubnetID) - c, err := validateSubnet(ctx, log, oc, vnet, "properties.workerProfiles["+strconv.Itoa(i)+"].subnetId", subnet.SubnetID) - if err != nil { - return err - } - CIDRArray = append(CIDRArray, c) - } - } - masterSubnetCIDR, err := validateSubnet(ctx, log, oc, vnet, "properties.masterProfile.subnetId", oc.Properties.MasterProfile.SubnetID) - if err != nil { - return err - } - - _, podCIDR, err := net.ParseCIDR(oc.Properties.NetworkProfile.PodCIDR) - if err != nil { - return err - } - - _, serviceCIDR, err := net.ParseCIDR(oc.Properties.NetworkProfile.ServiceCIDR) - if err != nil { - return err - } - - CIDRArray = append(CIDRArray, masterSubnetCIDR, podCIDR, serviceCIDR) - - err = cidr.VerifyNoOverlap(CIDRArray, &net.IPNet{IP: net.IPv4zero, Mask: net.IPMask(net.IPv4zero)}) - if err != nil { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The provided CIDRs must not overlap: '%s'.", err) - } - - if vnet.DhcpOptions != nil && - vnet.DhcpOptions.DNSServers != nil && - len(*vnet.DhcpOptions.DNSServers) > 0 { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidLinkedVNet, "", "The provided vnet '%s' is invalid: custom DNS servers are not supported.", *vnet.ID) - } - - return nil + return net, nil } -func validateProviders(ctx context.Context, log *logrus.Entry, spProviders features.ProvidersClient) error { - log.Print("validateProviders") +func (dv *openShiftClusterDynamicValidator) validateProviders(ctx context.Context) error { + dv.log.Print("validateProviders") - providers, err := spProviders.List(ctx, nil, "") + providers, err := dv.providers.List(ctx, nil, "") if err != nil { return err } @@ -460,28 +288,38 @@ func validateProviders(ctx context.Context, log *logrus.Entry, spProviders featu return nil } -func validateActions(ctx context.Context, log *logrus.Entry, r *azure.Resource, actions []string, authorizer refreshable.Authorizer, client authorization.PermissionsClient) error { - timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Minute) - defer cancel() +func validateServicePrincipalProfile(ctx context.Context, log *logrus.Entry, env env.Interface, oc *api.OpenShiftCluster, sub *api.SubscriptionDocument) (refreshable.Authorizer, error) { + log.Print("validateServicePrincipalProfile") - return wait.PollImmediateUntil(10*time.Second, func() (bool, error) { - permissions, err := client.ListForResource(ctx, r.ResourceGroup, r.Provider, "", r.ResourceType, r.ResourceName) - if detailedErr, ok := err.(autorest.DetailedError); ok && - detailedErr.StatusCode == http.StatusForbidden { - _, err = authorizer.RefreshWithContext(ctx, log) - return false, err - } - if err != nil { - return false, err + token, err := aad.GetToken(ctx, log, oc, sub, env.Environment().ResourceManagerEndpoint) + if err != nil { + return nil, err + } + + p := &jwt.Parser{} + c := &azureClaim{} + _, _, err = p.ParseUnverified(token.OAuthToken(), c) + if err != nil { + return nil, err + } + + for _, role := range c.Roles { + if role == "Application.ReadWrite.OwnedBy" { + return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalCredentials, "properties.servicePrincipalProfile", "The provided service principal must not have the Application.ReadWrite.OwnedBy permission.") } + } + + return refreshable.NewAuthorizer(token), nil +} - for _, action := range actions { - ok, err := utilpermissions.CanDoAction(permissions, action) - if !ok || err != nil { - return false, err +func findSubnet(vnet *mgmtnetwork.VirtualNetwork, subnetID string) *mgmtnetwork.Subnet { + if vnet.Subnets != nil { + for _, s := range *vnet.Subnets { + if strings.EqualFold(*s.ID, subnetID) { + return &s } } + } - return true, nil - }, timeoutCtx.Done()) + return nil } diff --git a/pkg/api/validate/openshiftcluster_validatedynamic_test.go b/pkg/api/validate/openshiftcluster_validatedynamic_test.go index d10bfc92e2a..f6ca3fe3626 100644 --- a/pkg/api/validate/openshiftcluster_validatedynamic_test.go +++ b/pkg/api/validate/openshiftcluster_validatedynamic_test.go @@ -6,22 +6,16 @@ package validate import ( "context" "errors" - "net/http" "testing" mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-07-01/network" - mgmtauthorization "github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-09-01-preview/authorization" mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features" - "github.com/Azure/go-autorest/autorest" - "github.com/Azure/go-autorest/autorest/azure" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" "github.com/sirupsen/logrus" "github.com/Azure/ARO-RP/pkg/api" - mock_authorization "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/authorization" mock_features "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/features" - mockrefreshable "github.com/Azure/ARO-RP/pkg/util/mocks/refreshable" ) func TestValidateProviders(t *testing.T) { @@ -36,7 +30,7 @@ func TestValidateProviders(t *testing.T) { wantErr string }{ { - name: "all registered", + name: "pass", mocks: func(providersClient *mock_features.MockProvidersClient) { providersClient.EXPECT(). List(gomock.Any(), nil, ""). @@ -69,7 +63,7 @@ func TestValidateProviders(t *testing.T) { }, }, { - name: "compute not registered", + name: "fail: compute not registered", mocks: func(providersClient *mock_features.MockProvidersClient) { providersClient.EXPECT(). List(gomock.Any(), nil, ""). @@ -103,7 +97,7 @@ func TestValidateProviders(t *testing.T) { wantErr: "400: ResourceProviderNotRegistered: : The resource provider 'Microsoft.Compute' is not registered.", }, { - name: "storage missing", + name: "fail: storage missing", mocks: func(providersClient *mock_features.MockProvidersClient) { providersClient.EXPECT(). List(gomock.Any(), nil, ""). @@ -147,7 +141,51 @@ func TestValidateProviders(t *testing.T) { tt.mocks(providerClient) - err := validateProviders(ctx, logrus.NewEntry(logrus.StandardLogger()), providerClient) + dv := &openShiftClusterDynamicValidator{ + log: logrus.NewEntry(logrus.StandardLogger()), + providers: providerClient, + } + + err := dv.validateProviders(ctx) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } + }) + } +} + +func TestValidateVnetLocation(t *testing.T) { + ctx := context.Background() + + for _, tt := range []struct { + name string + location string + wantErr string + }{ + { + name: "pass", + location: "eastus", + }, + { + name: "fail: location differs", + location: "neverland", + wantErr: "400: InvalidLinkedVNet: : The vnet location 'neverland' must match the cluster location 'eastus'.", + }, + } { + t.Run(tt.name, func(t *testing.T) { + dv := &openShiftClusterDynamicValidator{ + log: logrus.NewEntry(logrus.StandardLogger()), + oc: &api.OpenShiftCluster{ + Location: "eastus", + }, + } + + vnet := &mgmtnetwork.VirtualNetwork{ + Location: to.StringPtr(tt.location), + } + + err := dv.validateVnetLocation(ctx, vnet) if err != nil && err.Error() != tt.wantErr || err == nil && tt.wantErr != "" { t.Error(err) @@ -156,17 +194,13 @@ func TestValidateProviders(t *testing.T) { } } -func TestValidateVnet(t *testing.T) { +func TestValidateSubnet(t *testing.T) { ctx := context.Background() resourceGroupID := "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" vnetID := resourceGroupID + "/providers/Microsoft.Network/virtualNetworks/testVnet" - masterSubnet := vnetID + "/subnet/masterSubnet" - workerSubnet := vnetID + "/subnet/workerSubnet" - workerSubnet2 := vnetID + "/subnet/workerSubnet2" + genericSubnet := vnetID + "/subnet/genericSubnet" masterNSGv1 := resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-controlplane-nsg" - workerNSGv1 := resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-node-nsg" - commonNSGv2 := resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-nsg" for _, tt := range []struct { name string @@ -178,508 +212,248 @@ func TestValidateVnet(t *testing.T) { name: "pass", }, { - name: "pass (creating)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].NetworkSecurityGroup = nil - (*vnet.Subnets)[1].NetworkSecurityGroup = nil - }, + name: "pass (master subnet)", modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.ProvisioningState = api.ProvisioningStateCreating - }, - }, - { - name: "missing subnet (master)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - vnet.Location = to.StringPtr("westeurope") - }, - wantErr: "400: InvalidLinkedVNet: : The vnet location 'westeurope' must match the cluster location 'eastus'.", - }, - { - name: "missing subnet (master)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - *vnet.Subnets = (*vnet.Subnets)[1:] - }, - wantErr: "400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' could not be found.", - }, - { - name: "missing subnet (worker)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - *vnet.Subnets = (*vnet.Subnets)[:1] - }, - wantErr: `400: InvalidLinkedVNet: properties.workerProfiles[0].subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/workerSubnet' could not be found.`, - }, - { - name: "invalid PLS network policy (master)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].PrivateLinkServiceNetworkPolicies = nil - }, - wantErr: "400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' is invalid: must have privateLinkServiceNetworkPolicies disabled.", - }, - { - name: "invalid service endpoint (master)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].ServiceEndpoints = nil - }, - wantErr: "400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' is invalid: must have Microsoft.ContainerRegistry serviceEndpoint.", - }, - { - name: "invalid service endpoint (worker)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[1].ServiceEndpoints = nil - }, - wantErr: `400: InvalidLinkedVNet: properties.workerProfiles[0].subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/workerSubnet' is invalid: must have Microsoft.ContainerRegistry serviceEndpoint.`, - }, - { - name: "invalid master nsg arch v1", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].NetworkSecurityGroup = nil - }, - wantErr: `400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' is invalid: must have network security group '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/networkSecurityGroups/aro-controlplane-nsg' attached.`, - }, - { - name: "invalid master nsg arch v2", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].NetworkSecurityGroup = nil - (*vnet.Subnets)[1].NetworkSecurityGroup.ID = &commonNSGv2 - }, - modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.ArchitectureVersion = api.ArchitectureVersionV2 - }, - wantErr: `400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' is invalid: must have network security group '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/networkSecurityGroups/aro-nsg' attached.`, - }, - { - name: "invalid worker nsg arch v1", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[1].NetworkSecurityGroup = nil - }, - wantErr: `400: InvalidLinkedVNet: properties.workerProfiles[0].subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/workerSubnet' is invalid: must have network security group '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/networkSecurityGroups/aro-node-nsg' attached.`, - }, - { - name: "invalid worker nsg arch v2", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].NetworkSecurityGroup.ID = &commonNSGv2 - (*vnet.Subnets)[1].NetworkSecurityGroup = nil - }, - modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.ArchitectureVersion = api.ArchitectureVersionV2 + oc.Properties.MasterProfile = api.MasterProfile{ + SubnetID: genericSubnet, + } }, - wantErr: `400: InvalidLinkedVNet: properties.workerProfiles[0].subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/workerSubnet' is invalid: must have network security group '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/networkSecurityGroups/aro-nsg' attached.`, }, { - name: "invalid master nsg (creating)", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[1].NetworkSecurityGroup = nil - }, + name: "pass (cluster in creating provisioning status)", modifyOC: func(oc *api.OpenShiftCluster) { oc.Properties.ProvisioningState = api.ProvisioningStateCreating }, - wantErr: `400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' is invalid: must not have a network security group attached.`, - }, - { - name: "invalid worker nsg (creating)", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { (*vnet.Subnets)[0].NetworkSecurityGroup = nil }, - modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.ProvisioningState = api.ProvisioningStateCreating - }, - wantErr: `400: InvalidLinkedVNet: properties.workerProfiles[0].subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/workerSubnet' is invalid: must not have a network security group attached.`, - }, - { - name: "invalid master subnet size", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].AddressPrefix = to.StringPtr("10.0.0.0/28") - }, - wantErr: "400: InvalidLinkedVNet: properties.masterProfile.subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/masterSubnet' is invalid: must be /27 or larger.", }, { - name: "invalid worker subnet size", + name: "fail: subnet doe not exist on vnet", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[1].AddressPrefix = to.StringPtr("10.0.0.0/28") + vnet.Subnets = nil }, - wantErr: `400: InvalidLinkedVNet: properties.workerProfiles[0].subnetId: The provided subnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet/subnet/workerSubnet' is invalid: must be /27 or larger.`, + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' could not be found.", }, { - name: "master and worker subnets overlap", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].AddressPrefix = (*vnet.Subnets)[1].AddressPrefix + name: "fail: private link service network policies enabled on master subnet", + modifyOC: func(oc *api.OpenShiftCluster) { + oc.Properties.MasterProfile = api.MasterProfile{ + SubnetID: genericSubnet, + } }, - wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.1.0/24 overlaps with 10.0.1.0/24'.", - }, - { - name: "master and pod subnets overlap", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].AddressPrefix = to.StringPtr("10.0.2.0/24") + (*vnet.Subnets)[0].PrivateLinkServiceNetworkPolicies = to.StringPtr("Enabled") }, - wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.2.0/24 overlaps with 10.0.2.0/24'.", + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' is invalid: must have privateLinkServiceNetworkPolicies disabled.", }, { - name: "worker and pod subnets overlap", + name: "fail: container registry endpoint doesn't exist", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[1].AddressPrefix = to.StringPtr("10.0.2.0/24") + (*vnet.Subnets)[0].ServiceEndpoints = nil }, - wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.2.0/24 overlaps with 10.0.2.0/24'.", + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' is invalid: must have Microsoft.ContainerRegistry serviceEndpoint.", }, { - name: "master and service subnets overlap", + name: "fail: network provisioning state not succeeded", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].AddressPrefix = to.StringPtr("10.0.3.0/24") + (*(*vnet.Subnets)[0].ServiceEndpoints)[0].ProvisioningState = mgmtnetwork.Failed }, - wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.3.0/24 overlaps with 10.0.3.0/24'.", + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' is invalid: must have Microsoft.ContainerRegistry serviceEndpoint.", }, { - name: "worker and service subnets overlap", - modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[1].AddressPrefix = to.StringPtr("10.0.3.0/24") + name: "fail: provisioning state creating: subnet has NSG", + modifyOC: func(oc *api.OpenShiftCluster) { + oc.Properties.ProvisioningState = api.ProvisioningStateCreating }, - wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.3.0/24 overlaps with 10.0.3.0/24'.", + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' is invalid: must not have a network security group attached.", }, { - name: "two worker pools on the same subnet", + name: "fail: invalid architecture version returns no NSG", modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.WorkerProfiles = append(oc.Properties.WorkerProfiles, api.WorkerProfile{ - SubnetID: workerSubnet, - }) + oc.Properties.ArchitectureVersion = 9001 }, - wantErr: "", + wantErr: "unknown architecture version 9001", }, { - name: "two worker pools on the on two different subnets which do not overlap with pod or service cidr", - modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.WorkerProfiles = append(oc.Properties.WorkerProfiles, api.WorkerProfile{ - SubnetID: workerSubnet2, - }) + name: "fail: nsg id doesn't match expected", + modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { + (*vnet.Subnets)[0].NetworkSecurityGroup.ID = to.StringPtr("not matching") }, - wantErr: "", + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' is invalid: must have network security group '" + masterNSGv1 + "' attached.", }, { - name: "two worker pools on the on two different subnets one of which overlaps with pod or service cidr", + name: "fail: invalid subnet CIDR", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[2].AddressPrefix = to.StringPtr("10.0.3.0/24") - }, - modifyOC: func(oc *api.OpenShiftCluster) { - oc.Properties.WorkerProfiles = append(oc.Properties.WorkerProfiles, api.WorkerProfile{ - SubnetID: workerSubnet2, - }) + (*vnet.Subnets)[0].AddressPrefix = to.StringPtr("not-valid") }, - wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.3.0/24 overlaps with 10.0.3.0/24'.", + wantErr: "invalid CIDR address: not-valid", }, { - name: "custom dns set", + name: "fail: too small subnet CIDR", modifyVnet: func(vnet *mgmtnetwork.VirtualNetwork) { - vnet.DhcpOptions.DNSServers = &[]string{ - "172.16.1.1", - } + (*vnet.Subnets)[0].AddressPrefix = to.StringPtr("10.0.0.0/28") }, - wantErr: "400: InvalidLinkedVNet: : The provided vnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet' is invalid: custom DNS servers are not supported.", + wantErr: "400: InvalidLinkedVNet: : The provided subnet '" + genericSubnet + "' is invalid: must be /27 or larger.", }, } { - t.Run(tt.name, func(t *testing.T) { - oc := &api.OpenShiftCluster{ - Location: "eastus", - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - NetworkProfile: api.NetworkProfile{ - PodCIDR: "10.0.2.0/24", - ServiceCIDR: "10.0.3.0/24", - }, - MasterProfile: api.MasterProfile{ - SubnetID: masterSubnet, - }, - WorkerProfiles: []api.WorkerProfile{ - { - SubnetID: workerSubnet, - }, - }, + oc := &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + ResourceGroupID: resourceGroupID, }, - } - - vnet := &mgmtnetwork.VirtualNetwork{ - Location: to.StringPtr("eastus"), - ID: &vnetID, - VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{ - DhcpOptions: &mgmtnetwork.DhcpOptions{ - DNSServers: &[]string{}, - }, - Subnets: &[]mgmtnetwork.Subnet{ - { - ID: &masterSubnet, - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - AddressPrefix: to.StringPtr("10.0.0.0/24"), - NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{ - ID: &masterNSGv1, - }, - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - ProvisioningState: mgmtnetwork.Succeeded, - }, - }, - PrivateLinkServiceNetworkPolicies: to.StringPtr("Disabled"), - }, - }, - { - ID: &workerSubnet, - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - AddressPrefix: to.StringPtr("10.0.1.0/24"), - NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{ - ID: &workerNSGv1, - }, - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - ProvisioningState: mgmtnetwork.Succeeded, - }, - }, + }, + } + vnet := &mgmtnetwork.VirtualNetwork{ + ID: &vnetID, + VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{ + Subnets: &[]mgmtnetwork.Subnet{ + { + ID: &genericSubnet, + SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ + AddressPrefix: to.StringPtr("10.0.0.0/24"), + NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{ + ID: &masterNSGv1, }, - }, - { - ID: &workerSubnet2, - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - AddressPrefix: to.StringPtr("10.0.4.0/24"), - NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{ - ID: &workerNSGv1, - }, - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - ProvisioningState: mgmtnetwork.Succeeded, - }, + ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ + { + Service: to.StringPtr("Microsoft.ContainerRegistry"), + ProvisioningState: mgmtnetwork.Succeeded, }, }, + PrivateLinkServiceNetworkPolicies: to.StringPtr("Disabled"), }, }, }, - } - - if tt.modifyOC != nil { - tt.modifyOC(oc) - } - if tt.modifyVnet != nil { - tt.modifyVnet(vnet) - } - - err := validateVnet(ctx, logrus.NewEntry(logrus.StandardLogger()), oc, vnet) - if err != nil && err.Error() != tt.wantErr || - err == nil && tt.wantErr != "" { - t.Error(err) - } - }) - } -} -func TestValidateVnetPermissions(t *testing.T) { - ctx := context.Background() - - resourceGroupID := "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" - vnetID := resourceGroupID + "/providers/Microsoft.Network/virtualNetworks/testVnet" - - controller := gomock.NewController(t) - defer controller.Finish() - - for _, tt := range []struct { - name string - mocks func(*mock_authorization.MockPermissionsClient, func()) - wantErr string - }{ - { - name: "pass", - mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { - permissionsClient.EXPECT(). - ListForResource(gomock.Any(), "", "", "", "", ""). - Return([]mgmtauthorization.Permission{ - { - Actions: &[]string{ - "Microsoft.Network/virtualNetworks/join/action", - "Microsoft.Network/virtualNetworks/read", - "Microsoft.Network/virtualNetworks/write", - "Microsoft.Network/virtualNetworks/subnets/join/action", - "Microsoft.Network/virtualNetworks/subnets/read", - "Microsoft.Network/virtualNetworks/subnets/write", - }, - NotActions: &[]string{}, - }, - }, nil) }, - }, - { - name: "fail: missing permissions", - mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { - permissionsClient.EXPECT(). - ListForResource(gomock.Any(), "", "", "", "", ""). - Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { - cancel() - }). - Return( - []mgmtauthorization.Permission{ - { - Actions: &[]string{}, - NotActions: &[]string{}, - }, - }, - nil, - ) - }, - wantErr: "400: InvalidResourceProviderPermissions: : The resource provider does not have Network Contributor permission on vnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet'.", - }, - { - name: "fail: not found", - mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { - permissionsClient.EXPECT(). - ListForResource(gomock.Any(), "", "", "", "", ""). - Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { - cancel() - }). - Return( - nil, - autorest.DetailedError{ - StatusCode: http.StatusNotFound, - }, - ) - }, - wantErr: "400: InvalidLinkedVNet: : The vnet '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/virtualNetworks/testVnet' could not be found.", - }, - } { - t.Run(tt.name, func(t *testing.T) { - ctx, cancel := context.WithCancel(ctx) - defer cancel() + } - permissionsClient := mock_authorization.NewMockPermissionsClient(controller) + if tt.modifyOC != nil { + tt.modifyOC(oc) + } + if tt.modifyVnet != nil { + tt.modifyVnet(vnet) + } - tt.mocks(permissionsClient, cancel) + dv := &openShiftClusterDynamicValidator{ + log: logrus.NewEntry(logrus.StandardLogger()), + oc: oc, + } - err := validateVnetPermissions(ctx, logrus.NewEntry(logrus.StandardLogger()), mockrefreshable.NewMockAuthorizer(controller), permissionsClient, vnetID, &azure.Resource{}, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider") - if err != nil && err.Error() != tt.wantErr || - err == nil && tt.wantErr != "" { - t.Error(err) - } - }) + // purposefully hardcoding path to "" so it is not needed in the wantErr message + _, err := dv.validateSubnet(ctx, vnet, "", genericSubnet) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } } } -func TestValidateRouteTablePermissionsSubnet(t *testing.T) { +func TestValidateCIDRRanges(t *testing.T) { ctx := context.Background() resourceGroupID := "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" vnetID := resourceGroupID + "/providers/Microsoft.Network/virtualNetworks/testVnet" masterSubnet := vnetID + "/subnet/masterSubnet" - rtID := resourceGroupID + "/providers/Microsoft.Network/routeTables/testRT" - - controller := gomock.NewController(t) - defer controller.Finish() + workerSubnet := vnetID + "/subnet/workerSubnet" + masterNSGv1 := resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-controlplane-nsg" + workerNSGv1 := resourceGroupID + "/providers/Microsoft.Network/networkSecurityGroups/aro-node-nsg" for _, tt := range []struct { - name string - mocks func(*mock_authorization.MockPermissionsClient, func()) - vnet func(*mgmtnetwork.VirtualNetwork) - subnet string - wantErr string + name string + modifyOC func(*api.OpenShiftCluster) + modifyVnet func(*mgmtnetwork.VirtualNetwork) + wantErr string }{ { name: "pass", - mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { - permissionsClient.EXPECT(). - ListForResource(gomock.Any(), "testGroup", "Microsoft.Network", "", "routeTables", "testRT"). - Return([]mgmtauthorization.Permission{ - { - Actions: &[]string{ - "Microsoft.Network/routeTables/join/action", - "Microsoft.Network/routeTables/read", - "Microsoft.Network/routeTables/write", - }, - NotActions: &[]string{}, - }, - }, nil) - }, - subnet: masterSubnet, - }, - { - name: "pass (no route table)", - vnet: func(vnet *mgmtnetwork.VirtualNetwork) { - (*vnet.Subnets)[0].RouteTable = nil - }, - subnet: masterSubnet, }, { - name: "fail: missing permissions", - mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { - permissionsClient.EXPECT(). - ListForResource(gomock.Any(), "testGroup", "Microsoft.Network", "", "routeTables", "testRT"). - Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { - cancel() - }). - Return( - []mgmtauthorization.Permission{ - { - Actions: &[]string{}, - NotActions: &[]string{}, - }, - }, - nil, - ) - }, - subnet: masterSubnet, - wantErr: "400: InvalidResourceProviderPermissions: : The resource provider does not have Network Contributor permission on route table '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/routeTables/testRT'.", - }, - { - name: "fail: not found", - mocks: func(permissionsClient *mock_authorization.MockPermissionsClient, cancel func()) { - permissionsClient.EXPECT(). - ListForResource(gomock.Any(), "testGroup", "Microsoft.Network", "", "routeTables", "testRT"). - Do(func(arg0, arg1, arg2, arg3, arg4, arg5 interface{}) { - cancel() - }). - Return( - nil, - autorest.DetailedError{ - StatusCode: http.StatusNotFound, - }, - ) + name: "fail: conflicting ranges", + modifyOC: func(oc *api.OpenShiftCluster) { + oc.Properties.NetworkProfile.ServiceCIDR = "10.0.0.0/24" }, - subnet: masterSubnet, - wantErr: "400: InvalidLinkedRouteTable: : The route table '/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/routeTables/testRT' could not be found.", - }, - { - name: "fail: subnet not found", - subnet: "doesnotexist", - wantErr: "400: InvalidLinkedVNet: properties.masterProfile.subnetId: The subnet 'doesnotexist' could not be found.", + wantErr: "400: InvalidLinkedVNet: : The provided CIDRs must not overlap: '10.0.0.0/24 overlaps with 10.0.0.0/24'.", }, } { - t.Run(tt.name, func(t *testing.T) { - ctx, cancel := context.WithCancel(ctx) - defer cancel() + oc := &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + ResourceGroupID: resourceGroupID, + }, + NetworkProfile: api.NetworkProfile{ + PodCIDR: "10.0.2.0/24", + ServiceCIDR: "10.0.3.0/24", + }, + MasterProfile: api.MasterProfile{ + SubnetID: masterSubnet, + }, + WorkerProfiles: []api.WorkerProfile{ + { + SubnetID: workerSubnet, + }, + { + SubnetID: workerSubnet, + }, + }, + }, + } - vnet := &mgmtnetwork.VirtualNetwork{ - VirtualNetworkPropertiesFormat: &mgmtnetwork.VirtualNetworkPropertiesFormat{ - Subnets: &[]mgmtnetwork.Subnet{ - { - ID: &masterSubnet, - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - RouteTable: &mgmtnetwork.RouteTable{ - ID: &rtID, + 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, + }, + ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ + { + Service: to.StringPtr("Microsoft.ContainerRegistry"), + ProvisioningState: mgmtnetwork.Succeeded, + }, + }, + PrivateLinkServiceNetworkPolicies: to.StringPtr("Disabled"), + }, + }, + { + ID: &workerSubnet, + SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ + AddressPrefix: to.StringPtr("10.0.1.0/24"), + NetworkSecurityGroup: &mgmtnetwork.SecurityGroup{ + ID: &workerNSGv1, + }, + ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ + { + Service: to.StringPtr("Microsoft.ContainerRegistry"), + ProvisioningState: mgmtnetwork.Succeeded, }, }, }, }, }, - } - - permissionsClient := mock_authorization.NewMockPermissionsClient(controller) + }, + } - if tt.mocks != nil { - tt.mocks(permissionsClient, cancel) - } + if tt.modifyOC != nil { + tt.modifyOC(oc) + } + if tt.modifyVnet != nil { + tt.modifyVnet(vnet) + } - if tt.vnet != nil { - tt.vnet(vnet) - } + dv := &openShiftClusterDynamicValidator{ + log: logrus.NewEntry(logrus.StandardLogger()), + oc: oc, + } - err := validateRouteTablePermissionsSubnet(ctx, logrus.NewEntry(logrus.StandardLogger()), mockrefreshable.NewMockAuthorizer(controller), permissionsClient, vnet, tt.subnet, "properties.masterProfile.subnetId", api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider") - if err != nil && err.Error() != tt.wantErr || - err == nil && tt.wantErr != "" { - t.Error(err) - } - }) + err := dv.validateCIDRRanges(ctx, vnet) + if err != nil && err.Error() != tt.wantErr || + err == nil && tt.wantErr != "" { + t.Error(err) + } } } diff --git a/pkg/api/validate/quota.go b/pkg/api/validate/quota.go index c1fad951932..d90f72cf22a 100644 --- a/pkg/api/validate/quota.go +++ b/pkg/api/validate/quota.go @@ -101,7 +101,7 @@ func NewAzureQuotaValidator(ctx context.Context, log *logrus.Entry, env env.Inte return nil, err } - spAuthorizer, err := validateServicePrincipalProfile(ctx, log, &oc.Properties.ServicePrincipalProfile, subscriptionDoc.Subscription.Properties.TenantID, env.Environment().ResourceManagerEndpoint) + spAuthorizer, err := validateServicePrincipalProfile(ctx, log, env, oc, subscriptionDoc) if err != nil { return nil, err } diff --git a/pkg/cluster/deploystorage.go b/pkg/cluster/deploystorage.go index 47b10499fdb..375933e37c5 100644 --- a/pkg/cluster/deploystorage.go +++ b/pkg/cluster/deploystorage.go @@ -47,7 +47,7 @@ func (m *manager) clusterSPObjectID(ctx context.Context) (string, error) { var clusterSPObjectID string spp := &m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile - token, err := aad.GetToken(ctx, m.log, &m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile, m.subscriptionDoc.Subscription.Properties.TenantID, m.env.Environment().GraphEndpoint) + token, err := aad.GetToken(ctx, m.log, m.doc.OpenShiftCluster, m.subscriptionDoc, m.env.Environment().GraphEndpoint) if err != nil { return "", err } diff --git a/pkg/cluster/validate.go b/pkg/cluster/validate.go index 7b8f6dc1167..ce6ba088f96 100644 --- a/pkg/cluster/validate.go +++ b/pkg/cluster/validate.go @@ -11,7 +11,7 @@ import ( ) func (m *manager) validateResources(ctx context.Context) error { - ocDynamicValidator := validate.NewOpenShiftClusterFullDynamicValidator( + ocDynamicValidator := validate.NewOpenShiftClusterDynamicValidator( m.log, m.env, m.doc.OpenShiftCluster, m.subscriptionDoc, m.fpAuthorizer, ) return ocDynamicValidator.Dynamic(ctx) diff --git a/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go b/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go index bc994fd7de4..1be22e4b221 100644 --- a/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go +++ b/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go @@ -39,8 +39,9 @@ type ClusterSpec struct { Location string `json:"location,omitempty"` GenevaLogging GenevaLoggingSpec `json:"genevaLogging,omitempty"` InternetChecker InternetCheckerSpec `json:"internetChecker,omitempty"` - VNetID string `json:"vnetId,omitempty"` - Features FeaturesSpec `json:"features,omitempty"` + VnetID string `json:"vnetId,omitempty"` + + Features FeaturesSpec `json:"features,omitempty"` } // FeaturesSpec defines ARO operator feature gates diff --git a/pkg/operator/deploy/deploy.go b/pkg/operator/deploy/deploy.go index 84bc18be171..33b5960816f 100644 --- a/pkg/operator/deploy/deploy.go +++ b/pkg/operator/deploy/deploy.go @@ -167,7 +167,7 @@ func (o *operator) resources() ([]runtime.Object, error) { ACRDomain: o.env.ACRDomain(), AZEnvironment: o.env.Environment().Name, Location: o.env.Location(), - VNetID: vnetID, + VnetID: vnetID, GenevaLogging: arov1alpha1.GenevaLoggingSpec{ ConfigVersion: o.env.ClustersGenevaLoggingConfigVersion(), MonitoringGCSEnvironment: o.env.ClustersGenevaLoggingEnvironment(), diff --git a/pkg/util/aad/aad.go b/pkg/util/aad/aad.go index 74979a6ec1c..071bad8be09 100644 --- a/pkg/util/aad/aad.go +++ b/pkg/util/aad/aad.go @@ -20,8 +20,10 @@ import ( // GetToken authenticates in the customer's tenant as the cluster service // principal and returns a token. -func GetToken(ctx context.Context, log *logrus.Entry, spp *api.ServicePrincipalProfile, tenantID, resource string) (*adal.ServicePrincipalToken, error) { - conf := auth.NewClientCredentialsConfig(spp.ClientID, string(spp.ClientSecret), tenantID) +func GetToken(ctx context.Context, log *logrus.Entry, oc *api.OpenShiftCluster, subscriptionDoc *api.SubscriptionDocument, resource string) (*adal.ServicePrincipalToken, error) { + spp := &oc.Properties.ServicePrincipalProfile + + conf := auth.NewClientCredentialsConfig(spp.ClientID, string(spp.ClientSecret), subscriptionDoc.Subscription.Properties.TenantID) conf.Resource = resource sp, err := conf.ServicePrincipalToken() diff --git a/pkg/util/steps/refreshing.go b/pkg/util/steps/refreshing.go index 2199c311e32..fbb925dc62e 100644 --- a/pkg/util/steps/refreshing.go +++ b/pkg/util/steps/refreshing.go @@ -5,6 +5,7 @@ package steps import ( "context" + "errors" "fmt" "time" @@ -15,6 +16,8 @@ import ( "github.com/Azure/ARO-RP/pkg/util/refreshable" ) +var ErrWantRefresh = errors.New("want refresh") + // AuthorizationRefreshingAction returns a wrapper Step which will refresh // `authorizer` if the step returns an Azure AuthenticationError and rerun it. // The step will be retried until `retryTimeout` is hit. Any other error will be @@ -68,7 +71,8 @@ func (s authorizationRefreshingActionStep) run(ctx context.Context, log *logrus. // Don't refresh if we have timed out if timeoutCtx.Err() == nil && (azureerrors.HasAuthorizationFailedError(err) || - azureerrors.HasLinkedAuthorizationFailedError(err)) { + azureerrors.HasLinkedAuthorizationFailedError(err) || + err == ErrWantRefresh) { log.Print(err) // Try refreshing auth. if s.authorizer == nil {