Merge branch 'master' into context-common-gh-rest-clients

cmd/webhook/main.go

@@ -102,7 +102,7 @@ func main() {
 	rolebindingValidator := &validatingwebhook.RoleBindingRequestValidator{
 		Client: cl,
 	}
-	checlusterValidator := &validatingwebhook.CheClusterRequestValidator{
+	k8sImagePullerRequestValidator := &validatingwebhook.K8sImagePullerRequestValidator{
 		Client: cl,
 	}
 	spacebindingrequestValidator := &validatingwebhook.SpaceBindingRequestValidator{
@@ -113,7 +113,7 @@ func main() {
 	mux.HandleFunc("/mutate-users-pods", mutatingwebhook.HandleMutateUserPods)
 	mux.HandleFunc("/mutate-virtual-machines", mutatingwebhook.HandleMutateVirtualMachines)
 	mux.HandleFunc("/validate-users-rolebindings", rolebindingValidator.HandleValidate)
-	mux.HandleFunc("/validate-users-checlusters", checlusterValidator.HandleValidate)
+	mux.HandleFunc("/validate-users-kubernetesimagepullers", k8sImagePullerRequestValidator.HandleValidate)
 	mux.HandleFunc("/validate-spacebindingrequests", spacebindingrequestValidator.HandleValidate)
 
 	webhookServer := &http.Server{ //nolint:gosec //TODO: configure ReadHeaderTimeout (gosec G112)

controllers/idler/idler_controller_test.go

@@ -671,7 +671,7 @@ func TestAppNameTypeForControllers(t *testing.T) {
 			}()
 
 			//when
-			appType, appName, deletedByController, err := reconciler.scaleControllerToZero(logf.FromContext(context.TODO()), p.ObjectMeta)
+			appType, appName, deletedByController, err := reconciler.scaleControllerToZero(context.TODO(), p.ObjectMeta)
 
 			//then
 			require.NoError(t, err)
@@ -814,7 +814,7 @@ func TestCreateNotification(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
 
 		//when
-		err := reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err := reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 		//then
 		require.NoError(t, err)
 		require.True(t, condition.IsTrue(idler.Status.Conditions, toolchainv1alpha1.IdlerTriggeredNotificationCreated))
@@ -827,7 +827,7 @@ func TestCreateNotification(t *testing.T) {
 
 		t.Run("Notification not created if already sent", func(t *testing.T) {
 			//when
-			err = reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+			err = reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 			//then
 			require.NoError(t, err)
 			err = hostCl.Client.Get(context.TODO(), types.NamespacedName{Name: "alex-stage-idled", Namespace: hostCl.OperatorNamespace}, &notification)
@@ -852,7 +852,7 @@ func TestCreateNotification(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
 
 		//when
-		err := reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err := reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 		//then
 		require.NoError(t, err)
 		require.True(t, condition.IsTrue(idler.Status.Conditions, toolchainv1alpha1.IdlerTriggeredNotificationCreated))
@@ -871,7 +871,7 @@ func TestCreateNotification(t *testing.T) {
 			return errors.New("can't update condition")
 		}
 		//when
-		err := reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err := reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 
 		//then
 		require.EqualError(t, err, "can't update condition")
@@ -882,7 +882,7 @@ func TestCreateNotification(t *testing.T) {
 
 		// second reconcile will not create the notification again but set the status
 		cl.MockStatusUpdate = nil
-		err = reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err = reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 		require.NoError(t, err)
 		require.True(t, condition.IsTrue(idler.Status.Conditions, toolchainv1alpha1.IdlerTriggeredNotificationCreated))
 	})
@@ -895,7 +895,7 @@ func TestCreateNotification(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet)
 
 		//when
-		err := reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err := reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 		//then
 		require.EqualError(t, err, "could not get the MUR: masteruserrecords.toolchain.dev.openshift.com \"alex\" not found")
 	})
@@ -910,7 +910,7 @@ func TestCreateNotification(t *testing.T) {
 		delete(mur.Annotations, toolchainv1alpha1.MasterUserRecordEmailAnnotationKey)
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
 		//when
-		err := reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err := reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 		require.EqualError(t, err, "no email found for the user in MURs")
 	})
 
@@ -923,7 +923,7 @@ func TestCreateNotification(t *testing.T) {
 		mur.Annotations[toolchainv1alpha1.MasterUserRecordEmailAnnotationKey] = "invalid-email-address"
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
 		//when
-		err := reconciler.createNotification(logf.FromContext(context.TODO()), idler, "testPodName", "testapptype")
+		err := reconciler.createNotification(context.TODO(), idler, "testPodName", "testapptype")
 		require.EqualError(t, err, "unable to create Notification CR from Idler: The specified recipient [invalid-email-address] is not a valid email address: mail: missing '@' or angle-addr")
 	})
 }
@@ -949,7 +949,7 @@ func TestGetUserEmailFromMUR(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
 		hostCluster, _ := reconciler.GetHostCluster()
 		//when
-		emails, err := reconciler.getUserEmailsFromMURs(logf.FromContext(context.TODO()), hostCluster, idler)
+		emails, err := reconciler.getUserEmailsFromMURs(context.TODO(), hostCluster, idler)
 		//then
 		require.NoError(t, err)
 		require.NotEmpty(t, emails)
@@ -968,7 +968,7 @@ func TestGetUserEmailFromMUR(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur, mur2, mur3)
 		hostCluster, _ := reconciler.GetHostCluster()
 		//when
-		emails, err := reconciler.getUserEmailsFromMURs(logf.FromContext(context.TODO()), hostCluster, idler)
+		emails, err := reconciler.getUserEmailsFromMURs(context.TODO(), hostCluster, idler)
 		//then
 		require.NoError(t, err)
 		require.NotEmpty(t, emails)
@@ -983,7 +983,7 @@ func TestGetUserEmailFromMUR(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler)
 		hostCluster, _ := reconciler.GetHostCluster()
 		//when
-		emails, err := reconciler.getUserEmailsFromMURs(logf.FromContext(context.TODO()), hostCluster, idler)
+		emails, err := reconciler.getUserEmailsFromMURs(context.TODO(), hostCluster, idler)
 		//then
 		require.EqualError(t, err, "nstemplatesets.toolchain.dev.openshift.com \"alex\" not found")
 		require.Len(t, emails, 0)
@@ -997,7 +997,7 @@ func TestGetUserEmailFromMUR(t *testing.T) {
 		reconciler, _, _, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet)
 		hostCluster, _ := reconciler.GetHostCluster()
 		//when
-		emails, err := reconciler.getUserEmailsFromMURs(logf.FromContext(context.TODO()), hostCluster, idler)
+		emails, err := reconciler.getUserEmailsFromMURs(context.TODO(), hostCluster, idler)
 		//then
 		require.Error(t, err)
 		require.Len(t, emails, 0)

deploy/webhook/member-operator-webhook.yaml

@@ -218,22 +218,26 @@ objects:
       namespaceSelector:
         matchLabels:
           toolchain.dev.openshift.com/provider: codeready-toolchain
-    - name: users.checlusters.webhook.sandbox
+    # The users.kubernetesimagepullers.webhook.sandbox validation webhook ensures that KubernetesImagePuller CRs cannot be created by a sandbox user.
+    # This webhook is needed to prevent user-created KubernetesImagePuller CRs from interfering with the devworkspace-controller-manager-* pod, as high memory
+    # usage was previously observed.
+    # The webhook code is available at member-operator/pkg/webhook/validatingwebhook/validate_k8simagepuller_request.go
+    - name: users.kubernetesimagepullers.webhook.sandbox
       admissionReviewVersions:
         - v1
       clientConfig:
         caBundle: ${CA_BUNDLE}
         service:
           name: member-operator-webhook
           namespace: ${NAMESPACE}
-          path: "/validate-users-checlusters"
+          path: "/validate-users-kubernetesimagepullers"
           port: 443
       matchPolicy: Equivalent
       rules:
         - operations: ["CREATE"]
-          apiGroups: ["org.eclipse.che"]
-          apiVersions: ["v2"]
-          resources: ["checlusters"]
+          apiGroups: ["che.eclipse.org"]
+          apiVersions: ["v1alpha1"]
+          resources: ["kubernetesimagepullers"]
           scope: "Namespaced"
       sideEffects: None
       timeoutSeconds: 5

pkg/webhook/deploy/deployment_test.go

@@ -227,7 +227,8 @@ 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"},"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-users-checlusters","port":443}},"failurePolicy":"Fail","matchPolicy":"Equivalent","name":"users.checlusters.webhook.sandbox","namespaceSelector":{"matchLabels":{"toolchain.dev.openshift.com/provider":"codeready-toolchain"}},"reinvocationPolicy":"Never","rules":[{"apiGroups":["org.eclipse.che"],"apiVersions":["v2"],"operations":["CREATE"],"resources":["checlusters"],"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}]}`, 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"},"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-users-kubernetesimagepullers","port": 443}},"failurePolicy": "Fail","matchPolicy": "Equivalent","name": "users.kubernetesimagepullers.webhook.sandbox","namespaceSelector": {"matchLabels": {"toolchain.dev.openshift.com/provider": "codeready-toolchain"}},"reinvocationPolicy": "Never","rules": [{"apiGroups": ["che.eclipse.org"],"apiVersions": ["v1alpha1"],"operations": ["CREATE"],"resources": ["kubernetesimagepullers"],"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}]}`, caBundle, namespace)
 }
 
 func serviceAccount(namespace string) string {

pkg/webhook/validatingwebhook/validate_checluster_request.go → pkg/webhook/validatingwebhook/validate_k8simagepuller_request.go

@@ -5,7 +5,6 @@ import (
 	"html"
 	"io"
 	"net/http"
-	"strings"
 
 	toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
 
@@ -16,11 +15,11 @@ import (
 	runtimeClient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-type CheClusterRequestValidator struct {
+type K8sImagePullerRequestValidator struct {
 	Client runtimeClient.Client
 }
 
-func (v CheClusterRequestValidator) HandleValidate(w http.ResponseWriter, r *http.Request) {
+func (v K8sImagePullerRequestValidator) HandleValidate(w http.ResponseWriter, r *http.Request) {
 	var respBody []byte
 	body, err := io.ReadAll(r.Body)
 	defer func() {
@@ -42,7 +41,7 @@ func (v CheClusterRequestValidator) HandleValidate(w http.ResponseWriter, r *htt
 	}
 }
 
-func (v CheClusterRequestValidator) validate(body []byte) []byte {
+func (v K8sImagePullerRequestValidator) validate(body []byte) []byte {
 	log.Info("incoming request", "body", string(body))
 	admReview := admissionv1.AdmissionReview{}
 	if _, _, err := deserializer.Decode(body, nil, &admReview); err != nil {
@@ -51,24 +50,19 @@ func (v CheClusterRequestValidator) validate(body []byte) []byte {
 		log.Error(err, "unable to deserialize the admission review object", "body", escapedBody)
 		return denyAdmissionRequest(admReview, errors.Wrapf(err, "unable to deserialize the admission review object - body: %v", escapedBody))
 	}
-	requestingUsername := admReview.Request.UserInfo.Username
-	// allow admission request if the user is a system user
-	if strings.HasPrefix(requestingUsername, "system:") {
-		return allowAdmissionRequest(admReview)
-	}
 	//check if the requesting user is a sandbox user
 	requestingUser := &userv1.User{}
 	err := v.Client.Get(context.TODO(), types.NamespacedName{
 		Name: admReview.Request.UserInfo.Username,
 	}, requestingUser)
 
 	if err != nil {
-		log.Error(err, "unable to find the user requesting creation of the CheCluster resource", "username", admReview.Request.UserInfo.Username)
-		return denyAdmissionRequest(admReview, errors.New("unable to find the user requesting the  creation of the CheCluster resource"))
+		log.Error(err, "unable to find the user requesting creation of the KubernetesImagePuller resource", "username", admReview.Request.UserInfo.Username)
+		return denyAdmissionRequest(admReview, errors.New("unable to find the user requesting the  creation of the KubernetesImagePuller resource"))
 	}
 	if requestingUser.GetLabels()[toolchainv1alpha1.ProviderLabelKey] == toolchainv1alpha1.ProviderLabelValue {
-		log.Info("sandbox user is trying to create a CheCluster", "AdmissionReview", admReview)
-		return denyAdmissionRequest(admReview, errors.New("this is a Dev Sandbox enforced restriction. you are trying to create a CheCluster resource, which is not allowed"))
+		log.Info("sandbox user is trying to create a KubernetesImagePuller", "AdmissionReview", admReview)
+		return denyAdmissionRequest(admReview, errors.New("this is a Dev Sandbox enforced restriction. you are trying to create a KubernetesImagePuller resource, which is not allowed"))
 	}
 	// at this point, it is clear the user isn't a sandbox user, allow request
 	return allowAdmissionRequest(admReview)