Skip to content

add permissions for jenkins#10649

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
deads2k:jenkins-roles
Aug 30, 2016
Merged

add permissions for jenkins#10649
openshift-bot merged 1 commit intoopenshift:masterfrom
deads2k:jenkins-roles

Conversation

@deads2k
Copy link
Copy Markdown
Contributor

@deads2k deads2k commented Aug 25, 2016

Adds permissions to the admin, edit, and view roles for jenkins access as allowed by openshift/jenkins-openshift-login-plugin#2 .

@bparees @gabemontero are we trying to push this into 1.3?

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 25, 2016

@openshift/api-review I intentionally chose verbs that don't match our normal resource verbs so that these permissions can never grant "normal" API access.

)

const GroupName = ""
const FutureGroupName = "build.openshift.io"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

builds? To match "apps" and "extensions"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

builds? To match "apps" and "extensions"?

"batch" and "imagepolicy"? We've gone back and forth before in one of @mfojtik's pulls. I kind of expect that a group is a singular construct. Probably because I say, "<collective noun> is", not "<collective noun> are"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

builds? To match "apps" and "extensions"?
"batch" and "imagepolicy"? We've gone back and forth before in one of @mfojtik's pulls. I kind of expect that a group is a singular construct. Probably because I say, " is", not " are"

I don't feel strongly, but I do want to be consistent in openshift.

Also: authorization and authentication are singular nouns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically all of those nouns describe processes. The word to describe a build process is "building". Also "app" sounds wrong. You generally don't pluralize processes, but you do pluralize groups of things.

Is there a rule that we are using singular nouns? Or the rule is that we're describing what the group does / contains.

@smarterclayton
Copy link
Copy Markdown
Contributor

So "get" and "view"? Seems a bit weird - how about "external-get" and "external-put"? I don't hate it, but I suspect people will get confused there.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Aug 25, 2016

@bparees @gabemontero are we trying to push this into 1.3?

no

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 25, 2016

Technically all of those nouns describe processes. The word to describe a build process is "building". Also "app" sounds wrong. You generally don't pluralize processes, but you do pluralize groups of things.

Is there a rule that we are using singular nouns? Or the rule is that we're describing what the group does / contains.

Found the original issue: #9372 (comment), copying here:

  1. authorization
  2. build
  3. deploy
  4. image
  5. oauth
  6. project
  7. route
  8. sdn - networking?
  9. template
  10. user
  11. security - I don't like the name. This holds SCC today.

How about we decide for real this time and I'll create a const in each one.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Aug 25, 2016

so if i get jenkins view/edit/admin permissions on a project, i'm going to have those permissions in all the jenkins servers within a project I guess?

are we ok w/ that coarse a level of granularity? (I think I am, but it seems worth making it clear)

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Aug 25, 2016

So "get" and "view"? Seems a bit weird - how about "external-get" and "external-put"? I don't hate it, but I suspect people will get confused there.

it's always going to be used in conjunction w/ the "jenkins" resource, right? so "jenkins/external-get" sounds weird to me. (whereas jenkins/view or jenkins/get does not)

@smarterclayton
Copy link
Copy Markdown
Contributor

smarterclayton commented Aug 25, 2016 via email

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 26, 2016

so if i get jenkins view/edit/admin permissions on a project, i'm going to have those permissions in all the jenkins servers within a project I guess?

are we ok w/ that coarse a level of granularity? (I think I am, but it seems worth making it clear)

I would start there. If you want to find a way to name a jenkins, you could check for access on a particular one, but it would be a considerable amount of work to subdivide the permissions. We've said the ACL boundary is project, I think I'd try to stay there.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 26, 2016

Future names added to each group.

@smarterclayton how do you feel about the verb choice? Sounds like @bparees prefers admin, edit, view.

@smarterclayton
Copy link
Copy Markdown
Contributor

I think adding new verbs that are synonyms to existing verbs is risky from a confusion point of view.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 29, 2016

I think adding new verbs that are synonyms to existing verbs is risky from a confusion point of view.

Ok, we'll need three verbs to give the three levels: admin, edit, view for jenkins. I mapped them to common roles that happen to match names and be verbs, but I'm open to other mappings.

Keep in mind that if you use get for any of them, you'll have a direct impact on the authorization of "normal" resources, so you'll be claiming one of @bparees's names.

@smarterclayton
Copy link
Copy Markdown
Contributor

I'm fine with admin/edit/view - david and I discussed and we could make the
argument that if you use subresource you still have to pick an arbitrary
verb (get, etc) which did not feel right. We may regret adding more verbs
later, but it's the cleanest mapping today.

On Mon, Aug 29, 2016 at 7:48 AM, David Eads notifications@github.com
wrote:

I think adding new verbs that are synonyms to existing verbs is risky from
a confusion point of view.

Ok, we'll need three verbs to give the three levels: admin, edit, view for
jenkins. I mapped them to common roles that happen to match names and be
verbs, but I'm open to other mappings.

Keep in mind that if you use get for any of them, you'll have a direct
impact on the authorization of "normal" resources, so you'll be claiming
one of @bparees https://github.com/bparees's names.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10649 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p6SOh9Xz-ZxwtCzAZ_00NEBpxMP3ks5qkscggaJpZM4JtaJl
.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 30, 2016

@bparees this is a safe change. You want it in 1.3?

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Aug 30, 2016

@deads2k i don't think we care, so it can wait.

@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 30, 2016

Spoke with @bparees in irc. This is needed for 1.3 compatibility with new jenkins images. [merge]

@mfojtik mfojtik added this to the 1.3.0 milestone Aug 30, 2016
@deads2k
Copy link
Copy Markdown
Contributor Author

deads2k commented Aug 30, 2016

[test]

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin test up to d1098eb

@openshift-bot
Copy link
Copy Markdown
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8553/)

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin merge up to d1098eb

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Aug 30, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8555/) (Image: devenv-rhel7_4954)

@openshift-bot openshift-bot merged commit 26e5ef8 into openshift:master Aug 30, 2016
@deads2k deads2k deleted the jenkins-roles branch September 6, 2016 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants