Repository: Azure/ARO-RP
Pull request #2114 (2022-05-04 21:31:11)

Add Force Delete Parameter to Delete Kubernetes Object Admin Endpoint for New Geneva Action
This PR adds a force parameter to the delete kubernetes object admin endpoint that can be used to force delete pods, and only pods. New parameter will be leveraged by an updated Geneva Action (Phase 3)

### Which issue this PR addresses:

[USER STORY 14336714 - Modify Kube Delete Admin Endpoint to Accept Force Parameter for Pods](https://dev.azure.com/msazure/AzureRedHatOpenShift/_workitems/edit/14336714)

### What this PR does / why we need it:

This PR adds a force parameter to the delete kubernetes object admin endpoint that can be used to force delete pods, and only pods. This new parameter will eventually be used by an updated Geneva Action (Phase 3). Functionality was requested by ARO support.

### Test plan for issue:

Unit tests have been added for the new parameter. I also ran the RP locally, created a cluster, and manually curled the "kubernetesObjects" endpoint to confirm...
- the force delete of a pod
- a normal delete of a pod
- the denial of a force delete for any other resource
- the default delete is not a force delete

### Is there any documentation that needs to be updated for this PR?

No - once the Geneva Action frontend has been updated, that documentation will be updated to include this parameter and its functionality.

zacharyljones: had to update unit test namespaces - "openshift-project" is no restricted.

go lint does fail but it is unrelated to the changes in this PR
petrkotas: /azp run ci
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
zacharyljones: @ross-bryan @bennerv @rogbas I added e2e tests specifically for this change - Is this sufficient or would you still like a demo?
zacharyljones: > I added e2e tests specifically for this change - Is this sufficient or would you still like a demo?

@ross-bryan @bennerv Demo has been uploaded here: https://microsoft.sharepoint.com/:v:/t/AzureRedHatOpenShift/EYJnF9rDchRHu0bYs07gmjMBsOElPL9qTeCaZuZ7BwM7Pg?e=sgQ4Ne
github-actions[bot]: Please rebase pull request.
zacharyljones: @hawkowl rebased to resolve conflicts with recent admin api changes.

Patch:
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/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{