Repository: Azure/ARO-RP
Pull request #2700 (2023-02-09 10:40:18)

Removes dependency on a versioned MCO client
### What this PR does / why we need it:

Operator is not longer dependant on a versioned MCO client and is now using a split client which uses cache for read operations.

### Test plan for issue:

Existing tests should cover it

### Is there any documentation that needs to be updated for this PR?

No, just refactoring.



Patch:
diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go
index bf449bad9..9cdd51b4e 100644
--- a/cmd/aro/operator.go
+++ b/cmd/aro/operator.go
@@ -8,7 +8,6 @@ import (
 	"flag"
 	"fmt"
 
-	mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
 	"github.com/sirupsen/logrus"
 	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -80,10 +79,6 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 	if err != nil {
 		return err
 	}
-	mcocli, err := mcoclient.NewForConfig(restConfig)
-	if err != nil {
-		return err
-	}
 	// TODO (NE): dh is sometimes passed, sometimes created later. Can we standardize?
 	dh, err := dynamichelper.New(log, restConfig)
 	if err != nil {
@@ -113,7 +108,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 		}
 		if err = (workaround.NewReconciler(
 			log.WithField("controller", workaround.ControllerName),
-			client, kubernetescli, mcocli, restConfig)).SetupWithManager(mgr); err != nil {
+			client, kubernetescli, dh)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", workaround.ControllerName, err)
 		}
 		if err = (routefix.NewReconciler(
@@ -133,17 +128,17 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 		}
 		if err = (dnsmasq.NewClusterReconciler(
 			log.WithField("controller", dnsmasq.ClusterControllerName),
-			client, mcocli, dh)).SetupWithManager(mgr); err != nil {
+			client, dh)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", dnsmasq.ClusterControllerName, err)
 		}
 		if err = (dnsmasq.NewMachineConfigReconciler(
 			log.WithField("controller", dnsmasq.MachineConfigControllerName),
-			client, mcocli, dh)).SetupWithManager(mgr); err != nil {
+			client, dh)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", dnsmasq.MachineConfigControllerName, err)
 		}
 		if err = (dnsmasq.NewMachineConfigPoolReconciler(
 			log.WithField("controller", dnsmasq.MachineConfigPoolControllerName),
-			client, mcocli, dh)).SetupWithManager(mgr); err != nil {
+			client, dh)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", dnsmasq.MachineConfigPoolControllerName, err)
 		}
 		if err = (node.NewReconciler(
diff --git a/pkg/operator/controllers/autosizednodes/autosizednodes_controller.go b/pkg/operator/controllers/autosizednodes/autosizednodes_controller.go
index d84550f1d..019e5b85c 100644
--- a/pkg/operator/controllers/autosizednodes/autosizednodes_controller.go
+++ b/pkg/operator/controllers/autosizednodes/autosizednodes_controller.go
@@ -93,7 +93,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 
 	// If already exists, update the spec
 	config.Spec = defaultConfig.Spec
-	err = r.client.Update(ctx, &config, &client.UpdateOptions{})
+	err = r.client.Update(ctx, &config)
 	if err != nil {
 		err = fmt.Errorf("could not update KubeletConfig: %w", err)
 	}
diff --git a/pkg/operator/controllers/dnsmasq/cluster_controller.go b/pkg/operator/controllers/dnsmasq/cluster_controller.go
index a6a782931..9abfc8bbf 100644
--- a/pkg/operator/controllers/dnsmasq/cluster_controller.go
+++ b/pkg/operator/controllers/dnsmasq/cluster_controller.go
@@ -7,9 +7,8 @@ import (
 	"context"
 
 	"github.com/openshift/installer/pkg/aro/dnsmasq"
-	mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
+	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"
 	kruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -31,16 +30,14 @@ const (
 type ClusterReconciler struct {
 	log *logrus.Entry
 
-	mcocli mcoclient.Interface
-	dh     dynamichelper.Interface
+	dh dynamichelper.Interface
 
 	client client.Client
 }
 
-func NewClusterReconciler(log *logrus.Entry, client client.Client, mcocli mcoclient.Interface, dh dynamichelper.Interface) *ClusterReconciler {
+func NewClusterReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *ClusterReconciler {
 	return &ClusterReconciler{
 		log:    log,
-		mcocli: mcocli,
 		dh:     dh,
 		client: client,
 	}
@@ -61,7 +58,8 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request)
 	}
 
 	r.log.Debug("running")
-	mcps, err := r.mcocli.MachineconfigurationV1().MachineConfigPools().List(ctx, metav1.ListOptions{})
+	mcps := &mcv1.MachineConfigPoolList{}
+	err = r.client.List(ctx, mcps)
 	if err != nil {
 		r.log.Error(err)
 		return reconcile.Result{}, err
diff --git a/pkg/operator/controllers/dnsmasq/machineconfig_controller.go b/pkg/operator/controllers/dnsmasq/machineconfig_controller.go
index da3673145..70d68d9c2 100644
--- a/pkg/operator/controllers/dnsmasq/machineconfig_controller.go
+++ b/pkg/operator/controllers/dnsmasq/machineconfig_controller.go
@@ -8,10 +8,8 @@ import (
 	"regexp"
 
 	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"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -28,18 +26,16 @@ const (
 type MachineConfigReconciler struct {
 	log *logrus.Entry
 
-	mcocli mcoclient.Interface
-	dh     dynamichelper.Interface
+	dh dynamichelper.Interface
 
 	client client.Client
 }
 
 var rxARODNS = regexp.MustCompile("^99-(.*)-aro-dns$")
 
-func NewMachineConfigReconciler(log *logrus.Entry, client client.Client, mcocli mcoclient.Interface, dh dynamichelper.Interface) *MachineConfigReconciler {
+func NewMachineConfigReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineConfigReconciler {
 	return &MachineConfigReconciler{
 		log:    log,
-		mcocli: mcocli,
 		dh:     dh,
 		client: client,
 	}
@@ -66,7 +62,8 @@ func (r *MachineConfigReconciler) Reconcile(ctx context.Context, request ctrl.Re
 	}
 	role := m[1]
 
-	_, err = r.mcocli.MachineconfigurationV1().MachineConfigPools().Get(ctx, role, metav1.GetOptions{})
+	mcp := &mcv1.MachineConfigPool{}
+	err = r.client.Get(ctx, types.NamespacedName{Name: role}, mcp)
 	if kerrors.IsNotFound(err) {
 		return reconcile.Result{}, nil
 	}
diff --git a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go
index 0d5251554..05b515790 100644
--- a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go
+++ b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go
@@ -7,10 +7,8 @@ import (
 	"context"
 
 	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"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -27,16 +25,14 @@ const (
 type MachineConfigPoolReconciler struct {
 	log *logrus.Entry
 
-	mcocli mcoclient.Interface
-	dh     dynamichelper.Interface
+	dh dynamichelper.Interface
 
 	client client.Client
 }
 
-func NewMachineConfigPoolReconciler(log *logrus.Entry, client client.Client, mcocli mcoclient.Interface, dh dynamichelper.Interface) *MachineConfigPoolReconciler {
+func NewMachineConfigPoolReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineConfigPoolReconciler {
 	return &MachineConfigPoolReconciler{
 		log:    log,
-		mcocli: mcocli,
 		dh:     dh,
 		client: client,
 	}
@@ -57,7 +53,8 @@ func (r *MachineConfigPoolReconciler) Reconcile(ctx context.Context, request ctr
 	}
 
 	r.log.Debug("running")
-	_, err = r.mcocli.MachineconfigurationV1().MachineConfigPools().Get(ctx, request.Name, metav1.GetOptions{})
+	mcp := &mcv1.MachineConfigPool{}
+	err = r.client.Get(ctx, types.NamespacedName{Name: request.Name}, mcp)
 	if kerrors.IsNotFound(err) {
 		return reconcile.Result{}, nil
 	}
diff --git a/pkg/operator/controllers/workaround/systemreserved.go b/pkg/operator/controllers/workaround/systemreserved.go
index ce83e23d0..35adc06cd 100644
--- a/pkg/operator/controllers/workaround/systemreserved.go
+++ b/pkg/operator/controllers/workaround/systemreserved.go
@@ -8,11 +8,12 @@ import (
 	"encoding/json"
 
 	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"
 	kruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/autosizednodes"
@@ -23,7 +24,7 @@ import (
 type systemreserved struct {
 	log *logrus.Entry
 
-	mcocli mcoclient.Interface
+	client client.Client
 	dh     dynamichelper.Interface
 
 	versionFixed *version.Version
@@ -31,13 +32,13 @@ type systemreserved struct {
 
 var _ Workaround = &systemreserved{}
 
-func NewSystemReserved(log *logrus.Entry, mcocli mcoclient.Interface, dh dynamichelper.Interface) *systemreserved {
+func NewSystemReserved(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *systemreserved {
 	verFixed, err := version.ParseVersion("4.99.0") // TODO set this correctly when known.
 	utilruntime.Must(err)
 
 	return &systemreserved{
 		log:          log,
-		mcocli:       mcocli,
+		client:       client,
 		dh:           dh,
 		versionFixed: verFixed,
 	}
@@ -91,7 +92,8 @@ func (sr *systemreserved) Ensure(ctx context.Context) error {
 
 	// Step 1. Add label to worker MachineConfigPool.
 	// Get the worker MachineConfigPool, modify it to add a label aro.openshift.io/limits: "", and apply the modified config.
-	mcp, err := sr.mcocli.MachineconfigurationV1().MachineConfigPools().Get(ctx, workerMachineConfigPoolName, metav1.GetOptions{})
+	mcp := &mcv1.MachineConfigPool{}
+	err := sr.client.Get(ctx, types.NamespacedName{Name: workerMachineConfigPoolName}, mcp)
 	if err != nil {
 		return err
 	}
@@ -102,7 +104,7 @@ func (sr *systemreserved) Ensure(ctx context.Context) error {
 		}
 		mcp.Labels[labelName] = labelValue
 
-		_, err = sr.mcocli.MachineconfigurationV1().MachineConfigPools().Update(ctx, mcp, metav1.UpdateOptions{})
+		err = sr.client.Update(ctx, mcp)
 		if err != nil {
 			return err
 		}
@@ -119,8 +121,8 @@ func (sr *systemreserved) Ensure(ctx context.Context) error {
 
 func (sr *systemreserved) Remove(ctx context.Context) error {
 	sr.log.Debug("remove systemreserved")
-
-	mcp, err := sr.mcocli.MachineconfigurationV1().MachineConfigPools().Get(ctx, workerMachineConfigPoolName, metav1.GetOptions{})
+	mcp := &mcv1.MachineConfigPool{}
+	err := sr.client.Get(ctx, types.NamespacedName{Name: workerMachineConfigPoolName}, mcp)
 	if err != nil {
 		return err
 	}
@@ -130,7 +132,7 @@ func (sr *systemreserved) Remove(ctx context.Context) error {
 	}
 	delete(mcp.Labels, labelName)
 
-	_, err = sr.mcocli.MachineconfigurationV1().MachineConfigPools().Update(ctx, mcp, metav1.UpdateOptions{})
+	err = sr.client.Update(ctx, mcp)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/operator/controllers/workaround/workaround_controller.go b/pkg/operator/controllers/workaround/workaround_controller.go
index 04afbbf38..da91d95ca 100644
--- a/pkg/operator/controllers/workaround/workaround_controller.go
+++ b/pkg/operator/controllers/workaround/workaround_controller.go
@@ -8,11 +8,9 @@ import (
 	"time"
 
 	configv1 "github.com/openshift/api/config/v1"
-	mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -38,15 +36,11 @@ type Reconciler struct {
 	client client.Client
 }
 
-func NewReconciler(log *logrus.Entry, client client.Client, kubernetescli kubernetes.Interface, mcocli mcoclient.Interface, restConfig *rest.Config) *Reconciler {
-	dh, err := dynamichelper.New(log, restConfig)
-	if err != nil {
-		panic(err)
-	}
-
+// TODO: use client.Client instead of dynamichelper here.
+func NewReconciler(log *logrus.Entry, client client.Client, kubernetescli kubernetes.Interface, dh dynamichelper.Interface) *Reconciler {
 	return &Reconciler{
 		log:         log,
-		workarounds: []Workaround{NewSystemReserved(log, mcocli, dh), NewIfReload(log, kubernetescli)},
+		workarounds: []Workaround{NewSystemReserved(log, client, dh), NewIfReload(log, kubernetescli)},
 		client:      client,
 	}
 }
diff --git a/pkg/util/version/const.go b/pkg/util/version/const.go
index 1da6d0822..5e1d2310c 100644
--- a/pkg/util/version/const.go
+++ b/pkg/util/version/const.go
@@ -64,7 +64,7 @@ var (
 
 // FluentbitImage contains the location of the Fluentbit container image
 func FluentbitImage(acrDomain string) string {
-	return acrDomain + "/fluentbit:1.7.8-1"
+	return acrDomain + "/fluentbit:1.9.9-1"
 }
 
 // MdmImage contains the location of the MDM container image


Test patch:
diff --git a/pkg/operator/controllers/workaround/systemreserved_test.go b/pkg/operator/controllers/workaround/systemreserved_test.go
index a6c3431e2..657277a36 100644
--- a/pkg/operator/controllers/workaround/systemreserved_test.go
+++ b/pkg/operator/controllers/workaround/systemreserved_test.go
@@ -10,10 +10,10 @@ import (
 
 	"github.com/golang/mock/gomock"
 	mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
-	mcofake "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kruntime "k8s.io/apimachinery/pkg/runtime"
-	ktesting "k8s.io/client-go/testing"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/Azure/ARO-RP/pkg/util/cmp"
 	utillog "github.com/Azure/ARO-RP/pkg/util/log"
@@ -22,61 +22,58 @@ import (
 
 func TestSystemreservedEnsure(t *testing.T) {
 	tests := []struct {
-		name                         string
-		mcocli                       *mcofake.Clientset
-		mocker                       func(mdh *mock_dynamichelper.MockInterface)
-		machineConfigPoolNeedsUpdate bool
-		wantErr                      bool
+		name string
+		mcp  *mcv1.MachineConfigPool
 	}{
 		{
 			name: "first time create",
-			mcocli: mcofake.NewSimpleClientset(&mcv1.MachineConfigPool{
+			mcp: &mcv1.MachineConfigPool{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "worker",
 				},
-			}),
-			machineConfigPoolNeedsUpdate: true,
-			mocker: func(mdh *mock_dynamichelper.MockInterface) {
-				mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil)
 			},
 		},
 		{
-			name: "nothing to be done",
-			mcocli: mcofake.NewSimpleClientset(&mcv1.MachineConfigPool{
+			name: "label already exists",
+			mcp: &mcv1.MachineConfigPool{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:   "worker",
 					Labels: map[string]string{labelName: labelValue},
 				},
-			}),
-			mocker: func(mdh *mock_dynamichelper.MockInterface) {
-				mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil)
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+
 			controller := gomock.NewController(t)
 			defer controller.Finish()
 
+			clientFake := fake.NewClientBuilder().WithObjects(tt.mcp).Build()
+
 			mdh := mock_dynamichelper.NewMockInterface(controller)
 			sr := &systemreserved{
-				mcocli: tt.mcocli,
 				dh:     mdh,
+				client: clientFake,
 				log:    utillog.GetLogger(),
 			}
 
-			var updated bool
-			tt.mcocli.PrependReactor("update", "machineconfigpools", func(action ktesting.Action) (handled bool, ret kruntime.Object, err error) {
-				updated = true
-				return false, nil, nil
-			})
+			mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil)
 
-			tt.mocker(mdh)
-			if err := sr.Ensure(context.Background()); (err != nil) != tt.wantErr {
-				t.Errorf("systemreserved.Ensure() error = %v, wantErr %v", err, tt.wantErr)
+			err := sr.Ensure(context.Background())
+			if err != nil {
+				t.Error(err)
 			}
-			if tt.machineConfigPoolNeedsUpdate != updated {
-				t.Errorf("systemreserved.Ensure() updated %v, machineConfigPoolNeedsUpdate = %v", updated, tt.machineConfigPoolNeedsUpdate)
+
+			result := &mcv1.MachineConfigPool{}
+			err = clientFake.Get(ctx, types.NamespacedName{Name: workerMachineConfigPoolName}, result)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if val, ok := result.Labels[labelName]; !ok || val != labelValue {
+				t.Error(result.Labels)
 			}
 		})
 	}