Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Separate build strategy permissions into distinct roles #8528

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Separate build strategy permissions into distinct roles #8528

merged 1 commit into from
Apr 16, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Apr 15, 2016 •
edited
Loading

Resolves #8526 by making build strategy permissions individually grantable/revocable. Removes build strategy permissions from the admin and edit roles and adds three default roles, one for each build strategy.

As before, all users that can create builds are granted permission to use all
build strategies (Docker, Source-to-Image, and Custom).

Each build strategy has a corresponding build subresource and role. A user must have permission to create a build and permission to create on the build strategy subresource in order to create builds using that strategy. Default roles are provided which grant the create permission on the build strategy subresource.

Strategy Subresource Role
Docker builds/docker system:build-strategy-docker
Source-to-Image builds/source system:build-strategy-source
Custom builds/custom system:build-strategy-custom

@liggitt
Copy link
Contributor Author

liggitt commented Apr 15, 2016

@csrwng @deads2k PTAL

I think I dislike this least of the options discussed in #8526 (comment)

@deads2k
Copy link
Contributor

deads2k commented Apr 15, 2016

Post 1.2?

deads2k
deads2k reviewed Apr 15, 2016
View reviewed changes
pkg/cmd/server/bootstrappolicy/policy.go
Rules: []authorizationapi.PolicyRule{
{
APIGroups: []string{api.GroupName},
Verbs: sets.NewString("create", "update"),
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't need update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@deads2k
Copy link
Contributor

deads2k commented Apr 15, 2016

I think I dislike this least of the options discussed in #8526 (comment)

Eh, less userfriendly in the end. Feels like the cheap way out to me.

deads2k
deads2k reviewed Apr 15, 2016
View reviewed changes
pkg/cmd/server/bootstrappolicy/policy.go
@@ -942,5 +979,23 @@ func GetBootstrapClusterRoleBindings() []authorizationapi.ClusterRoleBinding {
{Kind: authorizationapi.SystemGroupKind, Name: UnauthenticatedGroup},
},
},

// Grant all build strategy roles by default
// Cluster admins can remove these role bindings if they don't want them
Copy link
Contributor

Choose a reason for hiding this comment

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

add something like "and its "safe" because system:authenticated is excluded from clusterrolebinding reconciliation by default."

@liggitt liggitt changed the title Refactor build strategy permissions into distinct roles Separate build strategy permissions into distinct roles Apr 15, 2016
@deads2k
Copy link
Contributor

deads2k commented Apr 15, 2016

Minor comments, lgtm, but this release is supposed to be done.

@danmcp or @smarterclayton for approval.

@liggitt
Copy link
Contributor Author

liggitt commented Apr 15, 2016

[test]

@csrwng
Copy link
Contributor

csrwng commented Apr 15, 2016

LGTM

@smarterclayton
Copy link
Contributor

Approved for 1.2 given the security impact if we reset it.

@liggitt liggitt mentioned this pull request Apr 15, 2016
Merged
@smarterclayton
Copy link
Contributor 

hack/../test/cmd/policy.sh: line 48: os:cmd::expect_success: command not found
!!! Error in hack/../test/cmd/policy.sh:48
    'os:cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-custom system:authenticated'' exited with status 127
Call stack:
    1: hack/../test/cmd/policy.sh:48 main(...)
Exiting with status 1
!!! Error in hack/test-cmd.sh:300
    '${test}' exited with status 1
Call stack:
    1: hack/test-cmd.sh:300 main(...)
Exiting with status 1

@liggitt
Copy link
Contributor Author

liggitt commented Apr 16, 2016

"image cache/mysql:pullthrough not found" flake

@liggitt liggitt mentioned this pull request Apr 16, 2016
Closed
@liggitt
Copy link
Contributor Author

liggitt commented Apr 16, 2016

[merge]

@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 16, 2016 •
edited by liggitt
Loading

This flake is worrying me in that we have no idea why it is happening.
@soltysh we need to track this down - what other debug can we add to
identify the failure?

@liggitt
Copy link
Contributor Author

liggitt commented Apr 16, 2016

Image import flake:

FAILURE after 59.474s: test/cmd/newapp.sh:108: executing 'oc get imagestreamtags wildfly:latest' expecting success; re-trying every 0.2s until completion or 60.000s: the command timed out

[test]

@liggitt
Copy link
Contributor Author

liggitt commented Apr 16, 2016

UI flake on test, image import flake on merge
[test][merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5608/) (Image: devenv-rhel7_3984)

@liggitt
Copy link
Contributor Author

liggitt commented Apr 16, 2016

TestTryOrdering flake, [test][merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 3ae0a8d

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 3ae0a8d

@openshift-bot
Copy link
Contributor

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

@openshift-bot openshift-bot merged commit 0a5b12b into openshift:master Apr 16, 2016
@liggitt liggitt deleted the reconcile branch April 16, 2016 15:32
@smarterclayton
Copy link
Contributor

Fixed TestTryOrdering upstream in
kubernetes/kubernetes#24382

On Sat, Apr 16, 2016 at 11:00 AM, OpenShift Bot
[email protected] wrote:

Merged #8528.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@liggitt @deads2k @csrwng @smarterclayton @openshift-bot