From 097841b8a6da06c8a2b3afd4795c36f9a4314764 Mon Sep 17 00:00:00 2001 From: Aldo Fuster Turpin Date: Tue, 31 Jan 2023 14:13:43 +0100 Subject: [PATCH] refactor: remove log in parameters and rename functions --- pkg/cluster/delete.go | 2 +- pkg/cluster/removeprivatednszone.go | 166 +--------- pkg/cluster/removeprivatednszone_test.go | 253 +++++---------- pkg/util/mocks/net/net.go | 177 +++++++++++ pkg/util/net/generate.go | 8 + pkg/util/net/net.go | 138 ++++++--- pkg/util/net/net_test.go | 378 +++++++++++++++++++++++ pkg/util/ready/ready.go | 77 ++++- pkg/util/ready/ready_test.go | 339 ++++++++++++++++++++ pkg/util/version/clusterversion.go | 21 +- 10 files changed, 1167 insertions(+), 392 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 28681c65bde..a75ec235a13 100644 --- a/pkg/cluster/delete.go +++ b/pkg/cluster/delete.go @@ -196,7 +196,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 = utilnet.DeletePrivateDNSVirtualNetworkLinks(ctx, m.virtualNetworkLinks, *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 7e0320557ac..2f5aef8551d 100644 --- a/pkg/cluster/removeprivatednszone.go +++ b/pkg/cluster/removeprivatednszone.go @@ -5,173 +5,11 @@ package cluster import ( "context" - "strings" - - 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" - mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" - "github.com/sirupsen/logrus" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/util/retry" utilnet "github.com/Azure/ARO-RP/pkg/util/net" - "github.com/Azure/ARO-RP/pkg/util/ready" - "github.com/Azure/ARO-RP/pkg/util/stringutils" - "github.com/Azure/ARO-RP/pkg/util/version" ) 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 - if err := utilnet.UpdateDNSs(ctx, m.configcli.ConfigV1().DNSes(), m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID); err != nil { - m.log.Print(err) - } - return nil - } - - if !m.clusterHasSameNumberOfNodesAndMachineConfigPools(ctx) { - return nil - } - - if !version.ClusterVersionIsGreaterThan4_3(ctx, m.configcli, m.log) { - return nil - } - - if err = utilnet.UpdateDNSs(ctx, m.configcli.ConfigV1().DNSes(), m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID); err != nil { - m.log.Print(err) - return nil - } - - utilnet.RemoveZones(ctx, m.log, m.virtualNetworkLinks, m.privateZones, zones, resourceGroup) - return nil -} - -func (m *manager) clusterHasSameNumberOfNodesAndMachineConfigPools(ctx context.Context) bool { - machineConfigPoolList, err := m.mcocli.MachineconfigurationV1().MachineConfigPools().List(ctx, metav1.ListOptions{}) - if err != nil { - m.log.Print(err) - return false - } - - nMachineConfigPools, errorOcurred := validateMachineConfigPoolsAndGetCounter(machineConfigPoolList.Items, m.log) - if errorOcurred { - return false - } - - nodes, err := m.kubernetescli.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) - if err != nil { - m.log.Print(err) - return false - } - - nNodes := len(nodes.Items) - if nNodes != nMachineConfigPools { - m.log.Printf("cluster has %d nodes but %d under MCPs, not removing private DNS zone", nNodes, nMachineConfigPools) - return false - } - return true -} - -func validateMachineConfigPoolsAndGetCounter(machineConfigPools []mcv1.MachineConfigPool, logEntry *logrus.Entry) (nMachineConfigPools int, errOccurred bool) { - for _, mcp := range machineConfigPools { - if !utilnet.McpContainsARODNSConfig(mcp) { - logEntry.Printf("ARO DNS config not found in MCP %s", mcp.Name) - return 0, true - } - - if !ready.MachineConfigPoolIsReady(&mcp) { - logEntry.Printf("MCP %s not ready", mcp.Name) - return 0, true - } - - nMachineConfigPools += int(mcp.Status.MachineCount) - } - return nMachineConfigPools, false -} - -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 -} - -func (m *manager) updateDNSs(ctx context.Context) error { - fn := updateClusterDNSFn(ctx, m.configcli.ConfigV1().DNSes(), m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID) - return retry.RetryOnConflict(retry.DefaultRetry, fn) -} - -func updateClusterDNSFn(ctx context.Context, dnsInterface v1.DNSInterface, resourceGroupID string) func() error { - return func() error { - dns, err := dnsInterface.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 = dnsInterface.Update(ctx, dns, metav1.UpdateOptions{}) - return err - } -} - -func clusterVersionIsAtLeast4_4(ctx context.Context, configcli configclient.Interface, logEntry *logrus.Entry) (bool, error) { - cv, err := configcli.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) - if err != nil { - return false, err - } - v, err := version.GetClusterVersion(cv) - if err != nil { - logEntry.Print(err) - return false, nil - } - - if v.Lt(version.NewVersion(4, 4)) { - // 4.3 uses SRV records for etcd - logEntry.Printf("cluster version < 4.4, not removing private DNS zone") - return false, nil - } - return true, nil -} - -func (m *manager) removeZones(ctx context.Context, privateZones []mgmtprivatedns.PrivateZone, resourceGroup string) { - for _, privateZone := range privateZones { - if err := utilnet.DeletePrivateDNSVirtualNetworkLinks(ctx, m.virtualNetworkLinks, *privateZone.ID); err != nil { - m.log.Print(err) - return - } - - r, err := azure.ParseResourceID(*privateZone.ID) - if err != nil { - m.log.Print(err) - return - } - - if err = m.privateZones.DeleteAndWait(ctx, resourceGroup, r.ResourceName, ""); err != nil { - m.log.Print(err) - return - } - } + resourceGroupID := m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID + return utilnet.RemovePrivateDNSZone(ctx, m.log, m.privateZones, m.configcli, m.mcocli, m.kubernetescli, m.virtualNetworkLinks, resourceGroupID) } diff --git a/pkg/cluster/removeprivatednszone_test.go b/pkg/cluster/removeprivatednszone_test.go index cf452e534cb..c527649bb40 100644 --- a/pkg/cluster/removeprivatednszone_test.go +++ b/pkg/cluster/removeprivatednszone_test.go @@ -24,11 +24,14 @@ import ( "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" ) +const resourceGroupID = "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" +const id = "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.Network/privateDnsZones/zone1" + func TestRemovePrivateDNSZone(t *testing.T) { ctx := context.Background() - const resourceGroupID = "/subscriptions/0000000-0000-0000-0000-000000000000/resourceGroups/testGroup" for _, tt := range []struct { name string @@ -38,174 +41,66 @@ func TestRemovePrivateDNSZone(t *testing.T) { mcocli mcoclient.Interface configcli configclient.Interface wantDNSPrivateZoneRemoved bool + wantError string }{ { - name: "no private zones", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, + name: "returns nil manager.mcocli error", + wantError: "manager.mcocli is nil", + doc: simpleDoc(), mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { - privateZones.EXPECT(). - ListByResourceGroup(ctx, "testGroup", nil). - Return(nil, nil) + privateZones.EXPECT().ListByResourceGroup(ctx, gomock.Any(), nil).Return(privateZone(), 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, + mcocli: nil, + kubernetescli: nil, }, { - name: "has private zone, dnsmasq config not yet reconciled", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, + name: "returns nil manager.kubernetesclient error", + wantError: "manager.kubernetescli is nil", + doc: simpleDoc(), 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) + privateZones.EXPECT().ListByResourceGroup(ctx, gomock.Any(), nil).Return(privateZone(), nil) }, - mcocli: mcofake.NewSimpleClientset( - &mcv1.MachineConfigPool{}, - ), + kubernetescli: nil, + mcocli: mcofake.NewSimpleClientset(simpleMcp()), }, { - name: "has private zone, pool not yet ready", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, - }, + name: "no private zones", + doc: simpleDoc(), 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) + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(nil, 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, - }, - }, - ), + configcli: configfake.NewSimpleClientset(simpleDns()), + wantDNSPrivateZoneRemoved: true, }, { - name: "has private zone, node mismatch", - doc: &api.OpenShiftClusterDocument{ - OpenShiftCluster: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - ResourceGroupID: resourceGroupID, - }, - }, - }, + name: "has private zone, dnsmasq config not yet reconciled", //TODO + doc: simpleDoc(), + 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{}), + }, + { + name: "has private zone, pool not yet ready", + doc: simpleDoc(), 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) + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(privateZone(), 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", - }, - }, - }, - }, - }, - ), + mcocli: mcofake.NewSimpleClientset(simpleMcp()), }, { 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, - }, - }, - }, - }, + doc: simpleDoc(), 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) + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(privateZone(), nil) }, - kubernetescli: fake.NewSimpleClientset( - &corev1.Node{}, - ), + kubernetescli: fake.NewSimpleClientset(&corev1.Node{}), mcocli: mcofake.NewSimpleClientset( &mcv1.MachineConfigPool{ - ObjectMeta: metav1.ObjectMeta{ - Name: "master", - }, + ObjectMeta: metav1.ObjectMeta{Name: "master"}, Status: mcv1.MachineConfigPoolStatus{ Configuration: mcv1.MachineConfigPoolStatusConfiguration{ - Source: []corev1.ObjectReference{ - { - Name: "99-master-aro-dns", - }, - }, + Source: []corev1.ObjectReference{{Name: "99-master-aro-dns"}}, }, MachineCount: 1, UpdatedMachineCount: 1, @@ -231,23 +126,9 @@ func TestRemovePrivateDNSZone(t *testing.T) { }, { 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, - }, - }, - }, - }, + doc: simpleDoc(), 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) + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(privateZone(), nil) virtualNetworkLinks.EXPECT(). List(ctx, "testGroup", "zone1", nil). @@ -301,16 +182,7 @@ func TestRemovePrivateDNSZone(t *testing.T) { }, }, }, - &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", - }, - }, - }, + simpleDns(), ), wantDNSPrivateZoneRemoved: true, }, @@ -334,9 +206,7 @@ func TestRemovePrivateDNSZone(t *testing.T) { } err := m.removePrivateDNSZone(ctx) - if err != nil { - t.Fatal(err) - } + utilerror.AssertErrorMessage(t, err, tt.wantError) if tt.wantDNSPrivateZoneRemoved { dns, err := m.configcli.ConfigV1().DNSes().Get(ctx, "cluster", metav1.GetOptions{}) @@ -350,3 +220,42 @@ func TestRemovePrivateDNSZone(t *testing.T) { }) } } + +func privateZone() []mgmtprivatedns.PrivateZone { + return []mgmtprivatedns.PrivateZone{{ID: to.StringPtr(id)}} +} + +func simpleDoc() *api.OpenShiftClusterDocument { + return &api.OpenShiftClusterDocument{ + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ResourceGroupID: resourceGroupID}, + }, + }, + } +} + +func simpleDns() *configv1.DNS { + return &configv1.DNS{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Spec: configv1.DNSSpec{ + PrivateZone: &configv1.DNSZone{ID: id}, + }, + } +} + +func simpleMcp() *mcv1.MachineConfigPool { + return &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "master", + }, + Status: mcv1.MachineConfigPoolStatus{ + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "99-master-aro-dns"}}, + }, + MachineCount: 1, + }, + } +} diff --git a/pkg/util/mocks/net/net.go b/pkg/util/mocks/net/net.go new file mode 100644 index 00000000000..e0a407e08de --- /dev/null +++ b/pkg/util/mocks/net/net.go @@ -0,0 +1,177 @@ +// 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 "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 +} + +// Create mocks base method. +func (m *MockDNSIClient) Create(arg0 context.Context, arg1 *v1.DNS, arg2 v10.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 v10.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 v10.DeleteOptions, arg2 v10.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 v10.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 v10.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 v10.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 v10.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 v10.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 v10.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 fe521df6807..5530636fd9d 100644 --- a/pkg/util/net/net.go +++ b/pkg/util/net/net.go @@ -12,13 +12,18 @@ import ( 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" - mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/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 @@ -85,36 +90,83 @@ func setBuffers(rc syscall.RawConn, sz int) error { return err } -func managedDomainSuffixes() []string { - return []string{ - publicCloudManagedDomainSuffix, - govCloudManagedDomainSuffix, +func RemovePrivateDNSZone( + ctx context.Context, log *logrus.Entry, privateZones privatedns.PrivateZonesClient, + configcli configclient.Interface, mcocli mcoclient.Interface, + kubernetescli kubernetes.Interface, virtualNetworkLinks privatedns.VirtualNetworkLinksClient, + resourceGroupID string) error { + + resourceGroup := stringutils.LastTokenByte(resourceGroupID, '/') + zones, err := privateZones.ListByResourceGroup(ctx, resourceGroup, nil) + if err != nil { + log.Print(err) + return nil } -} -func IsManagedDomain(domain string) bool { - suffixes := managedDomainSuffixes() - for _, suffix := range suffixes { - if strings.HasSuffix(domain, suffix) { - return true + if len(zones) == 0 { + // fix up any clusters that we already upgraded + if err := UpdateDNSs(ctx, configcli.ConfigV1().DNSes(), resourceGroupID); err != nil { + log.Print(err) } + return nil + } + + if mcocli == nil { + return errors.New("manager.mcocli is nil") + } + + if kubernetescli == nil { + return errors.New("manager.kubernetescli is nil") + } + + mcpLister := mcocli.MachineconfigurationV1().MachineConfigPools() + nodeLister := kubernetescli.CoreV1().Nodes() + sameNumberOfNodesAndMachines, err := ready.SameNumberOfNodesAndMachines(ctx, mcpLister, nodeLister) + if err != nil { + log.Print(err) + return nil } - return false + + if !sameNumberOfNodesAndMachines { + return nil + } + + clusterVersionIsLessThan4_4, err := version.ClusterVersionIsLessThan4_4(ctx, configcli) + if err != nil { + log.Print(err) + return nil + } + + if clusterVersionIsLessThan4_4 { + log.Printf("cluster version < 4.4, not removing private DNS zone") + return nil + } + + if err = UpdateDNSs(ctx, configcli.ConfigV1().DNSes(), resourceGroupID); err != nil { + log.Print(err) + return nil + } + + if err := RemoveZones(ctx, virtualNetworkLinks, privateZones, zones, resourceGroup); err != nil { + log.Print(err) + return nil + } + return nil } -func DeletePrivateDNSVirtualNetworkLinks(ctx context.Context, virtualNetworkLinksClient privatedns.VirtualNetworkLinksClient, resourceID string) error { +func DeletePrivateDNSVNetLinks(ctx context.Context, vNetLinksClient privatedns.VirtualNetworkLinksClient, resourceID string) error { r, err := azure.ParseResourceID(resourceID) if err != nil { return err } - virtualNetworkLinks, err := virtualNetworkLinksClient.List(ctx, r.ResourceGroup, r.ResourceName, nil) + vNetLinks, err := vNetLinksClient.List(ctx, r.ResourceGroup, r.ResourceName, nil) if err != nil { return err } - for _, virtualNetworkLink := range virtualNetworkLinks { - err = virtualNetworkLinksClient.DeleteAndWait(ctx, r.ResourceGroup, r.ResourceName, *virtualNetworkLink.Name, "") + for _, vNetLink := range vNetLinks { + err = vNetLinksClient.DeleteAndWait(ctx, r.ResourceGroup, r.ResourceName, *vNetLink.Name, "") if err != nil { return err } @@ -123,34 +175,48 @@ func DeletePrivateDNSVirtualNetworkLinks(ctx context.Context, virtualNetworkLink return nil } -func RemoveZones(ctx context.Context, log *logrus.Entry, virtualNetworkLinksClient privatedns.VirtualNetworkLinksClient, privateZonesClient privatedns.PrivateZonesClient, privateZones []mgmtprivatedns.PrivateZone, resourceGroup string) { +func RemoveZones(ctx context.Context, + vNetLinksClient privatedns.VirtualNetworkLinksClient, + privateZoneClient privatedns.PrivateZonesClient, + privateZones []mgmtprivatedns.PrivateZone, + resourceGroup string) error { for _, privateZone := range privateZones { - if err := DeletePrivateDNSVirtualNetworkLinks(ctx, virtualNetworkLinksClient, *privateZone.ID); err != nil { - log.Print(err) - return + if err := DeletePrivateDNSVNetLinks(ctx, vNetLinksClient, *privateZone.ID); err != nil { + return err } r, err := azure.ParseResourceID(*privateZone.ID) if err != nil { - log.Print(err) - return + return err } - if err = privateZonesClient.DeleteAndWait(ctx, resourceGroup, r.ResourceName, ""); err != nil { - log.Print(err) - return + if privateZoneClient == nil { + return errors.New("privateZoneClient is nil") + } + + if err := privateZoneClient.DeleteAndWait(ctx, resourceGroup, r.ResourceName, ""); err != nil { + return err } } + return nil } -func UpdateDNSs(ctx context.Context, dnsInterface v1.DNSInterface, resourceGroupID string) error { - fn := updateClusterDNSFn(ctx, dnsInterface, resourceGroupID) +func UpdateDNSs(ctx context.Context, dnsI DNSIClient, resourceGroupID string) error { + fn := updateClusterDNSFn(ctx, dnsI, resourceGroupID) return retry.RetryOnConflict(retry.DefaultRetry, fn) } -func updateClusterDNSFn(ctx context.Context, dnsInterface v1.DNSInterface, resourceGroupID string) func() error { +type DNSIClient interface { + v1.DNSInterface +} + +func updateClusterDNSFn(ctx context.Context, dnsI DNSIClient, resourceGroupID string) func() error { return func() error { - dns, err := dnsInterface.Get(ctx, "cluster", metav1.GetOptions{}) + if dnsI == nil { + return errors.New("dnsI interface is nil") + } + + dns, err := dnsI.Get(ctx, "cluster", metav1.GetOptions{}) if err != nil { return err } @@ -164,19 +230,7 @@ func updateClusterDNSFn(ctx context.Context, dnsInterface v1.DNSInterface, resou dns.Spec.PrivateZone = nil - _, err = dnsInterface.Update(ctx, dns, metav1.UpdateOptions{}) + _, err = dnsI.Update(ctx, dns, metav1.UpdateOptions{}) return err } } - -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/net/net_test.go b/pkg/util/net/net_test.go new file mode 100644 index 00000000000..258d475af0a --- /dev/null +++ b/pkg/util/net/net_test.go @@ -0,0 +1,378 @@ +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, "dnsI 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) { + 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: "returns nil manager.mcocli error", + wantError: "manager.mcocli is nil", + doc: simpleDoc(), + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, gomock.Any(), nil).Return(privateZone(), nil) + }, + }, + { + name: "returns nil manager.kubernetesclient error", + wantError: "manager.kubernetescli is nil", + doc: simpleDoc(), + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, gomock.Any(), nil).Return(privateZone(), nil) + }, + kubernetescli: nil, + mcocli: mcofake.NewSimpleClientset(simpleMcp()), + }, + { + name: "no private zones", + doc: simpleDoc(), + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(nil, nil) + }, + configcli: configfake.NewSimpleClientset(simpleDns()), + wantDNSPrivateZoneRemoved: true, + }, + { + name: "has private zone, dnsmasq config not yet reconciled", + doc: simpleDoc(), + 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{}), + kubernetescli: fake.NewSimpleClientset( + &corev1.Node{}, + ), + }, + { + name: "has private zone, pool not yet ready", + doc: simpleDoc(), + mocks: func(privateZones *mock_privatedns.MockPrivateZonesClient, virtualNetworkLinks *mock_privatedns.MockVirtualNetworkLinksClient) { + privateZones.EXPECT().ListByResourceGroup(ctx, "testGroup", nil).Return(privateZone(), nil) + }, + mcocli: mcofake.NewSimpleClientset(simpleMcp()), + kubernetescli: fake.NewSimpleClientset( + &corev1.Node{}, + ), + }, + { + name: "has private zone, nodes match, 4.4, dnsmasq rolled out", + doc: simpleDoc(), + 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", + }, + }, + }, + }, + simpleDns(), + ), + 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) + + err := RemovePrivateDNSZone(ctx, utillog.GetLogger(), privateZones, tt.configcli, tt.mcocli, tt.kubernetescli, virtualNetworkLinks, resourceGroupID) + + 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) + } + } + }) + } +} + +func privateZone() []mgmtprivatedns.PrivateZone { + return []mgmtprivatedns.PrivateZone{{ID: to.StringPtr(id)}} +} + +func simpleDoc() *api.OpenShiftClusterDocument { + return &api.OpenShiftClusterDocument{ + OpenShiftCluster: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ResourceGroupID: resourceGroupID}, + }, + }, + } +} + +func simpleDns() *configv1.DNS { + return &configv1.DNS{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Spec: configv1.DNSSpec{ + PrivateZone: &configv1.DNSZone{ID: id}, + }, + } +} + +func simpleMcp() *mcv1.MachineConfigPool { + return &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "master", + }, + Status: mcv1.MachineConfigPoolStatus{ + Configuration: mcv1.MachineConfigPoolStatusConfiguration{ + Source: []corev1.ObjectReference{{Name: "99-master-aro-dns"}}, + }, + MachineCount: 1, + }, + } +} diff --git a/pkg/util/ready/ready.go b/pkg/util/ready/ready.go index cb1540b3b63..be5a2ec8f46 100644 --- a/pkg/util/ready/ready.go +++ b/pkg/util/ready/ready.go @@ -5,9 +5,10 @@ package ready import ( "context" + "fmt" "net" - mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + v1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" mcoclientv1 "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/typed/machineconfiguration.openshift.io/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -172,7 +173,7 @@ func StatefulSetIsReady(s *appsv1.StatefulSet) bool { // MachineConfigPoolIsReady returns true if a MachineConfigPool is considered // ready -func MachineConfigPoolIsReady(s *mcv1.MachineConfigPool) bool { +func MachineConfigPoolIsReady(s *v1.MachineConfigPool) bool { return s.Status.MachineCount == s.Status.UpdatedMachineCount && s.Status.MachineCount == s.Status.ReadyMachineCount && s.Generation == s.Status.ObservedGeneration @@ -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) (*v1.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 []v1.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 v1.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..e921b656fc3 100644 --- a/pkg/util/ready/ready_test.go +++ b/pkg/util/ready/ready_test.go @@ -9,6 +9,7 @@ import ( "testing" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + v1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" mcofake "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/fake" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -16,6 +17,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 +653,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 *v1.MachineConfigPoolList + expectedErr error +} + +func (m *fakeMcpLister) List(ctx context.Context, opts metav1.ListOptions) (*v1.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 7564fa44ba7..62d026f4281 100644 --- a/pkg/util/version/clusterversion.go +++ b/pkg/util/version/clusterversion.go @@ -9,7 +9,7 @@ import ( configv1 "github.com/openshift/api/config/v1" configclient "github.com/openshift/client-go/config/clientset/versioned" - "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func GetClusterVersion(cv *configv1.ClusterVersion) (*Version, error) { @@ -22,17 +22,16 @@ func GetClusterVersion(cv *configv1.ClusterVersion) (*Version, error) { return nil, errors.New("unknown cluster version") } -func ClusterVersionIsGreaterThan4_3(ctx context.Context, configcli configclient.Interface, logEntry *logrus.Entry) bool { - v, err := GetClusterVersion(ctx, configcli) +func ClusterVersionIsLessThan4_4(ctx context.Context, configcli configclient.Interface) (bool, error) { + cv, err := configcli.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) if err != nil { - logEntry.Print(err) - return false + return false, err } - - if v.Lt(NewVersion(4, 4)) { - // 4.3 uses SRV records for etcd - logEntry.Printf("cluster version < 4.4, not removing private DNS zone") - return false + v, err := GetClusterVersion(cv) + if err != nil { + return false, err } - return true + + // 4.3 uses SRV records for etcd + return v.Lt(NewVersion(4, 4)), nil }