feat(plugins) Deny plugin creation by user (#829)

e2e/plugin/e2e_test.go

@@ -70,11 +70,24 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 	It("should have a cluster resource created", func() {
 		By("verifying if the cluster resource is created")
 		Eventually(func(g Gomega) {
-			err := adminClient.Get(ctx, client.ObjectKey{Name: remoteClusterName, Namespace: env.TestNamespace}, &greenhousev1alpha1.Cluster{})
+			var remoteCluster greenhousev1alpha1.Cluster
+			err := adminClient.Get(ctx, client.ObjectKey{Name: remoteClusterName, Namespace: env.TestNamespace}, &remoteCluster)
+			g.Expect(err).ToNot(HaveOccurred())
+			if remoteCluster.Labels == nil {
+				remoteCluster.Labels = map[string]string{}
+			}
+			remoteCluster.Labels["app"] = "test"
+			err = adminClient.Update(ctx, &remoteCluster)
 			g.Expect(err).ToNot(HaveOccurred())
-
 		}).Should(Succeed(), "cluster resource should be created")
 
+		Eventually(func(g Gomega) {
+			var remoteCluster greenhousev1alpha1.Cluster
+			err := adminClient.Get(ctx, client.ObjectKey{Name: remoteClusterName, Namespace: env.TestNamespace}, &remoteCluster)
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(remoteCluster.Labels["app"]).To(Equal("test"))
+		}).Should(Succeed(), "cluster resource should be updated")
+
 		By("verifying the cluster status is ready")
 		shared.ClusterIsReady(ctx, adminClient, remoteClusterName, env.TestNamespace)
 	})
@@ -119,6 +132,7 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 			g.Expect(len(pluginList.Items)).To(BeEquivalentTo(1))
 			g.Expect(pluginList.Items[0].Status.HelmReleaseStatus).ToNot(BeNil())
 			g.Expect(pluginList.Items[0].Status.HelmReleaseStatus.Status).To(BeEquivalentTo("deployed"))
+			testPlugin = &pluginList.Items[0]
 		}).Should(Succeed())
 
 		By("Checking deployment")
@@ -161,8 +175,29 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 			}
 		}).Should(Succeed())
 
+		By("Add annotation to allow delete plugin preset")
+		Eventually(func(g Gomega) {
+			err = adminClient.Get(ctx, client.ObjectKeyFromObject(testPluginPreset), testPluginPreset)
+			g.Expect(err).NotTo(HaveOccurred())
+			if testPluginPreset.Annotations == nil {
+				testPluginPreset.Annotations = map[string]string{}
+			}
+			delete(testPluginPreset.Annotations, "greenhouse.sap/prevent-deletion")
+			err = adminClient.Update(ctx, testPluginPreset)
+			g.Expect(err).NotTo(HaveOccurred())
+		}).Should(Succeed())
+
+		By("Deleting plugin preset")
+		Eventually(func(g Gomega) {
+			err = adminClient.Delete(ctx, testPluginPreset)
+			g.Expect(err).NotTo(HaveOccurred())
+		}).Should(Succeed())
+
 		By("Deleting plugin")
-		test.EventuallyDeleted(ctx, adminClient, testPluginPreset)
+		Eventually(func(g Gomega) {
+			err = adminClient.Delete(ctx, testPlugin)
+			g.Expect(err).NotTo(HaveOccurred())
+		}).Should(Succeed())
 
 		By("Check, is deployment deleted")
 		Eventually(func(g Gomega) bool {

e2e/shared/cluster.go

@@ -31,9 +31,6 @@ func OnboardRemoteCluster(ctx context.Context, k8sClient client.Client, kubeConf
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
 			Namespace: namespace,
-			Labels: map[string]string{
-				"app": "test",
-			},
 		},
 		Type: greenhouseapis.SecretTypeKubeConfig,
 		Data: map[string][]byte{

pkg/admission/plugin_webhook_test.go

@@ -228,20 +228,31 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 	})
 
 	It("should not accept a plugin without a clusterName", func() {
-		testPlugin = test.NewPlugin(test.Ctx, "test-plugin", setup.Namespace(), test.WithPluginDefinition(testPluginDefinition.Name), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = test.NewPlugin(test.Ctx, "test-plugin", setup.Namespace(),
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 		expectClusterMustBeSetError(test.K8sClient.Create(test.Ctx, testPlugin))
 	})
 
 	It("should reject the plugin when the cluster with clusterName does not exist", func() {
 		By("creating the plugin")
-		testPlugin = test.NewPlugin(test.Ctx, "test-plugin", setup.Namespace(), test.WithPluginDefinition(testPluginDefinition.Name), test.WithCluster("non-existent-cluster"), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = test.NewPlugin(test.Ctx, "test-plugin", setup.Namespace(),
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster("non-existent-cluster"),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 
 		expectClusterNotFoundError(test.K8sClient.Create(test.Ctx, testPlugin))
 	})
 
 	It("should accept the plugin when the cluster with clusterName exists", func() {
 		By("creating the plugin")
-		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin", test.WithPluginDefinition(testPluginDefinition.Name), test.WithCluster(testCluster.Name), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin",
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster(testCluster.Name),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 
 		By("checking the label on the plugin")
 		actPlugin := &greenhousev1alpha1.Plugin{}
@@ -253,7 +264,11 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 	})
 
 	It("should reject to update a plugin when the clusterName changes", func() {
-		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin", test.WithPluginDefinition(testPluginDefinition.Name), test.WithCluster(testCluster.Name), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin",
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster(testCluster.Name),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 		testPlugin.Spec.ClusterName = "wrong-cluster-name"
 		err := test.K8sClient.Update(test.Ctx, testPlugin)
 
@@ -262,15 +277,23 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 	})
 
 	It("should reject to update a plugin when the clustername is removed", func() {
-		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin", test.WithPluginDefinition(testPluginDefinition.Name), test.WithCluster(testCluster.Name), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin",
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster(testCluster.Name),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 		testPlugin.Spec.ClusterName = ""
 		err := test.K8sClient.Update(test.Ctx, testPlugin)
 		Expect(err).To(HaveOccurred(), "there should be an error changing the plugin's clusterName")
 		Expect(err.Error()).To(ContainSubstring(validation.FieldImmutableErrorMsg))
 	})
 
 	It("should reject to update a plugin when the releaseNamespace changes", func() {
-		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin", test.WithPluginDefinition(testPluginDefinition.Name), test.WithCluster(testCluster.Name), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin",
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster(testCluster.Name),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 		testPlugin.Spec.ReleaseNamespace = "foo-bar"
 		err := test.K8sClient.Update(test.Ctx, testPlugin)
 		Expect(err).To(HaveOccurred(), "there should be an error changing the plugin's releaseNamespace")
@@ -279,7 +302,11 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 
 	It("should reject to update a plugin when the pluginDefinition changes", func() {
 		secondPluginDefinition := setup.CreatePluginDefinition(test.Ctx, "foo-bar")
-		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin", test.WithPluginDefinition(testPluginDefinition.Name), test.WithCluster(testCluster.Name), test.WithReleaseNamespace("test-namespace"))
+		testPlugin = setup.CreatePlugin(test.Ctx, "test-plugin",
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster(testCluster.Name),
+			test.WithReleaseNamespace("test-namespace"),
+			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
 
 		testPlugin.Spec.PluginDefinition = secondPluginDefinition.Name
 		err := test.K8sClient.Update(test.Ctx, testPlugin)

pkg/apis/greenhouse/v1alpha1/plugin_types.go

@@ -80,6 +80,8 @@ const (
 	HelmUninstallFailedReason ConditionReason = "HelmUninstallFailed"
 )
 
+const AllowCreateAnnotation = "greenhouse.sap/allow-create"
+
 // PluginStatus defines the observed state of Plugin
 type PluginStatus struct {
 	// HelmReleaseStatus reflects the status of the latest HelmChart release.

pkg/controllers/organization/service_proxy.go

@@ -51,6 +51,9 @@ func (r *OrganizationReconciler) reconcileServiceProxy(ctx context.Context, org
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      serviceProxyName,
 			Namespace: org.Name,
+			Annotations: map[string]string{
+				greenhousesapv1alpha1.AllowCreateAnnotation: "true",
+			},
 		},
 		Spec: greenhousesapv1alpha1.PluginSpec{
 			PluginDefinition: serviceProxyName,

pkg/controllers/plugin/helm_controller.go

@@ -106,7 +106,7 @@ func (r *HelmReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 	pluginStatus := initPluginStatus(plugin)
 	defer func() {
 		if statusErr := setPluginStatus(ctx, r.Client, plugin, pluginStatus); statusErr != nil {
-			log.FromContext(ctx).Error(statusErr, "failed to set status")
+			log.FromContext(ctx).Error(statusErr, "failed to set status", "plugin", plugin, "status", pluginStatus)
 		}
 	}()
 

pkg/controllers/plugin/pluginpreset_controller.go

@@ -169,14 +169,13 @@ func (r *PluginPresetReconciler) reconcilePluginPreset(ctx context.Context, pres
 			return err
 		}
 
-		log.FromContext(ctx).Info("======================== OOOOOOOOOO", "cluster", cluster)
 		_, err = clientutil.CreateOrPatch(ctx, r.Client, plugin, func() error {
 			// Add annotation to allow plugin creation
 			if plugin.Annotations == nil {
 				plugin.Annotations = make(map[string]string)
 			}
 
-			plugin.Annotations["greenhouse.sap/allow-create"] = "true"
+			plugin.Annotations[greenhousev1alpha1.AllowCreateAnnotation] = "true"
 
 			// Label the plugin with the managed resource label to identify it as managed by the PluginPreset.
 			plugin.SetLabels(map[string]string{greenhouseapis.LabelKeyPluginPreset: preset.Name})

pkg/controllers/plugin/remote_cluster_test.go

@@ -41,6 +41,9 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-plugindefinition",
 			Namespace: test.TestNamespace,
+			Annotations: map[string]string{
+				greenhousev1alpha1.AllowCreateAnnotation: "true",
+			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
 			ClusterName:      "test-cluster",
@@ -57,6 +60,9 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-plugin-secretref",
 			Namespace: test.TestNamespace,
+			Annotations: map[string]string{
+				greenhousev1alpha1.AllowCreateAnnotation: "true",
+			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
 			PluginDefinition: "test-plugindefinition",
@@ -80,6 +86,9 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-plugin-in-made-up-namespace",
 			Namespace: test.TestNamespace,
+			Annotations: map[string]string{
+				greenhousev1alpha1.AllowCreateAnnotation: "true",
+			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
 			PluginDefinition: testPluginDefinition.GetName(),
@@ -96,6 +105,9 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-plugin-crd",
 			Namespace: test.TestNamespace,
+			Annotations: map[string]string{
+				greenhousev1alpha1.AllowCreateAnnotation: "true",
+			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
 			ClusterName:      "test-cluster",
@@ -112,6 +124,9 @@ var (
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-plugin-exposed",
 			Namespace: test.TestNamespace,
+			Annotations: map[string]string{
+				greenhousev1alpha1.AllowCreateAnnotation: "true",
+			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
 			ClusterName:      "test-cluster",

pkg/controllers/plugin/suite_test.go

@@ -164,6 +164,9 @@ var _ = Describe("HelmControllerTest", Serial, func() {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      PluginName,
 				Namespace: Namespace,
+				Annotations: map[string]string{
+					greenhousev1alpha1.AllowCreateAnnotation: "true",
+				},
 			},
 			Spec: greenhousev1alpha1.PluginSpec{
 				PluginDefinition: PluginDefinitionName,
@@ -517,6 +520,9 @@ var _ = Describe("HelmControllerTest", Serial, func() {
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      pluginName,
 					Namespace: Namespace,
+					Annotations: map[string]string{
+						greenhousev1alpha1.AllowCreateAnnotation: "true",
+					},
 				},
 				Spec: greenhousev1alpha1.PluginSpec{
 					PluginDefinition: pluginWithEveryOption,
@@ -660,6 +666,9 @@ var _ = When("the pluginDefinition is UI only", func() {
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      "uiplugin",
 				Namespace: "default",
+				Annotations: map[string]string{
+					greenhousev1alpha1.AllowCreateAnnotation: "true",
+				},
 			},
 			Spec: greenhousev1alpha1.PluginSpec{
 				PluginDefinition: "myuiplugin",

pkg/test/resources.go

@@ -242,6 +242,17 @@ func NewPlugin(ctx context.Context, name, namespace string, opts ...func(*greenh
 	return plugin
 }
 
+// WithAnnotation sets the annotation for plugin
+func WithAnnotation(key, value string) func(plugin *greenhousev1alpha1.Plugin) {
+	return func(plugin *greenhousev1alpha1.Plugin) {
+		if plugin.Annotations == nil {
+			plugin.Annotations = map[string]string{}
+		}
+
+		plugin.Annotations[key] = value
+	}
+}
+
 // WithRules overrides the default rules of a TeamRole
 func WithRules(rules []rbacv1.PolicyRule) func(*greenhousev1alpha1.TeamRole) {
 	return func(tr *greenhousev1alpha1.TeamRole) {