add tokenreviews endpoint to implement webhook #28788

deads2k · 2016-07-11T19:13:03Z

Wires up an API resource under apis/authentication.k8s.io/v1beta1 to expose the webhook token authentication API as an API resource. This allows one API server to use another for authentication and uses existing policy engines for the "authoritative" API server to controller access to the endpoint.

@cjcullen you wrote the initial type

deads2k · 2016-07-11T20:05:36Z

The package had to move to make client generation work, which need to be generate to have kubectl available to call create.

deads2k · 2016-07-11T20:13:45Z

pkg/registry/tokenreview/storage.go

+)
+
+type REST struct {
+	tokenAuthenticator authenticator.Request


@kubernetes/kube-iam I was torn about this. We have an unknown number of token authenticators that are included in the Authentication chain. I could plumb through a set of union token authentications down through the masterconfig that are required to operate the same way as the authentication chain or I could create a fake request that only has a bearer token and count on the existing chain to do the right thing.

I chose to make the fake request, figuring it was easier to get right. My only concern is about whether some authenticator does weird things with requests it doesn't recognize.

thockin · 2016-07-12T05:58:20Z

Out of curiosity, why odes this one API have "k8s.io" in the name?

deads2k · 2016-07-12T11:48:53Z

Out of curiosity, why odes this one API have "k8s.io" in the name?

API groups were supposed to dns names so that they'd namespace nicely (like a java package) with prioritized completion for the CLI tooling. We have authorization.k8s.io, rbac.authorization.k8s.io, and authentication.k8s.io. See comment here: #24902 (comment)

The package folders were going to follow that, but there were some generator problems that were easier to resolve by eliminating the dots.

deads2k · 2016-07-12T12:11:27Z

@k8s-bot test this issue #26760

erictune · 2016-07-12T15:53:06Z

@kubernetes/sig-cluster-federation
This will be useful for delegating auth from the federation apiserver to the main apiserver of the cluster that hosts it.

deads2k · 2016-07-12T18:14:11Z

@smarterclayton I need help with protobuf, tests complained about not having it, but the generator seems decidedly unhappy about map[string][]string

smarterclayton · 2016-07-12T22:02:50Z

Yes there's an issue open. The optional protobuf work supports it - we
need to convert []String to a type, then use the "optional" field to handle
whether it's set. Cannot generate protobuf for map[string][]string,
blocks authorization from being generated
#27259

On Tue, Jul 12, 2016 at 3:02 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e build/test failed for commit 91a13dd
91a13dd
.

Test Results
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/28788/kubernetes-pull-build-test-e2e-gce/48728

Build Log
http://pr-test.k8s.io/28788/kubernetes-pull-build-test-e2e-gce/48728/build-log.txt

Test Artifacts
https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/28788/kubernetes-pull-build-test-e2e-gce/48728/artifacts/

Internal Jenkins Results
http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//48728

Please reference the list of currently known flakes
https://github.com/kubernetes/kubernetes/issues?q=is:issue+label:kind/flake+is:open
when examining this failure. If you request a re-test, you must reference
the issue describing the flake.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28788 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p57zN1hgAKxoyw7RGCpwGDUYPCULks5qU-S_gaJpZM4JJr0u
.

deads2k · 2016-07-20T19:47:15Z

comments addressed, now with happy generated code.

erictune · 2016-07-20T20:57:42Z

CJ okayed via side channel.

erictune · 2016-07-20T21:01:12Z

API looks good.

LGTM

erictune · 2016-07-20T21:01:32Z

@stts thank you for the careful code review.

k8s-github-robot · 2016-07-21T04:48:31Z

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

k8s-bot · 2016-07-21T05:23:30Z

GCE e2e build/test passed for commit 2256c6e.

k8s-github-robot · 2016-07-21T05:23:44Z

Automatic merge from submit-queue

wojtek-t · 2016-07-21T07:40:32Z

It seems that this PR broke GKE. After this PR was merged, I'm seeing a bunch of logs like these:

23:23:00 Created [https://test-container.sandbox.googleapis.com/v1/projects/k8s-jkns-e2e-gke-ci/zones/us-central1-f/clusters/jenkins-e2e].
23:23:00 kubeconfig entry generated for jenkins-e2e.
23:23:00 NAME         ZONE           MASTER_VERSION                    MASTER_IP       MACHINE_TYPE   NODE_VERSION                      NUM_NODES  STATUS
23:23:00 jenkins-e2e  us-central1-f  1.4.0-alpha.1.564+8ead63f1270455  146.148.90.159  n1-standard-2  1.4.0-alpha.1.564+8ead63f1270455  3          RUNNING
23:23:00 ... calling validate-cluster
23:23:01 failed to find client for version v1: error: You must be logged in to the server (the server has asked for the client to provide credentials)
23:23:01 (kubectl failed, will retry 2 times)
23:23:02 failed to find client for version v1: error: You must be logged in to the server (the server has asked for the client to provide credentials)
23:23:02 (kubectl failed, will retry 1 times)

I'm going to revert this one to fix the merge-queue.

Link to failure:
http://ci-test.k8s.io/kubernetes-e2e-gke/11616/

piosz · 2016-07-21T07:49:35Z

The interesting thing here is that GKE smoke tests passed. There might be some issue with testing infrastructure.

@kubernetes/test-infra-maintainers

wojtek-t · 2016-07-21T09:23:12Z

FYI - reverting the PR helped.

It seems to me that GKE smoke tests are not catching what they are supposed to catch. What are they actually doing?

wojtek-t · 2016-07-21T09:32:57Z

@kubernetes/test-infra-maintainers @kubernetes/goog-gke - since it was failing in verify-cluster, it clearly means that GKE smoke tests are configured differently than all other suites. This should be fixed.

cjcullen · 2016-07-22T00:53:38Z

pkg/apis/authentication/types.go

 // TokenReview attempts to authenticate a token to a known user.
 type TokenReview struct {
 	unversioned.TypeMeta
+	// ObjectMeta fulfills the meta.ObjectMetaAccessor interface so that the stock
+	// REST handler paths work
+	api.ObjectMeta


I don't have the logs to prove it, but I think that these additional fields are choking up our deserialization on the GKE server side. I'll see what we can do about that.

I don't have the logs to prove it, but I think that these additional fields are choking up our deserialization on the GKE server side. I'll see what we can do about that.

Thanks, let me know?

Confirmed that we do strict field checking on our side. I either need to figure out if we can turn that off, or add all of the fields from ObjectMeta to our server-side TokenReview representation.

Why does this PR add api.ObjectMeta to the TokenReview, but #20573 doesn't add it to SubjectAccessReview?

Why does this PR add api.ObjectMeta to the TokenReview, but #20573 doesn't add it to SubjectAccessReview?

When I did the SAR long ago it wasn't used. I'll probably have to add it there too.

@cjcullen any word on making the token review more tolerant?

@cjcullen

Automatic merge from submit-queue Token review endpoint Unrevert of #28788, which was rolled back because of #29375 @cjcullen @wojtek-t I'd like to remerge if possible. Have we gotten the field checking mentioned here relaxed? #28788 (comment)

@cjcullen

Automatic merge from submit-queue Token review endpoint Unrevert of #28788, which was rolled back because of kubernetes/kubernetes#29375 @cjcullen @wojtek-t I'd like to remerge if possible. Have we gotten the field checking mentioned here relaxed? kubernetes/kubernetes#28788 (comment)

ingvagabund · 2016-10-05T15:05:53Z

@deads2k not every file under pkg/client/clientset_generated was generated automatically, right? There are at least two that I believe were hand-crafted:

pkg/client/clientset_generated/internalclientset/typed/authentication/unversioned/fake/fake_generated_expansion.go
pkg/client/clientset_generated/internalclientset/typed/authentication/unversioned/generated_expansion.go

Or is there another command besides ./hack/update-codegen.sh that needs to be run?

ingvagabund · 2016-10-05T15:10:40Z

@lavalamp @wojtek-t @caesarxuchao any thoughts on that? I noticed there are other files that were hand-crafted. Or at least I could not find any template for them under cmd/libs/go2idl/client-gen directory. E.g.

pkg/client/clientset_generated/internalclientset/typed/core/unversioned/event_expansion.go
pkg/client/clientset_generated/internalclientset/typed/core/unversioned/namespace_expansion.go
pkg/client/clientset_generated/internalclientset/typed/core/unversioned/pod_expansion.go
pkg/client/clientset_generated/internalclientset/typed/authorization/unversioned/subjectaccessreview_expansion.go

I can open an issue on that if it makes sense or there is not already one for "not every file under clientset_generated is generated".

deads2k · 2016-10-05T15:47:36Z

I can open an issue on that if it makes sense or there is not already one for "not every file under clientset_generated is generated".

The expansions are there for the specific purpose of being hand crafted.

caesarxuchao · 2016-10-05T18:31:03Z

Right. Will comments in expansion files and doc.go be enough to make clear these are not generated? I can send a PR to do that.

ingvagabund · 2016-10-05T20:10:50Z

@deads2k thanks. I can confirm that by observation. Found 21 files under clientset_generated ending with _expansion.go that I needed to modify. 21 is still ok, though it can get annoying to do it regularly if their number starts to grow over time.

@caesarxuchao that would be great. Could the generator (./hack/update-codegen.sh) also report that? E.g. find all files under affected clientset_generated directories and list them with warning all of them are possible candidates for manual update.

googlebot added the cla: yes label Jul 11, 2016

deads2k force-pushed the wire-authentication branch 2 times, most recently from e8fa1dc to 40fee80 Compare July 11, 2016 19:14

k8s-github-robot assigned thockin Jul 11, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 11, 2016

deads2k force-pushed the wire-authentication branch from 40fee80 to 9689774 Compare July 11, 2016 20:04

deads2k reviewed Jul 11, 2016
View reviewed changes

deads2k force-pushed the wire-authentication branch from 9689774 to 45fae76 Compare July 11, 2016 20:16

thockin assigned cjcullen and unassigned thockin Jul 12, 2016

deads2k force-pushed the wire-authentication branch 3 times, most recently from 75f4f59 to 28ca5b1 Compare July 12, 2016 15:16

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 12, 2016

deads2k force-pushed the wire-authentication branch from 28ca5b1 to 91a13dd Compare July 12, 2016 18:13

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2016

erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2016

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2016

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Jul 20, 2016

k8s-github-robot merged commit 8ead63f into kubernetes:master Jul 21, 2016

wojtek-t mentioned this pull request Jul 21, 2016

Revert "add tokenreviews endpoint to implement webhook" #29361

Merged

deads2k mentioned this pull request Jul 21, 2016

GKE smoke tests don't match GKE cluster-up #29375

Closed

cjcullen reviewed Jul 22, 2016
View reviewed changes

deads2k mentioned this pull request Jul 29, 2016

Token review endpoint #29796

Merged

deads2k deleted the wire-authentication branch September 6, 2016 17:23

jianzhangbjz mentioned this pull request May 19, 2017

Cannot generate protobuf for map[string][]string #46024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tokenreviews endpoint to implement webhook #28788

add tokenreviews endpoint to implement webhook #28788

deads2k commented Jul 11, 2016

deads2k commented Jul 11, 2016

deads2k Jul 11, 2016

thockin commented Jul 12, 2016

deads2k commented Jul 12, 2016

deads2k commented Jul 12, 2016

erictune commented Jul 12, 2016

deads2k commented Jul 12, 2016

smarterclayton commented Jul 12, 2016

deads2k commented Jul 20, 2016

erictune commented Jul 20, 2016

erictune commented Jul 20, 2016

erictune commented Jul 20, 2016

k8s-github-robot commented Jul 21, 2016

k8s-bot commented Jul 21, 2016

k8s-github-robot commented Jul 21, 2016

wojtek-t commented Jul 21, 2016 •

edited by piosz

Loading

piosz commented Jul 21, 2016

wojtek-t commented Jul 21, 2016

wojtek-t commented Jul 21, 2016

cjcullen Jul 22, 2016

deads2k Jul 22, 2016

cjcullen Jul 22, 2016

deads2k Jul 22, 2016

liggitt Jul 26, 2016

ingvagabund commented Oct 5, 2016

ingvagabund commented Oct 5, 2016

deads2k commented Oct 5, 2016

caesarxuchao commented Oct 5, 2016

ingvagabund commented Oct 5, 2016 •

edited

Loading

add tokenreviews endpoint to implement webhook #28788

add tokenreviews endpoint to implement webhook #28788

Conversation

deads2k commented Jul 11, 2016

deads2k commented Jul 11, 2016

deads2k Jul 11, 2016

Choose a reason for hiding this comment

thockin commented Jul 12, 2016

deads2k commented Jul 12, 2016

deads2k commented Jul 12, 2016

erictune commented Jul 12, 2016

deads2k commented Jul 12, 2016

smarterclayton commented Jul 12, 2016

deads2k commented Jul 20, 2016

erictune commented Jul 20, 2016

erictune commented Jul 20, 2016

erictune commented Jul 20, 2016

k8s-github-robot commented Jul 21, 2016

k8s-bot commented Jul 21, 2016

k8s-github-robot commented Jul 21, 2016

wojtek-t commented Jul 21, 2016 • edited by piosz Loading

piosz commented Jul 21, 2016

wojtek-t commented Jul 21, 2016

wojtek-t commented Jul 21, 2016

cjcullen Jul 22, 2016

Choose a reason for hiding this comment

deads2k Jul 22, 2016

Choose a reason for hiding this comment

cjcullen Jul 22, 2016

Choose a reason for hiding this comment

deads2k Jul 22, 2016

Choose a reason for hiding this comment

liggitt Jul 26, 2016

Choose a reason for hiding this comment

ingvagabund commented Oct 5, 2016

ingvagabund commented Oct 5, 2016

deads2k commented Oct 5, 2016

caesarxuchao commented Oct 5, 2016

ingvagabund commented Oct 5, 2016 • edited Loading

wojtek-t commented Jul 21, 2016 •

edited by piosz

Loading

ingvagabund commented Oct 5, 2016 •

edited

Loading