Repository: Azure/ARO-RP
Pull request #2719 (2023-02-14 16:15:06)

Operator standardise dynamichelper
### What this PR does / why we need it:

Previously `dynamichelper` was sometimes passed into other constructors, sometimes created inside the controllers. This PR standardises on passing it into the constructors.



github-actions[bot]: Please rebase pull request.

Patch:
diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go
index 9139a0473..e3d7202a4 100644
--- a/cmd/aro/operator.go
+++ b/cmd/aro/operator.go
@@ -79,7 +79,6 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 	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 {
 		return err
@@ -88,7 +87,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 	if role == pkgoperator.RoleMaster {
 		if err = (genevalogging.NewReconciler(
 			log.WithField("controller", genevalogging.ControllerName),
-			client, restConfig)).SetupWithManager(mgr); err != nil {
+			client, dh)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", genevalogging.ControllerName, err)
 		}
 		if err = (clusteroperatoraro.NewReconciler(
@@ -113,7 +112,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 		}
 		if err = (routefix.NewReconciler(
 			log.WithField("controller", routefix.ControllerName),
-			client, restConfig)).SetupWithManager(mgr); err != nil {
+			client, dh)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", routefix.ControllerName, err)
 		}
 		if err = (monitoring.NewReconciler(
diff --git a/pkg/operator/controllers/genevalogging/genevalogging_controller.go b/pkg/operator/controllers/genevalogging/genevalogging_controller.go
index e4d75db30..30b478eff 100644
--- a/pkg/operator/controllers/genevalogging/genevalogging_controller.go
+++ b/pkg/operator/controllers/genevalogging/genevalogging_controller.go
@@ -11,7 +11,6 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -37,16 +36,16 @@ const (
 type Reconciler struct {
 	log *logrus.Entry
 
-	restConfig *rest.Config
-
+	dh     dynamichelper.Interface
 	client client.Client
 }
 
-func NewReconciler(log *logrus.Entry, client client.Client, restConfig *rest.Config) *Reconciler {
+func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *Reconciler {
 	return &Reconciler{
-		log:        log,
-		restConfig: restConfig,
-		client:     client,
+		log: log,
+
+		dh:     dh,
+		client: client,
 	}
 }
 
@@ -70,15 +69,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 		return reconcile.Result{}, err
 	}
 
-	// TODO: dh should be a field in r, but the fact that it is initialised here
-	// each time currently saves us in the case that the controller runs before
-	// the SCC API is registered.
-	dh, err := dynamichelper.New(r.log, r.restConfig)
-	if err != nil {
-		r.log.Error(err)
-		return reconcile.Result{}, err
-	}
-
 	resources, err := r.resources(ctx, instance, operatorSecret.Data[GenevaCertName], operatorSecret.Data[GenevaKeyName])
 	if err != nil {
 		r.log.Error(err)
@@ -97,7 +87,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 		return reconcile.Result{}, err
 	}
 
-	err = dh.Ensure(ctx, resources...)
+	err = r.dh.Ensure(ctx, resources...)
 	if err != nil {
 		r.log.Error(err)
 		return reconcile.Result{}, err
diff --git a/pkg/operator/controllers/routefix/routefix_controller.go b/pkg/operator/controllers/routefix/routefix_controller.go
index 7129ae049..7e09caeaf 100644
--- a/pkg/operator/controllers/routefix/routefix_controller.go
+++ b/pkg/operator/controllers/routefix/routefix_controller.go
@@ -15,7 +15,6 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -38,8 +37,8 @@ type Reconciler struct {
 	log *logrus.Entry
 
 	client client.Client
+	dh     dynamichelper.Interface
 
-	restConfig *rest.Config
 	verFixed46 *version.Version
 	verFixed47 *version.Version
 }
@@ -50,11 +49,13 @@ var (
 )
 
 // NewReconciler creates a new Reconciler
-func NewReconciler(log *logrus.Entry, client client.Client, restConfig *rest.Config) *Reconciler {
+func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *Reconciler {
 	return &Reconciler{
-		log:        log,
-		client:     client,
-		restConfig: restConfig,
+		log: log,
+
+		client: client,
+		dh:     dh,
+
 		verFixed46: verFixed46,
 		verFixed47: verFixed47,
 	}
@@ -98,15 +99,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 func (r *Reconciler) deploy(ctx context.Context, instance *arov1alpha1.Cluster) (ctrl.Result, error) {
 	r.log.Debugf("deploying RouteFix")
 
-	// TODO: dh should be a field in r, but the fact that it is initialised here
-	// each time currently saves us in the case that the controller runs before
-	// the SCC API is registered.
-	dh, err := dynamichelper.New(r.log, r.restConfig)
-	if err != nil {
-		r.log.Error(err)
-		return reconcile.Result{}, err
-	}
-
 	resources, err := r.resources(ctx, instance)
 	if err != nil {
 		r.log.Error(err)
@@ -125,7 +117,7 @@ func (r *Reconciler) deploy(ctx context.Context, instance *arov1alpha1.Cluster)
 		return reconcile.Result{}, err
 	}
 
-	err = dh.Ensure(ctx, resources...)
+	err = r.dh.Ensure(ctx, resources...)
 	if err != nil {
 		r.log.Error(err)
 		return reconcile.Result{}, err
diff --git a/pkg/portal/regions.go b/pkg/portal/regions.go
index 46a791955..5e76635e8 100644
--- a/pkg/portal/regions.go
+++ b/pkg/portal/regions.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"strings"
 
 	"github.com/Azure/ARO-RP/pkg/util/azureclient"
 )
@@ -38,6 +39,7 @@ var (
 		"northeurope",
 		"norwaywest",
 		"norwayeast",
+		"qatar",
 		"southafricanorth",
 		"southcentralus",
 		"southeastasia",
@@ -72,7 +74,8 @@ func (p *portal) regions(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if value, found := os.LookupEnv("AZURE_ENVIRONMENT"); found {
-		if value == azureclient.PublicCloud.Environment.Name {
+		// AZURE_ENVIRONMENT variable can either be AZUREPUBLICCLOUD or AZUREUSGOVERNMENTCLOUD
+		if strings.EqualFold(value, azureclient.PublicCloud.Environment.Name) {
 			for _, region := range PROD_REGIONS {
 				resp.Regions = append(resp.Regions, Region{
 					Name: region,


Test patch:
diff --git a/pkg/portal/regions_test.go b/pkg/portal/regions_test.go
index 7b0f4db96..3715cd758 100644
--- a/pkg/portal/regions_test.go
+++ b/pkg/portal/regions_test.go
@@ -152,6 +152,10 @@ func TestRegionListPublic(t *testing.T) {
 				Name: "norwayeast",
 				URL:  "https://norwayeast.admin.aro.azure.com",
 			},
+			{
+				Name: "qatar",
+				URL:  "https://qatar.admin.aro.azure.com",
+			},
 			{
 				Name: "southafricanorth",
 				URL:  "https://southafricanorth.admin.aro.azure.com",