Add the ability to filter what steps in this plugin are available to use in pipelines #196

rsandell · 2023-03-19T02:55:56Z

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

rsandell · 2023-03-19T02:58:11Z

I had an itch and had to scratch it.
Asking review from some people to check my jolt cola infused hacking high :)

rsandell · 2023-03-19T03:05:41Z

src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/StepSelectionConfigurationTest.java

+public class StepSelectionConfigurationTest {
+
+    @Rule
+    public RealJenkinsRule r = new RealJenkinsRule().withDebugPort(4001).withDebugServer(false);


Can I keep this in the code or do I need to remove it before merging?

using a static port in a test is liklely to cause port conflicts when run on other systems (and random failures). so I would remove it.

jglick · 2023-03-22T13:59:59Z

I do not think it makes sense to do something like this in a random plugin. If there is a genuine need to block certain steps from certain folders or the whole controller, that should be done generically. You can also use https://plugins.jenkins.io/extension-filter/ in general.

What is the motivation here? Certain steps in this plugin are known to be bad ideas, and they should simply be deprecated for everyone.

jglick · 2023-03-22T15:00:47Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/StepSelectionConfiguration.java

+        ExtensionList<StepDescriptor> stepDescriptors = ExtensionList.lookup(StepDescriptor.class);
+
+        FilterImpl filter = ExtensionList.lookupSingleton(FilterImpl.class);
+        Collection<StepDescriptor> definedStepDescriptors = getAllStepDescriptors();
+        List<StepDescriptor> toRemove = new ArrayList<>();
+        final List<StepDescriptor> toAdd = new ArrayList<>();
+        for (StepDescriptor descriptor : definedStepDescriptors) {
+            if (stepDescriptors.contains(descriptor)) {
+                if (!filter.allows(StepDescriptor.class, new ExtensionComponent<>(descriptor))) {
+                    toRemove.add(descriptor);
+                }
+            } else if (filter.allows(StepDescriptor.class, new ExtensionComponent<>(descriptor))) {
+                toAdd.add(descriptor);
+            }
+        }
+        stepDescriptors.refresh(new ExtensionComponentSet() {
+            @Override
+            public <T> Collection<ExtensionComponent<T>> find(Class<T> type) {
+                if (type == StepDescriptor.class) {
+                    return toAdd.stream().map(stepDescriptor -> new ExtensionComponent<T>(type.cast(stepDescriptor))).collect(Collectors.toSet());
+                } else {
+                    return Collections.emptyList();
+                }
+            }
+        });
+        stepDescriptors.removeAll(toRemove);


This is rather low-level and will result in a cryptic error message about an undefined function. Much better to use https://javadoc.jenkins.io/plugin/workflow-api/org/jenkinsci/plugins/workflow/flow/StepListener.html#notifyOfNewStep(org.jenkinsci.plugins.workflow.steps.Step,org.jenkinsci.plugins.workflow.steps.StepContext) to just fail the step with a polite AbortException.

daniel-beck · 2023-03-23T10:57:35Z

I suggest splitting this plugin up into multiple ones, each with one or few steps that make sense to be in the same component, and keeping this plugin as an empty shell with dependencies to all of them. This way, admins can uninstall what they don't want.

rsandell · 2023-04-26T20:32:23Z

It is not random in the sense that it only filters the steps that are defined in this plugin.
The StepListener approach would still make the steps show up in the Snippetizer and create a FlowNode for it but break the build if called, so that is not very user friendly.
The extension-filter plugin requires knowledge about how pipeline steps and other extension points are implemented which also doesn't provide much user friendlyness. And can give a false sense of security when you for example only want to allow one step; in this implementation you can add just that to the allow list. With the extension-filter approach you would need to exclude all other steps, and then be diligent when upgrading the plugin in case the have been new steps added and make sure to exclude those as well.

Yes some sets of steps could be extracted into new plugins, but due to the goal of this plugin there would always be a hodge podge of random good to have steps where one of them might be interesting for someone while others could be risky.

jglick · 2023-04-26T22:33:43Z

It is not random in the sense that it only filters the steps that are defined in this plugin.

Perhaps I should rephrase—there is no precedent for a plugin suppressing the existence of its own steps, and this plugin should not be setting such a precedent. If there is a compelling use case for an administrator to block selected Pipeline steps from being run in any job (and I have yet to hear the explanation for that), then it should be done in a separate generic plugin that offers a list of all Pipeline steps.

(Along similar lines, CloudBees CI includes a Pipeline policy system producing build-time errors under configurable conditions.)

The StepListener approach would still make the steps show up in the Snippetizer

If there are specific steps in this plugin that are known to be problematic for specific reasons, they should be placed in the “advanced/deprecated” section with suitable warnings. Start with the most misguided: #47.

and create a FlowNode for it but break the build if called, so that is not very user friendly.

It would break the build with a clear message, which seems a lot friendlier than having the build throw a cryptic stack trace from DSL about an undefined function.

jglick · 2023-06-29T15:12:51Z

compare https://github.com/samrocketman/restrict-steps-plugin

jtnord

do we still desire this? should it be closed given existing comments?

jtnord · 2023-03-20T10:20:39Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/StepSelectionConfiguration.java

+     * @return The unfiltered list of all {@link StepDescriptor}s defined in this plugin.
+     */
+    public static Collection<StepDescriptor> getAllStepDescriptors() {
+        Hudson jenkins = Hudson.getInstance();


Hudson :-p

jtnord · 2023-03-20T10:28:19Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/StepSelectionConfiguration.java

+    }
+
+    public synchronized String getDeny() {
+        return String.join("\n", deny);


generally a textAreas value in HTML is normalized to \r\n

https://html.spec.whatwg.org/multipage/form-elements.html#the-textarea-element:~:text=For%20historical%20reasons%2C%20the%20element%27s,%2C%20with%20newlines%20normalized.

jtnord · 2023-03-20T10:34:36Z

src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/StepSelectionConfigurationTest.java

+            Set<String> steps = ExtensionList.lookup(StepDescriptor.class).stream().map(StepDescriptor::getFunctionName).collect(Collectors.toSet());
+            assertThat(steps, hasItems(allSteps));
+
+            //blacklist something


Suggested change

//blacklist something

//deny something

rsandell added 2 commits March 18, 2023 02:39

Filter out unwanted steps from use

a94f499

Some tests and functional backend

4aea878

rsandell requested review from daniel-beck, dwnusbaum, jglick and jtnord March 19, 2023 02:58

rsandell commented Mar 19, 2023

View reviewed changes

jglick reviewed Mar 22, 2023

View reviewed changes

Configuration page

73f89eb

Merge branch 'master' into allow_deny

9c03329

jtnord reviewed Oct 13, 2023

View reviewed changes

Uh oh!

Add the ability to filter what steps in this plugin are available to use in pipelines #196

Are you sure you want to change the base?

Add the ability to filter what steps in this plugin are available to use in pipelines #196

Uh oh!

Conversation

rsandell commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rsandell commented Mar 19, 2023

Uh oh!

rsandell Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

jtnord Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

jglick commented Mar 22, 2023

Uh oh!

jglick Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-beck commented Mar 23, 2023

Uh oh!

rsandell commented Apr 26, 2023

Uh oh!

jglick commented Apr 26, 2023

Uh oh!

jglick commented Jun 29, 2023

Uh oh!

jtnord left a comment

Choose a reason for hiding this comment

Uh oh!

jtnord Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

jtnord Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

jtnord Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rsandell commented Mar 19, 2023 •

edited

Loading