feat(plugins): deny plugin creation #843
Conversation
gciezkowski-acc
changed the title
gciezkowski-acc
changed the title
gciezkowski-acc
force-pushed
the
feat/829_deny_plugin_creation
branch
2 times, most recently
from
02a1e9a to
ef77e22
Compare
gciezkowski-acc
force-pushed
the
feat/829_deny_plugin_creation
branch
from
ef77e22 to
79334e6
Compare
gciezkowski-acc
requested review from
IvoGoman,
uwe-mayer and
abhijith-darshan
There was a problem hiding this comment.
Thanks for this.
It came to mind, and this might be a blocker:
Currently we can only deploy Plugins to the central cluster (no clusterName in Plugin.Spec).
Shipping this would deny creating any Plugin on the central cluster, as there is no way to reference it in a Preset...
cc @IvoGoman
| @@ -90,16 +104,26 @@ var _ = Describe("Plugin E2E", Ordered, func() { | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| Expect(len(pluginDefinitionList.Items)).To(BeEquivalentTo(1)) | |||
|
|
|||
| By("Creating the plugin") | |||
| By("Try to creating the plugin") | |||
There was a problem hiding this comment.
| By("Try to creating the plugin") | |
| By("Trying to create the plugin") |
| @@ -80,6 +80,8 @@ const ( | |||
| HelmUninstallFailedReason ConditionReason = "HelmUninstallFailed" | |||
| ) | |||
|
|
|||
| const AllowCreateAnnotation = "greenhouse.sap/allow-create" | |||
There was a problem hiding this comment.
This should live in well_known.go
and be reused from there...
There was a problem hiding this comment.
Please also add go docs for the Annotation
| @@ -144,6 +149,12 @@ func ValidateDeletePlugin(_ context.Context, _ client.Client, _ runtime.Object) | |||
| return nil, nil | |||
| } | |||
|
|
|||
| func allowCreatePlugin(plugin *greenhousev1alpha1.Plugin) bool { | |||
| _, ok := plugin.Annotations["greenhouse.sap/allow-create"] | |||
There was a problem hiding this comment.
Please do not use strings. Reuse from well_known.go. See comment in plugin_types.go
| @@ -80,6 +80,8 @@ const ( | |||
| HelmUninstallFailedReason ConditionReason = "HelmUninstallFailed" | |||
| ) | |||
|
|
|||
| const AllowCreateAnnotation = "greenhouse.sap/allow-create" | |||
There was a problem hiding this comment.
| const AllowCreateAnnotation = "greenhouse.sap/allow-create" | |
| const AllowPluginCreateAnnotation = "greenhouse.sap/allow-plugin-create" |
There was a problem hiding this comment.
Please move to well_known.go :)
| test.WithReleaseNamespace(env.TestNamespace), | ||
| test.WithPluginOptionValue("replicaCount", &apiextensionsv1.JSON{Raw: []byte("1")}, nil)) | ||
| err = adminClient.Create(ctx, testPlugin) | ||
| Expect(err).To(HaveOccurred()) |
There was a problem hiding this comment.
Let's not go the PluginPreset way here. Let's annotate the test plugin with AllowPluginCreateAnnotation and test from there. So the Plugin creation path can test the resources are created on the remote and the PluginPreset creation path tests the subsequent Plugins are created according to the clusterSelector
There was a problem hiding this comment.
We could add the step of trying to apply the Plugin without annotation first. And expecting it to fail.
| if testPluginPreset.Annotations == nil { | ||
| testPluginPreset.Annotations = map[string]string{} | ||
| } | ||
| delete(testPluginPreset.Annotations, "greenhouse.sap/prevent-deletion") |
There was a problem hiding this comment.
Obsolete if we only test the Plugin without preset, but:
No strings please. This should also live in well_known.go
Or was it a deliberate decision to not expose this annotation publicly @IvoGoman ?
| @@ -279,7 +302,11 @@ var _ = Describe("Validate plugin spec fields", Ordered, func() { | |||
|
|
|||
| It("should reject to update a plugin when the pluginDefinition changes", func() { | |||
There was a problem hiding this comment.
I would expect a test
"It should deny creation of a Plugin without AllowPluginCreationAnnotation"
| Resources: []string{"plugins", "pluginpresets"}, | ||
| Resources: []string{"pluginpresets"}, | ||
| }, | ||
| // Grant read, update and delete permissions for PluginPresets to organization plugin admins. No create |
There was a problem hiding this comment.
| // 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 |
| @@ -208,6 +208,21 @@ func SetOptionValueForPlugin(plugin *greenhousev1alpha1.Plugin, key, value strin | |||
| }) | |||
| } | |||
|
|
|||
| // SetOptionValueForPluginPreset sets the value of a PluginOtionValue | |||
There was a problem hiding this comment.
| // SetOptionValueForPluginPreset sets the value of a PluginOtionValue | |
| // SetOptionValueForPluginPreset sets the value of a PluginOptionValue |
There was a problem hiding this comment.
I like it, but it seems unused in our current code base?
Description
Deny the creation of plugin by the user. When plugin has annotation
greenhouse.sap/allow-createyou can create the plugin.What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
You can run the test using command:
make clean-e2e && make cli && make setup-webhook && make setup-e2e && make e2e-local SCENARIO=pluginAdded to documentation?
Checklist