diff --git a/pkg/operator/controllers/base/aro_controller.go b/pkg/operator/controllers/base/aro_controller.go index ddb9642752a..47a8b03f80c 100644 --- a/pkg/operator/controllers/base/aro_controller.go +++ b/pkg/operator/controllers/base/aro_controller.go @@ -5,6 +5,7 @@ package base import ( "context" + "fmt" configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" @@ -111,7 +112,8 @@ func (c *AROController) IsClusterUpgrading(ctx context.Context) (bool, error) { cv := &configv1.ClusterVersion{} err := c.Client.Get(ctx, types.NamespacedName{Name: "version"}, cv) if err != nil { - c.Log.Errorf("error getting the ClusterVersion: %v", err) + err = fmt.Errorf("error getting the ClusterVersion: %w", err) + c.Log.Error(err) return false, err } diff --git a/pkg/operator/controllers/dnsmasq/cluster_controller.go b/pkg/operator/controllers/dnsmasq/cluster_controller.go index 7fb9ecaacbf..1e1c4df4dd5 100644 --- a/pkg/operator/controllers/dnsmasq/cluster_controller.go +++ b/pkg/operator/controllers/dnsmasq/cluster_controller.go @@ -20,6 +20,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" "github.com/Azure/ARO-RP/pkg/operator/controllers/base" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" "github.com/Azure/ARO-RP/pkg/util/dynamichelper" ) @@ -29,17 +30,17 @@ const ( type ClusterReconciler struct { base.AROController - dh dynamichelper.Interface + ch clienthelper.Interface } -func NewClusterReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *ClusterReconciler { +func NewClusterReconciler(log *logrus.Entry, client client.Client, ch clienthelper.Interface) *ClusterReconciler { return &ClusterReconciler{ AROController: base.AROController{ Log: log, Client: client, Name: ClusterControllerName, }, - dh: dh, + ch: ch, } } @@ -68,11 +69,6 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request) return reconcile.Result{}, err } - if !allowReconcile { - r.Log.Debugf("skipping reconciliation of %s", r.Name) - return reconcile.Result{}, nil - } - r.Log.Debug("running") mcps := &mcv1.MachineConfigPoolList{} err = r.Client.List(ctx, mcps) @@ -82,7 +78,7 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request) return reconcile.Result{}, err } - err = reconcileMachineConfigs(ctx, instance, r.dh, r.Client, allowReconcile, restartDnsmasq, mcps.Items...) + err = reconcileMachineConfigs(ctx, instance, r.ch, r.Client, allowReconcile, restartDnsmasq, mcps.Items...) if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) @@ -105,7 +101,7 @@ func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -func reconcileMachineConfigs(ctx context.Context, instance *arov1alpha1.Cluster, dh dynamichelper.Interface, c client.Client, allowReconcile bool, restartDnsmasq bool, mcps ...mcv1.MachineConfigPool) error { +func reconcileMachineConfigs(ctx context.Context, instance *arov1alpha1.Cluster, ch clienthelper.Interface, c client.Client, allowReconcile bool, restartDnsmasq bool, mcps ...mcv1.MachineConfigPool) error { var resources []kruntime.Object for _, mcp := range mcps { resource, err := dnsmasqMachineConfig(instance.Spec.Domain, instance.Spec.APIIntIP, instance.Spec.IngressIP, mcp.Name, instance.Spec.GatewayDomains, instance.Spec.GatewayPrivateEndpointIP, restartDnsmasq) @@ -130,7 +126,7 @@ func reconcileMachineConfigs(ctx context.Context, instance *arov1alpha1.Cluster, // create or update. If we are not allowed to reconcile, we do not want to // perform any updates, but we do want to perform initial configuration. if allowReconcile { - return dh.Ensure(ctx, resources...) + return ch.Ensure(ctx, resources...) } else { for _, i := range resources { err := c.Create(ctx, i.(client.Object)) diff --git a/pkg/operator/controllers/dnsmasq/cluster_controller_test.go b/pkg/operator/controllers/dnsmasq/cluster_controller_test.go index ecceb4f5f87..01f51edfbc2 100644 --- a/pkg/operator/controllers/dnsmasq/cluster_controller_test.go +++ b/pkg/operator/controllers/dnsmasq/cluster_controller_test.go @@ -8,7 +8,6 @@ import ( "testing" "time" - "github.com/golang/mock/gomock" configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" @@ -20,7 +19,8 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" - mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" + testclienthelper "github.com/Azure/ARO-RP/test/util/clienthelper" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -32,24 +32,22 @@ func TestClusterReconciler(t *testing.T) { defaultDegraded := utilconditions.ControllerDefaultDegraded(ClusterControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} - fakeDh := func(controller *gomock.Controller) *mock_dynamichelper.MockInterface { - return mock_dynamichelper.NewMockInterface(controller) - } - tests := []struct { name string objects []client.Object - mocks func(mdh *mock_dynamichelper.MockInterface) + wantCreated map[string]int + wantUpdated map[string]int request ctrl.Request wantErrMsg string wantConditions []operatorv1.OperatorCondition }{ { - name: "no cluster", - objects: []client.Object{}, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, - request: ctrl.Request{}, - wantErrMsg: "clusters.aro.openshift.io \"cluster\" not found", + name: "no cluster", + objects: []client.Object{}, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "clusters.aro.openshift.io \"cluster\" not found", }, { name: "controller disabled", @@ -67,7 +65,8 @@ func TestClusterReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, @@ -96,9 +95,8 @@ func TestClusterReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any()).Times(1) - }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, @@ -124,9 +122,10 @@ func TestClusterReconciler(t *testing.T) { Spec: mcv1.MachineConfigPoolSpec{}, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any(), gomock.AssignableToTypeOf(&mcv1.MachineConfig{})).Times(1) + wantCreated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, }, + wantUpdated: map[string]int{}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, @@ -151,20 +150,105 @@ func TestClusterReconciler(t *testing.T) { Spec: mcv1.MachineConfigPoolSpec{}, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, - request: ctrl.Request{}, - wantErrMsg: `clusterversions.config.openshift.io "version" not found`, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, wantConditions: []operatorv1.OperatorCondition{ defaultAvailable, defaultProgressing, { Type: "DnsmasqClusterControllerDegraded", Status: "True", - Message: `clusterversions.config.openshift.io "version" not found`, + Message: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, LastTransitionTime: transitionTime, }, }, }, { - name: "valid MachineConfigPool, cluster not updating, not forced, does nothing", + name: "valid MachineConfigPool, cluster not updating, not forced, does create", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.10.11", + }, + }, + }, + }, + }, + wantCreated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, + }, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool, cluster not updating, not forced, does not update", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-master-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.10.11", + }, + }, + }, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool, while cluster updating, updates existing ARO DNS MachineConfig", objects: []client.Object{ &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, @@ -182,10 +266,19 @@ func TestClusterReconciler(t *testing.T) { Status: mcv1.MachineConfigPoolStatus{}, Spec: mcv1.MachineConfigPoolSpec{}, }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-master-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, &configv1.ClusterVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "version", }, + Spec: configv1.ClusterVersionSpec{ + DesiredUpdate: &configv1.Update{ + Version: "4.10.12", + }, + }, Status: configv1.ClusterVersionStatus{ History: []configv1.UpdateHistory{ { @@ -196,8 +289,9 @@ func TestClusterReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Times(0) + wantCreated: map[string]int{}, + wantUpdated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, }, request: ctrl.Request{}, wantErrMsg: "", @@ -241,10 +335,10 @@ func TestClusterReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any(), gomock.AssignableToTypeOf(&mcv1.MachineConfig{})).Times(1) + wantCreated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, }, - + wantUpdated: map[string]int{}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, @@ -253,26 +347,42 @@ func TestClusterReconciler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := gomock.NewController(t) - defer controller.Finish() + createTally := make(map[string]int) + updateTally := make(map[string]int) - client := ctrlfake.NewClientBuilder(). + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). WithObjects(tt.objects...). - Build() + Build()).WithCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) - dh := fakeDh(controller) - tt.mocks(dh) + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) r := NewClusterReconciler( - logrus.NewEntry(logrus.StandardLogger()), + log, client, - dh, + ch, ) ctx := context.Background() _, err := r.Reconcile(ctx, tt.request) utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(tt.wantCreated, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(tt.wantUpdated, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } }) } } diff --git a/pkg/operator/controllers/dnsmasq/machineconfig_controller.go b/pkg/operator/controllers/dnsmasq/machineconfig_controller.go index 50af4bc767c..b045bbeede9 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfig_controller.go +++ b/pkg/operator/controllers/dnsmasq/machineconfig_controller.go @@ -17,7 +17,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" "github.com/Azure/ARO-RP/pkg/operator/controllers/base" - "github.com/Azure/ARO-RP/pkg/util/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" ) const ( @@ -27,19 +27,19 @@ const ( type MachineConfigReconciler struct { base.AROController - dh dynamichelper.Interface + ch clienthelper.Interface } var rxARODNS = regexp.MustCompile("^99-(.*)-aro-dns$") -func NewMachineConfigReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineConfigReconciler { +func NewMachineConfigReconciler(log *logrus.Entry, client client.Client, ch clienthelper.Interface) *MachineConfigReconciler { return &MachineConfigReconciler{ AROController: base.AROController{ Log: log, Client: client, Name: MachineConfigControllerName, }, - dh: dh, + ch: ch, } } @@ -89,7 +89,7 @@ func (r *MachineConfigReconciler) Reconcile(ctx context.Context, request ctrl.Re return reconcile.Result{}, nil } - err = reconcileMachineConfigs(ctx, instance, r.dh, r.Client, allowReconcile, instance.Spec.OperatorFlags.GetSimpleBoolean(operator.RestartDnsmasqEnabled), *mcp) + err = reconcileMachineConfigs(ctx, instance, r.ch, r.Client, allowReconcile, instance.Spec.OperatorFlags.GetSimpleBoolean(operator.RestartDnsmasqEnabled), *mcp) if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) diff --git a/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go b/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go index 318da59a958..5c7960448cb 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go +++ b/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go @@ -20,7 +20,9 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" + testclienthelper "github.com/Azure/ARO-RP/test/util/clienthelper" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -31,9 +33,6 @@ func TestMachineConfigReconciler(t *testing.T) { defaultProgressing := utilconditions.ControllerDefaultProgressing(MachineConfigControllerName) defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} - fakeDh := func(controller *gomock.Controller) *mock_dynamichelper.MockInterface { - return mock_dynamichelper.NewMockInterface(controller) - } tests := []struct { name string @@ -139,19 +138,22 @@ func TestMachineConfigReconciler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := gomock.NewController(t) - defer controller.Finish() + createTally := make(map[string]int) + updateTally := make(map[string]int) - client := ctrlfake.NewClientBuilder(). + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). WithObjects(tt.objects...). - Build() - dh := fakeDh(controller) - tt.mocks(dh) + Build()) + + client.WithCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) r := NewMachineConfigReconciler( logrus.NewEntry(logrus.StandardLogger()), client, - dh, + ch, ) ctx := context.Background() _, err := r.Reconcile(ctx, tt.request) diff --git a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go index 457d98da1db..c448481960b 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go +++ b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go @@ -16,7 +16,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" "github.com/Azure/ARO-RP/pkg/operator/controllers/base" - "github.com/Azure/ARO-RP/pkg/util/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" ) const ( @@ -26,17 +26,17 @@ const ( type MachineConfigPoolReconciler struct { base.AROController - dh dynamichelper.Interface + ch clienthelper.Interface } -func NewMachineConfigPoolReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineConfigPoolReconciler { +func NewMachineConfigPoolReconciler(log *logrus.Entry, client client.Client, ch clienthelper.Interface) *MachineConfigPoolReconciler { return &MachineConfigPoolReconciler{ AROController: base.AROController{ Log: log, Client: client, Name: MachineConfigPoolControllerName, }, - dh: dh, + ch: ch, } } @@ -78,7 +78,7 @@ func (r *MachineConfigPoolReconciler) Reconcile(ctx context.Context, request ctr return reconcile.Result{}, err } - err = reconcileMachineConfigs(ctx, instance, r.dh, r.Client, allowReconcile, restartDnsmasq, *mcp) + err = reconcileMachineConfigs(ctx, instance, r.ch, r.Client, allowReconcile, restartDnsmasq, *mcp) if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) diff --git a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go index 4eb50a02c1b..11aba0f33eb 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go +++ b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go @@ -20,7 +20,9 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" + testclienthelper "github.com/Azure/ARO-RP/test/util/clienthelper" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -32,10 +34,6 @@ func TestMachineConfigPoolReconciler(t *testing.T) { defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigPoolControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} - fakeDh := func(controller *gomock.Controller) *mock_dynamichelper.MockInterface { - return mock_dynamichelper.NewMockInterface(controller) - } - tests := []struct { name string objects []client.Object @@ -145,16 +143,22 @@ func TestMachineConfigPoolReconciler(t *testing.T) { controller := gomock.NewController(t) defer controller.Finish() - client := ctrlfake.NewClientBuilder(). + createTally := make(map[string]int) + updateTally := make(map[string]int) + + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). WithObjects(tt.objects...). - Build() - dh := fakeDh(controller) - tt.mocks(dh) + Build()) + + client.WithCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) r := NewMachineConfigPoolReconciler( - logrus.NewEntry(logrus.StandardLogger()), + log, client, - dh, + ch, ) ctx := context.Background() _, err := r.Reconcile(ctx, tt.request)