Repository: Azure/ARO-RP
Pull request #2177 (2022-06-13 16:05:48)

Add e2e tests for ImageConfig operator
### Which issue this PR addresses:

<!--
Please include a link to the ADO work item as well as any GitHub issues.

Usage: `Fixes #<GitHub issue number>`, or `Fixes (paste link of issue)`.
-->
[ADO User Story 11051435](https://msazure.visualstudio.com/AzureRedHatOpenShift/_boards/board/t/Chainsaw/Stories/?workitem=11051435)

### What this PR does / why we need it:

This PR adds end-to-end tests for the ImageConfig reconciler. The tests themselves are basically a duplication of the unit tests but in a live cluster. Deployment of images from allowed or blocked registries are not currently tested.
<!--
Include a brief summary of what the PR is intended to accomplish and how the PR
does it. (2-3 sentences)
-->

### Test plan for issue:

Run e2e tests.
<!--
How did you test that this PR works?

- Are there unit tests?
- Are there integration/e2e tests?
- If it is not possible to write automated tests, explain why and document how
  to manually test and verify the feature.
-->

### Is there any documentation that needs to be updated for this PR?

<!--
- If yes and the docs are in GitHub, include doc updates in the PR.
- If yes and the docs are not in GitHub (i.e. ADO wiki), include a link to the
  docs.
- If no, explain why (e.g. "tech debt cleanup, N/A").
-->


microsoft-cla[bot]: [![CLA assistant check](https://cla.opensource.microsoft.com/pull/badge/not_signed)](https://cla.opensource.microsoft.com/Azure/ARO-RP?pullRequest=2177) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla.opensource.microsoft.com/Azure/ARO-RP?pullRequest=2177) before we can accept your contribution.<br/><br/>:x: dem4gus [sign now](https://cla.opensource.microsoft.com/Azure/ARO-RP?pullRequest=2177)<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla.opensource.microsoft.com/check/Azure/ARO-RP?pullRequest=2177) it.</sub>
github-actions[bot]: Please rebase pull request.
s-amann: /azp run e2e
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
cadenmarchese: The test looks good to me, I would just recommend the following:
- Remove the non-test-specific stuff (such as the return code changes) from this PR and move them into a separate PR
- Squash/re-do your commits so that there's just 1 or 2

Nice work!
dem4gus: Put some constants in a `const()` block to address some offline feedback.

Patch:
diff --git a/pkg/util/dynamichelper/dynamichelper.go b/pkg/util/dynamichelper/dynamichelper.go
index c9c213eac..8892fe028 100644
--- a/pkg/util/dynamichelper/dynamichelper.go
+++ b/pkg/util/dynamichelper/dynamichelper.go
@@ -39,7 +39,7 @@ type dynamicHelper struct {
 	GVRResolver
 
 	log     *logrus.Entry
-	restcli *rest.RESTClient
+	restcli rest.Interface
 }
 
 func New(log *logrus.Entry, restconfig *rest.Config) (Interface, error) {


Test patch:
diff --git a/docs/testing.md b/docs/testing.md
index 340ba1cc3..79f6802c6 100644
--- a/docs/testing.md
+++ b/docs/testing.md
@@ -61,7 +61,8 @@ End to end tests are run using ginkgo. You can run subsets of tests or ignore so
 # source your environment file
 . ./secrets/env
 
-# set the LOCAL_E2E env if you are testing locally
+# set the CLUSTER and LOCAL_E2E env if you are testing locally
+export CLUSTER=<cluster-name>
 export LOCAL_E2E="true"
 
 # source the e2e helper file
diff --git a/hack/e2e/run-rp-and-e2e.sh b/hack/e2e/run-rp-and-e2e.sh
index 7d2638bf8..7fd4f10d7 100755
--- a/hack/e2e/run-rp-and-e2e.sh
+++ b/hack/e2e/run-rp-and-e2e.sh
@@ -89,13 +89,13 @@ if [ -z "${LOCAL_E2E}" ] ; then
 fi
 
 if [ -z "${CLUSTER}" ] ; then
-    echo "CLUSTER is not set , aborting"
-    exit 1
+    echo "CLUSTER is not set, aborting"
+    return 1
 fi
 
 if [ -z "${DATABASE_NAME}" ] ; then
-    echo "DATABASE_NAME is not set , aborting"
-    exit 1
+    echo "DATABASE_NAME is not set, aborting"
+    return 1
 fi
 
 echo "######################################"
@@ -117,11 +117,11 @@ echo
 echo "PROXY_HOSTNAME=$PROXY_HOSTNAME"
 echo "######################################"
 
-[ "$LOCATION" ] || ( echo ">> LOCATION is not set please validate your ./secrets/env"; exit 128 )
-[ "$RESOURCEGROUP" ] || ( echo ">> RESOURCEGROUP is not set; please validate your ./secrets/env"; exit 128 )
-[ "$PROXY_HOSTNAME" ] || ( echo ">> PROXY_HOSTNAME is not set; please validate your ./secrets/env"; exit 128 )
-[ "$DATABASE_ACCOUNT_NAME" ] || ( echo ">> DATABASE_ACCOUNT_NAME is not set; please validate your ./secrets/env"; exit 128 )
-[ "$DATABASE_NAME" ] || ( echo ">> DATABASE_NAME is not set; please validate your ./secrets/env"; exit 128 )
-[ "$AZURE_SUBSCRIPTION_ID" ] || ( echo ">> AZURE_SUBSCRIPTION_ID is not set; please validate your ./secrets/env"; exit 128 )
+[ "$LOCATION" ] || ( echo ">> LOCATION is not set please validate your ./secrets/env"; return 128 )
+[ "$RESOURCEGROUP" ] || ( echo ">> RESOURCEGROUP is not set; please validate your ./secrets/env"; return 128 )
+[ "$PROXY_HOSTNAME" ] || ( echo ">> PROXY_HOSTNAME is not set; please validate your ./secrets/env"; return 128 )
+[ "$DATABASE_ACCOUNT_NAME" ] || ( echo ">> DATABASE_ACCOUNT_NAME is not set; please validate your ./secrets/env"; return 128 )
+[ "$DATABASE_NAME" ] || ( echo ">> DATABASE_NAME is not set; please validate your ./secrets/env"; return 128 )
+[ "$AZURE_SUBSCRIPTION_ID" ] || ( echo ">> AZURE_SUBSCRIPTION_ID is not set; please validate your ./secrets/env"; return 128 )
 
 az account set -s $AZURE_SUBSCRIPTION_ID >/dev/null
diff --git a/pkg/util/dynamichelper/dynamichelper_test.go b/pkg/util/dynamichelper/dynamichelper_test.go
index b7103d823..2da2c4bde 100644
--- a/pkg/util/dynamichelper/dynamichelper_test.go
+++ b/pkg/util/dynamichelper/dynamichelper_test.go
@@ -4,6 +4,8 @@ package dynamichelper
 // Licensed under the Apache License 2.0.
 
 import (
+	"context"
+	"net/http"
 	"reflect"
 	"testing"
 	"time"
@@ -18,11 +20,73 @@ import (
 	kruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/cli-runtime/pkg/resource"
+	"k8s.io/client-go/rest/fake"
 
 	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
 	"github.com/Azure/ARO-RP/pkg/util/cmp"
 )
 
+type mockGVRResolver struct{}
+
+func (gvr mockGVRResolver) Refresh() error {
+	return nil
+}
+
+func (gvr mockGVRResolver) Resolve(groupKind, optionalVersion string) (*schema.GroupVersionResource, error) {
+	return &schema.GroupVersionResource{Group: "metal3.io", Version: "v1alpha1", Resource: "configmap"}, nil
+}
+
+func TestEsureDeleted(t *testing.T) {
+	ctx := context.Background()
+
+	mockGVRResolver := mockGVRResolver{}
+
+	mockRestCLI := &fake.RESTClient{
+		GroupVersion:         schema.GroupVersion{Group: "testgroup", Version: "v1"},
+		NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer,
+		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
+			switch req.Method {
+			case "DELETE":
+				switch req.URL.Path {
+				case "/apis/metal3.io/v1alpha1/namespaces/test-ns-1/configmap/test-name-1":
+					return &http.Response{StatusCode: http.StatusNotFound}, nil
+				case "/apis/metal3.io/v1alpha1/namespaces/test-ns-2/configmap/test-name-2":
+					return &http.Response{StatusCode: http.StatusInternalServerError}, nil
+				case "/apis/metal3.io/v1alpha1/namespaces/test-ns-3/configmap/test-name-3":
+					return &http.Response{StatusCode: http.StatusOK}, nil
+				default:
+					t.Fatalf("unexpected path: %#v\n%#v", req.URL, req)
+					return nil, nil
+				}
+			default:
+				t.Fatalf("unexpected request: %s %#v\n%#v", req.Method, req.URL, req)
+				return nil, nil
+			}
+		}),
+	}
+
+	dh := &dynamicHelper{
+		GVRResolver: mockGVRResolver,
+		restcli:     mockRestCLI,
+	}
+
+	err := dh.EnsureDeleted(ctx, "configmap", "test-ns-1", "test-name-1")
+	if err != nil {
+		t.Errorf("no error should be bounced for status not found, but got: %v", err)
+	}
+
+	err = dh.EnsureDeleted(ctx, "configmap", "test-ns-2", "test-name-2")
+	if err == nil {
+		t.Errorf("function should handle failure response (non-404) correctly")
+	}
+
+	err = dh.EnsureDeleted(ctx, "configmap", "test-ns-3", "test-name-3")
+	if err != nil {
+		t.Errorf("function should handle success response correctly")
+	}
+}
+
 func TestMerge(t *testing.T) {
 	serviceInternalTrafficPolicy := corev1.ServiceInternalTrafficPolicyCluster
 
diff --git a/test/e2e/operator.go b/test/e2e/operator.go
index 229457060..aa56d37f4 100644
--- a/test/e2e/operator.go
+++ b/test/e2e/operator.go
@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -19,6 +20,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/ghodss/yaml"
+	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/ugorji/go/codec"
 	corev1 "k8s.io/api/core/v1"
@@ -419,3 +421,108 @@ var _ = Describe("ARO Operator - MHC Deployment", func() {
 		Expect(err).NotTo(HaveOccurred())
 	})
 })
+
+var _ = Describe("ARO Operator - ImageConfig Reconciler", func() {
+	const (
+		imageconfigFlag  = "aro.imageconfig.enabled"
+		optionalRegistry = "quay.io"
+		timeout          = 5 * time.Minute
+	)
+	ctx := context.Background()
+
+	var requiredRegistries []string
+	var imageconfig *configv1.Image
+
+	getLocationRegistries := func(acrDomain, acrDnsSuffix, location string) []string {
+		regionalAcr := fmt.Sprintf("arosvc.%s.data.%s", location, acrDnsSuffix)
+		requiredRegistries := []string{acrDomain, regionalAcr}
+		return requiredRegistries
+	}
+
+	sliceEqual := func(a, b []string) bool {
+		if len(a) != len(b) {
+			return false
+		}
+		sort.Strings(a)
+		sort.Strings(b)
+
+		for idx, entry := range b {
+			if a[idx] != entry {
+				return false
+			}
+		}
+		return true
+	}
+
+	verifyLists := func(expectedAllowlist, expectedBlocklist []string) (bool, error) {
+		// have to do this because using declaration assignment in following line results in pre-declared imageconfig var not being used
+		var err error
+		imageconfig, err = clients.ConfigClient.ConfigV1().Images().Get(ctx, "cluster", metav1.GetOptions{})
+		Expect(err).NotTo(HaveOccurred())
+
+		allowList := imageconfig.Spec.RegistrySources.AllowedRegistries
+		blockList := imageconfig.Spec.RegistrySources.BlockedRegistries
+
+		return sliceEqual(allowList, expectedAllowlist) && sliceEqual(blockList, expectedBlocklist), nil
+	}
+
+	BeforeEach(func() {
+		instance, err := clients.AROClusters.AroV1alpha1().Clusters().Get(ctx, "cluster", metav1.GetOptions{})
+		Expect(err).NotTo(HaveOccurred())
+
+		if !instance.Spec.OperatorFlags.GetSimpleBoolean(imageconfigFlag) {
+			Skip("ImageConfig Controller is not enabled, skipping test")
+		}
+
+		imageconfig, err = clients.ConfigClient.ConfigV1().Images().Get(ctx, "cluster", metav1.GetOptions{})
+		Expect(err).NotTo(HaveOccurred())
+		requiredRegistries = getLocationRegistries(instance.Spec.ACRDomain, "azurecr.io", instance.Spec.Location)
+	})
+
+	AfterEach(func() {
+		imageconfig.Spec.RegistrySources.AllowedRegistries = nil
+		imageconfig.Spec.RegistrySources.BlockedRegistries = nil
+
+		_, err := clients.ConfigClient.ConfigV1().Images().Update(ctx, imageconfig, metav1.UpdateOptions{})
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(func(g Gomega) {
+			g.Expect(verifyLists(nil, nil)).To(BeTrue())
+		}).WithTimeout(timeout).Should(Succeed())
+	})
+
+	It("should have empty allow and block lists by default", func() {
+		allowList := imageconfig.Spec.RegistrySources.AllowedRegistries
+		blockList := imageconfig.Spec.RegistrySources.BlockedRegistries
+
+		Expect(allowList).To(BeEmpty())
+		Expect(blockList).To(BeEmpty())
+	})
+
+	It("should add registries to the allow list", func() {
+
+		expectedAllowlist := append(requiredRegistries, optionalRegistry)
+
+		imageconfig.Spec.RegistrySources.AllowedRegistries = append(imageconfig.Spec.RegistrySources.AllowedRegistries, optionalRegistry)
+
+		_, err := clients.ConfigClient.ConfigV1().Images().Update(ctx, imageconfig, metav1.UpdateOptions{})
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(func(g Gomega) {
+			g.Expect(verifyLists(expectedAllowlist, nil)).To(BeTrue())
+		}).WithTimeout(timeout).Should(Succeed())
+	})
+
+	It("should add optional registries to the block list and skip required registries", func() {
+		expectedBlocklist := []string{optionalRegistry}
+
+		imageconfig.Spec.RegistrySources.BlockedRegistries = append(imageconfig.Spec.RegistrySources.BlockedRegistries, optionalRegistry, requiredRegistries[0])
+
+		_, err := clients.ConfigClient.ConfigV1().Images().Update(ctx, imageconfig, metav1.UpdateOptions{})
+		Expect(err).NotTo(HaveOccurred())
+
+		Eventually(func(g Gomega) {
+			g.Expect(verifyLists(nil, expectedBlocklist)).To(BeTrue())
+		}).WithTimeout(timeout).Should(Succeed())
+	})
+})
diff --git a/test/e2e/setup.go b/test/e2e/setup.go
index c57d754ee..4fd59106a 100644
--- a/test/e2e/setup.go
+++ b/test/e2e/setup.go
@@ -13,6 +13,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	"github.com/Azure/go-autorest/autorest/azure/auth"
+	configclient "github.com/openshift/client-go/config/clientset/versioned"
 	machineclient "github.com/openshift/client-go/machine/clientset/versioned"
 	projectclient "github.com/openshift/client-go/project/clientset/versioned"
 	mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned"
@@ -56,6 +57,7 @@ type clientSet struct {
 	MachineAPI    machineclient.Interface
 	MachineConfig mcoclient.Interface
 	AROClusters   aroclient.Interface
+	ConfigClient  configclient.Interface
 	Project       projectclient.Interface
 }
 
@@ -128,6 +130,11 @@ func newClientSet(ctx context.Context) (*clientSet, error) {
 		return nil, err
 	}
 
+	configcli, err := configclient.NewForConfig(restconfig)
+	if err != nil {
+		return nil, err
+	}
+
 	return &clientSet{
 		OpenshiftClustersv20200430:        redhatopenshift20200430.NewOpenShiftClustersClient(_env.Environment(), _env.SubscriptionID(), authorizer),
 		Operationsv20200430:               redhatopenshift20200430.NewOperationsClient(_env.Environment(), _env.SubscriptionID(), authorizer),
@@ -150,6 +157,7 @@ func newClientSet(ctx context.Context) (*clientSet, error) {
 		MachineConfig: mcocli,
 		AROClusters:   arocli,
 		Project:       projectcli,
+		ConfigClient:  configcli,
 	}, nil
 }