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

exec credential provider: v1 documentation #28429

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

ankeesler
Copy link
Contributor

Adds documentation for client-go credential plugin GA version.

KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/541-external-credential-providers

@k8s-ci-robot k8s-ci-robot added this to the 1.22 milestone Jun 15, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2021
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Jun 15, 2021

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

🔨 Explore the source changes: a30e63d

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/60eda78e8ab3f60007f4af52

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jun 15, 2021
@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 15, 2021
@ankeesler ankeesler force-pushed the exec-credential-v1 branch 2 times, most recently from 1bda5d9 to 0cfc62d Compare June 16, 2021 14:16
apiVersion: "client.authentication.k8s.io/v1beta1"
# set an environment variable, pass an argument to the tool that indicates which version the exec plugin expects,
# or read the version from the ExecCredential object in the KUBERNETES_EXEC_INFO environment variable.
apiVersion: "client.authentication.k8s.io/v1"
Copy link
Member

Choose a reason for hiding this comment

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

when we have multiple versions available, we've added per-version examples in tabs

see https://github.com/kubernetes/website/blame/master/content/en/docs/reference/access-authn-authz/extensible-admission-controllers.md#L98 as an example

Copy link
Contributor Author

@ankeesler ankeesler Jun 17, 2021

Choose a reason for hiding this comment

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

i went ahead and added tabs for all of the non-trivial API objects.

6a3abda

# returned. Valid values are "Never" (this exec plugin never uses standard input),
# "IfAvailable" (this exec plugin wants to use standard input if it is available),
# or "Always" (this exec plugin requires standard input to function). Required.
interactiveMode: IfAvailable
Copy link
Member

Choose a reason for hiding this comment

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

I assume people will copy/paste this example blindly, and interactive credential plugins are perhaps not the ideal... would it make sense to make the example be "Never"?

Copy link
Contributor Author

@ankeesler ankeesler Jun 17, 2021

Choose a reason for hiding this comment

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

ok, i can get on board with interactive credential plugins are perhaps not the ideal. maybe s/ideal/the norm/? i originally had IfAvailable because it was the default in v1beta1, but i think your argument makes sense. 5c22507

Comment on lines 952 to 1031
apiVersion: "client.authentication.k8s.io/v1"
interactiveMode: IfAvailable
Copy link
Member

Choose a reason for hiding this comment

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

same comment about multiple version examples in tabs, and making the default example use "Never"

Copy link
Contributor Author

@ankeesler ankeesler Jun 17, 2021

Choose a reason for hiding this comment

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

to the plugin. Plugins should use the `spec.interactive` field of the input
`ExecCredential` object from the `KUBERNETES_EXEC_INFO` environment variable in order to
determine if `stdin` has been provided. A plugin's `stdin` requirements (i.e., whether
`stdin` is optional, strictly required, or strictly not required in order for the plugin
Copy link
Member

Choose a reason for hiding this comment

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

"strictly not required" reads weirdly... maybe "never used"?

Copy link
Contributor Author

@ankeesler ankeesler Jun 17, 2021

Choose a reason for hiding this comment

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

validated "never used" with a couple of other people, so i went with that. c06d8ff


To use bearer token credentials, the plugin returns a token in the status of the
[`ExecCredential`](/docs/reference/config-api/client-authentication.v1beta1/#client-authentication-k8s-io-v1beta1-ExecCredential)

```json
{
"apiVersion": "client.authentication.k8s.io/v1beta1",
"apiVersion": "client.authentication.k8s.io/v1",
Copy link
Member

Choose a reason for hiding this comment

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

multiple tabs for examples in different versions

Copy link
Contributor Author

@ankeesler ankeesler Jun 17, 2021

Choose a reason for hiding this comment

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

@@ -1026,6 +1050,7 @@ The following `ExecCredential` manifest describes a cluster information sample.
"you": ["can", "put", "anything", "here"]
}
}
Copy link
Member

Choose a reason for hiding this comment

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

missing trailing comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, good catch; 7d5a81b

@PI-Victor
Copy link
Member

/assign

@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 Jun 17, 2021
@ankeesler ankeesler force-pushed the exec-credential-v1 branch 2 times, most recently from 297c8aa to 6a3abda Compare June 17, 2021 19:18
[kubeconfig](/docs/concepts/configuration/organize-cluster-access-kubeconfig/) (see table
below for valid values).

| `interactiveMode` Value | Meaning |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PI-Victor do you know of a way that I can ensure that this table renders without splitting words on different lines?
image

Copy link
Member

Choose a reason for hiding this comment

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

sorry i missed this, @sftim have any idea on this one? does it need some special css code to fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, HTML table layout.

I would raise a separate issue about the rendering and label it area/web-development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raised issue here: #28815

unfortunately couldn't coax the bot into applying the area/web-development...perhaps someone can enlighten me

@liggitt
Copy link
Member

liggitt commented Jun 18, 2021

content lgtm, though hold for v1 API merge

@ankeesler
Copy link
Contributor Author

v1 API has merged - kubernetes/kubernetes#102890

[kubeconfig](/docs/concepts/configuration/organize-cluster-access-kubeconfig/) (see table
below for valid values).

| `interactiveMode` Value | Meaning |
Copy link
Contributor

Choose a reason for hiding this comment

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

Optionally: add a caption using the table shortcode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a caption here: cbbded0

@PI-Victor
Copy link
Member

to quote an ex-team lead, i think we beat this one to death.

@ankeesler i see there are no more PRs for this enhancement and it's not marked as 'At Risk' do you still have any more work for this or can you squash so we can merge it?

@ankeesler
Copy link
Contributor Author

i have no more work associated with this enhancement - i have squashed into one commit. thanks for keeping this moving forward.

@sftim
Copy link
Contributor

sftim commented Jul 8, 2021

/sig auth
@ankeesler if this is ready for review please retitle it to remove the “WIP:”

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jul 8, 2021
@ankeesler ankeesler changed the title WIP: exec credential provider: v1 documentation exec credential provider: v1 documentation Jul 8, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2021
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.

per issue #28013 we could also document that kubeconfig exec has a security risk,
and emphasize that this risk is present whenever you use a config from an untrusted source without checking it

That improvement is not required for this PR to merge.

Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

Minor stuff.

`ExecCredential` object from the `KUBERNETES_EXEC_INFO` environment variable in order to
determine if `stdin` has been provided. A plugin's `stdin` requirements (i.e., whether
`stdin` is optional, strictly required, or never used in order for the plugin
to run successfully) must be declared via the `user.exec.interactiveMode` field in the
Copy link
Member

Choose a reason for hiding this comment

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

Should this note that the "must" is only true for the v1 API?

Copy link
Contributor Author

@ankeesler ankeesler Jul 13, 2021

Choose a reason for hiding this comment

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

gave this a go here e4653f1


To use bearer token credentials, the plugin returns a token in the status of the
[`ExecCredential`](/docs/reference/config-api/client-authentication.v1beta1/#client-authentication-k8s-io-v1beta1-ExecCredential)
{{< /table >}}
Copy link
Member

Choose a reason for hiding this comment

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

Er, should this be higher up in the doc right after the table ends?

Copy link
Contributor Author

@ankeesler ankeesler Jul 13, 2021

Choose a reason for hiding this comment

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

whoops. fbe3f67

@ankeesler
Copy link
Contributor Author

@sftim i'm considering your comment above about adding something in the docs about #28013. appreciate you calling that out.

@enj do you have an opinion on whether we should add something in this PR for the above doc issue? my gut feel was to keep the PR specifically targeted at GA stuff on the grounds that smaller PRs are usually better.

@enj
Copy link
Member

enj commented Jul 13, 2021

@enj do you have an opinion on whether we should add something in this PR for the above doc issue? my gut feel was to keep the PR specifically targeted at GA stuff on the grounds that smaller PRs are usually better.

+1 to keeping this PR targeted at GA stuff with a follow-up PR to add docs around not running kubeconfigs from untrusted sources.

Feel free to squash, this PR looks ready to me.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>
@ankeesler
Copy link
Contributor Author

cool, thanks @enj. PR hath been squashed

@enj
Copy link
Member

enj commented Jul 13, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 515d5b71a372a3f9349c53f5a0f814313a857ca3

@PI-Victor
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PI-Victor

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 Jul 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit 83f6cb6 into kubernetes:dev-1.22 Jul 21, 2021
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/auth Categorizes an issue or PR as relevant to SIG Auth. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants