Update VM webhooks for 4.18 (codeready-toolchain#643)

Co-authored-by: Rajiv Senthilnathan <rsenthil@redhat.com>

Author: MatousJobanek

deploy/webhook/member-operator-webhook.yaml

@@ -293,7 +293,7 @@ objects:
           port: 443
       matchPolicy: Equivalent
       rules:
-        - operations: ["CREATE", "UPDATE"]
+        - operations: ["UPDATE"]
           apiGroups: ["kubevirt.io"]
           apiVersions: ["*"]
           resources: ["virtualmachines"]

pkg/webhook/deploy/deployment_test.go

@@ -227,7 +227,7 @@ func mutatingWebhookConfig(namespace, caBundle string) string {
 }
 
 func validatingWebhookConfig(namespace, caBundle string) string {
-	return fmt.Sprintf(`{"apiVersion": "admissionregistration.k8s.io/v1","kind": "ValidatingWebhookConfiguration","metadata": {"labels": {"app": "member-operator-webhook","toolchain.dev.openshift.com/provider": "codeready-toolchain"},"name": "member-operator-validating-webhook-%[2]s"},"webhooks": [{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-users-rolebindings","port": 443}},"failurePolicy": "Ignore","matchPolicy": "Equivalent","name": "users.rolebindings.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["rbac.authorization.k8s.io","authorization.openshift.io"],"apiVersions": ["v1"],"operations": ["CREATE","UPDATE"],"resources": ["rolebindings"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5},{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-spacebindingrequests","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.spacebindingrequests.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["toolchain.dev.openshift.com"],"apiVersions": ["v1alpha1"],"operations": ["CREATE","UPDATE"],"resources": ["spacebindingrequests"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5},{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-ssprequests","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.virtualmachines.ssp.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["ssp.kubevirt.io"],"apiVersions": ["*"],"operations": ["CREATE","UPDATE"],"resources": ["ssps"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5},{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-vmrequests","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.virtualmachines.validating.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["kubevirt.io"],"apiVersions": ["*"],"operations": ["CREATE","UPDATE"],"resources": ["virtualmachines"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5}]}`, caBundle, namespace)
+	return fmt.Sprintf(`{"apiVersion": "admissionregistration.k8s.io/v1","kind": "ValidatingWebhookConfiguration","metadata": {"labels": {"app": "member-operator-webhook","toolchain.dev.openshift.com/provider": "codeready-toolchain"},"name": "member-operator-validating-webhook-%[2]s"},"webhooks": [{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-users-rolebindings","port": 443}},"failurePolicy": "Ignore","matchPolicy": "Equivalent","name": "users.rolebindings.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["rbac.authorization.k8s.io","authorization.openshift.io"],"apiVersions": ["v1"],"operations": ["CREATE","UPDATE"],"resources": ["rolebindings"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5},{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-spacebindingrequests","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.spacebindingrequests.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["toolchain.dev.openshift.com"],"apiVersions": ["v1alpha1"],"operations": ["CREATE","UPDATE"],"resources": ["spacebindingrequests"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5},{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-ssprequests","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.virtualmachines.ssp.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["ssp.kubevirt.io"],"apiVersions": ["*"],"operations": ["CREATE","UPDATE"],"resources": ["ssps"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5},{"admissionReviewVersions": ["v1"],"clientConfig": {"caBundle": "%[1]s","service": {"name": "member-operator-webhook","namespace": "%[2]s","path": "/validate-vmrequests","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.virtualmachines.validating.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["kubevirt.io"],"apiVersions": ["*"],"operations": ["UPDATE"],"resources": ["virtualmachines"],"scope": "Namespaced"}],"sideEffects": "None","timeoutSeconds": 5}]}`, caBundle, namespace)
 }
 
 func serviceAccount(namespace string) string {

pkg/webhook/mutatingwebhook/vm_mutate.go

@@ -26,6 +26,8 @@ var sandboxToleration = map[string]interface{}{
 	"operator": "Exists",
 }
 
+const manualRunStrategy = "Manual"
+
 func HandleMutateVirtualMachines(w http.ResponseWriter, r *http.Request) {
 	handleMutate(vmLogger, w, r, vmMutator)
 }
@@ -50,6 +52,9 @@ func vmMutator(admReview admissionv1.AdmissionReview) *admissionv1.AdmissionResp
 	// instead of changing the object we need to tell K8s how to change the object via patch
 	vmPatchItems := []map[string]interface{}{}
 
+	// ensure runStrategy is set to Manual, see https://docs.redhat.com/en/documentation/openshift_container_platform/4.11/html/virtualization/virtual-machines#virt-about-runstrategies-vms_virt-create-vms
+	vmPatchItems = ensureManualRunStrategy(unstructuredRequestObj, vmPatchItems)
+
 	// ensure limits are set in a best effort approach, if the limits are not set for any reason the request will still be allowed
 	vmPatchItems = ensureLimits(unstructuredRequestObj, vmPatchItems)
 
@@ -254,6 +259,22 @@ func ensureLimits(unstructuredObj *unstructured.Unstructured, patchItems []map[s
 	return patchItems
 }
 
+// ensureManualRunStrategy ensures the VM's RunStrategy is set to Manual
+func ensureManualRunStrategy(unstructuredRequestObj *unstructured.Unstructured, patchItems []map[string]interface{}) []map[string]interface{} {
+	// get existing runStrategy, if any
+	runStrategy, _, err := unstructured.NestedString(unstructuredRequestObj.Object, "spec", "runStrategy")
+	if err != nil {
+		vmLogger.Error(err, "unable to get runStrategy from VirtualMachine", "VirtualMachine", unstructuredRequestObj)
+		return patchItems
+	}
+
+	if runStrategy != manualRunStrategy {
+		patchItems = append(patchItems, addManualRunStrategy())
+	}
+
+	return patchItems
+}
+
 // ensureTolerations ensures tolerations are set on the VirtualMachine
 func ensureTolerations(unstructuredRequestObj *unstructured.Unstructured, patchItems []map[string]interface{}) []map[string]interface{} {
 	// get existing tolerations, if any
@@ -309,3 +330,11 @@ func addTolerations(tolerations []interface{}) map[string]interface{} {
 		"value": tolerations,
 	}
 }
+
+func addManualRunStrategy() map[string]interface{} {
+	return map[string]interface{}{
+		"op":    "add",
+		"path":  "/spec/runStrategy",
+		"value": manualRunStrategy,
+	}
+}

pkg/webhook/mutatingwebhook/vm_mutate_test.go

@@ -38,12 +38,12 @@ func TestVMMutator(t *testing.T) {
 		actualResponse := vmMutator(admReview)
 
 		// then
-		expectedPatches := []map[string]interface{}{expectedTolerationsPatch, expectedVolumesPatch}
+		expectedPatches := []map[string]interface{}{addManualRunStrategy(), expectedTolerationsPatch, expectedVolumesPatch}
 		actualPatchContent := actualResponse.Patch
 		expectedPatchContent, err := json.Marshal(expectedPatches)
 		require.NoError(t, err)
 		require.Equal(t, string(expectedPatchContent), string(actualPatchContent))
-		assert.Equal(t, expectedVMMutateRespSuccess(t, expectedTolerationsPatch, expectedVolumesPatch), *actualResponse)
+		assert.Equal(t, expectedVMMutateRespSuccess(t, addManualRunStrategy(), expectedTolerationsPatch, expectedVolumesPatch), *actualResponse)
 	})
 
 	t.Run("fail", func(t *testing.T) {
@@ -454,6 +454,36 @@ func TestEnsureTolerations(t *testing.T) {
 	})
 }
 
+func TestEnsureManualRunStrategy(t *testing.T) {
+
+	t.Run("success", func(t *testing.T) {
+		t.Run("no existing runStrategy", func(t *testing.T) {
+			// given
+			vmAdmReviewRequestObj := vmAdmReviewRequestObject(t)
+			// expect only sandbox toleration
+			expectedPatchItems := []map[string]interface{}{addManualRunStrategy()}
+
+			// when
+			actualPatchItems := ensureManualRunStrategy(vmAdmReviewRequestObj, []map[string]interface{}{})
+
+			// then
+			assertPatchesEqual(t, expectedPatchItems, actualPatchItems)
+		})
+
+		t.Run("runStrategy already set", func(t *testing.T) {
+			// given
+			vmAdmReviewRequestObj := vmAdmReviewRequestObject(t, setRunStrategy("Always")) // it doesn't matter what runStrategy is set, the patch is always applied since it's an 'add' JSON patch
+			expectedPatchItems := []map[string]interface{}{addManualRunStrategy()}
+
+			// when
+			actualPatchItems := ensureManualRunStrategy(vmAdmReviewRequestObj, []map[string]interface{}{})
+
+			// then
+			assertPatchesEqual(t, expectedPatchItems, actualPatchItems)
+		})
+	})
+}
+
 type admissionReviewOption func(t *testing.T, unstructuredAdmReview *unstructured.Unstructured)
 
 func setDomainResourcesRequests(requests map[string]string) admissionReviewOption {
@@ -548,6 +578,13 @@ func cloudInitVolume(cicType cloudInitConfigType, userData string) map[string]in
 	return volume
 }
 
+func setRunStrategy(runStrategy string) admissionReviewOption {
+	return func(t *testing.T, unstructuredAdmReview *unstructured.Unstructured) {
+		err := unstructured.SetNestedField(unstructuredAdmReview.Object, runStrategy, "request", "object", "spec", "runStrategy")
+		require.NoError(t, err)
+	}
+}
+
 func vmAdmReviewRequestObject(t *testing.T, options ...admissionReviewOption) *unstructured.Unstructured {
 	return admReviewRequestObject(t, vmRawAdmissionReviewJSONTemplate, options...)
 }

pkg/webhook/validatingwebhook/validate_vm_request.go

@@ -11,6 +11,8 @@ import (
 	runtimeClient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+const manualRunStrategy = "Manual"
+
 type VMRequestValidator struct {
 	Client runtimeClient.Client
 }
@@ -53,24 +55,15 @@ func (v VMRequestValidator) validate(body []byte) []byte {
 		return denyAdmissionRequest(admReview, errors.New("failed to validate VirtualMachine request"))
 	}
 
-	hasRunStrategy, err := hasRunningStrategy(unstructuredRequestObj)
+	runStrategy, hasRunStrategy, err := unstructured.NestedString(unstructuredRequestObj.Object, "spec", "runStrategy")
 	if err != nil {
 		log.Error(err, "failed to unmarshal VirtualMachine json object", "AdmissionReview", admReview)
 		return denyAdmissionRequest(admReview, errors.New("failed to validate VirtualMachine request"))
 	}
-	if hasRunStrategy {
-		log.Info("sandbox user is trying to create a VM with RunStrategy configured", "AdmissionReview", admReview) // not allowed because it interferes with the Dev Sandbox Idler
-		return denyAdmissionRequest(admReview, errors.New("this is a Dev Sandbox enforced restriction. Configuring RunStrategy is not allowed"))
+	if hasRunStrategy && runStrategy != manualRunStrategy {
+		log.Info("sandbox user is trying to configure a VM with RunStrategy set to a value other than Manual", "AdmissionReview", admReview) // other run strategies are not allowed because it conflicts with the Dev Sandbox Idler
+		return denyAdmissionRequest(admReview, errors.New("this is a Dev Sandbox enforced restriction. Only 'Manual' RunStrategy is permitted"))
 	}
-	// the user is not creating a VM with the 'runStrategy' configured, allowing the request.
+	// the user is configuring a VM without the 'runStrategy' configured or with it configured to Manual, allowing the request.
 	return allowAdmissionRequest(admReview)
 }
-
-func hasRunningStrategy(unstructuredObj *unstructured.Unstructured) (bool, error) {
-	_, runStrategyFound, err := unstructured.NestedString(unstructuredObj.Object, "spec", "runStrategy")
-	if err != nil {
-		return runStrategyFound, err
-	}
-
-	return runStrategyFound, nil
-}

pkg/webhook/validatingwebhook/validate_vm_request_test.go

@@ -2,6 +2,7 @@ package validatingwebhook
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -23,9 +24,9 @@ func TestHandleValidateVMAdmissionRequestBlocked(t *testing.T) {
 	ts := httptest.NewServer(http.HandlerFunc(v.HandleValidate))
 	defer ts.Close()
 
-	t.Run("sandbox user trying to create a VM resource with RunStrategy is denied", func(t *testing.T) {
+	t.Run("sandbox user trying to update a VM resource with RunStrategy other than 'Manual' is denied", func(t *testing.T) {
 		// when
-		resp, err := http.Post(ts.URL, "application/json", bytes.NewBuffer(newCreateVMAdmissionRequest(t, VMAdmReviewTmplParams{"CREATE", "johnsmith"}, createVMWithRunStrategyJSONTmpl)))
+		resp, err := http.Post(ts.URL, "application/json", bytes.NewBuffer(newCreateVMAdmissionRequest(t, VMAdmReviewTmplParams{"UPDATE", "johnsmith"}, createVMWithRunStrategyJSONTmpl("Always"))))
 
 		// then
 		require.NoError(t, err)
@@ -34,12 +35,12 @@ func TestHandleValidateVMAdmissionRequestBlocked(t *testing.T) {
 			require.NoError(t, resp.Body.Close())
 		}()
 		require.NoError(t, err)
-		test.VerifyRequestBlocked(t, body, "this is a Dev Sandbox enforced restriction. Configuring RunStrategy is not allowed", "b6ae2ab4-782b-11ee-b962-0242ac120002")
+		test.VerifyRequestBlocked(t, body, "this is a Dev Sandbox enforced restriction. Only 'Manual' RunStrategy is permitted", "b6ae2ab4-782b-11ee-b962-0242ac120002")
 	})
 
-	t.Run("sandbox user trying to update a VM resource with RunStrategy is denied", func(t *testing.T) {
+	t.Run("sandbox user trying to update a VM resource with RunStrategy 'Manual' is allowed", func(t *testing.T) {
 		// when
-		resp, err := http.Post(ts.URL, "application/json", bytes.NewBuffer(newCreateVMAdmissionRequest(t, VMAdmReviewTmplParams{"UPDATE", "johnsmith"}, createVMWithRunStrategyJSONTmpl)))
+		resp, err := http.Post(ts.URL, "application/json", bytes.NewBuffer(newCreateVMAdmissionRequest(t, VMAdmReviewTmplParams{"UPDATE", "johnsmith"}, createVMWithRunStrategyJSONTmpl("Manual"))))
 
 		// then
 		require.NoError(t, err)
@@ -48,25 +49,10 @@ func TestHandleValidateVMAdmissionRequestBlocked(t *testing.T) {
 			require.NoError(t, resp.Body.Close())
 		}()
 		require.NoError(t, err)
-		test.VerifyRequestBlocked(t, body, "this is a Dev Sandbox enforced restriction. Configuring RunStrategy is not allowed", "b6ae2ab4-782b-11ee-b962-0242ac120002")
-	})
-
-	t.Run("sandbox user trying to create a VM resource without RunStrategy is allowed", func(t *testing.T) {
-		// when
-		resp, err := http.Post(ts.URL, "application/json", bytes.NewBuffer(newCreateVMAdmissionRequest(t, VMAdmReviewTmplParams{"CREATE", "johnsmith"}, createVMWithoutRunStrategyJSONTmpl)))
-
-		// then
-		require.NoError(t, err)
-		body, err := io.ReadAll(resp.Body)
-		defer func() {
-			require.NoError(t, resp.Body.Close())
-		}()
-		require.NoError(t, err)
-
 		test.VerifyRequestAllowed(t, body, "b6ae2ab4-782b-11ee-b962-0242ac120002")
 	})
 
-	t.Run("sandbox user trying to update a VM resource without RunStrategy is allowed", func(t *testing.T) {
+	t.Run("sandbox user trying to update a VM resource without RunStrategy is allowed", func(t *testing.T) { // note: RunStrategy can be removed as it's not needed (it's automatically set upon creation)
 		// when
 		resp, err := http.Post(ts.URL, "application/json", bytes.NewBuffer(newCreateVMAdmissionRequest(t, VMAdmReviewTmplParams{"UPDATE", "johnsmith"}, createVMWithoutRunStrategyJSONTmpl)))
 
@@ -80,7 +66,6 @@ func TestHandleValidateVMAdmissionRequestBlocked(t *testing.T) {
 
 		test.VerifyRequestAllowed(t, body, "b6ae2ab4-782b-11ee-b962-0242ac120002")
 	})
-
 }
 
 func newVMRequestValidator(t *testing.T) *VMRequestValidator {
@@ -114,7 +99,8 @@ type VMAdmReviewTmplParams struct {
 	Username string
 }
 
-var createVMWithRunStrategyJSONTmpl = `{
+func createVMWithRunStrategyJSONTmpl(runStrategy string) string {
+	return fmt.Sprintf(`{
     "kind": "AdmissionReview",
     "apiVersion": "admission.k8s.io/v1",
     "request": {
@@ -156,7 +142,7 @@ var createVMWithRunStrategyJSONTmpl = `{
                 "namespace": "{{.Username}}-dev"
             },
             "spec": {
-                "runStrategy": "Always"
+                "runStrategy": "%s"
             }
         },
         "oldObject": null,
@@ -168,7 +154,8 @@ var createVMWithRunStrategyJSONTmpl = `{
             "fieldValidation": "Ignore"
         }
     }
-}`
+}`, runStrategy)
+}
 
 var createVMWithoutRunStrategyJSONTmpl = `{
     "kind": "AdmissionReview",