diff --git a/pkg/cluster/ensureendpoints.go b/pkg/cluster/ensureendpoints.go index a39630d9bad..764933e2346 100644 --- a/pkg/cluster/ensureendpoints.go +++ b/pkg/cluster/ensureendpoints.go @@ -6,6 +6,11 @@ package cluster import ( "context" "fmt" + "strings" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v2" + "github.com/Azure/go-autorest/autorest/to" "github.com/Azure/ARO-RP/pkg/api" ) @@ -14,12 +19,35 @@ import ( // subnets for storage account access, but only if egress lockdown is // not enabled. func (m *manager) ensureServiceEndpoints(ctx context.Context) error { + // Only add service endpoints to the subnet if egress lockdown is not enabled. + if m.doc.OpenShiftCluster.Properties.FeatureProfile.GatewayEnabled { + return nil + } + subnetIds, err := m.getSubnetIds() if err != nil { return err } - return m.subnet.CreateOrUpdateFromIds(ctx, subnetIds, m.doc.OpenShiftCluster.Properties.FeatureProfile.GatewayEnabled) + for _, subnetId := range subnetIds { + r, err := arm.ParseResourceID(subnetId) + if err != nil { + return err + } + subnet, err := m.armSubnets.Get(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, nil) + if err != nil { + return err + } + shouldUpdate := addEndpointsToSubnet(api.SubnetsEndpoints, &subnet.Subnet) + if !shouldUpdate { + continue + } + err = m.armSubnets.CreateOrUpdateAndWait(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, subnet.Subnet, nil) + if err != nil { + return err + } + } + return nil } func (m *manager) getSubnetIds() ([]string, error) { @@ -36,3 +64,49 @@ func (m *manager) getSubnetIds() ([]string, error) { } return subnets, nil } + +// addEndpointsToSubnet adds the endpoints (that either are missing in subnet +// or aren't in succeeded state in the subnet) to the subnet and returns the updated subnet +func addEndpointsToSubnet(endpoints []string, subnet *armnetwork.Subnet) (subnetChanged bool) { + for _, endpoint := range endpoints { + endpointFound, serviceEndpointPtr := subnetContainsEndpoint(subnet, endpoint) + + if !endpointFound || *serviceEndpointPtr.ProvisioningState != armnetwork.ProvisioningStateSucceeded { + addEndpointToSubnet(endpoint, subnet) + subnetChanged = true + } + } + + return subnetChanged +} + +// subnetContainsEndpoint returns false and nil if subnet does not contain the endpoint. +// If the subnet does contain the endpoint, true and a pointer to the service endpoint +// is returned to be able to do additional checks and perform actions accordingly. +func subnetContainsEndpoint(subnet *armnetwork.Subnet, endpoint string) (endpointFound bool, serviceEndpointPtr *armnetwork.ServiceEndpointPropertiesFormat) { + if subnet == nil || subnet.Properties.ServiceEndpoints == nil { + return false, nil + } + + for _, serviceEndpoint := range subnet.Properties.ServiceEndpoints { + if endpointFound = strings.EqualFold(*serviceEndpoint.Service, endpoint); endpointFound { + return true, serviceEndpoint + } + } + + return false, nil +} + +// addEndpointToSubnet appends the endpoint to the slice of ServiceEndpoints of the subnet. +func addEndpointToSubnet(endpoint string, subnet *armnetwork.Subnet) { + if subnet.Properties.ServiceEndpoints == nil { + subnet.Properties.ServiceEndpoints = []*armnetwork.ServiceEndpointPropertiesFormat{} + } + + serviceEndpoint := armnetwork.ServiceEndpointPropertiesFormat{ + Service: to.StringPtr(endpoint), + Locations: []*string{to.StringPtr("*")}, + } + + subnet.Properties.ServiceEndpoints = append(subnet.Properties.ServiceEndpoints, &serviceEndpoint) +} diff --git a/pkg/cluster/ensureendpoints_test.go b/pkg/cluster/ensureendpoints_test.go index 0849ea30bf1..4f93ee4e4d3 100644 --- a/pkg/cluster/ensureendpoints_test.go +++ b/pkg/cluster/ensureendpoints_test.go @@ -5,12 +5,16 @@ package cluster import ( "context" + "reflect" "testing" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v2" "go.uber.org/mock/gomock" + "github.com/stretchr/testify/assert" + "k8s.io/utils/ptr" "github.com/Azure/ARO-RP/pkg/api" - mock_subnet "github.com/Azure/ARO-RP/pkg/util/mocks/subnet" + mock_armnetwork "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/azuresdk/armnetwork" ) var ( @@ -19,88 +23,462 @@ var ( vnetName = "vnet" subnetNameWorker = "worker" subnetNameMaster = "master" - subnetIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker - subnetIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster + subnetIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnet/" + subnetNameWorker + subnetIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnet/" + subnetNameMaster + masterSubnet = armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{}, + }, + } + workerSubnet = armnetwork.Subnet{ + ID: ptr.To(subnetIdWorker), + Properties: &armnetwork.SubnetPropertiesFormat{ + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{}, + }, + } ) -func TestEnsureServiceEndpointsShouldCall_SubnetManager_CreateOrUpdateFromIds_AndReturnNoError(t *testing.T) { - oc := &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - MasterProfile: api.MasterProfile{SubnetID: subnetIdMaster}, - WorkerProfiles: []api.WorkerProfile{ - {SubnetID: subnetIdWorker}, +func TestEnsureServiceEndpoints(t *testing.T) { + for _, tt := range []struct { + name string + oc *api.OpenShiftCluster + mock func(subnets *mock_armnetwork.MockSubnetsClient) + expectedErr string + }{ + { + name: "It should do nothing when egress lockdown is enabled", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + MasterProfile: api.MasterProfile{SubnetID: subnetIdMaster}, + WorkerProfiles: []api.WorkerProfile{ + {SubnetID: subnetIdWorker}, + }, + FeatureProfile: api.FeatureProfile{ + GatewayEnabled: true, + }, + }, + }, + mock: func(subnets *mock_armnetwork.MockSubnetsClient) { + subnets.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) }, }, - } - - subnetIds := []string{oc.Properties.MasterProfile.SubnetID, oc.Properties.WorkerProfiles[0].SubnetID} - - controller := gomock.NewController(t) - defer controller.Finish() - - ctx := context.Background() - - subnetManagerMock := mock_subnet.NewMockManager(controller) + { + name: "It should update subnet when egress lockdown is disabled", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + MasterProfile: api.MasterProfile{SubnetID: subnetIdMaster}, + WorkerProfiles: []api.WorkerProfile{ + {SubnetID: subnetIdWorker}, + }, + }, + }, + mock: func(subnets *mock_armnetwork.MockSubnetsClient) { + subnets.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameMaster, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: masterSubnet}, nil) + subnets.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: workerSubnet}, nil) + expectedMasterSubnet := armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + } + expectedWorkerSubnet := armnetwork.Subnet{ + ID: ptr.To(subnetIdWorker), + Properties: &armnetwork.SubnetPropertiesFormat{ + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + } + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameMaster, gomock.Any(), nil).DoAndReturn( + func(_, _, _, _ interface{}, subnet armnetwork.Subnet, _ interface{}) error { + if !reflect.DeepEqual(subnet, expectedMasterSubnet) { + t.Errorf("expected master subnet to be %v, got %v", expectedMasterSubnet, subnet) + } + return nil + }) + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, gomock.Any(), nil).DoAndReturn( + func(_, _, _, _ interface{}, subnet armnetwork.Subnet, _ interface{}) error { + if !reflect.DeepEqual(subnet, expectedWorkerSubnet) { + t.Errorf("expected worker subnet to be %v, got %v", expectedWorkerSubnet, subnet) + } + return nil + }) + }, + }, + { + name: "It should return error when subnet ID is empty", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + MasterProfile: api.MasterProfile{SubnetID: subnetIdMaster}, + WorkerProfiles: []api.WorkerProfile{ + { + Name: "workerProfile", + SubnetID: "", + }, + }, + }, + }, + mock: func(subnets *mock_armnetwork.MockSubnetsClient) { + subnets.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + }, + expectedErr: "WorkerProfile 'workerProfile' has no SubnetID; check that the corresponding MachineSet is valid", + }, + { + name: "It should not update subnet when subnet already have service endpoints", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + MasterProfile: api.MasterProfile{SubnetID: subnetIdMaster}, + WorkerProfiles: []api.WorkerProfile{ + {SubnetID: subnetIdWorker}, + }, + }, + }, + mock: func(subnets *mock_armnetwork.MockSubnetsClient) { + masterSubnetWithServiceEndpoints := masterSubnet + masterSubnetWithServiceEndpoints.Properties.ServiceEndpoints = []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + } + workerSubnetWithServiceEndpoints := workerSubnet + workerSubnetWithServiceEndpoints.Properties.ServiceEndpoints = []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + } + subnets.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameMaster, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: masterSubnetWithServiceEndpoints}, nil) + subnets.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: workerSubnetWithServiceEndpoints}, nil) + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + }, + }, + { + name: "It updates subnet when subnet already have one of the service endpoints", + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + MasterProfile: api.MasterProfile{SubnetID: subnetIdMaster}, + WorkerProfiles: []api.WorkerProfile{ + {SubnetID: subnetIdWorker}, + }, + }, + }, + mock: func(subnets *mock_armnetwork.MockSubnetsClient) { + masterSubnetWithServiceEndpoints := masterSubnet + masterSubnetWithServiceEndpoints.Properties.ServiceEndpoints = []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + } + workerSubnetWithServiceEndpoints := workerSubnet + workerSubnetWithServiceEndpoints.Properties.ServiceEndpoints = []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + } + subnets.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameMaster, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: masterSubnetWithServiceEndpoints}, nil) + subnets.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: workerSubnetWithServiceEndpoints}, nil) + expectedMasterSubnet := armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + } + expectedWorkerSubnet := armnetwork.Subnet{ + ID: ptr.To(subnetIdWorker), + Properties: &armnetwork.SubnetPropertiesFormat{ + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + } + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameMaster, gomock.Any(), nil).DoAndReturn( + func(_, _, _, _ interface{}, subnet armnetwork.Subnet, _ interface{}) error { + if !reflect.DeepEqual(subnet, expectedMasterSubnet) { + t.Errorf("expected master subnet to be %v, got %v", expectedMasterSubnet, subnet) + } + return nil + }) + subnets.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, gomock.Any(), nil).DoAndReturn( + func(_, _, _, _ interface{}, subnet armnetwork.Subnet, _ interface{}) error { + if !reflect.DeepEqual(subnet, expectedWorkerSubnet) { + t.Errorf("expected worker subnet to be %v, got %v", expectedWorkerSubnet, subnet) + } + return nil + }) + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() - subnetManagerMock. - EXPECT(). - CreateOrUpdateFromIds(ctx, subnetIds, false). - Return(nil) + ctx := context.Background() - m := &manager{ - subnet: subnetManagerMock, - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: oc, - }, - } + subnetsClient := mock_armnetwork.NewMockSubnetsClient(controller) + tt.mock(subnetsClient) - err := m.ensureServiceEndpoints(ctx) - expectedError := "" + m := &manager{ + armSubnets: subnetsClient, + doc: &api.OpenShiftClusterDocument{ + OpenShiftCluster: tt.oc, + }, + } - if (err != nil && err.Error() != expectedError) || (err == nil && expectedError != "") { - t.Fatalf("expected error '%v', but got '%v'", expectedError, err) + err := m.ensureServiceEndpoints(ctx) + if tt.expectedErr != "" { + assert.EqualError(t, err, tt.expectedErr) + } else { + assert.NoError(t, err) + } + }) } } -func TestEnsureServiceEndpointsShouldReturnWorkerProfileHasNoSubnetIdErrorAndShouldNotCall_SubnetManager_CreateOrUpdateFromIds(t *testing.T) { - oc := &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - MasterProfile: api.MasterProfile{ - SubnetID: subnetIdMaster, - }, - WorkerProfiles: []api.WorkerProfile{ - { - Name: "profile_name", - SubnetID: "", +func TestAddEndpointsToSubnets(t *testing.T) { + for _, tt := range []struct { + name string + subnet *armnetwork.Subnet + newEndpoints []string + expectedSubnet *armnetwork.Subnet + expectedResult bool + }{ + { + name: "addEndpointsToSubnet should when the subnet contains all new endpoints in succeeded state", + subnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + }, + }, + }, + newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, + expectedSubnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + }, }, }, + expectedResult: false, }, - } - - controller := gomock.NewController(t) - defer controller.Finish() - - ctx := context.Background() - - subnetManagerMock := mock_subnet.NewMockManager(controller) - - subnetManagerMock. - EXPECT(). - CreateOrUpdateFromIds(gomock.Any(), gomock.Any(), gomock.Any()). - Times(0). - Return(nil) - - m := &manager{ - subnet: subnetManagerMock, - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: oc, + { + name: "addEndpointsToSubnet should update the subnet when it has no service endpoints", + subnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{}, + }, + }, + newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, + expectedSubnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + }, + expectedResult: true, }, - } - - err := m.ensureServiceEndpoints(ctx) - expectedError := "WorkerProfile 'profile_name' has no SubnetID; check that the corresponding MachineSet is valid" + { + name: "addEndpointsToSubnet should update the subnet when ServiceEndpoints is nil", + subnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{}, + }, + newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, + expectedSubnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + }, + expectedResult: true, + }, + { + name: "addEndpointsToSubnet should update the subnet (with 4 endpoints: 2 previous in failed state + 2 new) when it contains all new endpoints but those are not in succeeded state", + subnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateFailed), + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateFailed), + }, + }, + }, + }, + newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, + expectedSubnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateFailed), + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateFailed), + }, + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + }, + expectedResult: true, + }, + { + name: "addEndpointsToSubnet should return an updated Subnet (with 2 endpoints: 1 previous was already in succeeded state + 1 new (it was missing))", + subnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + }, + }, + }, + newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, + expectedSubnet: &armnetwork.Subnet{ + ID: ptr.To(subnetIdMaster), + Properties: &armnetwork.SubnetPropertiesFormat{ + ServiceEndpoints: []*armnetwork.ServiceEndpointPropertiesFormat{ + { + Service: ptr.To("Microsoft.ContainerRegistry"), + Locations: []*string{ptr.To("*")}, + ProvisioningState: ptr.To(armnetwork.ProvisioningStateSucceeded), + }, + { + Service: ptr.To("Microsoft.Storage"), + Locations: []*string{ptr.To("*")}, + }, + }, + }, + }, + expectedResult: true, + }, + } { + t.Run(tt.name, func(t *testing.T) { + result := addEndpointsToSubnet(tt.newEndpoints, tt.subnet) - if (err != nil && err.Error() != expectedError) || (err == nil && expectedError != "") { - t.Fatalf("expected error '%v', but got '%v'", expectedError, err) + assert.Equal(t, tt.expectedResult, result) + if !reflect.DeepEqual(tt.expectedSubnet, tt.subnet) { + t.Fatalf("subnet is different than expectedSubnet. Expected %v, but got %v", tt.expectedSubnet, tt.subnet) + } + }) } } diff --git a/pkg/util/mocks/subnet/subnet.go b/pkg/util/mocks/subnet/subnet.go index 77d65bb013b..b3a82098d2f 100644 --- a/pkg/util/mocks/subnet/subnet.go +++ b/pkg/util/mocks/subnet/subnet.go @@ -56,20 +56,6 @@ func (mr *MockManagerMockRecorder) CreateOrUpdate(arg0, arg1, arg2 any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdate", reflect.TypeOf((*MockManager)(nil).CreateOrUpdate), arg0, arg1, arg2) } -// CreateOrUpdateFromIds mocks base method. -func (m *MockManager) CreateOrUpdateFromIds(arg0 context.Context, arg1 []string, arg2 bool) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateFromIds", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 -} - -// CreateOrUpdateFromIds indicates an expected call of CreateOrUpdateFromIds. -func (mr *MockManagerMockRecorder) CreateOrUpdateFromIds(arg0, arg1, arg2 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateFromIds", reflect.TypeOf((*MockManager)(nil).CreateOrUpdateFromIds), arg0, arg1, arg2) -} - // Get mocks base method. func (m *MockManager) Get(arg0 context.Context, arg1 string) (*network.Subnet, error) { m.ctrl.T.Helper() diff --git a/pkg/util/subnet/addendpoints.go b/pkg/util/subnet/addendpoints.go deleted file mode 100644 index c91082bab64..00000000000 --- a/pkg/util/subnet/addendpoints.go +++ /dev/null @@ -1,71 +0,0 @@ -package subnet - -// Copyright (c) Microsoft Corporation. -// Licensed under the Apache License 2.0. -import ( - "strings" - - mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" - "github.com/Azure/go-autorest/autorest/to" -) - -// addEndpointsToSubnets adds the endpoints (that either are missing in subnets -// or aren't in succeded state in subnets) to the subnets and returns those updated subnets. -// This method does not talk to any external dependecies to remain pure bussiness logic. -// The result of this function should be passed to a subnet manager to update the subnets -// in Azure. -func addEndpointsToSubnets(endpoints []string, subnets []*mgmtnetwork.Subnet) (subnetsToBeUpdated []*mgmtnetwork.Subnet) { - for _, subnet := range subnets { - if subnetChanged := addEndpointsToSubnet(endpoints, subnet); subnetChanged { - subnetsToBeUpdated = append(subnetsToBeUpdated, subnet) - } - } - - return subnetsToBeUpdated -} - -// addEndpointsToSubnet adds the endpoints (that either are missing in subnet -// or aren't in succeded state in the subnet) to the subnet and returns the updated subnet -func addEndpointsToSubnet(endpoints []string, subnet *mgmtnetwork.Subnet) (subnetChanged bool) { - for _, endpoint := range endpoints { - endpointFound, serviceEndpointPtr := subnetContainsEndpoint(subnet, endpoint) - - if !endpointFound || serviceEndpointPtr.ProvisioningState != mgmtnetwork.Succeeded { - addEndpointToSubnet(endpoint, subnet) - subnetChanged = true - } - } - - return subnetChanged -} - -// subnetContainsEndpoint returns false and nil if subnet does not contain the endpoint. -// If the subnet does contain the endpoint, true and a pointer to the service endpoint -// is returned to be able to do additional checks and perform actions accordingly. -func subnetContainsEndpoint(subnet *mgmtnetwork.Subnet, endpoint string) (endpointFound bool, serviceEndpointPtr *mgmtnetwork.ServiceEndpointPropertiesFormat) { - if subnet == nil || subnet.ServiceEndpoints == nil { - return false, nil - } - - for _, serviceEndpoint := range *subnet.ServiceEndpoints { - if endpointFound = strings.EqualFold(*serviceEndpoint.Service, endpoint); endpointFound { - return true, &serviceEndpoint - } - } - - return false, nil -} - -// addEndpointToSubnet appends the endpoint to the slice of ServiceEndpoints of the subnet. -func addEndpointToSubnet(endpoint string, subnet *mgmtnetwork.Subnet) { - if subnet.ServiceEndpoints == nil { - subnet.ServiceEndpoints = &[]mgmtnetwork.ServiceEndpointPropertiesFormat{} - } - - serviceEndpoint := mgmtnetwork.ServiceEndpointPropertiesFormat{ - Service: to.StringPtr(endpoint), - Locations: &[]string{"*"}, - } - - *subnet.ServiceEndpoints = append(*subnet.ServiceEndpoints, serviceEndpoint) -} diff --git a/pkg/util/subnet/addendpoints_test.go b/pkg/util/subnet/addendpoints_test.go deleted file mode 100644 index 7e6f5c61861..00000000000 --- a/pkg/util/subnet/addendpoints_test.go +++ /dev/null @@ -1,239 +0,0 @@ -package subnet - -// Copyright (c) Microsoft Corporation. -// Licensed under the Apache License 2.0. - -import ( - "reflect" - "testing" - - mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" - "github.com/Azure/go-autorest/autorest/to" -) - -func TestAddEndpointsToSubnets(t *testing.T) { - var ( - subscriptionId = "0000000-0000-0000-0000-000000000000" - vnetResourceGroup = "vnet-rg" - vnetName = "vnet" - subnetNameWorker = "worker" - subnetNameMaster = "master" - subnetIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker - subnetIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster - ) - - type testData struct { - name string - subnets []*mgmtnetwork.Subnet - newEndpoints []string - expectedSubnets []*mgmtnetwork.Subnet - } - - tt := []testData{ - { - name: "addEndpointsToSubnets should return nil as subnets is nil", - subnets: nil, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: nil, - }, - { - name: "addEndpointsToSubnets should return nil as subnets is an empty slice", - subnets: []*mgmtnetwork.Subnet{}, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: nil, - }, - { - name: "addEndpointsToSubnets should return nil as all subnets contain all new endpoints and those are in succeeded state", - subnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Succeeded, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Succeeded, - }, - }, - }, - }, - { - ID: to.StringPtr(subnetIdWorker), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Succeeded, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Succeeded, - }, - }, - }, - }, - }, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: nil, - }, - { - name: "addEndpointsToSubnets should return a new updated Subnet because the original subnet's service endpoints is empty", - subnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{}, - }, - }, - }, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - }, - }, - }, - }, - }, - }, - { - name: "addEndpointsToSubnets should return a new updated Subnet because the original subnet's service endpoints is nil", - subnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{}, - }, - }, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - }, - }, - }, - }, - }, - }, - { - name: "addEndpointsToSubnets should return an updated Subnet (with 4 endpoints: 2 previous in failed state + 2 new) as subnet contains all new endpoints but those are not in succeeded state. ", - subnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Failed, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Failed, - }, - }, - }, - }, - }, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Failed, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Failed, - }, - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - }, - }, - }, - }, - }, - }, - { - name: "addEndpointsToSubnets should return an updated Subnet (with 2 endpoints: 1 previous was already in succeeded state + 1 new (it was missing))", - subnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Succeeded, - }, - }, - }, - }, - }, - newEndpoints: []string{"Microsoft.ContainerRegistry", "Microsoft.Storage"}, - expectedSubnets: []*mgmtnetwork.Subnet{ - { - ID: to.StringPtr(subnetIdMaster), - SubnetPropertiesFormat: &mgmtnetwork.SubnetPropertiesFormat{ - ServiceEndpoints: &[]mgmtnetwork.ServiceEndpointPropertiesFormat{ - { - Service: to.StringPtr("Microsoft.ContainerRegistry"), - Locations: &[]string{"*"}, - ProvisioningState: mgmtnetwork.Succeeded, - }, - { - Service: to.StringPtr("Microsoft.Storage"), - Locations: &[]string{"*"}, - }, - }, - }, - }, - }, - }, - } - - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - subnetsToBeUpdated := addEndpointsToSubnets(tc.newEndpoints, tc.subnets) - - if !reflect.DeepEqual(tc.expectedSubnets, subnetsToBeUpdated) { - t.Fatalf("expected subnets is different than subnetsToBeUpdated. Expected %v, but got %v", tc.expectedSubnets, subnetsToBeUpdated) - } - }) - } -} diff --git a/pkg/util/subnet/subnet.go b/pkg/util/subnet/subnet.go index 105f9ceba62..96ed80b2ddf 100644 --- a/pkg/util/subnet/subnet.go +++ b/pkg/util/subnet/subnet.go @@ -12,7 +12,6 @@ import ( "github.com/Azure/go-autorest/autorest/azure" "github.com/apparentlymart/go-cidr/cidr" - "github.com/Azure/ARO-RP/pkg/api" apisubnet "github.com/Azure/ARO-RP/pkg/api/util/subnet" "github.com/Azure/ARO-RP/pkg/util/azureclient" "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network" @@ -28,7 +27,6 @@ type Manager interface { GetAll(ctx context.Context, subnetIds []string) ([]*mgmtnetwork.Subnet, error) GetHighestFreeIP(ctx context.Context, subnetID string) (string, error) CreateOrUpdate(ctx context.Context, subnetID string, subnet *mgmtnetwork.Subnet) error - CreateOrUpdateFromIds(ctx context.Context, subnetIds []string, gatewayEnabled bool) error } type manager struct { @@ -147,28 +145,3 @@ func (m *manager) GetAll(ctx context.Context, subnetIds []string) ([]*mgmtnetwor } return subnets, nil } - -func (m *manager) CreateOrUpdateFromIds(ctx context.Context, subnetIds []string, gatewayEnabled bool) error { - subnets, err := m.GetAll(ctx, subnetIds) - if err != nil { - return err - } - - // Only add service endpoints to the subnets if egress lockdown is not enabled. - if !gatewayEnabled { - subnetsToBeUpdated := addEndpointsToSubnets(api.SubnetsEndpoints, subnets) - - return m.createOrUpdateSubnets(ctx, subnetsToBeUpdated) - } - - return m.createOrUpdateSubnets(ctx, subnets) -} - -func (m *manager) createOrUpdateSubnets(ctx context.Context, subnets []*mgmtnetwork.Subnet) error { - for _, subnet := range subnets { - if err := m.CreateOrUpdate(ctx, *subnet.ID, subnet); err != nil { - return err - } - } - return nil -}