feat(plugins) Codereview changes (#829)

e2e/plugin/e2e_test.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/cloudoperators/greenhouse/e2e/plugin/fixtures"
 	"github.com/cloudoperators/greenhouse/e2e/shared"
+	"github.com/cloudoperators/greenhouse/pkg/apis"
 	greenhousev1alpha1 "github.com/cloudoperators/greenhouse/pkg/apis/greenhouse/v1alpha1"
 	"github.com/cloudoperators/greenhouse/pkg/clientutil"
 	"github.com/cloudoperators/greenhouse/pkg/test"
@@ -105,26 +106,24 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(len(pluginDefinitionList.Items)).To(BeEquivalentTo(1))
 
-		By("Try to creating the plugin")
-		// Creating plugin
+		By("Trying to create the plugin without annotation")
 		testPlugin := fixtures.PreparePlugin("test-nginx-plugin", env.TestNamespace,
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithReleaseNamespace(env.TestNamespace),
 			test.WithPluginOptionValue("replicaCount", &apiextensionsv1.JSON{Raw: []byte("1")}, nil))
 		err = adminClient.Create(ctx, testPlugin)
 		Expect(err).To(HaveOccurred())
 
-		By("Creating the plugin preset")
-		testPluginPreset := fixtures.PreparePluginPreset("test-nginx-preset", env.TestNamespace, testPlugin.Spec)
-		err = adminClient.Create(ctx, testPluginPreset)
+		By("Trying to create the plugin without annotation")
+		testPlugin = fixtures.PreparePlugin("test-nginx-plugin", env.TestNamespace,
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithReleaseNamespace(env.TestNamespace),
+			test.WithPluginOptionValue("replicaCount", &apiextensionsv1.JSON{Raw: []byte("1")}, nil),
+			test.WithCluster(remoteClusterName),
+			test.WithAnnotation(apis.AllowPluginCreateAnnotation, "true"))
+		err = adminClient.Create(ctx, testPlugin)
 		Expect(err).ToNot(HaveOccurred())
 
-		By("Checking the plugin preset is ready")
-		Eventually(func(g Gomega) {
-			err = adminClient.Get(ctx, client.ObjectKeyFromObject(testPluginPreset), testPluginPreset)
-			g.Expect(err).ToNot(HaveOccurred())
-		}).Should(Succeed())
-
 		By("Checking the plugin status is ready")
 		pluginList := &greenhousev1alpha1.PluginList{}
 		Eventually(func(g Gomega) {
@@ -157,11 +156,11 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 
 		By("Updating replicas")
 		Eventually(func(g Gomega) {
-			namespacedName := types.NamespacedName{Name: testPluginPreset.Name, Namespace: testPluginPreset.Namespace}
-			err = adminClient.Get(ctx, namespacedName, testPluginPreset)
+			namespacedName := types.NamespacedName{Name: testPlugin.Name, Namespace: testPlugin.Namespace}
+			err = adminClient.Get(ctx, namespacedName, testPlugin)
 			g.Expect(err).NotTo(HaveOccurred())
-			test.SetOptionValueForPluginPreset(testPluginPreset, "replicaCount", "2")
-			err = adminClient.Update(ctx, testPluginPreset)
+			test.SetOptionValueForPlugin(testPlugin, "replicaCount", "2")
+			err = adminClient.Update(ctx, testPlugin)
 			g.Expect(err).NotTo(HaveOccurred())
 		}).Should(Succeed())
 
@@ -176,24 +175,6 @@ 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")
 		Eventually(func(g Gomega) {
 			err = adminClient.Delete(ctx, testPlugin)

pkg/admission/plugin_webhook.go

@@ -150,8 +150,8 @@ func ValidateDeletePlugin(_ context.Context, _ client.Client, _ runtime.Object)
 }
 
 func allowCreatePlugin(plugin *greenhousev1alpha1.Plugin) bool {
-	_, ok := plugin.Annotations["greenhouse.sap/allow-create"]
-	delete(plugin.Annotations, "greenhouse.sap/allow-create")
+	_, ok := plugin.Annotations[greenhouseapis.AllowPluginCreateAnnotation]
+	delete(plugin.Annotations, greenhouseapis.AllowPluginCreateAnnotation)
 	return ok
 }
 

pkg/admission/plugin_webhook_test.go

@@ -231,7 +231,7 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 		testPlugin = test.NewPlugin(test.Ctx, "test-plugin", setup.Namespace(),
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithReleaseNamespace("test-namespace"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "true"))
 		expectClusterMustBeSetError(test.K8sClient.Create(test.Ctx, testPlugin))
 	})
 
@@ -241,7 +241,7 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithCluster("non-existent-cluster"),
 			test.WithReleaseNamespace("test-namespace"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "true"))
 
 		expectClusterNotFoundError(test.K8sClient.Create(test.Ctx, testPlugin))
 	})
@@ -252,7 +252,7 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithCluster(testCluster.Name),
 			test.WithReleaseNamespace("test-namespace"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "true"))
 
 		By("checking the label on the plugin")
 		actPlugin := &greenhousev1alpha1.Plugin{}
@@ -268,7 +268,7 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithCluster(testCluster.Name),
 			test.WithReleaseNamespace("test-namespace"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "true"))
 		testPlugin.Spec.ClusterName = "wrong-cluster-name"
 		err := test.K8sClient.Update(test.Ctx, testPlugin)
 
@@ -281,7 +281,7 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithCluster(testCluster.Name),
 			test.WithReleaseNamespace("test-namespace"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "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")
@@ -293,20 +293,30 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() {
 			test.WithPluginDefinition(testPluginDefinition.Name),
 			test.WithCluster(testCluster.Name),
 			test.WithReleaseNamespace("test-namespace"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "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")
 		Expect(err.Error()).To(ContainSubstring(validation.FieldImmutableErrorMsg))
 	})
 
+	It("should deny creation of a Plugin without AllowPluginCreationAnnotation", func() {
+		testPlugin = test.NewPlugin(test.Ctx, "test-plugin-2", setup.Namespace(),
+			test.WithPluginDefinition(testPluginDefinition.Name),
+			test.WithCluster(testCluster.Name),
+			test.WithReleaseNamespace("test-namespace"))
+		err := test.K8sClient.Create(test.Ctx, testPlugin)
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("plugin creation is not allowed"))
+	})
+
 	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"),
-			test.WithAnnotation(greenhousev1alpha1.AllowCreateAnnotation, "true"))
+			test.WithAnnotation(greenhouseapis.AllowPluginCreateAnnotation, "true"))
 
 		testPlugin.Spec.PluginDefinition = secondPluginDefinition.Name
 		err := test.K8sClient.Update(test.Ctx, testPlugin)

pkg/apis/greenhouse/v1alpha1/plugin_types.go

@@ -80,8 +80,6 @@ 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/apis/well_known.go

@@ -48,6 +48,9 @@ const (
 
 	// LabelKeyExposeNamedPort is specifying the port to be exposed by name. LabelKeyExposeService needs to be set. Defaults to the first port if the named port is not found.
 	LabelKeyExposeNamedPort = "greenhouse.sap/exposeNamedPort"
+
+	// AllowPluginCreateAnnotation enables the creation of a plugin
+	AllowPluginCreateAnnotation = "greenhouse.sap/allow-plugin-create"
 )
 
 // TeamRole and TeamRoleBinding constants

pkg/controllers/organization/service_proxy.go

@@ -18,6 +18,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
+	greenhouseapis "github.com/cloudoperators/greenhouse/pkg/apis"
 	greenhousesapv1alpha1 "github.com/cloudoperators/greenhouse/pkg/apis/greenhouse/v1alpha1"
 	"github.com/cloudoperators/greenhouse/pkg/clientutil"
 	"github.com/cloudoperators/greenhouse/pkg/common"
@@ -52,7 +53,7 @@ func (r *OrganizationReconciler) reconcileServiceProxy(ctx context.Context, org
 			Name:      serviceProxyName,
 			Namespace: org.Name,
 			Annotations: map[string]string{
-				greenhousesapv1alpha1.AllowCreateAnnotation: "true",
+				greenhouseapis.AllowPluginCreateAnnotation: "true",
 			},
 		},
 		Spec: greenhousesapv1alpha1.PluginSpec{

pkg/controllers/plugin/pluginpreset_controller.go

@@ -175,7 +175,7 @@ func (r *PluginPresetReconciler) reconcilePluginPreset(ctx context.Context, pres
 				plugin.Annotations = make(map[string]string)
 			}
 
-			plugin.Annotations[greenhousev1alpha1.AllowCreateAnnotation] = "true"
+			plugin.Annotations[greenhouseapis.AllowPluginCreateAnnotation] = "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

@@ -42,7 +42,7 @@ var (
 			Name:      "test-plugindefinition",
 			Namespace: test.TestNamespace,
 			Annotations: map[string]string{
-				greenhousev1alpha1.AllowCreateAnnotation: "true",
+				greenhouseapis.AllowPluginCreateAnnotation: "true",
 			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
@@ -61,7 +61,7 @@ var (
 			Name:      "test-plugin-secretref",
 			Namespace: test.TestNamespace,
 			Annotations: map[string]string{
-				greenhousev1alpha1.AllowCreateAnnotation: "true",
+				greenhouseapis.AllowPluginCreateAnnotation: "true",
 			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
@@ -87,7 +87,7 @@ var (
 			Name:      "test-plugin-in-made-up-namespace",
 			Namespace: test.TestNamespace,
 			Annotations: map[string]string{
-				greenhousev1alpha1.AllowCreateAnnotation: "true",
+				greenhouseapis.AllowPluginCreateAnnotation: "true",
 			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
@@ -106,7 +106,7 @@ var (
 			Name:      "test-plugin-crd",
 			Namespace: test.TestNamespace,
 			Annotations: map[string]string{
-				greenhousev1alpha1.AllowCreateAnnotation: "true",
+				greenhouseapis.AllowPluginCreateAnnotation: "true",
 			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{
@@ -125,7 +125,7 @@ var (
 			Name:      "test-plugin-exposed",
 			Namespace: test.TestNamespace,
 			Annotations: map[string]string{
-				greenhousev1alpha1.AllowCreateAnnotation: "true",
+				greenhouseapis.AllowPluginCreateAnnotation: "true",
 			},
 		},
 		Spec: greenhousev1alpha1.PluginSpec{

pkg/controllers/plugin/suite_test.go

@@ -26,6 +26,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/cloudoperators/greenhouse/pkg/admission"
+	greenhouseapis "github.com/cloudoperators/greenhouse/pkg/apis"
 	greenhousev1alpha1 "github.com/cloudoperators/greenhouse/pkg/apis/greenhouse/v1alpha1"
 	"github.com/cloudoperators/greenhouse/pkg/clientutil"
 	"github.com/cloudoperators/greenhouse/pkg/helm"
@@ -165,7 +166,7 @@ var _ = Describe("HelmControllerTest", Serial, func() {
 				Name:      PluginName,
 				Namespace: Namespace,
 				Annotations: map[string]string{
-					greenhousev1alpha1.AllowCreateAnnotation: "true",
+					greenhouseapis.AllowPluginCreateAnnotation: "true",
 				},
 			},
 			Spec: greenhousev1alpha1.PluginSpec{
@@ -521,7 +522,7 @@ var _ = Describe("HelmControllerTest", Serial, func() {
 					Name:      pluginName,
 					Namespace: Namespace,
 					Annotations: map[string]string{
-						greenhousev1alpha1.AllowCreateAnnotation: "true",
+						greenhouseapis.AllowPluginCreateAnnotation: "true",
 					},
 				},
 				Spec: greenhousev1alpha1.PluginSpec{
@@ -667,7 +668,7 @@ var _ = When("the pluginDefinition is UI only", func() {
 				Name:      "uiplugin",
 				Namespace: "default",
 				Annotations: map[string]string{
-					greenhousev1alpha1.AllowCreateAnnotation: "true",
+					greenhouseapis.AllowPluginCreateAnnotation: "true",
 				},
 			},
 			Spec: greenhousev1alpha1.PluginSpec{

pkg/rbac/role.go

@@ -79,7 +79,7 @@ func OrganizationPluginAdminPolicyRules() []rbacv1.PolicyRule {
 			APIGroups: []string{greenhouseapisv1alpha1.GroupVersion.Group},
 			Resources: []string{"pluginpresets"},
 		},
-		// Grant read, update and delete permissions for PluginPresets to organization plugin admins. No create
+		// Grant read, update and delete permissions for Plugins to organization plugin admins. No create
 		{
 			Verbs:     []string{"get", "list", "watch", "update", "patch", "delete"},
 			APIGroups: []string{greenhouseapisv1alpha1.GroupVersion.Group},

pkg/test/resources.go

@@ -208,21 +208,6 @@ func SetOptionValueForPlugin(plugin *greenhousev1alpha1.Plugin, key, value strin
 	})
 }
 
-// SetOptionValueForPluginPreset sets the value of a PluginOtionValue
-func SetOptionValueForPluginPreset(pluginPreset *greenhousev1alpha1.PluginPreset, key, value string) {
-	for i, keyValue := range pluginPreset.Spec.Plugin.OptionValues {
-		if keyValue.Name == key {
-			pluginPreset.Spec.Plugin.OptionValues[i].Value.Raw = []byte(value)
-			return
-		}
-	}
-
-	pluginPreset.Spec.Plugin.OptionValues = append(pluginPreset.Spec.Plugin.OptionValues, greenhousev1alpha1.PluginOptionValue{
-		Name:  key,
-		Value: &apiextensionsv1.JSON{Raw: []byte(value)},
-	})
-}
-
 // NewPlugin returns a greenhousev1alpha1.Plugin object. Opts can be used to set the desired state of the Plugin.
 func NewPlugin(ctx context.Context, name, namespace string, opts ...func(*greenhousev1alpha1.Plugin)) *greenhousev1alpha1.Plugin {
 	GinkgoHelper()