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

Migrate to Kubernetes RBAC #15021

Merged
merged 18 commits into from
Aug 17, 2017
Merged

Migrate to Kubernetes RBAC #15021

merged 18 commits into from
Aug 17, 2017

Conversation

simo5
Copy link
Contributor

@simo5 simo5 commented Jul 3, 2017 •
edited by enj
Loading

Trello xref: https://trello.com/c/n3bR3Ys9

Fixes #12303
Fixes #13549
Fixes #13432
Fixes #15338
Fixes #14168
Fixes #10056

Need to investigate:

  • ...

Dependencies:

Followups:

Done:

  • Store ClusterRoles as native RBAC Objects via Kubernetes.
  • Provides backwards compatible API for the old policy based roles.
  • Use Kubernetes authorizer

TODO:

  • Delete policy end points
  • Decide what to do with overwrite policy
  • Remove or gate oc create policybinding
  • Move new impersonation code to pkg/auth/client/impersonate.go
  • Remove any unnecessary conversions
  • Review new proxy.go files
  • Remove reason logic allowed by rule in ...
  • Add interface assertion to proxy files
  • Confirm we need pkg/authorization/util/convert/convert.go
  • Confrim we need to expose some of the private conversion functions
  • Add protect/autoupdate annotation conversion to general conversion functions
  • Support watch on proxied endpoints
  • Cherry pick Status objects for 404 API errors will have the correct APIVersion kubernetes/kubernetes#49868 -> Update etcd stores to use DefaultQualifiedResource #15721
  • Fix upstream commits
  • Restore and version gate NewCmdMigrateAuthorization
  • Wrap other errors in proxy files? Remove all error wrapping
  • Make NewImpersonatingRBACFromContext more generic
  • Kube authorizer's reason on deny contains evaluation errors - do we want to preserve those?
  • Review ImpersonatingRESTClient in pkg/auth/client/impersonate.go
  • Review k8s.io/kubernetes/staging/src/k8s.io/client-go/rest/request.go
  • Review pkg/project/auth/cache.go and pkg/project/auth/cache_test.go
  • Review pkg/authorization/authorizer/scope/converter_test.go

@simo5 simo5 requested review from enj and deads2k July 5, 2017 08:29
@simo5 simo5 force-pushed the RBAC1 branch 2 times, most recently from 6e5f1c3 to 5e13b42 Compare July 5, 2017 09:04
@simo5 simo5 changed the title WIP: Proxy Cluster Roles to Native Kube RBAC Cluster Roles WIP: Proxy {Cluster}Role{Binding}s to Native Kube RBAC APIs Jul 5, 2017
@simo5 simo5 changed the title WIP: Proxy {Cluster}Role{Binding}s to Native Kube RBAC APIs Proxy {Cluster}Role{Binding}s to Native Kube RBAC APIs [do not merge] Jul 5, 2017
@simo5
Copy link
Contributor Author

simo5 commented Jul 5, 2017

Let's see if this causes more [test]s to be run

@simo5
Copy link
Contributor Author

simo5 commented Jul 5, 2017

Try again [test]

deads2k
deads2k reviewed Jul 5, 2017
View reviewed changes
pkg/authorization/registry/clusterrole/proxy/proxy.go Outdated
roleregistry "github.com/openshift/origin/pkg/authorization/registry/role"
rolestorage "github.com/openshift/origin/pkg/authorization/registry/role/policybased"
"github.com/openshift/origin/pkg/authorization/rulevalidation"
authzapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
Copy link
Contributor

Choose a reason for hiding this comment

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

canonical in this project is authorizationapi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it initally to make sure I was catching everything (compile error if not)
I can put it back, but man you guys love LOOOOOOOONG lines ...

deads2k
deads2k reviewed Jul 5, 2017
View reviewed changes
pkg/authorization/registry/clusterrole/proxy/proxy.go Outdated
)

type ClusterRoleStorage struct {
roleStorage rolestorage.VirtualStorage
func rbacToClusterRole(in *rbac.ClusterRole) (authzapi.ClusterRole, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Warnings here about this not being a deep copy. Seems like you'll want this helper colocated with your conversion functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why deep copy would be needed ?

deads2k
deads2k reviewed Jul 5, 2017
View reviewed changes
pkg/authorization/registry/clusterrole/proxy/proxy.go Outdated
return nil, err
}
return authorizationapi.ToClusterRoleList(ret.(*authorizationapi.RoleList)), err
roles, err := s.client.List(optv1)
Copy link
Contributor

Choose a reason for hiding this comment

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

you definitely need to impersonate or you'll have disabled all the escalation checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there is a reason this is not for merge yet :)

deads2k
deads2k reviewed Jul 5, 2017
View reviewed changes
pkg/authorization/registry/clusterrole/proxy/proxy.go Outdated
}

func (m *ClusterRoleStorage) CreateRoleWithEscalation(ctx apirequest.Context, obj *authorizationapi.Role) (*authorizationapi.Role, error) {
return m.roleStorage.CreateRoleWithEscalation(ctx, obj)
// FIXME: what's escalation exactly ?
Copy link
Contributor

Choose a reason for hiding this comment

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

it bypasses permissions coverage checks. Shouldn't be needed with RBAC

deads2k
deads2k reviewed Jul 5, 2017
View reviewed changes
pkg/authorization/registry/clusterrole/proxy/proxy.go Outdated
)

type ClusterRoleStorage struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

You need these old types in a "legacy" bucket used behind a version check inside of the openshift reconcile command until we drop support for the tool against old servers.

@simo5 simo5 force-pushed the RBAC1 branch 3 times, most recently from 998d5c5 to 10e565a Compare July 8, 2017 14:04
@simo5
Copy link
Contributor Author

simo5 commented Jul 10, 2017

@deads2k can you check if this way of impersonating works for you ? see the utility file with the impertonatingRESTClient stuff

@simo5 simo5 force-pushed the RBAC1 branch 2 times, most recently from f7cf91d to 300ce8e Compare July 11, 2017 12:52
@simo5
Copy link
Contributor Author

simo5 commented Jul 11, 2017

Please re [test]

@simo5 simo5 force-pushed the RBAC1 branch 5 times, most recently from 969abda to 7df8d1c Compare July 18, 2017 12:36
simo5 and others added 15 commits August 16, 2017 22:09
@simo5 @enj
Cleanup: Move conversion function
9811649 
Put it into a utility package for reusability, we'll need it
in other places in the next few commits

Signed-off-by: Simo Sorce <[email protected]>
@simo5 @enj
UPSTREAM: 50639: Extend SetHeader Requests method ito accept multiple…
230db26 
… values

This allows to set headers that are multivalued directly.
The headers variable is not directly accessible and currently
SetHeaders allows to set only one value.

Signed-off-by: Simo Sorce <[email protected]>
@simo5 @enj
Proxy {Cluster}Role{Binding}s to Native Kube RBAC
c96408a 
Store them as native RBAC Objects via Kubernetes.
Also:
- Provides backwards compatible Openshift API.
- Kills Policy Sync Controller
- Removes init of PolicyRegistry
- Move helpers closer to their users
- Remove TestRBACController
- Remove tests that check only PolicyBindings related stuff
- hack around TestAuthorizationResolution

Signed-off-by: Simo Sorce <[email protected]>
Signed-off-by: Monis Khan <[email protected]>
@enj
Use dynamic error wrapper on proxied endpoints
f080a1b 
Signed-off-by: Monis Khan <[email protected]>
@enj
UPSTREAM: 50702: Allow injection of policy in RBAC post start hook
1bb9a94 
This change allows the RBAC PostStartHook logic to be reused with
different policy data when bootstrapping the cluster.  Thus any
changes to the bootstrap logic are separated from the policy data.

Signed-off-by: Monis Khan <[email protected]>
@enj
Bootstrap Origin policies in post start hook
d278ea6 
As we now use K8s' Rbac we need to bootstrap Origin's own
additional policies into kube's rbac objects.
Also:
- Push conversions one step toward the edges
- Fix conversion and dont make policy.json
- delete TestBootstrapPolicyOverwritePolicyCommand for now

Signed-off-by: Monis Khan <[email protected]>
Signed-off-by: Simo Sorce <[email protected]>
@simo5 @enj
Version gate legacy oc commands to < 3.7
acd6597 
The following commands were version gated:

- oc create policybinding
- oc adm overwrite-policy
- oc adm migrate authorization

This is because in 3.7 we will store only k8s.io RBAC objects and
not the Origin Policy Objects.  All of the gated commands assume the
presence of policy objects, and thus it does not make sense to let
users run these against newer clusters.

Signed-off-by: Simo Sorce <[email protected]>
Signed-off-by: Monis Khan <[email protected]>
@enj
UPSTREAM: 50710: Refactor RBAC authorizer entry points
c7d2994 
This change refactors various RBAC authorizer functions to be more
flexible in their inputs.  This makes it easier to reuse the various
components that make up the authorizer.

Signed-off-by: Monis Khan <[email protected]>
@simo5 @enj
Change authorizer to use Kubernetes facilities
a94e0db 
- Origin Authorizer now a thin wrapper around k8s Rbac Authorizer
- Remove redundant authorizer tests
  Now that we delegate nearly 100% of the authorizer work to the
  upstream Rbac authorizer these tests are basically redundant,
  as upstream already has its battery of tests.

Signed-off-by: Simo Sorce <[email protected]>
Signed-off-by: Monis Khan <[email protected]>
@enj
UPSTREAM: 49902: Allow update to GC fields for RBAC resources
f0704fd 
This change makes it so that no escalation check is performed when
updating only the garbage collector fields (owner references and
finalizers) of RBAC resources.  This allows the
garbage collector to delete roles that grant permissions such
as "create", which it will never have.

Signed-off-by: Monis Khan <[email protected]>
@enj
Update admission to use moved GC helper
8cf692d 
Signed-off-by: Monis Khan <[email protected]>
@simo5 @enj
flakes fixes
c51da07 
- Increase timeout to avoid TestGCDefault flaking
  Locally raising timeout from 1 to 2 seconds made the test always
  pass, as opposed to always fail.

- Make TestOadmPodNetwork less flakey

Signed-off-by: Simo Sorce <[email protected]>
Signed-off-by: Monis Khan <[email protected]>
@enj
test/cmd fixes
806d7be 
- Fix test/cmd/admin.sh
- Fix images-old-policy
- Fix test/cmd/policy.sh
- Temp fix for test/cmd/router.sh

Signed-off-by: Monis Khan <[email protected]>
@simo5 @enj
(re)generated stuff
be05ce5
@simo5 @enj
Handle reconciliation annotation during conversion
33c4fc3 
Signed-off-by: Simo Sorce <[email protected]>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017
@enj
Copy link
Contributor

enj commented Aug 17, 2017

/test extended_builds
/test extended_gssapi
/test extended_image_ecosystem
/test extended_ldap_groups
/test extended_templates

@enj enj changed the title [DO NOT MERGE] Migrate to Kubernetes RBAC Migrate to Kubernetes RBAC Aug 17, 2017
@enj
Copy link
Contributor

enj commented Aug 17, 2017

/retest

@deads2k
Copy link
Contributor

deads2k commented Aug 17, 2017

@smarterclayton we talked about this yesterday. It needs the extra bake time.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, simo5

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2017
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit edf3ff5 into openshift:master Aug 17, 2017
@openshift-ci-robot
Copy link

@simo5: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/check a535c38 link /test check
ci/openshift-jenkins/extended_image_ecosystem 33c4fc3 link /test extended_image_ecosystem

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@enj enj mentioned this pull request Aug 18, 2017
Closed
@benjaminapetersen benjaminapetersen mentioned this pull request Aug 29, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh left review comments

@liggitt liggitt liggitt left review comments

@enj enj enj left review comments

@deads2k deads2k deads2k left review comments

Assignees

@deads2k deads2k

@simo5 simo5

Labels
api-approved approved Indicates a PR has been approved by an approver from all required OWNERS files. area/security component/auth lgtm Indicates that a PR is ready to be merged. needs-api-review queue/multiple-rebases size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v3.7