Merge pull request #2340 from dem4gus/imageconfig-env-aware

make ImageConfig environment aware

pkg/operator/controllers/imageconfig/image_controller.go

@@ -62,7 +62,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 	}
 
 	// Check for cloud type
-	requiredRegistries, err := getCloudAwareRegistries(instance)
+	requiredRegistries, err := GetCloudAwareRegistries(instance)
 	if err != nil {
 		// Not returning error as it will requeue again
 		return reconcile.Result{}, nil
@@ -118,19 +118,26 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 // Switch case to ensure the correct registries are added depending on the cloud environment (Gov or Public cloud)
-func getCloudAwareRegistries(instance *arov1alpha1.Cluster) ([]string, error) {
-	var requiredRegistries []string
+func GetCloudAwareRegistries(instance *arov1alpha1.Cluster) ([]string, error) {
+	var replicationRegistry string
+	var dnsSuffix string
+
+	acrDomain := instance.Spec.ACRDomain
+	acrSubdomain := strings.Split(acrDomain, ".")[0]
+	if acrDomain == "" || acrSubdomain == "" {
+		return nil, fmt.Errorf("azure container registry domain is not present or is malformed")
+	}
+
 	switch instance.Spec.AZEnvironment {
 	case azureclient.PublicCloud.Environment.Name:
-		requiredRegistries = []string{instance.Spec.ACRDomain, "arosvc." + instance.Spec.Location + ".data." + azure.PublicCloud.ContainerRegistryDNSSuffix}
-
+		dnsSuffix = azure.PublicCloud.ContainerRegistryDNSSuffix
 	case azureclient.USGovernmentCloud.Environment.Name:
-		requiredRegistries = []string{instance.Spec.ACRDomain, "arosvc." + instance.Spec.Location + ".data." + azure.USGovernmentCloud.ContainerRegistryDNSSuffix}
-
+		dnsSuffix = azure.USGovernmentCloud.ContainerRegistryDNSSuffix
 	default:
 		return nil, fmt.Errorf("cloud environment %s is not supported", instance.Spec.AZEnvironment)
 	}
-	return requiredRegistries, nil
+	replicationRegistry = fmt.Sprintf("%s.%s.data.%s", acrSubdomain, instance.Spec.Location, dnsSuffix)
+	return []string{acrDomain, replicationRegistry}, nil
 }
 
 // Helper function that filters registries to make sure they are added in consistent order

pkg/operator/controllers/imageconfig/image_controller_test.go

@@ -87,7 +87,7 @@ func TestImageConfigReconciler(t *testing.T) {
 				AllowedRegistries: []string{
 					"quay.io",
 					"arointsvc.azurecr.io",
-					"arosvc.eastus.data.azurecr.io",
+					"arointsvc.eastus.data.azurecr.io",
 				},
 			},
 		},
@@ -100,7 +100,7 @@ func TestImageConfigReconciler(t *testing.T) {
 						BlockedRegistries: []string{
 							"quay.io",
 							"arointsvc.azurecr.io",
-							"arosvc.eastus.data.azurecr.io",
+							"arointsvc.eastus.data.azurecr.io",
 						},
 					},
 				},
@@ -164,6 +164,64 @@ func TestImageConfigReconciler(t *testing.T) {
 			},
 			wantErr: `both AllowedRegistries and BlockedRegistries are present`,
 		},
+		{
+			name: "uses Public Cloud cluster's ACRDomain configuration for both Azure registries",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName},
+				Spec: arov1alpha1.ClusterSpec{
+					ACRDomain:     "fakesvc.azurecr.io",
+					AZEnvironment: azureclient.PublicCloud.Environment.Name,
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						controllerEnabled: strconv.FormatBool(true),
+					},
+					Location: "anyplace",
+				},
+			}),
+			configcli: configfake.NewSimpleClientset(&configv1.Image{
+				ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName},
+				Spec: configv1.ImageSpec{
+					RegistrySources: configv1.RegistrySources{
+						AllowedRegistries: []string{"quay.io"},
+					},
+				},
+			}),
+			wantRegistrySources: configv1.RegistrySources{
+				AllowedRegistries: []string{
+					"quay.io",
+					"fakesvc.azurecr.io",
+					"fakesvc.anyplace.data.azurecr.io",
+				},
+			},
+		},
+		{
+			name: "uses USGov Cloud cluster's ACRDomain configuration for both Azure registries",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName},
+				Spec: arov1alpha1.ClusterSpec{
+					ACRDomain:     "fakesvc.azurecr.us",
+					AZEnvironment: azureclient.USGovernmentCloud.Environment.Name,
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						controllerEnabled: strconv.FormatBool(true),
+					},
+					Location: "anyplace",
+				},
+			}),
+			configcli: configfake.NewSimpleClientset(&configv1.Image{
+				ObjectMeta: metav1.ObjectMeta{Name: arov1alpha1.SingletonClusterName},
+				Spec: configv1.ImageSpec{
+					RegistrySources: configv1.RegistrySources{
+						AllowedRegistries: []string{"quay.io"},
+					},
+				},
+			}),
+			wantRegistrySources: configv1.RegistrySources{
+				AllowedRegistries: []string{
+					"quay.io",
+					"fakesvc.azurecr.us",
+					"fakesvc.anyplace.data.azurecr.us",
+				},
+			},
+		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			ctx := context.Background()
@@ -222,12 +280,12 @@ func TestGetCloudAwareRegistries(t *testing.T) {
 			name: "public cloud",
 			instance: &arov1alpha1.Cluster{
 				Spec: arov1alpha1.ClusterSpec{
-					ACRDomain:     "arointsvc.azurecr.io",
+					ACRDomain:     "arosvc.azurecr.io",
 					AZEnvironment: azureclient.PublicCloud.Environment.Name,
 					Location:      "eastus",
 				},
 			},
-			wantResult: []string{"arointsvc.azurecr.io", "arosvc.eastus.data.azurecr.io"},
+			wantResult: []string{"arosvc.azurecr.io", "arosvc.eastus.data.azurecr.io"},
 		},
 		{
 			name: "us gov cloud",
@@ -238,7 +296,18 @@ func TestGetCloudAwareRegistries(t *testing.T) {
 					Location:      "eastus",
 				},
 			},
-			wantResult: []string{"arointsvc.azurecr.us", "arosvc.eastus.data.azurecr.us"},
+			wantResult: []string{"arointsvc.azurecr.us", "arointsvc.eastus.data.azurecr.us"},
+		},
+		{
+			name: "arbitrary name",
+			instance: &arov1alpha1.Cluster{
+				Spec: arov1alpha1.ClusterSpec{
+					ACRDomain:     "fakeacr.azurecr.io",
+					AZEnvironment: azureclient.PublicCloud.Environment.Name,
+					Location:      "anyplace",
+				},
+			},
+			wantResult: []string{"fakeacr.azurecr.io", "fakeacr.anyplace.data.azurecr.io"},
 		},
 		{
 			name: "unsupported cloud",
@@ -253,7 +322,7 @@ func TestGetCloudAwareRegistries(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			result, err := getCloudAwareRegistries(tt.instance)
+			result, err := GetCloudAwareRegistries(tt.instance)
 
 			if err != nil && err.Error() != tt.wantErr ||
 				err == nil && tt.wantErr != "" {

test/e2e/operator.go

@@ -30,8 +30,8 @@ import (
 	"k8s.io/client-go/util/retry"
 
 	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	imageController "github.com/Azure/ARO-RP/pkg/operator/controllers/imageconfig"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/monitoring"
-	"github.com/Azure/ARO-RP/pkg/util/azureclient"
 	"github.com/Azure/ARO-RP/pkg/util/conditions"
 	"github.com/Azure/ARO-RP/pkg/util/ready"
 	"github.com/Azure/ARO-RP/pkg/util/subnet"
@@ -434,22 +434,6 @@ var _ = Describe("ARO Operator - ImageConfig Reconciler", func() {
 	var requiredRegistries []string
 	var imageconfig *configv1.Image
 
-	// Reimplementation of function from image config controller
-	getCloudAwareRegistries := func(instance *arov1alpha1.Cluster) ([]string, error) {
-		var requiredRegistries []string
-		switch instance.Spec.AZEnvironment {
-		case azureclient.PublicCloud.Environment.Name:
-			requiredRegistries = []string{instance.Spec.ACRDomain, "arosvc." + instance.Spec.Location + ".data." + azure.PublicCloud.ContainerRegistryDNSSuffix}
-
-		case azureclient.USGovernmentCloud.Environment.Name:
-			requiredRegistries = []string{instance.Spec.ACRDomain, "arosvc." + instance.Spec.Location + ".data." + azure.USGovernmentCloud.ContainerRegistryDNSSuffix}
-
-		default:
-			return nil, fmt.Errorf("cloud environment %s is not supported", instance.Spec.AZEnvironment)
-		}
-		return requiredRegistries, nil
-	}
-
 	sliceEqual := func(a, b []string) bool {
 		if len(a) != len(b) {
 			return false
@@ -488,7 +472,7 @@ var _ = Describe("ARO Operator - ImageConfig Reconciler", func() {
 		imageconfig, err = clients.ConfigClient.ConfigV1().Images().Get(ctx, "cluster", metav1.GetOptions{})
 		Expect(err).NotTo(HaveOccurred())
 
-		requiredRegistries, err = getCloudAwareRegistries(instance)
+		requiredRegistries, err = imageController.GetCloudAwareRegistries(instance)
 		Expect(err).NotTo(HaveOccurred())
 	})