From 50eb625198303405b630657368d91a9c1dc29dc8 Mon Sep 17 00:00:00 2001 From: Tanmay Satam Date: Fri, 10 Nov 2023 15:57:55 -0500 Subject: [PATCH] Update subnet/storageaccount controllers to skip processing subnets that are NotFound in Azure --- .../storageaccounts/storageaccounts.go | 5 + .../storageaccounts/storageaccounts_test.go | 52 +++++++++ .../subnets/subnet_controller_test.go | 108 +++++++++++++++++- .../controllers/subnets/subnet_nsg.go | 5 + .../subnets/subnet_serviceendpoint.go | 5 + pkg/util/azureerrors/error.go | 7 ++ 6 files changed, 177 insertions(+), 5 deletions(-) diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts.go b/pkg/operator/controllers/storageaccounts/storageaccounts.go index 61b4aff9bff..24a96b77119 100644 --- a/pkg/operator/controllers/storageaccounts/storageaccounts.go +++ b/pkg/operator/controllers/storageaccounts/storageaccounts.go @@ -13,6 +13,7 @@ import ( imageregistryv1 "github.com/openshift/api/imageregistry/v1" "k8s.io/apimachinery/pkg/types" + "github.com/Azure/ARO-RP/pkg/util/azureerrors" "github.com/Azure/ARO-RP/pkg/util/stringutils" ) @@ -32,6 +33,10 @@ func (r *reconcileManager) reconcileAccounts(ctx context.Context) error { for _, subnet := range subnets { mgmtSubnet, err := r.subnets.Get(ctx, subnet.ResourceID) if err != nil { + if azureerrors.IsNotFoundError(err) { + r.log.Infof("Subnet %s not found, skipping", subnet.ResourceID) + break + } return err } diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts_test.go b/pkg/operator/controllers/storageaccounts/storageaccounts_test.go index 3095e62540e..cdd73e7fed6 100644 --- a/pkg/operator/controllers/storageaccounts/storageaccounts_test.go +++ b/pkg/operator/controllers/storageaccounts/storageaccounts_test.go @@ -5,11 +5,13 @@ package storageaccounts import ( "context" + "net/http" "strconv" "testing" mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" mgmtstorage "github.com/Azure/azure-sdk-for-go/services/storage/mgmt/2019-06-01/storage" + "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" imageregistryv1 "github.com/openshift/api/imageregistry/v1" @@ -212,6 +214,56 @@ func TestReconcileManager(t *testing.T) { storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, updated) }, }, + { + name: "Operator Flag enabled - not found error on getting worker subnet skips subnet", + operatorFlag: true, + mocks: func(storage *mock_storage.MockAccountsClient, kubeSubnet *mock_subnet.MockKubeManager, mgmtSubnet *mock_subnet.MockManager) { + // Azure subnets + masterSubnet := getValidSubnet(resourceIdMaster) + + notFoundErr := autorest.DetailedError{ + StatusCode: http.StatusNotFound, + } + + mgmtSubnet.EXPECT().Get(gomock.Any(), resourceIdMaster).Return(masterSubnet, nil) + mgmtSubnet.EXPECT().Get(gomock.Any(), resourceIdWorker).Return(nil, notFoundErr) + + // cluster subnets + kubeSubnet.EXPECT().List(gomock.Any()).Return([]subnet.Subnet{ + { + ResourceID: resourceIdMaster, + IsMaster: true, + }, + { + ResourceID: resourceIdWorker, + IsMaster: false, + }, + }, nil) + + // storage objects in azure + result := getValidAccount([]string{}) + updated := mgmtstorage.AccountUpdateParameters{ + AccountPropertiesUpdateParameters: &mgmtstorage.AccountPropertiesUpdateParameters{ + NetworkRuleSet: getValidAccount([]string{resourceIdMaster}).NetworkRuleSet, + }, + } + + storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, clusterStorageAccountName, gomock.Any()).Return(*result, nil) + storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, clusterStorageAccountName, updated) + + // we can't reuse these from above due to fact how gomock handles objects. + // they are modified by the functions so they are not the same anymore + result = getValidAccount([]string{}) + updated = mgmtstorage.AccountUpdateParameters{ + AccountPropertiesUpdateParameters: &mgmtstorage.AccountPropertiesUpdateParameters{ + NetworkRuleSet: getValidAccount([]string{resourceIdMaster}).NetworkRuleSet, + }, + } + + storage.EXPECT().GetProperties(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, gomock.Any()).Return(*result, nil) + storage.EXPECT().Update(gomock.Any(), clusterResourceGroupName, registryStorageAccountName, updated) + }, + }, { name: "Operator flag enabled - worker subnet rule to all accounts because storage service endpoint on worker subnet", operatorFlag: true, diff --git a/pkg/operator/controllers/subnets/subnet_controller_test.go b/pkg/operator/controllers/subnets/subnet_controller_test.go index 1c097282967..d22f3a83148 100644 --- a/pkg/operator/controllers/subnets/subnet_controller_test.go +++ b/pkg/operator/controllers/subnets/subnet_controller_test.go @@ -5,11 +5,13 @@ package subnets import ( "context" + "net/http" "reflect" "strconv" "testing" mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network" + "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" "github.com/sirupsen/logrus" @@ -34,11 +36,12 @@ var ( subnetNameWorker = "worker" subnetNameMaster = "master" - nsgv1NodeResourceId = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGNodeSuffixV1 - nsgv1MasterResourceId = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGControlPlaneSuffixV1 - nsgv2ResourceId = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGSuffixV2 - subnetResourceIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster - subnetResourceIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker + nsgv1NodeResourceId = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGNodeSuffixV1 + nsgv1MasterResourceId = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGControlPlaneSuffixV1 + nsgv2ResourceId = clusterResourceGroupId + "/providers/Microsoft.Network/networkSecurityGroups/" + infraId + apisubnet.NSGSuffixV2 + subnetResourceIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster + subnetResourceIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker + subnetResourceIdWorkerInvalid = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker + "-invalid" ) func getValidClusterInstance(operatorFlagEnabled bool, operatorFlagNSG bool, operatorFlagServiceEndpoint bool) *arov1alpha1.Cluster { @@ -177,6 +180,52 @@ func TestReconcileManager(t *testing.T) { mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorker, subnetObjectWorkerUpdate).Return(nil) }, }, + { + name: "Architecture V1 - skips invalid/not found subnets", + operatorFlagEnabled: true, + operatorFlagNSG: true, + operatorFlagServiceEndpoint: true, + wantAnnotationsUpdated: true, + subnetMock: func(mock *mock_subnet.MockManager, kmock *mock_subnet.MockKubeManager) { + kmock.EXPECT().List(gomock.Any()).Return([]subnet.Subnet{ + { + ResourceID: subnetResourceIdMaster, + IsMaster: true, + }, + { + ResourceID: subnetResourceIdWorker, + IsMaster: false, + }, + { + ResourceID: subnetResourceIdWorkerInvalid, + IsMaster: false, + }, + }, nil) + + subnetObjectMaster := getValidSubnet() + subnetObjectMaster.NetworkSecurityGroup.ID = to.StringPtr(nsgv1MasterResourceId + "new") + mock.EXPECT().Get(gomock.Any(), subnetResourceIdMaster).Return(subnetObjectMaster, nil).MaxTimes(2) + + subnetObjectMasterUpdate := getValidSubnet() + subnetObjectMasterUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv1MasterResourceId) + mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdMaster, subnetObjectMasterUpdate).Return(nil) + + subnetObjectWorker := getValidSubnet() + subnetObjectWorker.NetworkSecurityGroup.ID = to.StringPtr(nsgv1NodeResourceId + "new") + mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorker).Return(subnetObjectWorker, nil).MaxTimes(2) + + subnetObjectWorkerUpdate := getValidSubnet() + subnetObjectWorkerUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv1NodeResourceId) + mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorker, subnetObjectWorkerUpdate).Return(nil) + + notFoundErr := autorest.DetailedError{ + StatusCode: http.StatusNotFound, + } + + mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorkerInvalid).Return(nil, notFoundErr).AnyTimes() + mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorkerInvalid, gomock.Any()).Times(0) + }, + }, { name: "Architecture V1 - node only fixup", operatorFlagEnabled: true, @@ -275,6 +324,55 @@ func TestReconcileManager(t *testing.T) { instace.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2) }, }, + { + name: "Architecture V2 - skips invalid/not found subnets", + operatorFlagEnabled: true, + operatorFlagNSG: true, + operatorFlagServiceEndpoint: true, + wantAnnotationsUpdated: true, + subnetMock: func(mock *mock_subnet.MockManager, kmock *mock_subnet.MockKubeManager) { + kmock.EXPECT().List(gomock.Any()).Return([]subnet.Subnet{ + { + ResourceID: subnetResourceIdMaster, + IsMaster: true, + }, + { + ResourceID: subnetResourceIdWorker, + IsMaster: false, + }, + { + ResourceID: subnetResourceIdWorkerInvalid, + IsMaster: false, + }, + }, nil) + + subnetObjectMaster := getValidSubnet() + subnetObjectMaster.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId + "new") + mock.EXPECT().Get(gomock.Any(), subnetResourceIdMaster).Return(subnetObjectMaster, nil).MaxTimes(2) + + subnetObjectMasterUpdate := getValidSubnet() + subnetObjectMasterUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId) + mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdMaster, subnetObjectMasterUpdate).Return(nil) + + subnetObjectWorker := getValidSubnet() + subnetObjectWorker.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId + "new") + mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorker).Return(subnetObjectWorker, nil).MaxTimes(2) + + subnetObjectWorkerUpdate := getValidSubnet() + subnetObjectWorkerUpdate.NetworkSecurityGroup.ID = to.StringPtr(nsgv2ResourceId) + mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorker, subnetObjectWorkerUpdate).Return(nil) + + notFoundErr := autorest.DetailedError{ + StatusCode: http.StatusNotFound, + } + + mock.EXPECT().Get(gomock.Any(), subnetResourceIdWorkerInvalid).Return(nil, notFoundErr).AnyTimes() + mock.EXPECT().CreateOrUpdate(gomock.Any(), subnetResourceIdWorkerInvalid, gomock.Any()).Times(0) + }, + instance: func(instace *arov1alpha1.Cluster) { + instace.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2) + }, + }, { name: "Architecture V2 - endpoint fixup", operatorFlagEnabled: true, diff --git a/pkg/operator/controllers/subnets/subnet_nsg.go b/pkg/operator/controllers/subnets/subnet_nsg.go index 89624f85dd2..01e65ee414a 100644 --- a/pkg/operator/controllers/subnets/subnet_nsg.go +++ b/pkg/operator/controllers/subnets/subnet_nsg.go @@ -13,6 +13,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" apisubnet "github.com/Azure/ARO-RP/pkg/api/util/subnet" + "github.com/Azure/ARO-RP/pkg/util/azureerrors" "github.com/Azure/ARO-RP/pkg/util/subnet" ) @@ -25,6 +26,10 @@ func (r *reconcileManager) ensureSubnetNSG(ctx context.Context, s subnet.Subnet) subnetObject, err := r.subnets.Get(ctx, s.ResourceID) if err != nil { + if azureerrors.IsNotFoundError(err) { + r.log.Infof("Subnet %s not found, skipping", s.ResourceID) + return nil + } return err } if subnetObject.SubnetPropertiesFormat == nil { diff --git a/pkg/operator/controllers/subnets/subnet_serviceendpoint.go b/pkg/operator/controllers/subnets/subnet_serviceendpoint.go index 64823719121..99c57020bc8 100644 --- a/pkg/operator/controllers/subnets/subnet_serviceendpoint.go +++ b/pkg/operator/controllers/subnets/subnet_serviceendpoint.go @@ -13,6 +13,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/operator" + "github.com/Azure/ARO-RP/pkg/util/azureerrors" "github.com/Azure/ARO-RP/pkg/util/subnet" ) @@ -22,6 +23,10 @@ func (r *reconcileManager) ensureSubnetServiceEndpoints(ctx context.Context, s s subnetObject, err := r.subnets.Get(ctx, s.ResourceID) if err != nil { + if azureerrors.IsNotFoundError(err) { + r.log.Infof("Subnet %s not found, skipping. err: %w", s.ResourceID, err) + return nil + } return err } diff --git a/pkg/util/azureerrors/error.go b/pkg/util/azureerrors/error.go index 6184e9a1928..9211ef69501 100644 --- a/pkg/util/azureerrors/error.go +++ b/pkg/util/azureerrors/error.go @@ -5,6 +5,7 @@ package azureerrors import ( "encoding/json" + "net/http" "strings" "github.com/Azure/go-autorest/autorest" @@ -71,6 +72,12 @@ func IsDeploymentActiveError(err error) bool { return false } +func IsNotFoundError(err error) bool { + detailedErr, ok := err.(autorest.DetailedError) + + return ok && detailedErr.StatusCode == http.StatusNotFound +} + // IsInvalidSecretError returns if errors is InvalidCredentials error // Example: (adal.tokenRefreshError) adal: Refresh request failed. Status Code = '401'. // Response body: {"error":"invalid_client","error_description":"AADSTS7000215: