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

switch rbac to external #63967

Merged
merged 2 commits into from May 22, 2018

Conversation

@deads2k
Copy link
Contributor

deads2k commented May 17, 2018

The overall trajectory of the project is towards external types. Having all helpers agree on the version they operate on makes life much easier. We've already written one RBAC controller (role aggregation) and more may follow. v1 has been around for a while now and we know that any future changes have to reliably roundtrip through it. This pull switches all the core helpers over to use the external types.

@kubernetes/sig-auth-pr-reviews

`kubectl auth reconcile` only works with rbac.v1
@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 17, 2018

/retest

@ericchiang
Copy link
Member

ericchiang left a comment

Why the changes to pkg/registry/rbac? Was something else importing it?

@@ -199,17 +199,20 @@ func startMasterOrDie(masterConfig *master.Config, incomingServer *httptest.Serv
closeFn()
glog.Fatal(err)
}
var lastHealthContent []byte

This comment has been minimized.

@ericchiang

ericchiang May 17, 2018

Member

Is this supposed to be part of the PR?

This comment has been minimized.

@deads2k

deads2k May 17, 2018

Author Contributor

Is this supposed to be part of the PR?

I was chasing a typo, it came in handy for debugging. I suspect I won't be the last.

@deads2k deads2k changed the title [WIP] switch rbac to external switch rbac to external May 18, 2018

@deads2k deads2k force-pushed the deads2k:rbac-06-external branch from c1746cc to ce974c9 May 18, 2018

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 18, 2018

I think this is ready now.

@@ -59,7 +60,7 @@ func (s *Storage) Create(ctx context.Context, obj runtime.Object, createValidati
return s.StandardStorage.Create(ctx, obj, createValidation, includeUninitialized)
}

rules, err := s.ruleResolver.GetRoleReferenceRules(clusterRoleBinding.RoleRef, metav1.NamespaceNone)
rules, err := s.ruleResolver.GetRoleReferenceRules(rbacv1helpers.ConvertOrDieRoleRef(clusterRoleBinding.RoleRef), metav1.NamespaceNone)

This comment has been minimized.

@liggitt

liggitt May 18, 2018

Member

___OrDie inside a rest handler isn't something I'd expect

This comment has been minimized.

@deads2k

deads2k May 18, 2018

Author Contributor

___OrDie inside a rest handler isn't something I'd expect

I'm willing to change it, but it is a little difficult to come up with a way it won't round trip.

@@ -161,13 +166,13 @@ func (o *ReconcileOptions) RunReconcile() error {
}

switch t := info.Object.(type) {
case *rbac.Role:
case *rbacv1.Role:

This comment has been minimized.

@liggitt

liggitt May 18, 2018

Member

does this mean kubectl auth reconcile only works on rbac/v1 resources? that's notable

This comment has been minimized.

@deads2k

deads2k May 18, 2018

Author Contributor

does this mean kubectl auth reconcile only works on rbac/v1 resources? that's notable

Yes. It is consistent with the idea of kubectl moving to external versions and skewing only by a release.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 18, 2018

@sttts any idea what I've done with this generated deep copy?

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 18, 2018

/retest

@deads2k deads2k force-pushed the deads2k:rbac-06-external branch from ce974c9 to b554631 May 21, 2018

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 21, 2018

Added release note, removed use of OrDie.

Might have to chase the generation ghost

@deads2k deads2k force-pushed the deads2k:rbac-06-external branch from b554631 to 3ad82a7 May 21, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 21, 2018

switch to external looks good
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 21, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 21, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.


// Code generated by conversion-gen. DO NOT EDIT.

package v1

This comment has been minimized.

@sttts

sttts May 22, 2018

Contributor

this is pretty empty

This comment has been minimized.

@sttts

sttts May 22, 2018

Contributor

I guess you want a doc.go with +// +k8s:deepcopy-gen=package.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented May 22, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

deads2k added some commits Apr 19, 2018

@deads2k deads2k added lgtm and removed lgtm labels May 22, 2018

@deads2k deads2k force-pushed the deads2k:rbac-06-external branch from 3ad82a7 to ff743c7 May 22, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 22, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 22, 2018

New changes are detected. LGTM label has been removed.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 22, 2018

Alright, think I sorted out my generation problem, retagged.

@deads2k deads2k added the lgtm label May 22, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 22, 2018

Automatic merge from submit-queue (batch tested with PRs 62025, 63851, 64077, 63967, 63991). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2ff0bc2 into kubernetes:master May 22, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation deads2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@deads2k deads2k deleted the deads2k:rbac-06-external branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.