start bootstrapping cluster roles from kube #14026
Conversation
|
[test] |
deads2k
force-pushed
the
auth-12-upstream-roles
branch
from
372b355 to
e23441b
Compare
| role.Rules[j].Resources = authorizationapi.NormalizeResources(role.Rules[j].Resources) | ||
| } | ||
|
|
||
| // TODO roundtrip roles to pick up defaulting for API groups. Without this, the covers check in reconcile-cluster-roles will fail. |
There was a problem hiding this comment.
Conversion and defaulting are separate. You'll need to call kapi.Scheme.Default on the external version, won't you?
There was a problem hiding this comment.
They aren't separate here: https://github.com/openshift/origin/blob/master/pkg/authorization/api/v1/conversion.go#L95
Also this is pre-existing. Just moved it closer to its need.
|
Gotcha. I noticed it was a move after I wrote the comment.
…On Wed, May 3, 2017 at 12:44 PM, David Eads ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/cmd/server/bootstrappolicy/infra_sa_policy.go
<#14026 (comment)>:
> @@ -114,7 +118,28 @@ func (r *InfraServiceAccounts) addServiceAccount(saName string, role authorizati
role.Annotations = map[string]string{}
}
role.Annotations[roleSystemOnly] = roleIsSystemOnly
- r.saToRole[saName] = role
+
+ // TODO make this unnecessary
+ // we don't want to expose the resourcegroups externally because it makes it very difficult for customers to learn from
+ // our default roles and hard for them to reason about what power they are granting their users
+ for j := range role.Rules {
+ role.Rules[j].Resources = authorizationapi.NormalizeResources(role.Rules[j].Resources)
+ }
+
+ // TODO roundtrip roles to pick up defaulting for API groups. Without this, the covers check in reconcile-cluster-roles will fail.
They aren't separate here: https://github.com/openshift/
origin/blob/master/pkg/authorization/api/v1/conversion.go#L95
Also this is pre-existing. Just moved it closer to its need.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#14026 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAABYgMonEKrDp4KRf2XUISB5c9Zta9oks5r2K7YgaJpZM4NPp39>
.
|
deads2k
force-pushed
the
auth-12-upstream-roles
branch
2 times, most recently
from
cc41706 to
d7acdb1
Compare
deads2k
force-pushed
the
auth-12-upstream-roles
branch
from
d7acdb1 to
6306239
Compare
|
re[test] |
| kind: ClusterRole | ||
| metadata: | ||
| annotations: | ||
| rbac.authorization.kubernetes.io/autoupdate: "true" |
There was a problem hiding this comment.
Does origin do anything with this?
There was a problem hiding this comment.
Does origin do anything with this?
It will clone it back during the later sync: #14064
| creationTimestamp: null | ||
| labels: | ||
| kubernetes.io/bootstrapping: rbac-defaults | ||
| name: system:basic-user |
| creationTimestamp: null | ||
| labels: | ||
| kubernetes.io/bootstrapping: rbac-defaults | ||
| name: cluster-admin |
There was a problem hiding this comment.
So kube does not pluralize them like we do...
There was a problem hiding this comment.
So kube does not pluralize them like we do...
Right.
| out := []authorizationapi.ClusterRole{} | ||
| errs := []error{} | ||
|
|
||
| // add non-conflicting kube rbac roles |
There was a problem hiding this comment.
Comment does not make sense to me.
| out := []authorizationapi.ClusterRoleBinding{} | ||
| errs := []error{} | ||
|
|
||
| // add non-conflicting kube rbac roles |
There was a problem hiding this comment.
Comment does not make sense to me.
| extraWhitelistEntries := clusterRoleBindingConflicts.Difference(conflictingNames) | ||
| switch { | ||
| case len(extraRBACConflicts) > 0 && len(extraWhitelistEntries) > 0: | ||
| panic(fmt.Errorf("kube ClusterRoleBindings conflict with openshift ClusterRoleBindings: %v and ClusterRoleBinding whitelist contains a extraneous entries: %v ", extraRBACConflicts.List(), extraWhitelistEntries.List())) |
| extraRBACConflicts := conflictingNames.Difference(clusterRoleBindingConflicts) | ||
| extraWhitelistEntries := clusterRoleBindingConflicts.Difference(conflictingNames) | ||
| switch { | ||
| case len(extraRBACConflicts) > 0 && len(extraWhitelistEntries) > 0: |
There was a problem hiding this comment.
Not sure which is more correct: len(set) or set.Len().
There was a problem hiding this comment.
Not sure which is more correct: len(set) or set.Len().
len(set). It's native.
| case len(extraRBACConflicts) > 0 && len(extraWhitelistEntries) > 0: | ||
| panic(fmt.Errorf("kube ClusterRoleBindings conflict with openshift ClusterRoleBindings: %v and ClusterRoleBinding whitelist contains a extraneous entries: %v ", extraRBACConflicts.List(), extraWhitelistEntries.List())) | ||
| case len(extraRBACConflicts) > 0: | ||
| panic(fmt.Errorf("kube ClusterRoleBindings conflict with openshift ClusterRoleBindings: %v", extraRBACConflicts.List())) |
There was a problem hiding this comment.
Any reason you chose the panic the server route instead of failing a unit test somewhere?
There was a problem hiding this comment.
Any reason you chose the panic the server route instead of failing a unit test somewhere?
I've structured the code to allow both. Turns out, this should fail the unit test creating the list of bootstrap roles, right? Thought of that later.
deads2k
force-pushed
the
auth-12-upstream-roles
branch
from
6306239 to
ab36e11
Compare
|
comments addressed. This is needed for an approved pull: #14033 [merge] |
|
Evaluated for origin test up to ab36e11 |
|
Evaluated for origin merge up to ab36e11 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/583/) (Base Commit: 17ebd78) (Image: devenv-rhel7_6215) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1224/) (Base Commit: 17ebd78) |
This starts adding clusterroles and clusterrolebindings from kube RBAC rules as bootstrap origin roles. These will be plumbed to bootstrapping and to the reconcile commands.
This doesn't deprecate or remove any origin roles. Any origin role or binding takes priority.
Still todo:
dead.gofile to contain roles that we're retiring in favor of upstream roles. Upstream controllers for example.system:roles in openshift tosystem:openshift:orsystem:openshift:controller: