Update subnet/storageaccount controllers to skip processing subnets t…

…hat are NotFound in Azure
tsatam · Nov 10, 2023 · cf174b2 · cf174b2
1 parent 64b42f6
commit cf174b2
Show file tree

Hide file tree

Showing 6 changed files with 177 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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", s.ResourceID)
+				return nil
+			}
 			return err
 		}
 

diff --git 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: