Repository: Azure/ARO-RP
Pull request #2308 (2022-08-04 15:44:27)

added hiveEnsureResources unit tests
### Which issue this PR addresses:

https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/14962351/

### What this PR does / why we need it:

unit tests for hiveEnsureResources

### Is there any documentation that needs to be updated for this PR?

No


m1kola: @bizz001 this PR looks good, but python tests are failing. There is a fix for this in #2321. You will probably have to rebase your PRs to include the fix once merged.
ross-bryan: /azp run ci
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/Makefile b/Makefile
index d02a13214..006a6029b 100644
--- a/Makefile
+++ b/Makefile
@@ -195,7 +195,7 @@ test-python: pyenv az
 
 
 shared-cluster-login: 
-	oc login ${SHARED_CLUSTER_API} -u kubeadmin -p ${SHARED_CLUSTER_KUBEADMIN_PASSWORD}
+	@oc login ${SHARED_CLUSTER_API} -u kubeadmin -p ${SHARED_CLUSTER_KUBEADMIN_PASSWORD}
 
 unit-test-python:
 	hack/unit-test-python.sh
diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go
index 82c63f4df..42ccbc764 100644
--- a/pkg/cluster/install.go
+++ b/pkg/cluster/install.go
@@ -124,6 +124,10 @@ func (m *manager) Update(ctx context.Context) error {
 		// credentials rotation flow steps
 		steps.Action(m.createOrUpdateClusterServicePrincipalRBAC),
 		steps.Action(m.createOrUpdateDenyAssignment),
+		steps.Action(m.startVMs),
+		steps.Condition(m.apiServersReady, 30*time.Minute, true),
+		steps.Action(m.configureAPIServerCertificate),
+		steps.Action(m.configureIngressCertificate),
 		steps.Action(m.updateOpenShiftSecret),
 		steps.Action(m.updateAROSecret),
 		// Hive reconciliation: we mostly need it to make sure that
diff --git a/pkg/frontend/admin_openshiftcluster_kubernetesobjects.go b/pkg/frontend/admin_openshiftcluster_kubernetesobjects.go
index abad145c8..b97cf1ab5 100644
--- a/pkg/frontend/admin_openshiftcluster_kubernetesobjects.go
+++ b/pkg/frontend/admin_openshiftcluster_kubernetesobjects.go
@@ -74,12 +74,20 @@ func (f *frontend) _deleteAdminKubernetesObjects(ctx context.Context, r *http.Re
 	vars := mux.Vars(r)
 
 	groupKind, namespace, name := r.URL.Query().Get("kind"), r.URL.Query().Get("namespace"), r.URL.Query().Get("name")
+	force := strings.EqualFold(r.URL.Query().Get("force"), "true")
 
 	err := validateAdminKubernetesObjectsNonCustomer(r.Method, groupKind, namespace, name)
 	if err != nil {
 		return err
 	}
 
+	if force {
+		err := validateAdminKubernetesObjectsForceDelete(groupKind)
+		if err != nil {
+			return err
+		}
+	}
+
 	resourceID := strings.TrimPrefix(r.URL.Path, "/admin")
 
 	doc, err := f.dbOpenShiftClusters.Get(ctx, resourceID)
@@ -95,7 +103,7 @@ func (f *frontend) _deleteAdminKubernetesObjects(ctx context.Context, r *http.Re
 		return err
 	}
 
-	return k.KubeDelete(ctx, groupKind, namespace, name)
+	return k.KubeDelete(ctx, groupKind, namespace, name, force)
 }
 
 func (f *frontend) postAdminKubernetesObjects(w http.ResponseWriter, r *http.Request) {
diff --git a/pkg/frontend/adminactions/kubeactions.go b/pkg/frontend/adminactions/kubeactions.go
index 48511bc03..0180121b9 100644
--- a/pkg/frontend/adminactions/kubeactions.go
+++ b/pkg/frontend/adminactions/kubeactions.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"net/http"
 
+	"github.com/Azure/go-autorest/autorest/to"
 	configclient "github.com/openshift/client-go/config/clientset/versioned"
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
@@ -27,7 +28,7 @@ type KubeActions interface {
 	KubeGet(ctx context.Context, groupKind, namespace, name string) ([]byte, error)
 	KubeList(ctx context.Context, groupKind, namespace string) ([]byte, error)
 	KubeCreateOrUpdate(ctx context.Context, obj *unstructured.Unstructured) error
-	KubeDelete(ctx context.Context, groupKind, namespace, name string) error
+	KubeDelete(ctx context.Context, groupKind, namespace, name string, force bool) error
 	CordonNode(ctx context.Context, nodeName string, unschedulable bool) error
 	DrainNode(ctx context.Context, nodeName string) error
 	Upgrade(ctx context.Context, upgradeY bool) error
@@ -140,11 +141,16 @@ func (k *kubeActions) KubeCreateOrUpdate(ctx context.Context, o *unstructured.Un
 	return err
 }
 
-func (k *kubeActions) KubeDelete(ctx context.Context, groupKind, namespace, name string) error {
+func (k *kubeActions) KubeDelete(ctx context.Context, groupKind, namespace, name string, force bool) error {
 	gvr, err := k.gvrResolver.Resolve(groupKind, "")
 	if err != nil {
 		return err
 	}
 
-	return k.dyn.Resource(*gvr).Namespace(namespace).Delete(ctx, name, metav1.DeleteOptions{})
+	resourceDeleteOptions := metav1.DeleteOptions{}
+	if force {
+		resourceDeleteOptions.GracePeriodSeconds = to.Int64Ptr(0)
+	}
+
+	return k.dyn.Resource(*gvr).Namespace(namespace).Delete(ctx, name, resourceDeleteOptions)
 }
diff --git a/pkg/frontend/validate.go b/pkg/frontend/validate.go
index 363ebf554..334093715 100644
--- a/pkg/frontend/validate.go
+++ b/pkg/frontend/validate.go
@@ -111,6 +111,14 @@ func validateAdminKubernetesObjects(method, groupKind, namespace, name string) e
 	return nil
 }
 
+func validateAdminKubernetesObjectsForceDelete(groupKind string) error {
+	if !strings.EqualFold(groupKind, "Pod") {
+		return api.NewCloudError(http.StatusForbidden, api.CloudErrorCodeForbidden, "", "Force deleting groupKind '%s' is forbidden.", groupKind)
+	}
+
+	return nil
+}
+
 func validateAdminVMName(vmName string) error {
 	if vmName == "" || !rxKubernetesString.MatchString(vmName) {
 		return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, "", "The provided vmName '%s' is invalid.", vmName)
diff --git a/pkg/util/mocks/adminactions/adminactions.go b/pkg/util/mocks/adminactions/adminactions.go
index 28aa65d44..75e0d6514 100644
--- a/pkg/util/mocks/adminactions/adminactions.go
+++ b/pkg/util/mocks/adminactions/adminactions.go
@@ -81,17 +81,17 @@ func (mr *MockKubeActionsMockRecorder) KubeCreateOrUpdate(arg0, arg1 interface{}
 }
 
 // KubeDelete mocks base method.
-func (m *MockKubeActions) KubeDelete(arg0 context.Context, arg1, arg2, arg3 string) error {
+func (m *MockKubeActions) KubeDelete(arg0 context.Context, arg1, arg2, arg3 string, arg4 bool) error {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "KubeDelete", arg0, arg1, arg2, arg3)
+	ret := m.ctrl.Call(m, "KubeDelete", arg0, arg1, arg2, arg3, arg4)
 	ret0, _ := ret[0].(error)
 	return ret0
 }
 
 // KubeDelete indicates an expected call of KubeDelete.
-func (mr *MockKubeActionsMockRecorder) KubeDelete(arg0, arg1, arg2, arg3 interface{}) *gomock.Call {
+func (mr *MockKubeActionsMockRecorder) KubeDelete(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "KubeDelete", reflect.TypeOf((*MockKubeActions)(nil).KubeDelete), arg0, arg1, arg2, arg3)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "KubeDelete", reflect.TypeOf((*MockKubeActions)(nil).KubeDelete), arg0, arg1, arg2, arg3, arg4)
 }
 
 // KubeGet mocks base method.


Test patch:
diff --git a/pkg/cluster/hive_test.go b/pkg/cluster/hive_test.go
index b93dd9d9b..1303b1b52 100644
--- a/pkg/cluster/hive_test.go
+++ b/pkg/cluster/hive_test.go
@@ -272,3 +272,51 @@ func createManagerForTests(t *testing.T, existingNamespaceName string) *manager
 	}
 	return m
 }
+
+func TestHiveEnsureResources(t *testing.T) {
+	ctx := context.Background()
+	for _, tt := range []struct {
+		testName           string
+		clusterManagerMock func(mockCtrl *gomock.Controller, m *manager) *mock_hive.MockClusterManager
+		wantErr            string
+	}{
+		{
+			testName: "returns error if cluster manager returns error",
+			clusterManagerMock: func(mockCtrl *gomock.Controller, m *manager) *mock_hive.MockClusterManager {
+				mockClusterManager := mock_hive.NewMockClusterManager(mockCtrl)
+				mockClusterManager.EXPECT().CreateOrUpdate(ctx, m.subscriptionDoc, m.doc).Return(fmt.Errorf("cluster manager error"))
+				return mockClusterManager
+			},
+			wantErr: "cluster manager error",
+		},
+		{
+			testName: "does not return error if cluster manager is nil",
+		},
+		{
+			testName: "calls cluster manager CreateOrUpdate with correct parameters",
+			clusterManagerMock: func(mockCtrl *gomock.Controller, m *manager) *mock_hive.MockClusterManager {
+				mockClusterManager := mock_hive.NewMockClusterManager(mockCtrl)
+				mockClusterManager.EXPECT().CreateOrUpdate(ctx, m.subscriptionDoc, m.doc).Return(nil)
+				return mockClusterManager
+			},
+		},
+	} {
+		t.Run(tt.testName, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			m := createManagerForTests(t, "")
+
+			if tt.clusterManagerMock != nil {
+				m.hiveClusterManager = tt.clusterManagerMock(controller, m)
+			}
+
+			err := m.hiveEnsureResources(ctx)
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Error(err)
+			}
+		},
+		)
+	}
+}
diff --git a/pkg/frontend/admin_openshiftcluster_kubernetesobjects_test.go b/pkg/frontend/admin_openshiftcluster_kubernetesobjects_test.go
index bae3bbd56..056e630b7 100644
--- a/pkg/frontend/admin_openshiftcluster_kubernetesobjects_test.go
+++ b/pkg/frontend/admin_openshiftcluster_kubernetesobjects_test.go
@@ -32,6 +32,7 @@ func TestAdminKubernetesObjectsGetAndDelete(t *testing.T) {
 		objKind        string
 		objNamespace   string
 		objName        string
+		force          string
 		mocks          func(*test, *mock_adminactions.MockKubeActions)
 		method         string
 		wantStatusCode int
@@ -101,11 +102,39 @@ func TestAdminKubernetesObjectsGetAndDelete(t *testing.T) {
 			objName:      "config",
 			mocks: func(tt *test, k *mock_adminactions.MockKubeActions) {
 				k.EXPECT().
-					KubeDelete(gomock.Any(), tt.objKind, tt.objNamespace, tt.objName).
+					KubeDelete(gomock.Any(), tt.objKind, tt.objNamespace, tt.objName, false).
 					Return(nil)
 			},
 			wantStatusCode: http.StatusOK,
 		},
+		{
+			method:       http.MethodDelete,
+			name:         "force delete pod",
+			resourceID:   fmt.Sprintf("/subscriptions/%s/resourcegroups/resourceGroup/providers/Microsoft.RedHatOpenShift/openShiftClusters/resourceName", mockSubID),
+			objKind:      "Pod",
+			objNamespace: "openshift",
+			objName:      "aro-pod",
+			force:        "true",
+			mocks: func(tt *test, k *mock_adminactions.MockKubeActions) {
+				k.EXPECT().
+					KubeDelete(gomock.Any(), tt.objKind, tt.objNamespace, tt.objName, true).
+					Return(nil)
+			},
+			wantStatusCode: http.StatusOK,
+		},
+		{
+			method:       http.MethodDelete,
+			name:         "force delete not allowed",
+			resourceID:   fmt.Sprintf("/subscriptions/%s/resourcegroups/resourceGroup/providers/Microsoft.RedHatOpenShift/openShiftClusters/resourceName", mockSubID),
+			objKind:      "ConfigMap",
+			objNamespace: "openshift",
+			objName:      "config",
+			force:        "true",
+			mocks: func(tt *test, k *mock_adminactions.MockKubeActions) {
+			},
+			wantStatusCode: http.StatusForbidden,
+			wantError:      "403: Forbidden: : Force deleting groupKind 'ConfigMap' is forbidden.",
+		},
 		{
 			method:       http.MethodDelete,
 			name:         "no groupKind provided",
@@ -180,8 +209,13 @@ func TestAdminKubernetesObjectsGetAndDelete(t *testing.T) {
 
 			go f.Run(ctx, nil, nil)
 
+			requestStr := fmt.Sprintf("https://server/admin%s/kubernetesObjects?kind=%s&namespace=%s&name=%s", tt.resourceID, tt.objKind, tt.objNamespace, tt.objName)
+			if tt.method == http.MethodDelete && tt.force != "" {
+				requestStr = fmt.Sprintf("%s&force=%s", requestStr, tt.force)
+			}
+
 			resp, b, err := ti.request(tt.method,
-				fmt.Sprintf("https://server/admin%s/kubernetesObjects?kind=%s&namespace=%s&name=%s", tt.resourceID, tt.objKind, tt.objNamespace, tt.objName),
+				requestStr,
 				nil, nil)
 			if err != nil {
 				t.Fatal(err)
diff --git a/test/e2e/adminapi_kubernetesobjects.go b/test/e2e/adminapi_kubernetesobjects.go
index 7f665e69a..26cd829e0 100644
--- a/test/e2e/adminapi_kubernetesobjects.go
+++ b/test/e2e/adminapi_kubernetesobjects.go
@@ -27,8 +27,9 @@ var _ = Describe("[Admin API] Kubernetes objects action", func() {
 
 	When("in a standard openshift namespace", func() {
 		const namespace = "openshift"
+		const containerName = "e2e-test-container-name"
 
-		It("should be able to create, get, list, update and delete objects", func() {
+		It("should be able to create, get, list, update and delete objects, but force delete is only allowed for pods", func() {
 			defer func() {
 				// When ran successfully this test should delete the object,
 				// but we need to remove the object in case of failure
@@ -39,13 +40,22 @@ var _ = Describe("[Admin API] Kubernetes objects action", func() {
 				if !kerrors.IsNotFound(err) {
 					Expect(err).NotTo(HaveOccurred())
 				}
+				By("deleting the pod via Kubernetes API")
+				err = clients.Kubernetes.CoreV1().Pods(namespace).Delete(context.Background(), objName, metav1.DeleteOptions{})
+				// On successfully we expect NotFound error
+				if !kerrors.IsNotFound(err) {
+					Expect(err).NotTo(HaveOccurred())
+				}
 			}()
 
 			testConfigMapCreateOK(objName, namespace)
 			testConfigMapGetOK(objName, namespace)
 			testConfigMapListOK(objName, namespace)
 			testConfigMapUpdateOK(objName, namespace)
+			testConfigMapForceDeleteForbidden(objName, namespace)
 			testConfigMapDeleteOK(objName, namespace)
+			testPodCreateOK(containerName, objName, namespace)
+			testPodForceDeleteOK(objName, namespace)
 		})
 
 		testSecretOperationsForbidden(objName, namespace)
@@ -228,12 +238,28 @@ func testConfigMapUpdateOK(objName, namespace string) {
 	Expect(cm.Data).To(Equal(map[string]string{"key": "new_value"}))
 }
 
+func testConfigMapForceDeleteForbidden(objName, namespace string) {
+	By("force deleting the object via RP admin API")
+	params := url.Values{
+		"kind":      []string{"configmap"},
+		"namespace": []string{namespace},
+		"name":      []string{objName},
+		"force":     []string{"true"},
+	}
+	var cloudErr api.CloudError
+	resp, err := adminRequest(context.Background(), http.MethodDelete, "/admin"+resourceIDFromEnv()+"/kubernetesobjects", params, nil, &cloudErr)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(resp.StatusCode).To(Equal(http.StatusForbidden))
+	Expect(cloudErr.Code).To(Equal(api.CloudErrorCodeForbidden))
+}
+
 func testConfigMapDeleteOK(objName, namespace string) {
 	By("deleting the object via RP admin API")
 	params := url.Values{
 		"kind":      []string{"configmap"},
 		"namespace": []string{namespace},
 		"name":      []string{objName},
+		"force":     []string{"false"},
 	}
 	resp, err := adminRequest(context.Background(), http.MethodDelete, "/admin"+resourceIDFromEnv()+"/kubernetesobjects", params, nil, nil)
 	Expect(err).NotTo(HaveOccurred())
@@ -271,6 +297,7 @@ func testConfigMapDeleteForbidden(objName, namespace string) {
 		"kind":      []string{"configmap"},
 		"namespace": []string{namespace},
 		"name":      []string{objName},
+		"force":     []string{"false"},
 	}
 	var cloudErr api.CloudError
 	resp, err := adminRequest(context.Background(), http.MethodDelete, "/admin"+resourceIDFromEnv()+"/kubernetesobjects", params, nil, &cloudErr)
@@ -279,6 +306,49 @@ func testConfigMapDeleteForbidden(objName, namespace string) {
 	Expect(cloudErr.Code).To(Equal(api.CloudErrorCodeForbidden))
 }
 
+func testPodCreateOK(containerName, objName, namespace string) {
+	By("creating a new pod via RP admin API")
+	obj := mockPod(containerName, objName, namespace)
+	resp, err := adminRequest(context.Background(), http.MethodPost, "/admin"+resourceIDFromEnv()+"/kubernetesobjects", nil, obj, nil)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(resp.StatusCode).To(Equal(http.StatusOK))
+
+	By("checking that the pod was created via Kubernetes API")
+	pod, err := clients.Kubernetes.CoreV1().Pods(namespace).Get(context.Background(), objName, metav1.GetOptions{})
+	Expect(err).NotTo(HaveOccurred())
+	Expect(obj.Namespace).To(Equal(pod.Namespace))
+	Expect(obj.Name).To(Equal(pod.Name))
+	Expect(obj.Spec.Containers[0].Name).To(Equal(pod.Spec.Containers[0].Name))
+}
+
+func testPodForceDeleteOK(objName, namespace string) {
+	By("deleting the object via RP admin API")
+	params := url.Values{
+		"kind":      []string{"pod"},
+		"namespace": []string{namespace},
+		"name":      []string{objName},
+		"force":     []string{"true"},
+	}
+	resp, err := adminRequest(context.Background(), http.MethodDelete, "/admin"+resourceIDFromEnv()+"/kubernetesobjects", params, nil, nil)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(resp.StatusCode).To(Equal(http.StatusOK))
+
+	// To avoid flakes, we need it to be completely deleted before we can use it again
+	// in a separate run or in a separate It block
+	By("waiting for the pod to be deleted")
+	err = wait.PollImmediate(10*time.Second, time.Minute, func() (bool, error) {
+		_, err = clients.Kubernetes.CoreV1().Pods(namespace).Get(context.Background(), objName, metav1.GetOptions{})
+		if kerrors.IsNotFound(err) {
+			return true, nil
+		}
+		if err != nil {
+			log.Warn(err)
+		}
+		return false, nil // swallow error
+	})
+	Expect(err).NotTo(HaveOccurred())
+}
+
 func mockSecret(name, namespace string) corev1.Secret {
 	return corev1.Secret{
 		TypeMeta: metav1.TypeMeta{
diff --git a/test/e2e/operator.go b/test/e2e/operator.go
index aa56d37f4..bbfa07ec3 100644
--- a/test/e2e/operator.go
+++ b/test/e2e/operator.go
@@ -31,6 +31,7 @@ import (
 
 	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
 	"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"
@@ -433,10 +434,20 @@ var _ = Describe("ARO Operator - ImageConfig Reconciler", func() {
 	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
+	// 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 {
@@ -476,7 +487,9 @@ var _ = Describe("ARO Operator - ImageConfig Reconciler", func() {
 
 		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)
+
+		requiredRegistries, err = getCloudAwareRegistries(instance)
+		Expect(err).NotTo(HaveOccurred())
 	})
 
 	AfterEach(func() {