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.

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

Add NormalizePolicyRules to authorizationsync #14475

Merged

Conversation

enj
Copy link
Contributor

@enj enj commented Jun 5, 2017

NormalizePolicyRules lowercases APIGroups, Verbs and Resources. By using it before persistence, all RBAC roles will work with Kubernetes' case sensitive authorizer.

Fixes #13429

Supersedes #14410

[test]

Signed-off-by: Monis Khan mkhan@redhat.com

cc @deads2k

@deads2k
Copy link
Contributor

deads2k commented Jun 6, 2017

lgtm, but fix your cache mutation problem here: https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1933/

@@ -75,6 +75,9 @@ func (c *OriginClusterRoleToRBACClusterRoleController) syncClusterRole(name stri
return c.rbacClient.ClusterRoles().Delete(name, nil)
}

// normalize rules before conversion so RBAC's case sensitive authorizer will work
NormalizePolicyRules(originClusterRole.Rules)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mutating a cache. Seems like you'd do this on the RBAC rule after you converted and copied.

NormalizePolicyRules lowercases APIGroups, Verbs and Resources.  By
using it before persistence, all RBAC roles will work with Kubernetes'
case sensitive authorizer.

Signed-off-by: Monis Khan <mkhan@redhat.com>
@enj enj force-pushed the enj/i/normalize_policy_controllers/13429 branch from 96125e0 to 84385aa Compare June 6, 2017 19:48
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 84385aa

@enj
Copy link
Contributor Author

enj commented Jun 6, 2017

@deads2k cache mutation should be fixed now.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1985/) (Base Commit: 6a2194d)

@deads2k
Copy link
Contributor

deads2k commented Jun 7, 2017

@deads2k cache mutation should be fixed now.

@enj Minor changes on an already lgtm'ed pull don't need a recheck for committers. Use your judgement, but if you need a recheck on something subtle, please call it out.

[merge]

@enj
Copy link
Contributor Author

enj commented Jun 10, 2017

Flake #14496

@openshift openshift deleted a comment from openshift-bot Jun 10, 2017
@openshift openshift deleted a comment from openshift-bot Jun 10, 2017
@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 84385aa

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 10, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/947/) (Base Commit: cf830c4) (Image: devenv-rhel7_6338)

@openshift-bot openshift-bot merged commit d79627d into openshift:master Jun 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants