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

Document proxy flows #12305

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Jan 21, 2019

Document flows of proxy requests for aggregated API.

As discussed with @liggitt .

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 21, 2019
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language labels Jan 21, 2019

Note that it is the responsibility of the extension server implementation to provide the above. Many do it by default, leveraging the `k8s.io/apiserver/` package. Others may provide options to override it using command-line options.

### Extension Server Authorize the Request
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liggitt - this is the part I don't get. Didn't it already authorize the request at the first step?

Copy link
Member

Choose a reason for hiding this comment

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

from the extension API server's perspective, all that has been proven is the identity of the incoming request (authentication, not authorization). The auth proxy might be the aggregator which already ran an authorization check, but it could just as easily be a generic authenticating proxy that only checked identity and added user info into headers.

@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Jan 21, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 9cc9b4c

https://deploy-preview-12305--kubernetes-io-master-staging.netlify.com

@bradtopol bradtopol requested a review from liggitt January 21, 2019 21:57
@bradtopol bradtopol self-assigned this Jan 21, 2019
@bradtopol
Copy link
Contributor

bradtopol commented Jan 21, 2019

hi @liggitt would you be willing to serve as the technical reviewer of this content? Simply add a /lgtm when you are satisfied with its technical accuracy. Thanks!!!

@deitch deitch force-pushed the describe-agg-auth-flow branch from c092266 to 90f79a0 Compare January 22, 2019 14:06

## Authentication Flow

Unlike Custom Resource Definitions (CRDs), the Aggregation API involves another server - your Extension API Server - in addition to the standard Kubernetes apiserver. The Kubernetes apiserver will need to communicate with your extension server, and your extension server will need to communicate with the Kubernetes apiserver. In order for this communication to be secured, the apiserver uses x509 certificates to authenticate itself to the extension server.
Copy link
Member

Choose a reason for hiding this comment

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

nit: suggest API server over API Server and apiserver

Copy link
Member

Choose a reason for hiding this comment

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

also suggest explicitly saying "Kubernetes API server" and "Extension API server" everywhere instead of "apiserver" for clarity

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, that makes sense. Elsewhere in the docs I have seen "apiserver" so stuck with it. I don't care much one way or the other though, so will follow your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the original document uses "apiserver" everywhere (7 times). You sure you want me to change all of those as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But adding explicit "Kubernetes apiserver" vs "extension apiserver" (or "API server") is a definite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use "Kubernetes apiserver" and "extension apiserver" everywhere. If you feel we should change all cases of "apiserver" to "API server" (including the pre-existing ones), no problem, quick search-and-replace.


Note that it is the responsibility of the extension server implementation to provide the above. Many do it by default, leveraging the `k8s.io/apiserver/` package. Others may provide options to override it using command-line options.

### Extension Server Authorize the Request
Copy link
Member

Choose a reason for hiding this comment

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

from the extension API server's perspective, all that has been proven is the identity of the incoming request (authentication, not authorization). The auth proxy might be the aggregator which already ran an authorization check, but it could just as easily be a generic authenticating proxy that only checked identity and added user info into headers.

@deitch deitch force-pushed the describe-agg-auth-flow branch from 90f79a0 to 16dfc9b Compare January 24, 2019 08:16
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 24, 2019
@deitch deitch force-pushed the describe-agg-auth-flow branch 9 times, most recently from c5fd8d2 to 14fd384 Compare January 24, 2019 09:21
@deitch
Copy link
Contributor Author

deitch commented Jan 24, 2019

@liggitt I addressed everything in your comments. Just need a decision on "Kubernetes apiserver" and "extension apiserver" vs "Kubernetes API server" and "extension API server". I think I like "apiserver" just because it is consistent with what was there before, but your call.

@liggitt
Copy link
Member

liggitt commented Jan 24, 2019

one note about the role that allows submitting subjectaccessreviews, technical content lgtm otherwise, thanks!

@deitch deitch force-pushed the describe-agg-auth-flow branch from 14fd384 to 9cc9b4c Compare January 24, 2019 14:40
@deitch
Copy link
Contributor Author

deitch commented Jan 24, 2019

one note about the role that allows submitting subjectaccessreviews

Done!

@liggitt
Copy link
Member

liggitt commented Feb 6, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2019
@Rajakavitha1
Copy link
Contributor

@bradtopol could you please take a look at this PR and state your suggestions/approval.

@zparnold
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zparnold

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2019
@k8s-ci-robot k8s-ci-robot merged commit ef89679 into kubernetes:master Feb 18, 2019
@deitch deitch deleted the describe-agg-auth-flow branch February 18, 2019 08:16
@@ -22,13 +22,173 @@ Configuring the [aggregation layer](/docs/concepts/extend-kubernetes/api-extensi
There are a few setup requirements for getting the aggregation layer working in your environment to support mutual TLS auth between the proxy and extension apiservers. Kubernetes and the kube-apiserver have multiple CAs, so make sure that the proxy is signed by the aggregation layer CA and not by something else, like the master CA.
{{< /note >}}

{{% /capture %}}

{{% capture authflow %}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This tag isn't recognized by Hugo, the static site generator we use to build the docs. So the build ignores everything inside this capture tag. (Note that the change to the heading below does show up on the live site.) The tags are part of the template for the different topic types that we require; they can't be used with arbitrary strings to designate a section type.

Copy link
Contributor

Choose a reason for hiding this comment

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

one comment @Bradamant3 is that i run a quick test on my local dev env and i can confirm that by removing {{% capture authflow %}} and {{% /capture %}} does solve it however the text does show after the Enable Kubernetes Apiserver flags although in the file the new text added by @deitch is before

Copy link
Contributor

@Bradamant3 Bradamant3 Feb 28, 2019

Choose a reason for hiding this comment

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

Good catch, @DanyC97, and thanks very much for testing -- my suggestion to remove the offending capture tags was incomplete. The added content needs to be placed inside the steps capture tag. See https://kubernetes.io/docs/contribute/style/page-templates/#task-template.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deitch should you not have time/ spare cycle to fix this one, let me know and i'll take over

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @DanyC97 . I must have copied it over from somewhere when doing the previous PR and it lingered. I will clean it up and update.

should you not have time/ spare cycle to fix this one, let me know and i'll take over

Never have, but I will do it anyways. :-)

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 fixed the tags on the open PR that I have at #12890 since it was open anyways. Take a look there?

@Bradamant3
Copy link
Contributor

@deitch not sure how you'd like to proceed with this one -- I can revert the merge, but if you've deleted your local branch, you can submit a new PR instead. I suggest just removing the problem capture tags for the new content you've written.

kwiesmueller pushed a commit to kwiesmueller/website that referenced this pull request Feb 28, 2019
krmayankk pushed a commit to krmayankk/kubernetes.github.io that referenced this pull request Mar 11, 2019
yagonobre pushed a commit to yagonobre/website that referenced this pull request Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants