diff --git a/pkg/operator/controllers/storageaccounts/storageaccount_controller.go b/pkg/operator/controllers/storageaccounts/storageaccount_controller.go index 66a44d12d..f6ed4ce95 100644 --- a/pkg/operator/controllers/storageaccounts/storageaccount_controller.go +++ b/pkg/operator/controllers/storageaccounts/storageaccount_controller.go @@ -9,7 +9,6 @@ import ( "github.com/Azure/go-autorest/autorest/azure" machinev1beta1 "github.com/openshift/api/machine/v1beta1" "github.com/sirupsen/logrus" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -116,11 +115,15 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { return o.GetName() == arov1alpha1.SingletonClusterName }) + masterMachinePredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { + role, ok := o.GetLabels()["machine.openshift.io/cluster-api-machine-role"] + return ok && role == "master" + }) return ctrl.NewControllerManagedBy(mgr). For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)). - Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}). // to reconcile on machine replacement - Watches(&source.Kind{Type: &corev1.Node{}}, &handler.EnqueueRequestForObject{}). // to reconcile on node status change + Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}, builder.WithPredicates(masterMachinePredicate)). // to reconcile on master machine replacement + Watches(&source.Kind{Type: &machinev1beta1.MachineSet{}}, &handler.EnqueueRequestForObject{}). // to reconcile on worker machinesets Named(ControllerName). Complete(r) } diff --git a/pkg/operator/controllers/storageaccounts/storageaccounts.go b/pkg/operator/controllers/storageaccounts/storageaccounts.go index 61b4aff9b..24a96b771 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 3095e6254..cdd73e7fe 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 1c0972829..d22f3a831 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 89624f85d..01e65ee41 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 648237191..fc6642ffa 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: %v", s.ResourceID, err) + return nil + } return err } diff --git a/pkg/operator/controllers/subnets/subnets_controller.go b/pkg/operator/controllers/subnets/subnets_controller.go index 4a0a5b663..637faff28 100644 --- a/pkg/operator/controllers/subnets/subnets_controller.go +++ b/pkg/operator/controllers/subnets/subnets_controller.go @@ -11,7 +11,6 @@ import ( "github.com/Azure/go-autorest/autorest/azure" machinev1beta1 "github.com/openshift/api/machine/v1beta1" "github.com/sirupsen/logrus" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -155,11 +154,15 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { return o.GetName() == arov1alpha1.SingletonClusterName }) + masterMachinePredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { + role, ok := o.GetLabels()["machine.openshift.io/cluster-api-machine-role"] + return ok && role == "master" + }) return ctrl.NewControllerManagedBy(mgr). For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)). - Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}). // to reconcile on machine replacement - Watches(&source.Kind{Type: &corev1.Node{}}, &handler.EnqueueRequestForObject{}). // to reconcile on node status change + Watches(&source.Kind{Type: &machinev1beta1.Machine{}}, &handler.EnqueueRequestForObject{}, builder.WithPredicates(masterMachinePredicate)). // to reconcile on master machine replacement + Watches(&source.Kind{Type: &machinev1beta1.MachineSet{}}, &handler.EnqueueRequestForObject{}). // to reconcile on worker machinesets Named(ControllerName). Complete(r) } diff --git a/pkg/util/azureerrors/error.go b/pkg/util/azureerrors/error.go index 6184e9a19..9211ef695 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: diff --git a/pkg/util/subnet/cluster_subnets.go b/pkg/util/subnet/cluster_subnets.go index 7277ab1d7..64a8d2dc4 100644 --- a/pkg/util/subnet/cluster_subnets.go +++ b/pkg/util/subnet/cluster_subnets.go @@ -45,8 +45,8 @@ func (m *kubeManager) List(ctx context.Context) ([]Subnet, error) { // select all workers by the machine.openshift.io/cluster-api-machine-role: not equal to master Label selector, _ := labels.Parse("machine.openshift.io/cluster-api-machine-role!=master") - machines := &machinev1beta1.MachineList{} - err := m.client.List(ctx, machines, &client.ListOptions{ + machineSets := &machinev1beta1.MachineSetList{} + err := m.client.List(ctx, machineSets, &client.ListOptions{ Namespace: machineSetsNamespace, LabelSelector: selector, }) @@ -54,8 +54,8 @@ func (m *kubeManager) List(ctx context.Context) ([]Subnet, error) { return nil, err } - for _, machine := range machines.Items { - subnetDesc, err := m.getDescriptorFromProviderSpec(machine.Spec.ProviderSpec.Value) + for _, machineSet := range machineSets.Items { + subnetDesc, err := m.getDescriptorFromProviderSpec(machineSet.Spec.Template.Spec.ProviderSpec.Value) if err != nil { return nil, err } @@ -63,7 +63,7 @@ func (m *kubeManager) List(ctx context.Context) ([]Subnet, error) { } selector, _ = labels.Parse("machine.openshift.io/cluster-api-machine-role=master") - machines = &machinev1beta1.MachineList{} + machines := &machinev1beta1.MachineList{} err = m.client.List(ctx, machines, &client.ListOptions{ Namespace: machineSetsNamespace, LabelSelector: selector, diff --git a/pkg/util/subnet/cluster_subnets_test.go b/pkg/util/subnet/cluster_subnets_test.go index 76d025961..e5ddadc82 100644 --- a/pkg/util/subnet/cluster_subnets_test.go +++ b/pkg/util/subnet/cluster_subnets_test.go @@ -30,7 +30,7 @@ func TestListFromCluster(t *testing.T) { name string machinelabel string expect []Subnet - modify func(*machinev1beta1.Machine, *machinev1beta1.Machine) + modify func(*machinev1beta1.MachineSet, *machinev1beta1.Machine) wantErr string }{ { @@ -48,7 +48,7 @@ func TestListFromCluster(t *testing.T) { { name: "master missing providerSpec", expect: nil, - modify: func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) { + modify: func(worker *machinev1beta1.MachineSet, master *machinev1beta1.Machine) { master.Spec.ProviderSpec.Value.Raw = []byte("") }, wantErr: "json: error calling MarshalJSON for type *runtime.RawExtension: unexpected end of JSON input", @@ -56,8 +56,8 @@ func TestListFromCluster(t *testing.T) { { name: "worker missing providerSpec", expect: nil, - modify: func(worker *machinev1beta1.Machine, master *machinev1beta1.Machine) { - worker.Spec.ProviderSpec.Value.Raw = []byte("") + modify: func(worker *machinev1beta1.MachineSet, master *machinev1beta1.Machine) { + worker.Spec.Template.Spec.ProviderSpec.Value.Raw = []byte("") }, wantErr: "json: error calling MarshalJSON for type *runtime.RawExtension: unexpected end of JSON input", }, @@ -77,26 +77,34 @@ func TestListFromCluster(t *testing.T) { }, }, } - workerMachine := machinev1beta1.Machine{ + workerMachineSet := machinev1beta1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ - Name: "worker-0", + Name: "worker", Namespace: "openshift-machine-api", Labels: map[string]string{"machine.openshift.io/cluster-api-machine-role": "worker"}, }, - Spec: machinev1beta1.MachineSpec{ - ProviderSpec: machinev1beta1.ProviderSpec{ - Value: &kruntime.RawExtension{ - Raw: []byte("{\"resourceGroup\":\"workerRG\",\"publicIP\":false,\"osDisk\":{\"diskSizeGB\": 1024,\"managedDisk\":{\"storageAccountType\": \"Premium_LRS\"},\"osType\":\"Linux\"},\"image\":{\"offer\": \"aro4\",\"publisher\": \"azureopenshift\", \"resourceID\": \"\", \"sku\": \"aro_43\", \"version\": \"43.81.20200311\"},\"networkResourceGroup\":\"vnet-rg\",\"vnet\":\"vnet\",\"subnet\":\"workerSubnet\"}"), + Spec: machinev1beta1.MachineSetSpec{ + Template: machinev1beta1.MachineTemplateSpec{ + ObjectMeta: machinev1beta1.ObjectMeta{ + Labels: map[string]string{"machine.openshift.io/cluster-api-machine-role": "worker"}, + }, + Spec: machinev1beta1.MachineSpec{ + ProviderSpec: machinev1beta1.ProviderSpec{ + Value: &kruntime.RawExtension{ + Raw: []byte("{\"resourceGroup\":\"workerRG\",\"publicIP\":false,\"osDisk\":{\"diskSizeGB\": 1024,\"managedDisk\":{\"storageAccountType\": \"Premium_LRS\"},\"osType\":\"Linux\"},\"image\":{\"offer\": \"aro4\",\"publisher\": \"azureopenshift\", \"resourceID\": \"\", \"sku\": \"aro_43\", \"version\": \"43.81.20200311\"},\"networkResourceGroup\":\"vnet-rg\",\"vnet\":\"vnet\",\"subnet\":\"workerSubnet\"}"), + }, + }, }, }, }, } + if tt.modify != nil { - tt.modify(&workerMachine, &masterMachine) + tt.modify(&workerMachineSet, &masterMachine) } m := kubeManager{ - client: fake.NewClientBuilder().WithObjects(&workerMachine, &masterMachine).Build(), + client: fake.NewClientBuilder().WithObjects(&workerMachineSet, &masterMachine).Build(), subscriptionID: subscriptionId, } diff --git a/test/e2e/operator.go b/test/e2e/operator.go index 299e41d95..2528ff61e 100644 --- a/test/e2e/operator.go +++ b/test/e2e/operator.go @@ -19,6 +19,7 @@ import ( "github.com/Azure/go-autorest/autorest/to" "github.com/ghodss/yaml" configv1 "github.com/openshift/api/config/v1" + machinev1beta1 "github.com/openshift/api/machine/v1beta1" cov1Helpers "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/ugorji/go/codec" @@ -302,6 +303,7 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() { var testnsg mgmtnetwork.SecurityGroup const nsg = "e2e-nsg" + const emptyMachineSet = "e2e-test-machineset" gatherNetworkInfo := func(ctx context.Context) { By("gathering vnet name, resource group, location, and adds master/worker subnets to list to reconcile") @@ -309,8 +311,9 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() { Expect(err).NotTo(HaveOccurred()) location = *oc.Location - vnet, masterSubnet, err := apisubnet.Split(*oc.OpenShiftClusterProperties.MasterProfile.SubnetID) + vnet, masterSubnet, err := apisubnet.Split((*oc.OpenShiftClusterProperties.MasterProfile.SubnetID)) Expect(err).NotTo(HaveOccurred()) + _, workerSubnet, err := apisubnet.Split((*(*oc.OpenShiftClusterProperties.WorkerProfiles)[0].SubnetID)) Expect(err).NotTo(HaveOccurred()) @@ -323,6 +326,32 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() { Expect(err).NotTo(HaveOccurred()) resourceGroup = r.ResourceGroup vnetName = r.ResourceName + + // Store the existing NSGs for the cluster before the test runs, in order to ensure we clean up + // after the test finishes, success or failure. + // This is expensive but will prevent flakes. + By("gathering existing subnet NSGs") + for subnet := range subnetsToReconcile { + subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "") + Expect(err).NotTo(HaveOccurred()) + + subnetsToReconcile[subnet] = subnetObject.NetworkSecurityGroup.ID + } + } + + cleanUpSubnetNSGs := func(ctx context.Context) { + By("cleaning up subnet NSGs") + for subnet := range subnetsToReconcile { + subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "") + Expect(err).NotTo(HaveOccurred()) + + if subnetObject.NetworkSecurityGroup.ID != subnetsToReconcile[subnet] { + subnetObject.NetworkSecurityGroup.ID = subnetsToReconcile[subnet] + + err = clients.Subnet.CreateOrUpdateAndWait(ctx, resourceGroup, vnetName, subnet, subnetObject) + Expect(err).NotTo(HaveOccurred()) + } + } } createE2ENSG := func(ctx context.Context) { @@ -355,11 +384,20 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() { createE2ENSG(ctx) }) AfterEach(func(ctx context.Context) { + cleanUpSubnetNSGs(ctx) + By("deleting test NSG") err := clients.NetworkSecurityGroups.DeleteAndWait(ctx, resourceGroup, nsg) if err != nil { log.Warn(err) } + + By("deleting test machineset if it still exists") + err = clients.MachineAPI.MachineV1beta1().MachineSets("openshift-machine-api").Delete(ctx, emptyMachineSet, metav1.DeleteOptions{}) + Expect(err).To(SatisfyAny( + Not(HaveOccurred()), + MatchError(kerrors.IsNotFound), + )) }) It("must reconcile list of subnets when NSG is changed", func(ctx context.Context) { for subnet := range subnetsToReconcile { @@ -367,13 +405,34 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() { // Gets current subnet NSG and then updates it to testnsg. subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "") Expect(err).NotTo(HaveOccurred()) - // Updates the value to the original NSG in our subnetsToReconcile map - subnetsToReconcile[subnet] = subnetObject.NetworkSecurityGroup.ID + subnetObject.NetworkSecurityGroup = &testnsg + err = clients.Subnet.CreateOrUpdateAndWait(ctx, resourceGroup, vnetName, subnet, subnetObject) Expect(err).NotTo(HaveOccurred()) } + By("creating an empty MachineSet to force a reconcile") + Eventually(func(g Gomega, ctx context.Context) { + machinesets, err := clients.MachineAPI.MachineV1beta1().MachineSets("openshift-machine-api").List(ctx, metav1.ListOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(machinesets.Items).To(Not(BeEmpty())) + + newMachineSet := machinesets.Items[0].DeepCopy() + newMachineSet.Status = machinev1beta1.MachineSetStatus{} + newMachineSet.ObjectMeta = metav1.ObjectMeta{ + Name: emptyMachineSet, + Namespace: "openshift-machine-api", + Annotations: newMachineSet.ObjectMeta.Annotations, + Labels: newMachineSet.ObjectMeta.Labels, + } + newMachineSet.Name = emptyMachineSet + newMachineSet.Spec.Replicas = to.Int32Ptr(0) + + _, err = clients.MachineAPI.MachineV1beta1().MachineSets("openshift-machine-api").Create(ctx, newMachineSet, metav1.CreateOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + }).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).Should(Succeed()) + for subnet, correctNSG := range subnetsToReconcile { By(fmt.Sprintf("waiting for the subnet %q to be reconciled so it includes the original cluster NSG", subnet)) Eventually(func(g Gomega, ctx context.Context) {