From 397fe322d753627b0a0915648ba0f1cf7b1263c5 Mon Sep 17 00:00:00 2001 From: Aldo Fuster Turpin Date: Wed, 11 Jan 2023 02:32:10 +0100 Subject: [PATCH] refactor remove_private_DNS_zone: split in narrow functions and remove duplication --- pkg/cluster/delete.go | 24 +- pkg/cluster/removeprivatednszone.go | 152 +--------- pkg/cluster/removeprivatednszone_test.go | 348 ++-------------------- pkg/util/mocks/net/net.go | 208 +++++++++++++ pkg/util/net/generate.go | 8 + pkg/util/net/net.go | 179 +++++++++++ pkg/util/net/net_test.go | 360 +++++++++++++++++++++++ pkg/util/ready/ready.go | 73 +++++ pkg/util/ready/ready_test.go | 338 +++++++++++++++++++++ pkg/util/version/clusterversion.go | 17 ++ 10 files changed, 1221 insertions(+), 486 deletions(-) create mode 100644 pkg/util/mocks/net/net.go create mode 100644 pkg/util/net/generate.go create mode 100644 pkg/util/net/net_test.go diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go index 3796a7cb5fe..16a37073a59 100644 --- a/pkg/cluster/delete.go +++ b/pkg/cluster/delete.go @@ -26,6 +26,7 @@ import ( "github.com/Azure/ARO-RP/pkg/util/azureclient" "github.com/Azure/ARO-RP/pkg/util/azureerrors" "github.com/Azure/ARO-RP/pkg/util/dns" + utilnet "github.com/Azure/ARO-RP/pkg/util/net" "github.com/Azure/ARO-RP/pkg/util/oidcbuilder" "github.com/Azure/ARO-RP/pkg/util/rbac" "github.com/Azure/ARO-RP/pkg/util/stringutils" @@ -63,27 +64,6 @@ func (m *manager) deleteNic(ctx context.Context, nicName string) error { return m.interfaces.DeleteAndWait(ctx, resourceGroup, *nic.Name) } -func (m *manager) deletePrivateDNSVirtualNetworkLinks(ctx context.Context, resourceID string) error { - r, err := azure.ParseResourceID(resourceID) - if err != nil { - return err - } - - virtualNetworkLinks, err := m.virtualNetworkLinks.List(ctx, r.ResourceGroup, r.ResourceName, nil) - if err != nil { - return err - } - - for _, virtualNetworkLink := range virtualNetworkLinks { - err = m.virtualNetworkLinks.DeleteAndWait(ctx, r.ResourceGroup, r.ResourceName, *virtualNetworkLink.Name, "") - if err != nil { - return err - } - } - - return nil -} - func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string) error { r, err := azure.ParseResourceID(resourceID) if err != nil { @@ -224,7 +204,7 @@ func (m *manager) deleteResources(ctx context.Context) error { case "microsoft.network/privatednszones": m.log.Printf("deleting private DNS nested resources of %s", *resource.ID) - err = m.deletePrivateDNSVirtualNetworkLinks(ctx, *resource.ID) + err = utilnet.DeletePrivateDNSVNetLinks(ctx, m.virtualNetworkLinks, *resource.ID) if err != nil { return err } diff --git a/pkg/cluster/removeprivatednszone.go b/pkg/cluster/removeprivatednszone.go index cd3a8d36b6e..99d632d7de4 100644 --- a/pkg/cluster/removeprivatednszone.go +++ b/pkg/cluster/removeprivatednszone.go @@ -5,147 +5,21 @@ package cluster import ( "context" - "strings" - "github.com/Azure/go-autorest/autorest/azure" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/util/retry" - - "github.com/Azure/ARO-RP/pkg/util/ready" - "github.com/Azure/ARO-RP/pkg/util/stringutils" - "github.com/Azure/ARO-RP/pkg/util/version" + utilnet "github.com/Azure/ARO-RP/pkg/util/net" ) func (m *manager) removePrivateDNSZone(ctx context.Context) error { - resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/') - - zones, err := m.privateZones.ListByResourceGroup(ctx, resourceGroup, nil) - if err != nil { - m.log.Print(err) - return nil - } - - if len(zones) == 0 { - // fix up any clusters that we already upgraded - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - dns, err := m.configcli.ConfigV1().DNSes().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - return err - } - - if dns.Spec.PrivateZone == nil || - !strings.HasPrefix(strings.ToLower(dns.Spec.PrivateZone.ID), strings.ToLower(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID)) { - return nil - } - - dns.Spec.PrivateZone = nil - - _, err = m.configcli.ConfigV1().DNSes().Update(ctx, dns, metav1.UpdateOptions{}) - return err - }) - if err != nil { - m.log.Print(err) - } - - return nil - } - - mcps, err := m.mcocli.MachineconfigurationV1().MachineConfigPools().List(ctx, metav1.ListOptions{}) - if err != nil { - m.log.Print(err) - return nil - } - - var machineCount int - for _, mcp := range mcps.Items { - var found bool - for _, source := range mcp.Status.Configuration.Source { - if source.Name == "99-"+mcp.Name+"-aro-dns" { - found = true - break - } - } - - if !found { - m.log.Printf("ARO DNS config not found in MCP %s", mcp.Name) - return nil - } - - if !ready.MachineConfigPoolIsReady(&mcp) { - m.log.Printf("MCP %s not ready", mcp.Name) - return nil - } - - machineCount += int(mcp.Status.MachineCount) - } - - nodes, err := m.kubernetescli.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) - if err != nil { - m.log.Print(err) - return nil - } - - if len(nodes.Items) != machineCount { - m.log.Printf("cluster has %d nodes but %d under MCPs, not removing private DNS zone", len(nodes.Items), machineCount) - return nil - } - - cv, err := m.configcli.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) - if err != nil { - return err - } - v, err := version.GetClusterVersion(cv) - if err != nil { - m.log.Print(err) - return nil - } - - if v.Lt(version.NewVersion(4, 4)) { - // 4.3 uses SRV records for etcd - m.log.Printf("cluster version < 4.4, not removing private DNS zone") - return nil - } - - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - dns, err := m.configcli.ConfigV1().DNSes().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - return err - } - - if dns.Spec.PrivateZone == nil || - !strings.HasPrefix(strings.ToLower(dns.Spec.PrivateZone.ID), strings.ToLower(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID)) { - return nil - } - - dns.Spec.PrivateZone = nil - - _, err = m.configcli.ConfigV1().DNSes().Update(ctx, dns, metav1.UpdateOptions{}) - return err - }) - if err != nil { - m.log.Print(err) - return nil - } - - for _, zone := range zones { - err = m.deletePrivateDNSVirtualNetworkLinks(ctx, *zone.ID) - if err != nil { - m.log.Print(err) - return nil - } - - r, err := azure.ParseResourceID(*zone.ID) - if err != nil { - m.log.Print(err) - return nil - } - - err = m.privateZones.DeleteAndWait(ctx, resourceGroup, r.ResourceName, "") - if err != nil { - m.log.Print(err) - return nil - } - } - - return nil + resourceGroupID := m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID + config := utilnet.PrivateZoneRemovalConfig{ + Log: m.log, + PrivateZonesClient: m.privateZones, + Configcli: m.configcli, + Mcocli: m.mcocli, + Kubernetescli: m.kubernetescli, + VNetLinksClient: m.virtualNetworkLinks, + ResourceGroupID: resourceGroupID, + } + + return utilnet.RemovePrivateDNSZone(ctx, config) } diff --git a/pkg/cluster/removeprivatednszone_test.go b/pkg/cluster/removeprivatednszone_test.go index cf452e534cb..a454989bf98 100644 --- a/pkg/cluster/removeprivatednszone_test.go +++ b/pkg/cluster/removeprivatednszone_test.go @@ -5,348 +5,46 @@ package cluster import ( "context" + "errors" "testing" - mgmtprivatedns "github.com/Azure/azure-sdk-for-go/services/privatedns/mgmt/2018-09-01/privatedns" - "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" - configv1 "github.com/openshift/api/config/v1" - configclient "github.com/openshift/client-go/config/clientset/versioned" - configfake "github.com/openshift/client-go/config/clientset/versioned/fake" - mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" - mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned" - mcofake "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/fake" "github.com/sirupsen/logrus" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/kubernetes/fake" "github.com/Azure/ARO-RP/pkg/api" mock_privatedns "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/privatedns" + utilerror "github.com/Azure/ARO-RP/test/util/error" ) func TestRemovePrivateDNSZone(t *testing.T) { ctx := context.Background() const resourceGroupID = "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" - for _, tt := range []struct { - name string - doc *api.OpenShiftClusterDocument - mocks func(*mock_privatedns.MockPrivateZonesClient, *mock_privatedns.MockVirtualNetworkLinksClient) - kubernetescli kubernetes.Interface - mcocli mcoclient.Interface - configcli configclient.Interface - wantDNSPrivateZoneRemoved bool - }{ - { - name: "no private zones", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, - mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return(nil, nil) - }, - configcli: configfake.NewSimpleClientset( - &configv1.DNS{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - Spec: configv1.DNSSpec{ - PrivateZone: &configv1.DNSZone{ - ID: "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1", - }, - }, - }, - ), - wantDNSPrivateZoneRemoved: true, - }, - { - name: "has private zone, dnsmasq config not yet reconciled", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, - mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return([]mgmtprivatedns.PrivateZone{ - { - ID: to.StringPtr("/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1"), - }, - }, nil) - }, - mcocli: mcofake.NewSimpleClientset( - &mcv1.MachineConfigPool{}, - ), - }, - { - name: "has private zone, pool not yet ready", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, - mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return([]mgmtprivatedns.PrivateZone{ - { - ID: to.StringPtr("/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1"), - }, - }, nil) - }, - mcocli: mcofake.NewSimpleClientset( - &mcv1.MachineConfigPool{ - ObjectMeta: metav1.ObjectMeta{ - Name: "master", - }, - Status: mcv1.MachineConfigPoolStatus{ - Configuration: mcv1.MachineConfigPoolStatusConfiguration{ - Source: []corev1.ObjectReference{ - { - Name: "99-master-aro-dns", - }, - }, - }, - MachineCount: 1, - }, - }, - ), - }, - { - name: "has private zone, node mismatch", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, - mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return([]mgmtprivatedns.PrivateZone{ - { - ID: to.StringPtr("/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1"), - }, - }, nil) - }, - kubernetescli: fake.NewSimpleClientset( - &corev1.Node{}, - ), - mcocli: mcofake.NewSimpleClientset( - &mcv1.MachineConfigPool{ - ObjectMeta: metav1.ObjectMeta{ - Name: "master", - }, - Status: mcv1.MachineConfigPoolStatus{ - Configuration: mcv1.MachineConfigPoolStatusConfiguration{ - Source: []corev1.ObjectReference{ - { - Name: "99-master-aro-dns", - }, - }, - }, - }, - }, - ), - }, - { - name: "has private zone, nodes match, 4.3, dnsmasq rolled out", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, - mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return([]mgmtprivatedns.PrivateZone{ - { - ID: to.StringPtr("/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1"), - }, - }, nil) - }, - kubernetescli: fake.NewSimpleClientset( - &corev1.Node{}, - ), - mcocli: mcofake.NewSimpleClientset( - &mcv1.MachineConfigPool{ - ObjectMeta: metav1.ObjectMeta{ - Name: "master", - }, - Status: mcv1.MachineConfigPoolStatus{ - Configuration: mcv1.MachineConfigPoolStatusConfiguration{ - Source: []corev1.ObjectReference{ - { - Name: "99-master-aro-dns", - }, - }, - }, - MachineCount: 1, - UpdatedMachineCount: 1, - ReadyMachineCount: 1, - }, - }, - ), - configcli: configfake.NewSimpleClientset( - &configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - History: []configv1.UpdateHistory{ - { - State: configv1.CompletedUpdate, - Version: "4.3.999", - }, - }, - }, - }, - ), - }, - { - name: "has private zone, nodes match, 4.4, dnsmasq rolled out", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, - mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return([]mgmtprivatedns.PrivateZone{ - { - ID: to.StringPtr("/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1"), - }, - }, nil) + t.Run("should return nil when privateZones.ListByResourceGroup() returns an error", func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() - virtualNetworkLinks.EXPECT(). - List(ctx, "testGroup", "zone1", nil). - Return([]mgmtprivatedns.VirtualNetworkLink{ - { - Name: to.StringPtr("link1"), - }, - }, nil) + privateZones := mock_privatedns.NewMockPrivateZonesClient(controller) + privateZones.EXPECT().ListByResourceGroup(ctx, gomock.Any(), nil).Return(nil, errors.New("someError")) - virtualNetworkLinks.EXPECT(). - DeleteAndWait(ctx, "testGroup", "zone1", "link1", ""). - Return(nil) - - privateZones.EXPECT(). - DeleteAndWait(ctx, "testGroup", "zone1", ""). - Return(nil) - }, - kubernetescli: fake.NewSimpleClientset( - &corev1.Node{}, - ), - mcocli: mcofake.NewSimpleClientset( - &mcv1.MachineConfigPool{ - ObjectMeta: metav1.ObjectMeta{ - Name: "master", - }, - Status: mcv1.MachineConfigPoolStatus{ - Configuration: mcv1.MachineConfigPoolStatusConfiguration{ - Source: []corev1.ObjectReference{ - { - Name: "99-master-aro-dns", - }, - }, - }, - MachineCount: 1, - UpdatedMachineCount: 1, - ReadyMachineCount: 1, - }, - }, - ), - configcli: configfake.NewSimpleClientset( - &configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - History: []configv1.UpdateHistory{ - { - State: configv1.CompletedUpdate, - Version: "4.4.0", - }, - }, + doc := &api.OpenShiftClusterDocument{ + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + ResourceGroupID: resourceGroupID, }, }, - &configv1.DNS{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - Spec: configv1.DNSSpec{ - PrivateZone: &configv1.DNSZone{ - ID: "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1", - }, - }, - }, - ), - wantDNSPrivateZoneRemoved: true, - }, - } { - t.Run(tt.name, func(t *testing.T) { - controller := gomock.NewController(t) - defer controller.Finish() - - privateZones := mock_privatedns.NewMockPrivateZonesClient(controller) - virtualNetworkLinks := mock_privatedns.NewMockVirtualNetworkLinksClient(controller) - tt.mocks(privateZones, virtualNetworkLinks) - - m := &manager{ - log: logrus.NewEntry(logrus.StandardLogger()), - doc: tt.doc, - privateZones: privateZones, - virtualNetworkLinks: virtualNetworkLinks, - kubernetescli: tt.kubernetescli, - mcocli: tt.mcocli, - configcli: tt.configcli, - } + }, + } + m := &manager{ + doc: doc, + log: logrus.NewEntry(logrus.StandardLogger()), + privateZones: privateZones, + } - err := m.removePrivateDNSZone(ctx) - if err != nil { - t.Fatal(err) - } + err := m.removePrivateDNSZone(ctx) - if tt.wantDNSPrivateZoneRemoved { - dns, err := m.configcli.ConfigV1().DNSes().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - t.Fatal(err) - } - if dns.Spec.PrivateZone != nil { - t.Error(dns.Spec.PrivateZone) - } - } - }) - } + expectedError := "" + utilerror.AssertErrorMessage(t, err, expectedError) + }) } diff --git a/pkg/util/mocks/net/net.go b/pkg/util/mocks/net/net.go new file mode 100644 index 00000000000..7031e173663 --- /dev/null +++ b/pkg/util/mocks/net/net.go @@ -0,0 +1,208 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/Azure/ARO-RP/pkg/util/net (interfaces: DNSIClient) + +// Package mock_net is a generated GoMock package. +package mock_net + +import ( + context "context" + reflect "reflect" + + gomock "github.com/golang/mock/gomock" + v1 "github.com/openshift/api/config/v1" + v10 "github.com/openshift/client-go/config/applyconfigurations/config/v1" + v11 "k8s.io/apimachinery/pkg/apis/meta/v1" + types "k8s.io/apimachinery/pkg/types" + watch "k8s.io/apimachinery/pkg/watch" +) + +// MockDNSIClient is a mock of DNSIClient interface. +type MockDNSIClient struct { + ctrl *gomock.Controller + recorder *MockDNSIClientMockRecorder +} + +// MockDNSIClientMockRecorder is the mock recorder for MockDNSIClient. +type MockDNSIClientMockRecorder struct { + mock *MockDNSIClient +} + +// NewMockDNSIClient creates a new mock instance. +func NewMockDNSIClient(ctrl *gomock.Controller) *MockDNSIClient { + mock := &MockDNSIClient{ctrl: ctrl} + mock.recorder = &MockDNSIClientMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockDNSIClient) EXPECT() *MockDNSIClientMockRecorder { + return m.recorder +} + +// Apply mocks base method. +func (m *MockDNSIClient) Apply(arg0 context.Context, arg1 *v10.DNSApplyConfiguration, arg2 v11.ApplyOptions) (*v1.DNS, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Apply", arg0, arg1, arg2) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Apply indicates an expected call of Apply. +func (mr *MockDNSIClientMockRecorder) Apply(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Apply", reflect.TypeOf((*MockDNSIClient)(nil).Apply), arg0, arg1, arg2) +} + +// ApplyStatus mocks base method. +func (m *MockDNSIClient) ApplyStatus(arg0 context.Context, arg1 *v10.DNSApplyConfiguration, arg2 v11.ApplyOptions) (*v1.DNS, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ApplyStatus", arg0, arg1, arg2) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ApplyStatus indicates an expected call of ApplyStatus. +func (mr *MockDNSIClientMockRecorder) ApplyStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApplyStatus", reflect.TypeOf((*MockDNSIClient)(nil).ApplyStatus), arg0, arg1, arg2) +} + +// Create mocks base method. +func (m *MockDNSIClient) Create(arg0 context.Context, arg1 *v1.DNS, arg2 v11.CreateOptions) (*v1.DNS, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Create", arg0, arg1, arg2) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Create indicates an expected call of Create. +func (mr *MockDNSIClientMockRecorder) Create(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Create", reflect.TypeOf((*MockDNSIClient)(nil).Create), arg0, arg1, arg2) +} + +// Delete mocks base method. +func (m *MockDNSIClient) Delete(arg0 context.Context, arg1 string, arg2 v11.DeleteOptions) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// Delete indicates an expected call of Delete. +func (mr *MockDNSIClientMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockDNSIClient)(nil).Delete), arg0, arg1, arg2) +} + +// DeleteCollection mocks base method. +func (m *MockDNSIClient) DeleteCollection(arg0 context.Context, arg1 v11.DeleteOptions, arg2 v11.ListOptions) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteCollection", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteCollection indicates an expected call of DeleteCollection. +func (mr *MockDNSIClientMockRecorder) DeleteCollection(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteCollection", reflect.TypeOf((*MockDNSIClient)(nil).DeleteCollection), arg0, arg1, arg2) +} + +// Get mocks base method. +func (m *MockDNSIClient) Get(arg0 context.Context, arg1 string, arg2 v11.GetOptions) (*v1.DNS, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Get indicates an expected call of Get. +func (mr *MockDNSIClientMockRecorder) Get(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockDNSIClient)(nil).Get), arg0, arg1, arg2) +} + +// List mocks base method. +func (m *MockDNSIClient) List(arg0 context.Context, arg1 v11.ListOptions) (*v1.DNSList, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "List", arg0, arg1) + ret0, _ := ret[0].(*v1.DNSList) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// List indicates an expected call of List. +func (mr *MockDNSIClientMockRecorder) List(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockDNSIClient)(nil).List), arg0, arg1) +} + +// Patch mocks base method. +func (m *MockDNSIClient) Patch(arg0 context.Context, arg1 string, arg2 types.PatchType, arg3 []byte, arg4 v11.PatchOptions, arg5 ...string) (*v1.DNS, error) { + m.ctrl.T.Helper() + varargs := []interface{}{arg0, arg1, arg2, arg3, arg4} + for _, a := range arg5 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Patch", varargs...) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Patch indicates an expected call of Patch. +func (mr *MockDNSIClientMockRecorder) Patch(arg0, arg1, arg2, arg3, arg4 interface{}, arg5 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0, arg1, arg2, arg3, arg4}, arg5...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Patch", reflect.TypeOf((*MockDNSIClient)(nil).Patch), varargs...) +} + +// Update mocks base method. +func (m *MockDNSIClient) Update(arg0 context.Context, arg1 *v1.DNS, arg2 v11.UpdateOptions) (*v1.DNS, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Update", arg0, arg1, arg2) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Update indicates an expected call of Update. +func (mr *MockDNSIClientMockRecorder) Update(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Update", reflect.TypeOf((*MockDNSIClient)(nil).Update), arg0, arg1, arg2) +} + +// UpdateStatus mocks base method. +func (m *MockDNSIClient) UpdateStatus(arg0 context.Context, arg1 *v1.DNS, arg2 v11.UpdateOptions) (*v1.DNS, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateStatus", arg0, arg1, arg2) + ret0, _ := ret[0].(*v1.DNS) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UpdateStatus indicates an expected call of UpdateStatus. +func (mr *MockDNSIClientMockRecorder) UpdateStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateStatus", reflect.TypeOf((*MockDNSIClient)(nil).UpdateStatus), arg0, arg1, arg2) +} + +// Watch mocks base method. +func (m *MockDNSIClient) Watch(arg0 context.Context, arg1 v11.ListOptions) (watch.Interface, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Watch", arg0, arg1) + ret0, _ := ret[0].(watch.Interface) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Watch indicates an expected call of Watch. +func (mr *MockDNSIClientMockRecorder) Watch(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Watch", reflect.TypeOf((*MockDNSIClient)(nil).Watch), arg0, arg1) +} diff --git a/pkg/util/net/generate.go b/pkg/util/net/generate.go new file mode 100644 index 00000000000..68d62f45635 --- /dev/null +++ b/pkg/util/net/generate.go @@ -0,0 +1,8 @@ +package net + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +//go:generate rm -rf ../../util/mocks/$GOPACKAGE +//go:generate go run ../../../vendor/github.com/golang/mock/mockgen -destination=../../../pkg/util/mocks/$GOPACKAGE/$GOPACKAGE.go github.com/Azure/ARO-RP/pkg/util/$GOPACKAGE DNSIClient +//go:generate go run ../../../vendor/golang.org/x/tools/cmd/goimports -local=github.com/Azure/ARO-RP -e -w ../../../pkg/util/mocks/$GOPACKAGE/$GOPACKAGE.go diff --git a/pkg/util/net/net.go b/pkg/util/net/net.go index 5a39bdbbef3..4512d3fd152 100644 --- a/pkg/util/net/net.go +++ b/pkg/util/net/net.go @@ -4,9 +4,26 @@ package net // Licensed under the Apache License 2.0. import ( + "context" "errors" "net" + "strings" "syscall" + + mgmtprivatedns "github.com/Azure/azure-sdk-for-go/services/privatedns/mgmt/2018-09-01/privatedns" + "github.com/Azure/go-autorest/autorest/azure" + configclient "github.com/openshift/client-go/config/clientset/versioned" + v1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" + mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/util/retry" + + "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/privatedns" + "github.com/Azure/ARO-RP/pkg/util/ready" + "github.com/Azure/ARO-RP/pkg/util/stringutils" + "github.com/Azure/ARO-RP/pkg/util/version" ) // Listen returns a listener with its send and receive buffer sizes set, such @@ -72,3 +89,165 @@ func setBuffers(rc syscall.RawConn, sz int) error { return err } + +type PrivateZoneRemovalConfig struct { + Log *logrus.Entry + PrivateZonesClient privatedns.PrivateZonesClient + Configcli configclient.Interface + Mcocli mcoclient.Interface + Kubernetescli kubernetes.Interface + VNetLinksClient privatedns.VirtualNetworkLinksClient + ResourceGroupID string +} + +func RemovePrivateDNSZone(ctx context.Context, config PrivateZoneRemovalConfig) error { + if config.PrivateZonesClient == nil { + return errors.New("privateZonesClient is nil") + } + + resourceGroup := stringutils.LastTokenByte(config.ResourceGroupID, '/') + privateZones, err := config.PrivateZonesClient.ListByResourceGroup(ctx, resourceGroup, nil) + if err != nil { + config.Log.Print(err) + return nil + } + + if config.Configcli == nil { + return errors.New("configcli is nil") + } + + if len(privateZones) == 0 { + // fix up any clusters that we already upgraded + if err := UpdateDNSes(ctx, config.Configcli.ConfigV1().DNSes(), config.ResourceGroupID); err != nil { + config.Log.Print(err) + } + return nil + } + + if config.Mcocli == nil { + return errors.New("mcocli is nil") + } + + if config.Kubernetescli == nil { + return errors.New("kubernetescli is nil") + } + + mcpLister := config.Mcocli.MachineconfigurationV1().MachineConfigPools() + nodeLister := config.Kubernetescli.CoreV1().Nodes() + sameNumberOfNodesAndMachines, err := ready.SameNumberOfNodesAndMachines(ctx, mcpLister, nodeLister) + if err != nil { + config.Log.Print(err) + return nil + } + + if !sameNumberOfNodesAndMachines { + return nil + } + + clusterVersionIsLessThan4_4, err := version.ClusterVersionIsLessThan4_4(ctx, config.Configcli) + if err != nil { + config.Log.Print(err) + return nil + } + + if clusterVersionIsLessThan4_4 { + config.Log.Printf("cluster version < 4.4, not removing private DNS zone") + return nil + } + + if err = UpdateDNSes(ctx, config.Configcli.ConfigV1().DNSes(), config.ResourceGroupID); err != nil { + config.Log.Print(err) + return nil + } + + if err := RemoveZones(ctx, config.VNetLinksClient, config.PrivateZonesClient, privateZones, resourceGroup); err != nil { + config.Log.Print(err) + return nil + } + return nil +} + +func DeletePrivateDNSVNetLinks(ctx context.Context, vNetLinksClient privatedns.VirtualNetworkLinksClient, resourceID string) error { + r, err := azure.ParseResourceID(resourceID) + if err != nil { + return err + } + + if vNetLinksClient == nil { + return errors.New("vNetLinksClient is nil") + } + + vNetLinks, err := vNetLinksClient.List(ctx, r.ResourceGroup, r.ResourceName, nil) + if err != nil { + return err + } + + for _, vNetLink := range vNetLinks { + err = vNetLinksClient.DeleteAndWait(ctx, r.ResourceGroup, r.ResourceName, *vNetLink.Name, "") + if err != nil { + return err + } + } + + return nil +} + +func RemoveZones(ctx context.Context, + vNetLinksClient privatedns.VirtualNetworkLinksClient, + privateZoneClient privatedns.PrivateZonesClient, + privateZones []mgmtprivatedns.PrivateZone, + resourceGroup string) error { + for _, privateZone := range privateZones { + if err := DeletePrivateDNSVNetLinks(ctx, vNetLinksClient, *privateZone.ID); err != nil { + return err + } + + r, err := azure.ParseResourceID(*privateZone.ID) + if err != nil { + return err + } + + if privateZoneClient == nil { + return errors.New("privateZoneClient is nil") + } + + if err := privateZoneClient.DeleteAndWait(ctx, resourceGroup, r.ResourceName, ""); err != nil { + return err + } + } + return nil +} + +func UpdateDNSes(ctx context.Context, dnsI DNSIClient, resourceGroupID string) error { + fn := updateClusterDNSFn(ctx, dnsI, resourceGroupID) + return retry.RetryOnConflict(retry.DefaultRetry, fn) +} + +type DNSIClient interface { + v1.DNSInterface +} + +func updateClusterDNSFn(ctx context.Context, dnsClient DNSIClient, resourceGroupID string) func() error { + return func() error { + if dnsClient == nil { + return errors.New("dnsClient interface is nil") + } + + dns, err := dnsClient.Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + return err + } + + if dns.Spec.PrivateZone == nil || + !strings.HasPrefix( + strings.ToLower(dns.Spec.PrivateZone.ID), + strings.ToLower(resourceGroupID)) { + return nil + } + + dns.Spec.PrivateZone = nil + + _, err = dnsClient.Update(ctx, dns, metav1.UpdateOptions{}) + return err + } +} diff --git a/pkg/util/net/net_test.go b/pkg/util/net/net_test.go new file mode 100644 index 00000000000..d2b7e8ba578 --- /dev/null +++ b/pkg/util/net/net_test.go @@ -0,0 +1,360 @@ +package net + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "errors" + "testing" + + mgmtprivatedns "github.com/Azure/azure-sdk-for-go/services/privatedns/mgmt/2018-09-01/privatedns" + "github.com/Azure/go-autorest/autorest/to" + "github.com/golang/mock/gomock" + configv1 "github.com/openshift/api/config/v1" + configclient "github.com/openshift/client-go/config/clientset/versioned" + configfake "github.com/openshift/client-go/config/clientset/versioned/fake" + mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned" + mcofake "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/fake" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + + "github.com/Azure/ARO-RP/pkg/api" + utillog "github.com/Azure/ARO-RP/pkg/util/log" + mock_privatedns "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/privatedns" + mock_net "github.com/Azure/ARO-RP/pkg/util/mocks/net" + utilerror "github.com/Azure/ARO-RP/test/util/error" +) + +var ( + resourceGroupName = "testGroup" + subscriptionID = "0000000-0000-0000-0000-000000000000" + resourceGroupID = "/subscriptions/" + subscriptionID + "/resourceGroups/" + resourceGroupName + vnetName = "testVnet" + resourceID = resourceGroupID + "/providers/Microsoft.Network/virtualNetworks/" + vnetName +) + +const id = "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1" + +func TestUpdateClusterDNSFn(t *testing.T) { + type testCase struct { + name string + resourceGroupID string + wantErr string + ensureMocksBehavior func(dnsI mock_net.MockDNSIClient) + } + + testcases := []testCase{ + { + name: "should propagate the error from dnsi.Get", + wantErr: "some error", + ensureMocksBehavior: func(dnsI mock_net.MockDNSIClient) { + dnsI.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) + }, + }, + { + name: "should not call dnsI.Update when private zone id does not have the resource group id as prefix", + resourceGroupID: "rg", + ensureMocksBehavior: func(dnsI mock_net.MockDNSIClient) { + v := &configv1.DNS{ + Spec: configv1.DNSSpec{ + PrivateZone: &configv1.DNSZone{ + ID: "not_same_rg_the_id", + }, + }, + } + dnsI.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(v, nil) + dnsI.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + }, + }, + { + name: "should call dnsI.Update with Spec.PrivateZone equal to nil", + resourceGroupID: "rg_", + ensureMocksBehavior: func(dnsI mock_net.MockDNSIClient) { + v := configv1.DNS{ + Spec: configv1.DNSSpec{ + PrivateZone: &configv1.DNSZone{ + ID: "rg_the_id", + }, + }, + } + + dnsI.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(&v, nil) + dnsI.EXPECT().Update(gomock.Any(), MatchesNilPrivateZone{}, gomock.Any()) + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + dnsI := mock_net.NewMockDNSIClient(controller) + + if tc.ensureMocksBehavior != nil { + tc.ensureMocksBehavior(*dnsI) + } + + fn := updateClusterDNSFn(context.Background(), dnsI, tc.resourceGroupID) + err := fn() + utilerror.AssertErrorMessage(t, err, tc.wantErr) + }) + } + + t.Run("dnsI is nil", func(t *testing.T) { + fn := updateClusterDNSFn(context.Background(), nil, "") + err := fn() + utilerror.AssertErrorMessage(t, err, "dnsClient interface is nil") + }) +} + +type MatchesNilPrivateZone struct { +} + +func (MatchesNilPrivateZone) Matches(x interface{}) bool { + arg, ok := x.(*configv1.DNS) + if !ok { + return false + } + + return arg.Spec.PrivateZone == nil +} + +func (MatchesNilPrivateZone) String() string { + return "arg.Spec.PrivateZone does not match (is not nil)" +} + +func TestDeletePrivateDNSVNetLinks(t *testing.T) { + type testCase struct { + name string + resourceID string + wantErr string + ensureMocksBehavior func(vNetLinksClient *mock_privatedns.MockVirtualNetworkLinksClient) + } + testcases := []testCase{ + { + name: "propagates invalid resource id error", + resourceID: "invalid_resourceId", + wantErr: "parsing failed for invalid_resourceId. Invalid resource Id format", + ensureMocksBehavior: func(vNetLinksClient *mock_privatedns.MockVirtualNetworkLinksClient) { + vNetLinksClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil, errors.New("parsing failed for invalid_resourceId. Invalid resource Id format")) + }, + }, + { + name: "propagates error from vNetLinksClient.List", + wantErr: "some_error", + ensureMocksBehavior: func(vNetLinksClient *mock_privatedns.MockVirtualNetworkLinksClient) { + vNetLinksClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("some_error")) + }, + }, + { + name: "ppropagates error from vNetLinksClient.DeleteAndWait", + wantErr: "some_error", + ensureMocksBehavior: func(vNetLinksClient *mock_privatedns.MockVirtualNetworkLinksClient) { + name := "name" + listResult := []mgmtprivatedns.VirtualNetworkLink{{Name: &name}} + vNetLinksClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(listResult, nil) + vNetLinksClient.EXPECT().DeleteAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("some_error")) + }, + }, + { + name: "returns nil when no errors found", + ensureMocksBehavior: func(vNetLinksClient *mock_privatedns.MockVirtualNetworkLinksClient) { + name := "name" + listResult := []mgmtprivatedns.VirtualNetworkLink{{Name: &name}} + vNetLinksClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(listResult, nil) + vNetLinksClient.EXPECT().DeleteAndWait(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + vNetLinksClient := mock_privatedns.NewMockVirtualNetworkLinksClient(controller) + + if tc.ensureMocksBehavior != nil { + tc.ensureMocksBehavior(vNetLinksClient) + } + + err := DeletePrivateDNSVNetLinks(context.Background(), vNetLinksClient, resourceID) + utilerror.AssertErrorMessage(t, err, tc.wantErr) + }) + } +} + +func TestRemovePrivateDNSZone(t *testing.T) { + privateZone := []mgmtprivatedns.PrivateZone{{ID: to.StringPtr(id)}} + + doc := &api.OpenShiftClusterDocument{ + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ResourceGroupID: resourceGroupID}, + }, + }, + } + + dns := &configv1.DNS{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: configv1.DNSSpec{ + PrivateZone: &configv1.DNSZone{ID: id}, + }, + } + + mcp := &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "master", + }, + Status: mcv1.MachineConfigPoolStatus{ + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "99-master-aro-dns"}}, + }, + MachineCount: 1, + }, + } + + ctx := context.Background() + + for _, tt := range []struct { + name string + doc *api.OpenShiftClusterDocument + mocks func(*mock_privatedns.MockPrivateZonesClient, *mock_privatedns.MockVirtualNetworkLinksClient) + kubernetescli kubernetes.Interface + mcocli mcoclient.Interface + configcli configclient.Interface + wantDNSPrivateZoneRemoved bool + wantError string + }{ + { + name: "no private zones", + doc: doc, + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(nil, nil) + }, + configcli: configfake.NewSimpleClientset(dns), + wantDNSPrivateZoneRemoved: true, + }, + { + name: "has private zone, dnsmasq config not yet reconciled", + doc: doc, + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return([]mgmtprivatedns.PrivateZone{{ID: to.StringPtr(id)}}, nil) + }, + mcocli: mcofake.NewSimpleClientset(&mcv1.MachineConfigPool{}), + configcli: configfake.NewSimpleClientset(), + kubernetescli: fake.NewSimpleClientset( + &corev1.Node{}, + ), + }, + { + name: "has private zone, pool not yet ready", + doc: doc, + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(privateZone, nil) + }, + mcocli: mcofake.NewSimpleClientset(mcp), + kubernetescli: fake.NewSimpleClientset( + &corev1.Node{}, + ), + wantError: "configcli is nil", + }, + { + name: "has private zone, nodes match, 4.4, dnsmasq rolled out", + doc: doc, + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(privateZone, nil) + + virtualNetworkLinks.EXPECT(). + List(ctx, "testGroup", "zone1", nil). + Return([]mgmtprivatedns.VirtualNetworkLink{ + { + Name: to.StringPtr("link1"), + }, + }, nil) + + virtualNetworkLinks.EXPECT(). + DeleteAndWait(ctx, "testGroup", "zone1", "link1", ""). + Return(nil) + + privateZones.EXPECT(). + DeleteAndWait(ctx, "testGroup", "zone1", ""). + Return(nil) + }, + kubernetescli: fake.NewSimpleClientset( + &corev1.Node{}, + ), + mcocli: mcofake.NewSimpleClientset( + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "master", + }, + Status: mcv1.MachineConfigPoolStatus{ + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{ + { + Name: "99-master-aro-dns", + }, + }, + }, + MachineCount: 1, + UpdatedMachineCount: 1, + ReadyMachineCount: 1, + }, + }, + ), + configcli: configfake.NewSimpleClientset( + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.4.0", + }, + }, + }, + }, + dns, + ), + wantDNSPrivateZoneRemoved: true, + }, + } { + t.Run(tt.name, func(t *testing.T) { + controller := gomock.NewController(t) + defer controller.Finish() + + privateZones := mock_privatedns.NewMockPrivateZonesClient(controller) + virtualNetworkLinks := mock_privatedns.NewMockVirtualNetworkLinksClient(controller) + tt.mocks(privateZones, virtualNetworkLinks) + + config := PrivateZoneRemovalConfig{ + Log: utillog.GetLogger(), + PrivateZonesClient: privateZones, + Configcli: tt.configcli, + Mcocli: tt.mcocli, + Kubernetescli: tt.kubernetescli, + VNetLinksClient: virtualNetworkLinks, + ResourceGroupID: resourceGroupID, + } + err := RemovePrivateDNSZone(ctx, config) + + utilerror.AssertErrorMessage(t, err, tt.wantError) + + if tt.wantDNSPrivateZoneRemoved { + dns, err := tt.configcli.ConfigV1().DNSes().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if dns.Spec.PrivateZone != nil { + t.Error(dns.Spec.PrivateZone) + } + } + }) + } +} diff --git a/pkg/util/ready/ready.go b/pkg/util/ready/ready.go index cb1540b3b63..67e7d841df2 100644 --- a/pkg/util/ready/ready.go +++ b/pkg/util/ready/ready.go @@ -5,6 +5,7 @@ package ready import ( "context" + "fmt" "net" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" @@ -193,3 +194,75 @@ func CheckMachineConfigPoolIsReady(ctx context.Context, cli mcoclientv1.MachineC return MachineConfigPoolIsReady(s), nil } } + +type MCPLister interface { + List(ctx context.Context, opts metav1.ListOptions) (*mcv1.MachineConfigPoolList, error) +} + +type NodeLister interface { + List(ctx context.Context, opts metav1.ListOptions) (*corev1.NodeList, error) +} + +// SameNumberOfNodesAndMachines returns true if the cluster has the same number of nodes and total machines, +// and an error if any. Otherwise it returns false and nil. +func SameNumberOfNodesAndMachines(ctx context.Context, mcpLister MCPLister, nodeLister NodeLister) (bool, error) { + if mcpLister == nil { + return false, fmt.Errorf("mcpLister is nil") + } + + if nodeLister == nil { + return false, fmt.Errorf("nodeLister is nil") + } + + machineConfigPoolList, err := mcpLister.List(ctx, metav1.ListOptions{}) + if err != nil { + return false, err + } + + totalMachines, err := TotalMachinesInTheMCPs(machineConfigPoolList.Items) + if err != nil { + return false, err + } + + nodes, err := nodeLister.List(ctx, metav1.ListOptions{}) + if err != nil { + return false, err + } + + nNodes := len(nodes.Items) + if nNodes != totalMachines { + return false, fmt.Errorf("cluster has %d nodes but %d under MCPs, not removing private DNS zone", nNodes, totalMachines) + } + + return true, nil +} + +// TotalMachinesInTheMCPs returns the total number of machines in the machineConfigPools +// and an error, if any. +func TotalMachinesInTheMCPs(machineConfigPools []mcv1.MachineConfigPool) (int, error) { + totalMachines := 0 + for _, mcp := range machineConfigPools { + if !MCPContainsARODNSConfig(mcp) { + return 0, fmt.Errorf("ARO DNS config not found in MCP %s", mcp.Name) + } + + if !MachineConfigPoolIsReady(&mcp) { + return 0, fmt.Errorf("MCP %s not ready", mcp.Name) + } + + totalMachines += int(mcp.Status.MachineCount) + } + return totalMachines, nil +} + +func MCPContainsARODNSConfig(mcp mcv1.MachineConfigPool) bool { + for _, source := range mcp.Status.Configuration.Source { + mcpPrefix := "99-" + mcpSuffix := "-aro-dns" + + if source.Name == mcpPrefix+mcp.Name+mcpSuffix { + return true + } + } + return false +} diff --git a/pkg/util/ready/ready_test.go b/pkg/util/ready/ready_test.go index ddcf1ca643f..e28a6a5a5c3 100644 --- a/pkg/util/ready/ready_test.go +++ b/pkg/util/ready/ready_test.go @@ -16,6 +16,8 @@ import ( kruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" ktesting "k8s.io/client-go/testing" + + utilerror "github.com/Azure/ARO-RP/test/util/error" ) func TestNodeIsReady(t *testing.T) { @@ -650,3 +652,339 @@ func TestCheckPodsAreReadyError(t *testing.T) { t.Fatalf("check pod error is: %v", err) } } + +func TestTotalMachinesInTheMCPs(t *testing.T) { + mcpPrefix := "99-" + mcpSuffix := "-aro-dns" + + type testCase struct { + name string + machineConfigPools []mcv1.MachineConfigPool + want int + wantErr string + } + testCases := []testCase{ + { + name: "totalMachinesInTheMCPs returns ARO DNS config not found error", + want: 0, + machineConfigPools: []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + }, + Status: mcv1.MachineConfigPoolStatus{ + ObservedGeneration: 0, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "non matching name"}}, + }, + }, + }, + }, + wantErr: "ARO DNS config not found in MCP mcp_name", + }, + { + name: "totalMachinesInTheMCPs returns MCP is not ready error", + want: 0, + wantErr: "MCP mcp_name not ready", + machineConfigPools: []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + Generation: 1, + }, + Status: mcv1.MachineConfigPoolStatus{ + MachineCount: 1, + UpdatedMachineCount: 1, + ReadyMachineCount: 0, + ObservedGeneration: 1, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: mcpPrefix + "mcp_name" + mcpSuffix}}, + }, + }, + }, + }, + }, + { + name: "totalMachinesInTheMCPs returns 1", + want: 1, + machineConfigPools: []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + Generation: 1, + }, + Status: mcv1.MachineConfigPoolStatus{ + MachineCount: 1, + UpdatedMachineCount: 1, + ReadyMachineCount: 1, + ObservedGeneration: 1, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{ + { + Name: mcpPrefix + "mcp_name" + mcpSuffix, + }, + }, + }, + }, + }, + }, + wantErr: "", + }, + { + name: "totalMachinesInTheMCPs returns 0 and no error when there are no machine config pools", + want: 0, + machineConfigPools: nil, + wantErr: "", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, err := TotalMachinesInTheMCPs(tc.machineConfigPools) + utilerror.AssertErrorMessage(t, err, tc.wantErr) + + if got != tc.want { + t.Fatalf("expected %v, but got %v", tc.want, got) + } + }) + } +} + +func TestMcpContainsARODNSConfig(t *testing.T) { + mcpPrefix := "99-" + mcpSuffix := "-aro-dns" + + type testCase struct { + name string + mcp mcv1.MachineConfigPool + want bool + } + + testcases := []testCase{ + { + name: "Mcp contains ARO DNS config", + mcp: mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + }, + Status: mcv1.MachineConfigPoolStatus{ + ObservedGeneration: 0, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{ + { + Name: mcpPrefix + "mcp_name" + mcpSuffix, + }, + }, + }, + }, + }, + want: true, + }, + { + name: "Mcp does not contain ARO DNS config due to wrong source name", + mcp: mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + }, + Status: mcv1.MachineConfigPoolStatus{ + ObservedGeneration: 0, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{ + { + Name: "non matching name", + }, + }, + }, + }, + }, + want: false, + }, + { + name: "Mcp does not contain ARO DNS config due to empty status", + mcp: mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + }, + }, + want: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + got := MCPContainsARODNSConfig(tc.mcp) + if got != tc.want { + t.Fatalf("expected %v, but got %v", tc.want, got) + } + }) + } +} + +type fakeMcpLister struct { + expected *mcv1.MachineConfigPoolList + expectedErr error +} + +func (m *fakeMcpLister) List(ctx context.Context, opts metav1.ListOptions) (*mcv1.MachineConfigPoolList, error) { + return m.expected, m.expectedErr +} + +type fakeNodeLister struct { + expected *corev1.NodeList + expectedErr error +} + +func (m *fakeNodeLister) List(ctx context.Context, opts metav1.ListOptions) (*corev1.NodeList, error) { + return m.expected, m.expectedErr +} + +func TestSameNumberOfNodesAndMachines(t *testing.T) { + t.Run("should return nil mcpLister error", func(t *testing.T) { + got, err := SameNumberOfNodesAndMachines(context.Background(), nil, nil) + wantErr := "mcpLister is nil" + + if got != false { + t.Fatalf("got %v, but wanted %v", got, false) + } + utilerror.AssertErrorMessage(t, err, wantErr) + }) + t.Run("should return nil nodeLister error", func(t *testing.T) { + mcpLister := &fakeMcpLister{} + got, err := SameNumberOfNodesAndMachines(context.Background(), mcpLister, nil) + wantErr := "nodeLister is nil" + + if got != false { + t.Fatalf("got %v, but wanted %v", got, false) + } + utilerror.AssertErrorMessage(t, err, wantErr) + }) + + t.Run("should propagate error from mcpLister.List()", func(t *testing.T) { + expectedErr := "some_error" + mcpLister := &fakeMcpLister{expectedErr: errors.New(expectedErr)} + nodeLister := &fakeNodeLister{} + + got, err := SameNumberOfNodesAndMachines(context.Background(), mcpLister, nodeLister) + if got != false { + t.Fatalf("got %v, but wanted %v", got, false) + } + utilerror.AssertErrorMessage(t, err, expectedErr) + }) + t.Run("should propagate error from TotalMachinesInTheMCPs()", func(t *testing.T) { + expectedErr := "ARO DNS config not found in MCP mcp_name" + badMachineConfigPools := []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{Name: "mcp_name"}, + Status: mcv1.MachineConfigPoolStatus{ + ObservedGeneration: 0, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "non matching name"}}, + }, + }, + }, + } + machineConfigPoolList := &mcv1.MachineConfigPoolList{Items: badMachineConfigPools} + mcpLister := &fakeMcpLister{expected: machineConfigPoolList} + nodeLister := &fakeNodeLister{} + + got, err := SameNumberOfNodesAndMachines(context.Background(), mcpLister, nodeLister) + if got != false { + t.Fatalf("got %v, but wanted %v", got, false) + } + utilerror.AssertErrorMessage(t, err, expectedErr) + }) + + t.Run("should propagate error from nodeLister", func(t *testing.T) { + expectedErr := "some_error" + + machineConfigPools := []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + Generation: 1, + }, + Status: mcv1.MachineConfigPoolStatus{ + MachineCount: 1, + UpdatedMachineCount: 1, + ReadyMachineCount: 1, + ObservedGeneration: 1, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "99-" + "mcp_name" + "-aro-dns"}}, + }, + }, + }, + } + machineConfigPoolList := &mcv1.MachineConfigPoolList{Items: machineConfigPools} + mcpLister := &fakeMcpLister{expected: machineConfigPoolList} + nodeLister := &fakeNodeLister{expectedErr: errors.New(expectedErr)} + + got, err := SameNumberOfNodesAndMachines(context.Background(), mcpLister, nodeLister) + if got != false { + t.Fatalf("got %v, but wanted %v", got, false) + } + utilerror.AssertErrorMessage(t, err, expectedErr) + }) + + t.Run("should return true and no error", func(t *testing.T) { + expectedErr := "" + machineConfigPools := []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + Generation: 1, + }, + Status: mcv1.MachineConfigPoolStatus{ + MachineCount: 1, + UpdatedMachineCount: 1, + ReadyMachineCount: 1, + ObservedGeneration: 1, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "99-" + "mcp_name" + "-aro-dns"}}, + }, + }, + }, + } + machineConfigPoolList := &mcv1.MachineConfigPoolList{Items: machineConfigPools} + mcpLister := &fakeMcpLister{expected: machineConfigPoolList} + + nodes := &corev1.NodeList{Items: []corev1.Node{{}}} + nodeLister := &fakeNodeLister{expected: nodes} + + got, err := SameNumberOfNodesAndMachines(context.Background(), mcpLister, nodeLister) + if got != true { + t.Fatalf("got %v, but wanted %v", got, true) + } + utilerror.AssertErrorMessage(t, err, expectedErr) + }) + + t.Run("should return false and error due to different number of total machines and nodes", func(t *testing.T) { + expectedErr := "cluster has 3 nodes but 1 under MCPs, not removing private DNS zone" + machineConfigPools := []mcv1.MachineConfigPool{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mcp_name", + Generation: 1, + }, + Status: mcv1.MachineConfigPoolStatus{ + MachineCount: 1, + UpdatedMachineCount: 1, + ReadyMachineCount: 1, + ObservedGeneration: 1, + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "99-" + "mcp_name" + "-aro-dns"}}, + }, + }, + }, + } + machineConfigPoolList := &mcv1.MachineConfigPoolList{Items: machineConfigPools} + mcpLister := &fakeMcpLister{expected: machineConfigPoolList} + + nodes := &corev1.NodeList{Items: []corev1.Node{{}, {}, {}}} + nodeLister := &fakeNodeLister{expected: nodes} + + got, err := SameNumberOfNodesAndMachines(context.Background(), mcpLister, nodeLister) + if got != false { + t.Fatalf("got %v, but wanted %v", got, false) + } + utilerror.AssertErrorMessage(t, err, expectedErr) + }) +} diff --git a/pkg/util/version/clusterversion.go b/pkg/util/version/clusterversion.go index 370f294c2f8..2316b1b333a 100644 --- a/pkg/util/version/clusterversion.go +++ b/pkg/util/version/clusterversion.go @@ -4,9 +4,12 @@ package version // Licensed under the Apache License 2.0. import ( + "context" "errors" configv1 "github.com/openshift/api/config/v1" + configclient "github.com/openshift/client-go/config/clientset/versioned" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // GetClusterVersion fetches the version of the openshift cluster. @@ -34,3 +37,17 @@ func GetClusterVersion(cv *configv1.ClusterVersion) (*Version, error) { return nil, unknownErr } + +func ClusterVersionIsLessThan4_4(ctx context.Context, configcli configclient.Interface) (bool, error) { + cv, err := configcli.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) + if err != nil { + return false, err + } + v, err := GetClusterVersion(cv) + if err != nil { + return false, err + } + + // 4.3 uses SRV records for etcd + return v.Lt(NewVersion(4, 4)), nil +}