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

plugin/pkg/auth: add webhook implementation of authorizer.Authorizer plugin #20347

Merged
merged 3 commits into from Feb 27, 2016

Conversation

ericchiang
Copy link
Contributor

Following up on the proposal in #19828 this is an implementation of the authorizer.Authorizer plugin which calls out to a remote gRPC service to drive authorization. The details are in that issues, but in summary we (CoreOS) would like a way to build services that drive authentication separate from k8s.

Due to the current vendored state of the protobuf and grpc packages, I've had to cherry pick updates from #19931 to generate code that would compile. That PR is not passing tests, but I'd still like feedback on this PR and will update the commits when #19931 is resolved.

cc @philips @sym3tri @bobbyrullo

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@k8s-bot
Copy link

k8s-bot commented Jan 29, 2016

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Jan 29, 2016

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 29, 2016
@ericchiang ericchiang force-pushed the authz_grpc branch 2 times, most recently from d0d5862 to ef512d2 Compare January 29, 2016 23:15
@ericchiang ericchiang changed the title plugin/pkg/auth: add gRPC implementation of authorizer.Authorizer plugin [WIP] plugin/pkg/auth: add gRPC implementation of authorizer.Authorizer plugin Jan 29, 2016
@k8s-github-robot
Copy link

PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2016
@davidopp davidopp assigned erictune and unassigned davidopp Feb 1, 2016
@davidopp
Copy link
Member

davidopp commented Feb 1, 2016

Reassigning to @erictune

cc/ @lavalamp @smarterclayton @bgrant0607

@smarterclayton
Copy link
Contributor

This is definitely one of those things that start to make it desirable to have out of tree plugins compiled easily into our build process. If every authorizer in the world is compiled in, we're going to have massive binaries (the cloud providers today are already kind of large)

@bobbyrullo
Copy link
Contributor

@smarterclayton True. But it would be nice if (most? all?) plugins shipped with a GRPC (or whatever RPC) implementation; then it wouldn't matter where they lived and people could easily develop plugins without getting bogged down learning k8s internals.

@philips
Copy link
Contributor

philips commented Feb 1, 2016

@smarterclayton I am confused by your feedback. You are opposed to having stuff compiled into k8s but you want out of tree plugins compiled into the build system?

Is your feedback that you would like to see a way for someone to take the FooAuthz external plugin and compile it into the k8s API process instead of having to launch a FooAuthz service separately on cluster?

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 1, 2016 via email

@smarterclayton
Copy link
Contributor

I'm probably OK with extensions that clearly mirror Kube interfaces, but I am concerned by grpc versions across this plus etcd plus others. I also don't know that authorizer is ready to be totally stable forever (i.e. what backcompat guarantees do we provide). What guarantees do you guys think you need to make this useful? Minor version compat? Additive only changes?

@smarterclayton
Copy link
Contributor

We've talked about doing this for admission control as well, and an http implementation would mirror this with the exception that we would need to use external versioned structs and declare a formal support policy for how we change those.

@bobbyrullo
Copy link
Contributor

... we're pushing limits of how many external packages can be
imported

I totally get your point here; we were hoping that by introducing a network plugin, we can others could implement plugins as services, and thus reduce the need for external packages.

Is the goal here to ensure the exact Kube binaries released have built
in plugins that talk to specific CoreOS auth tools? Or that you can
maintain and ship a distribution of Kube that compiles in these
features easily? The latter has clear value to everyone, the former is
less clear.

Neither, I think. Definitely not the former - the auth tool that this GRPC plugin would talk to is yet to be born. And neither do we really want to distribute our own Kubernetes - we want to contribute to upstream, and ensure that our tools work with upstream.

In this case it's not clear what a well-loved standard for authz is, so instead of baking our own protocol and trying to get that mainlined into Kubernetes, it seems more generally useful to have a network based plugin that mirrors the kubernetes Go interface.

@bobbyrullo
Copy link
Contributor

Is there a build process that keeps this up to date

@ericchiang - you probably should make this go-generate-able, yes?

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 1, 2016 via email

@bobbyrullo
Copy link
Contributor

The bar is typically a bit higher

Sorry, lost context there - which bar?

@lavalamp
Copy link
Member

lavalamp commented Feb 2, 2016

My first thought is that we don't use gRPC anywhere else, and it seems arbitrary to start using it here.

@lavalamp
Copy link
Member

lavalamp commented Feb 2, 2016

Baking in an RPC is a big support/compatibility commitment. We don't currently have any outgoing RPCs or webhooks, although we've definitely talked about webhooks, and specifically about auth webhooks. So thanks in advance for bearing with us if we spend a lot of time here arguing discussing.

@ericchiang
Copy link
Contributor Author

My first thought is that we don't use gRPC anywhere else, and it seems arbitrary to start using it here.

I've used gRPC because we prefer it to REST, but the overall goal is to provide a webhook like mechanism for authz. If it would fit into the existing project better, REST would be fine.

Baking in an RPC is a big support/compatibility commitment. We don't currently have any outgoing RPCs or webhooks, although we've definitely talked about webhooks, and specifically about auth webhooks. So thanks in advance for bearing with us if we spend a lot of time here arguing discussing.

Since the existing API already has a lot of good language around API stability / compatibility would it be more reasonable to rethink this as an API addition of a REST webhook? That would allow us to add swagger definitions for documentation (rather than pointing others to a .proto file) and piggyback off of established rules for API groups and deprecation.

@erictune
Copy link
Member

erictune commented Feb 2, 2016

@cjcullen you have been looking at adding a generic REST authorizer.

You might be interested in this PR. If this PR pivots from gRPC to REST, as has been suggested, you might want to collaborate to see if you can use the same REST impl.

Also wonder if the remote service you are targeting would prefer to be called via gRPC over REST? If so, that might sway the whole "lets not link in gRPC" conversation.

@ericchiang
Copy link
Contributor Author

Also wonder if the remote service you are targeting would prefer to be called via gRPC over REST? If so, that might sway the whole "lets not link in gRPC" conversation.

gRPC is not necessary for the remote service.

Since the gRPC part of this PR seems to be the main issue, and because there's already been discussion of the REST implementation, I'll go ahead and rework this to use REST.

@k8s-bot
Copy link

k8s-bot commented Feb 18, 2016

GCE e2e test build/test passed for commit 7a436c554370470649f5a67953eb06b03e267c3e.

@k8s-github-robot
Copy link

PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 22, 2016

GCE e2e test build/test passed for commit b50ede6.

@ericchiang
Copy link
Contributor Author

@erictune would love to see this in 1.2 (noticed it's a v1.2-candidate). Please let me know if you have any further changes you'd like to see.

@erictune
Copy link
Member

lgtm

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2016
@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Feb 24, 2016

GCE e2e test build/test passed for commit b50ede6.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Feb 25, 2016

GCE e2e test build/test passed for commit b50ede6.

@derekwaynecarr
Copy link
Member

@k8s-bot test this issue #IGNORE

@k8s-bot
Copy link

k8s-bot commented Feb 25, 2016

GCE e2e test build/test passed for commit b50ede6.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Feb 27, 2016

GCE e2e build/test passed for commit b50ede6.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Feb 27, 2016
@k8s-github-robot k8s-github-robot merged commit 00d99ac into kubernetes:master Feb 27, 2016
@erictune erictune modified the milestones: v1.2, v1.2-candidate Mar 3, 2016
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jul 18, 2018
UPSTREAM: 62943: set updated replicas in statefulsets

Origin-commit: caded921683fb178f30744cd1d19501d74ad1ef8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet