Bug 1833558: create ClusteRole and ClusterRoleBinding when invoking oc adm policy add-scc-to-user #412

soltysh · 2020-05-08T21:59:06Z

This is new version of openshift/origin#22781 with some changes.
I'll need to update tests too.
/assign @stlaz

openshift-ci-robot · 2020-05-08T21:59:12Z

@soltysh: This pull request references Bugzilla bug 1833558, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1833558: create ClusteRole and ClusterRoleBinding when invoking oc adm policy add-scc-to-user

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

stlaz

A few comments and ideas.

I'm not sure about the situation around o.PrintFlags.OutputFormat (when they are unset), but the condition around dry-running a cluster-role creation seems wrong.

stlaz · 2020-05-11T10:43:54Z

pkg/cli/admin/policy/modify_scc.go

-	p, err := o.ToPrinter(message)
-	if err != nil {
-		return err
+	clusterRole := &rbacv1.ClusterRole{


It would make sense to have these clusterroles created already when the command is run. Manifests or bindata in KAS-o, depending on how SCCs are handled, that way the clusterroles would even be reconciled so we can always be sure that these commands don't have any side-effects.

I assume we need to keep the backward compatibility of oc so we'll still have to create these cluster roles for older clusters, but for the new ones, I would like to see these already created and maintained. You won't have to be able to create cluster roles in that case, too.

Hmm... I struggled with that thought, but ok. Lemme open a apiserver PR doing that too, and this will be a fallback in case we don't have that.

https://github.com/openshift/cluster-kube-apiserver-operator/tree/master/manifests could be a good spot for these as SCCs are already there

Hmm... I went with openshift/openshift-apiserver#108 since that seemed the most natural to me, scc belonging to openshift-apiserver.

Spoke with Standa on slack, I'll move this to kas-o, since SCCs CRDs are created there, so it's logical continuation.

openshift/cluster-kube-apiserver-operator#856

stlaz · 2020-05-11T11:00:17Z

pkg/cli/admin/policy/modify_scc.go

+		if err != nil {
+			return err
+		}
+		if o.DryRunStrategy == kcmdutil.DryRunClient || (o.PrintFlags.OutputFormat != nil && len(*o.PrintFlags.OutputFormat) > 0) {


no creating the clusterrole if an OutputFormat is specified?

Good catch, I copied that from modify_roles.go, which probably needs an update too.

stlaz · 2020-05-11T11:02:48Z

pkg/cli/admin/policy/modify_scc.go

+				return err
+			}
+		} else {
+			if _, err := o.RbacClient.ClusterRoles().Create(context.TODO(), clusterRole, metav1.CreateOptions{}); err != nil {


I wonder whether the apimachinery's CreateOptions.DryRun could be used here

Theoretically - yes, but we don't support that, yet.

stlaz · 2020-05-11T11:32:19Z

pkg/cli/admin/policy/modify_scc.go

+	addSubjects := RoleModificationOptions{
+		RoleKind:        clusterRole.Kind,
+		RoleName:        clusterRole.Name,
+		RoleBindingName: fmt.Sprintf(RBACNamesFmt, o.SCCName),


With the way SCCs originally worked, namespacing access to an SCC was not possible, but we can add namespaced access now by passing in RoleBindingNamespace.

soltysh · 2020-05-13T11:55:28Z

@stlaz comments addressed, ptal
I'll open a PR against origin and apiserver.

soltysh · 2020-05-13T13:20:35Z

Origin test changes in openshift/origin#24974

stlaz · 2020-05-13T13:21:37Z

pkg/cli/admin/policy/modify_scc.go

+		RoleKind:             "ClusterRole",
+		RoleName:             fmt.Sprintf(RBACNamesFmt, o.SCCName),
+		RoleBindingName:      fmt.Sprintf(RBACNamesFmt, o.SCCName),
+		RoleBindingNamespace: o.DefaultSubjectNamespace,


the default should still be cluster-scoped, add -n option make this available

Fixed in latest push.

soltysh · 2020-05-13T13:30:30Z

~~apiserver change in openshift/openshift-apiserver#108~~
replaced with openshift/cluster-kube-apiserver-operator#856

…c adm policy add-scc-to-user

stlaz · 2020-05-14T06:53:01Z

/lgtm
/retest

openshift-ci-robot · 2020-05-14T06:53:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-05-14T08:42:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T08:54:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T10:25:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T12:09:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T13:55:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T16:16:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T18:26:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T20:23:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-14T22:23:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T00:04:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T02:26:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T02:39:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T03:57:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T05:29:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T07:13:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T07:51:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T08:56:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T10:41:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T10:54:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T12:38:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-15T12:51:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

soltysh · 2020-05-15T13:22:26Z

/hold

soltysh · 2020-05-28T12:36:46Z

This is waiting for openshift/origin#25032 to be able to pass e2e-cmd

soltysh · 2020-05-28T20:29:41Z

/hold cancel
/retest

openshift-bot · 2020-05-29T02:51:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-05-29T04:35:51Z

@soltysh: Some pull requests linked via external trackers have merged: openshift/origin#25032, openshift/oc#412. The following pull requests linked via external trackers have not merged:

openshift/cluster-kube-apiserver-operator#856 is open
Bugzilla bug 1833558 has been moved to the MODIFIED state.

In response to this:

Bug 1833558: create ClusteRole and ClusterRoleBinding when invoking oc adm policy add-scc-to-user

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot assigned stlaz May 8, 2020

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label May 8, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 8, 2020

openshift-ci-robot requested review from mfojtik and smarterclayton May 8, 2020 21:59

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2020

stlaz reviewed May 11, 2020

View reviewed changes

soltysh force-pushed the scc_to_rbac branch from 0511999 to aec1ec9 Compare May 13, 2020 11:54

stlaz reviewed May 13, 2020

View reviewed changes

Bug 1833558: create ClusteRole and ClusterRoleBinding when invoking o…

aff24a5

…c adm policy add-scc-to-user

soltysh force-pushed the scc_to_rbac branch from aec1ec9 to aff24a5 Compare May 13, 2020 13:38

soltysh mentioned this pull request May 13, 2020

Bug 1833558: update test to be more resilient for oc adm policy add-scc-to-user changes openshift/origin#24974

Closed

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2020

openshift-ci-robot mentioned this pull request May 28, 2020

Bug 1833558: update test to be more resilient for oc adm policy add-sss-to-user changes openshift/origin#25032

Merged

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2020

openshift-merge-robot merged commit 0087eb3 into openshift:master May 29, 2020

soltysh deleted the scc_to_rbac branch May 29, 2020 05:04

openshift-ci-robot mentioned this pull request May 29, 2020

Bug 1833558: create cluster roles for modiyfing scc-s openshift/cluster-kube-apiserver-operator#856

Merged

Bug 1833558: create ClusteRole and ClusterRoleBinding when invoking oc adm policy add-scc-to-user #412

Bug 1833558: create ClusteRole and ClusterRoleBinding when invoking oc adm policy add-scc-to-user #412

Conversation

soltysh commented May 8, 2020

openshift-ci-robot commented May 8, 2020

stlaz 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

soltysh commented May 13, 2020

soltysh commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented May 13, 2020 • edited

stlaz commented May 14, 2020

openshift-ci-robot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 14, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

openshift-bot commented May 15, 2020

soltysh commented May 15, 2020

soltysh commented May 28, 2020

soltysh commented May 28, 2020

openshift-bot commented May 29, 2020

openshift-ci-robot commented May 29, 2020

soltysh commented May 13, 2020 •

edited