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

ServiceAccountIssuerDiscovery: Add user facing documentation #19328

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

mtaufen
Copy link
Contributor

@mtaufen mtaufen commented Feb 26, 2020

Extends the documentation on configuring service accounts to include
details on the ServiceAccountIssuerDiscovery feature.

/cc @VineethReddy02 @liggitt @mikedanese

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 26, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Feb 26, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 39a7d4f

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e6660592c539200084f9b62

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 26, 2020
@sftim
Copy link
Contributor

sftim commented Feb 27, 2020

This looks like it's pending approval for v1.18 release? If so, @mtaufen, you might want to add a /hold

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

@mtaufen thanks for this. Here's some feedback.


Somewhat related to this PR: I recommend creating a new task webpage that explains how to set up Service Account Issuer Discovery and to set --service-account-jwks-uri appropriately.

For the alpha, it's enough to file the issue (against k/website). Please consider adding that task page as a requirement for graduation to beta.

In the issue, mention that this section should link to the new task page.


{{< note >}}
The Service Account Issuer Discovery feature is __alpha__ in 1.18 and enabled by
setting the `ServiceAccountIssuerDiscovery` feature gate to `true` and then
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setting the `ServiceAccountIssuerDiscovery` feature gate to `true` and then
enabling the `ServiceAccountIssuerDiscovery` [feature gate](/docs/reference/command-line-tools-reference/feature-and then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{{< feature-state for_k8s_version="v1.18" state="alpha" >}}

{{< note >}}
The Service Account Issuer Discovery feature is __alpha__ in 1.18 and enabled by
Copy link
Contributor

Choose a reason for hiding this comment

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

This is repeating the feature-state line. I would tell readers how to enable the feature gate (what component?) and omit the repetition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 308 to 315
Note that the issuer URL must comply with the
[OIDC Discovery Spec](https://openid.net/specs/openid-connect-discovery-1_0.html). In
practice, this means it must use the `https` scheme, and should serve an OpenID
provider configuration at `{service-account-issuer}/.well-known/openid-configuration`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd start the {{< note >}} here (move the advice about enabling the feature gate to be before the note block).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

service account tokens issued by a cluster (the _identity provider_) with
external systems (_relying parties_).

When enabled, the Kubernetes API server will serve an OpenID Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When enabled, the Kubernetes API server will serve an OpenID Provider
When enabled, the Kubernetes API server provides a public OpenID Provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also added clarification on RBAC, thanks for the reminder with your comment about "public."

{{< note >}}
The responses served at `/.well-known/openid-configuration` and
`/openid/v1/jwks` are designed to be OIDC compatible, but not strictly OIDC
compliant. Today they contain only the parameters necessary to perform
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
compliant. Today they contain only the parameters necessary to perform
compliant. Those documents contain only the parameters necessary to perform

(“today” looks a bit like a statement about the future; see https://kubernetes.io/docs/contribute/style/style-guide/#avoid-statements-about-the-future)

I would definitely remove:

Additional, parameters may be
added if it is determined that doing so improves compatibility with common
client implementations used by relying parties.

That kind of detail belongs in the KEP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, already in the KEP. Sounds good. Done.

client implementations used by relying parties.
{{< /note >}}

The JWKS contains public keys that can be used to validate the Kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The JWKS contains public keys that can be used to validate the Kubernetes
The JWKS response contains public keys that can a relying party can use to validate the Kubernetes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 347 to 350
For additional technical details, see the
[Service Account Signing Key Retrieval KEP](https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/20190730-oidc-discovery.md)
and
[OIDC Discovery Spec](https://openid.net/specs/openid-connect-discovery-1_0.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

These could (I think should) move into the whatsnext capture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mikedanese
Copy link
Member

For tech review.

/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 27, 2020
@mtaufen
Copy link
Contributor Author

mtaufen commented Mar 4, 2020

This looks like it's pending approval for v1.18 release? If so, @mtaufen, you might want to add a /hold

@sftim The code is merged for 1.18 already. Unless you meant something else?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm
One nit which would be nice to fix (I'll re-review)

JSON Web Key Set (JWKS) at `/openid/v1/jwks`. The OpenID Provider Configuration
is sometimes referred to as the _discovery document_.

When enabled, the cluster is also configured with a default RBAC `ClusterRole`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
When enabled, the cluster is also configured with a default RBAC `ClusterRole`
When enabled, the cluster is also configured with a default RBAC ClusterRole

In the docs, API objects don't go inside backticks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020
@mtaufen
Copy link
Contributor Author

mtaufen commented Mar 5, 2020

Filed the issue to remind us to create a new task webpage: #19496

@sftim do you need to /approve also?

@sftim
Copy link
Contributor

sftim commented Mar 5, 2020

@VineethReddy02 is docs lead for the v1.18 release

@mtaufen
Copy link
Contributor Author

mtaufen commented Mar 5, 2020

/assign @VineethReddy02

@VineethReddy02
Copy link
Contributor

👋 Please rebase this PR on dev-1.18 in order to avoid merge conflicts.

Feel free to /hold cancel after you've rebased.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Here's some more feedback on the text @mtaufen

and then enabling the Service Account Token Projection feature as described above.

{{< note >}}
Note that the issuer URL must comply with the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that the issuer URL must comply with the
The issuer URL must comply with the

The shortcode {{< note >}} already marks this as a note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

See also the
[Cluster Admin Guide to Service Accounts](/docs/reference/access-authn-authz/service-accounts-admin/).
See also:
- [Cluster Admin Guide to Service Accounts](/docs/reference/access-authn-authz/service-accounts-admin/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line before this list to keep the Markdown parser happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


The Service Account Issuer Discovery feature is enabled by enabling the
`ServiceAccountIssuerDiscovery` [feature gate](/docs/reference/command-line-tools-reference/feature)
and then enabling the Service Account Token Projection feature as described above.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
and then enabling the Service Account Token Projection feature as described above.
and then enabling the Service Account Token Projection feature as described [above](#service-account-token-volume-projection).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@VineethReddy02
Copy link
Contributor

Hi, @mtaufen As today is the deadline for docs placeholder PR to be ready for review state? Can we have all the requested changes to the PR sometime soon?
Thanks!

Extends the documentation on configuring service accounts to include
details on the ServiceAccountIssuerDiscovery feature.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2020
@mtaufen
Copy link
Contributor Author

mtaufen commented Mar 9, 2020

/hold cancel

@VineethReddy02 all requested changes have been made.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Following on from #19328 (comment)
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2020
@VineethReddy02
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VineethReddy02

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 Mar 12, 2020
@k8s-ci-robot k8s-ci-robot merged commit e746d34 into kubernetes:dev-1.18 Mar 12, 2020
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. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants