Add optional usage restriction for a cloud using folder properties #282

sixtyeight · 2018-02-07T22:10:29Z

In our Jenkins setup we would like to control (at least at a basic level) who can use a cloud definition.

I added an optional "secret" field to the KubernetesCloud which is, if not empty, compared with the value of the "cloudSecret" attribute in the PodTemplateStepExecution before actually provisioning the new agent.
Using this approach enables us to define the secret credential for example at folder scope.

sixtyeight · 2018-02-07T23:58:08Z

Pipeline usage example:

// points to a simple jenkins "secret text" credential defined in folder for team1
withCredentials([string(credentialsId: 'openshift-team1-secret', variable: 'CLOUDSECRET')]) {

  podTemplate(cloudSecret: "${CLOUDSECRET}", cloud: 'openshift-team1', label: 'mypod', containers: [
    containerTemplate(name: 'maven', image: 'maven:3.3.9-jdk-8-alpine', ttyEnabled: true, command: 'cat'),
  ] )
    {
      node('mypod') {
        stage('Get a Maven project') {
          git 'https://github.com/metalab-kassomat/kassomat-changeomatic.git'
          container('maven') {
            stage('Build a Maven project') {
              sh 'mvn -B clean install'
            }
         }
       }
    }
}

Vlatombe · 2018-02-08T08:41:59Z

I understand the intent but I'm not sure requiring a secret string is the most elegant solution. Why not add a boolean property in the cloud, as well as a folder property?

Thinking further, one could imagine an extension point checking whether a given podTemplate is allowed given the context (e.g. forbid privileged containers in all folders but allow them in a specific one)

sixtyeight · 2018-02-09T04:08:02Z

I understand the intent but I'm not sure requiring a secret string is the most elegant solution. Why not add a boolean property in the cloud, as well as a folder property?

@Vlatombe
Could you please explain your proposal in more detail, I'm afraid I'm missing something.
What would the Boolean indicate?
What is the purpose of the folder property and how would it correlate with a specific cloud definition?

Vlatombe · 2018-02-09T09:24:18Z

The way I see it, you would define a boolean property in the cloud to allow/disallow usage of pod templates in pipeline by default.

Then, the folder property would define the list of Kubernetes clouds allowed to be used inside the folder.

sixtyeight · 2018-02-09T13:21:09Z

Then, the folder property would define the list of Kubernetes clouds allowed to be used inside the folder.

Thanks for the clarification, got you now. I assume this would require the Folders Plus Version of the Plugin because on my install it seems I can't add any Properties at all (v6.3)? In the configure view of the folder only the header "Properties" is visible but no add button.

Vlatombe · 2018-02-09T13:44:05Z

The FolderProperty is defined in the folder plugin (https://github.com/jenkinsci/cloudbees-folder-plugin/blob/master/src/main/java/com/cloudbees/hudson/plugins/folder/FolderProperty.java).

This section lists properties contributed by plugins. In your screenshot, we can see the JIRA plugin is contributing a FolderProperty implementation.

sixtyeight · 2018-02-09T15:10:09Z

In your screenshot, we can see the JIRA plugin is contributing a FolderProperty implementation.

Aaah, a Layer 8 problem on my side.

After adding the Folder Plugin Maven Dependency to the POM I added a KubernetesFolderProperty:

Any pointers for how to check this value from within the Kubernetes Plugin are highly appreciated :-)

Vlatombe · 2018-02-09T16:11:20Z

From PodTemplateStepExecution, there is already Run<?, ?> run = getContext().get(Run.class);.
Then you can retrieve the owning Job, then you can walk back the folder hierarchy and check the property.

Vlatombe

This is going in the right direction, added some additional comments

Vlatombe · 2018-02-12T09:21:10Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java

    }

    @Deprecated
    public KubernetesCloud(String name, List<? extends PodTemplate> templates, String serverUrl, String namespace,
-            String jenkinsUrl, String containerCapStr, int connectTimeout, int readTimeout, int retentionTimeout) {
+            String jenkinsUrl, String containerCapStr, int connectTimeout, int readTimeout, int retentionTimeout, boolean usageRestricted) {


Not needed (since you have @DataBoundSetter on the setter method) and it would break potential usages of this constructor.

Vlatombe · 2018-02-12T09:21:33Z

pom.xml

@@ -89,6 +89,12 @@
      <version>${jenkins-kubernetes-credentials.version}</version>
    </dependency>

+	<dependency>


indent with spaces.

Vlatombe · 2018-02-12T09:56:51Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

+        	ItemGroup<?> parent = job.getParent(); // Inspect the Parent of the Job
+
+    		Set<String> allowedClouds = new HashSet<>();
+            if(parent instanceof Folder) {


AbstractFolder is a better check than Folder.

Vlatombe · 2018-02-12T09:57:17Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

+        	ItemGroup<?> parent = job.getParent(); // Inspect the Parent of the Job
+
+    		Set<String> allowedClouds = new HashSet<>();
+            if(parent instanceof Folder) {


Could recurse into folder hierarchy.

collectAllowedClouds is already traversing the hierarchy internally.

Vlatombe · 2018-02-12T10:00:30Z

...main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty/config.jelly

+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:section title="Kubernetes Plugin">
+  	<f:entry title="${%Allowed Clouds}" field="allowedClouds">


This could be more structured, like providing a list of checkboxes (one line per cloud instance) and storing a collection of cloud names in the folder property class.

sixtyeight · 2018-02-15T21:01:17Z

Could please someone with more Jenkins and Jelly knowledge help me out with the UI stuff.

I tried to implement a list of Checkboxes (one for each available cloud) which, if a cloud is inherited from a parent folder, would render the corresponding Checkbox as selected and read-only. Only non-inherited selected clouds should be saved in the folder property.

sixtyeight · 2018-02-25T01:06:13Z

phew!

Folder for an admin user:

Same Folder for a non-admin user:

sixtyeight · 2018-02-25T03:05:57Z

Grants for clouds inherited from a parent folder are now shown as readonly:

sixtyeight · 2018-02-25T03:09:07Z

Initially the whole section is hidden inside an advanced tag:

sixtyeight · 2018-02-25T03:11:04Z

Any feedback highly appreciated :-)
@Vlatombe @carlossg

Vlatombe

I'm suggesting some changes to clean up the patch but this looks good overall. Will try the PR locally as well.

Vlatombe · 2018-03-02T08:22:00Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java

-
+
+    public boolean isUsageRestricted() {
+		return usageRestricted;


fix indentation

Vlatombe · 2018-03-02T08:26:40Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty.java

+            return isModifyable() ? "" : "disabled=\"disabled\"";
+        }
+
+        private boolean isModifyable() {


isModifiable

Vlatombe · 2018-03-02T08:29:18Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty.java

+     * 
+     * @return
+     */
+    public List<UsagePermission> getEffectivePermissions() {


Add

@SuppressWarnings("unused") // Used by jelly @Restricted(DoNotUse.class) // Used by jelly

Vlatombe · 2018-03-02T08:30:19Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty.java

+
+            String name = kubernetesCloud.name;
+            p.setName(name);
+            p.setGranted(false);


not necessary, a boolean is false by default.

Vlatombe · 2018-03-02T08:30:27Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty.java

+            String name = kubernetesCloud.name;
+            p.setName(name);
+            p.setGranted(false);
+            p.setInherited(false);


Vlatombe · 2018-03-02T08:49:45Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

        Cloud cloud = Jenkins.getInstance().getCloud(cloudName);
        if (cloud == null) {
            throw new AbortException(String.format("Cloud does not exist: %s", cloudName));
        }
        if (!(cloud instanceof KubernetesCloud)) {
-            throw new AbortException(String.format("Cloud is not a Kubernetes cloud: %s (%s)", cloudName,
-                    cloud.getClass().getName()));
+            throw new AbortException(


Formatting change, should be removed.

Vlatombe · 2018-03-02T08:50:21Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

        PodTemplateAction podTemplateAction = run.getAction(PodTemplateAction.class);
        NamespaceAction namespaceAction = run.getAction(NamespaceAction.class);
        String parentTemplates = podTemplateAction != null ? podTemplateAction.getParentTemplates() : null;

-        //Let's generate a random name based on the user specified to make sure that we don't have
-        //issues with concurrent builds, or messing with pre-existing configuration
+        // Let's generate a random name based on the user specified to make sure that we


Remove formatting change (older is better)

Vlatombe · 2018-03-02T08:56:36Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

        }
        KubernetesCloud kubernetesCloud = (KubernetesCloud) cloud;

        Run<?, ?> run = getContext().get(Run.class);
+        if (kubernetesCloud.isUsageRestricted()) {


Not the ideal design (ideally PodTemplateStepExecution should not depend on KubernetesFolderProperty) but could be revisited later by introducing a new extension point.

no idea how to do this and, to be totally honest, I don't feel the urge to dig any deeper into the internals of Jenkins and its quirks.

Vlatombe · 2018-03-02T08:57:39Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

@@ -127,8 +156,8 @@ public void onResume() {
            throw new RuntimeException(String.format("Cloud does not exist: %s", cloudName));
        }
        if (!(cloud instanceof KubernetesCloud)) {
-            throw new RuntimeException(String.format("Cloud is not a Kubernetes cloud: %s (%s)", cloudName,
-                    cloud.getClass().getName()));
+            throw new RuntimeException(


Avoid formatting change

Vlatombe · 2018-03-02T09:00:24Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFactoryAdapterTest.java

@@ -46,6 +50,18 @@

    private Map<String, String> systemProperties = new HashMap<>();

+    /**


Doesn't seem related to the main change but ok.

Vlatombe · 2018-03-02T09:11:54Z

src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly

@@ -55,6 +55,10 @@
      <f:entry title="${%Container Cleanup Timeout (minutes)}" field="retentionTimeout">
        <f:textbox default="5"/>
      </f:entry>
+
+      <f:entry title="${%Requires permission}" field="usageRestricted">


Missing inline help (Add help-usageRestricted.html).
Maybe the title could specify that this applies to pipeline jobs. (Requires permission to use in pipeline jobs)

Having the permission in the advanced section is not very discoverable (but this is a general problem in the plugin due to the high number of settings).

added help file but still in the advanced section.

Vlatombe · 2018-03-02T09:37:44Z

...main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty/config.jelly

+	<f:advanced>
+	<f:entry title="Usage restricted Kubernetes Clouds available to any Pipeline jobs inside this folder:"/>
+	<j:forEach var="permission" items="${instance.getEffectivePermissions()}">
+		<f:entry title="${permission.name}"><j:expr value="${permission.htmlCheckbox()}" escapeText="false" /></f:entry>


Why not use <f:checkbox> here?

couldn't figure out how to get the checkbox visually rendered as disabled in jelly.
by using readonly it looked like an active checkbox. i thought this would be confusing for the user.

sixtyeight · 2018-06-03T17:26:48Z

fixed most of the issues. sorry for letting this pr get stale but I didn't find time to do this sooner.

https://github.com/jenkinsci/kubernetes-plugin.git into usage-restriction Conflicts: src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

Code has changed

Vlatombe · 2018-07-02T13:20:47Z

Sorry for the review delay, going through a new round now.

* Merge with master * Reuse KubernetesFolderProperty instance on save * Some labels re-wording * Create taglib for checkbox with disabled attribute support

Vlatombe

I have submitted sixtyeight#1 with my suggested changes to this PR.

Usage restriction

carlossg · 2018-07-11T07:59:38Z

@Vlatombe does it look good to you now?

I think it deserves an entry in the README

sixtyeight · 2018-07-11T08:27:13Z

Hi, Yes! :-) Thanks for your help. I didn't find time to play around with it so far. Will write something this evening. Kind regards, Christian

…

Gesendet: Mittwoch, 11. Juli 2018 um 09:59 Uhr Von: "Carlos Sanchez" ***@***.***> An: jenkinsci/kubernetes-plugin ***@***.***> Cc: sixtyeight ***@***.***>, Author ***@***.***> Betreff: Re: [jenkinsci/kubernetes-plugin] added optional usage restriction for a cloud (#282) @Vlatombe does it look good to you now? I think it deserves an entry in the README -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #282 (comment)

Vlatombe · 2018-07-11T12:59:26Z

Lgtm Le mer. 11 juil. 2018 à 10:27, sixtyeight <notifications@github.com> a écrit :

Hi, Yes! :-) Thanks for your help. I didn't find time to play around with it so far. Will write something this evening. Kind regards, Christian > Gesendet: Mittwoch, 11. Juli 2018 um 09:59 Uhr > Von: "Carlos Sanchez" ***@***.***> > An: jenkinsci/kubernetes-plugin ***@***.***> > Cc: sixtyeight ***@***.***>, Author ***@***.***> > Betreff: Re: [jenkinsci/kubernetes-plugin] added optional usage restriction for a cloud (#282) > > @Vlatombe does it look good to you now? > > I think it deserves an entry in the README > > -- > You are receiving this because you authored the thread. > Reply to this email directly or view it on GitHub: > #282 (comment) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#282 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKdw1CrDVxs6r2PUdhVu6fzF_MRAZW0ks5uFbblgaJpZM4R9eTF> .

-- *Vincent Latombe* Software Engineer CloudBees, Inc. [image: CloudBees-Logo.png]

sixtyeight changed the title ~~added optional usage restriction for a cloud~~ [WIP] added optional usage restriction for a cloud Feb 9, 2018

sixtyeight changed the title ~~[WIP] added optional usage restriction for a cloud~~ [WiP] added optional usage restriction for a cloud Feb 9, 2018

sixtyeight changed the title ~~[WiP] added optional usage restriction for a cloud~~ added optional usage restriction for a cloud Feb 10, 2018

Vlatombe reviewed Feb 12, 2018

View reviewed changes

Vlatombe previously requested changes Mar 2, 2018

View reviewed changes

Vlatombe reviewed Mar 2, 2018

View reviewed changes

added support for restricting access to clouds

a9a7c2c

sixtyeight force-pushed the usage-restriction branch from 1343fb3 to a9a7c2c Compare June 3, 2018 02:57

tabs replaced with blanks

d83358e

carlossg assigned Vlatombe Jun 7, 2018

Merge branch 'master' of

af6db40

https://github.com/jenkinsci/kubernetes-plugin.git into usage-restriction Conflicts: src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java

Vlatombe added 2 commits July 2, 2018 15:25

Merge branch 'master' into usage-restriction

5b984d1

Some suggestions on top of the existing PR

3308580

* Merge with master * Reuse KubernetesFolderProperty instance on save * Some labels re-wording * Create taglib for checkbox with disabled attribute support

Vlatombe reviewed Jul 3, 2018

View reviewed changes

Merge pull request #1 from Vlatombe/usage-restriction

f307c76

Usage restriction

Merge branch 'master' into usage-restriction

41ac05b

carlossg changed the title ~~added optional usage restriction for a cloud~~ Add optional usage restriction for a cloud using folder properties Jul 31, 2018

carlossg merged commit e2f46de into jenkinsci:master Jul 31, 2018

adamdougal mentioned this pull request Sep 3, 2018

Add documentation for restricting cloud use to certain folders #376

Merged

jglick mentioned this pull request Aug 5, 2020

JENKINS-62788 Remove custom f:checkbox fork #811

Merged

		@@ -46,6 +50,18 @@

		private Map<String, String> systemProperties = new HashMap<>();

		/**

Add optional usage restriction for a cloud using folder properties #282

Add optional usage restriction for a cloud using folder properties #282

Conversation

sixtyeight commented Feb 7, 2018

sixtyeight commented Feb 7, 2018

Vlatombe commented Feb 8, 2018

sixtyeight commented Feb 9, 2018

Vlatombe commented Feb 9, 2018

sixtyeight commented Feb 9, 2018

Vlatombe commented Feb 9, 2018

sixtyeight commented Feb 9, 2018

Vlatombe commented Feb 9, 2018

Vlatombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixtyeight commented Feb 15, 2018

sixtyeight commented Feb 25, 2018

sixtyeight commented Feb 25, 2018

sixtyeight commented Feb 25, 2018

sixtyeight commented Feb 25, 2018

Vlatombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlatombe Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlatombe Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixtyeight commented Jun 3, 2018

Vlatombe commented Jul 2, 2018

Vlatombe left a comment

Choose a reason for hiding this comment

carlossg commented Jul 11, 2018

sixtyeight commented Jul 11, 2018 via email

Vlatombe commented Jul 11, 2018 via email

Vlatombe Mar 2, 2018 •

edited

Loading

Vlatombe Mar 2, 2018 •

edited

Loading