Proposal for adding headers with the command name to kubectl requests #855

pwittrock · 2019-02-26T02:54:12Z

KEP PR Approvers:

apelisse · 2019-02-26T23:19:44Z

keps/sig-cli/kubectl-commands-in-headers.md

+```
+
+```
+Kubectl-Command: apply


while Command and Flags is clear-ish, I'm still trying to understand what input and output are.
You should give an example of a more nested command (kubectl create secret generic), what would happen in that case?

Also as a side note, I thought it might be useful to send a random hash too, so that we can bundle commands issued from the same kubectl command (kubectl would create that has at the beginning and use it for all requests).

I thought it might be useful to send a random hash too

I think you are suggesting creating a session id? Perhaps we could use a UUID? This could even be used as a traceid if we got open tracing working :)

apelisse · 2019-02-27T03:35:14Z

Yes

…

On Tue, Feb 26, 2019 at 7:29 PM Phillip Wittrock ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In keps/sig-cli/kubectl-commands-in-headers.md <#855 (comment)> : > +## Proposal + +Include in requests made from kubectl to the apiserver: + +- the kubectl subcommand +- which flags were specified (but not the values) +- whether or not specific flags had specific values - e.g. `-f -` for stdin + +### Example + +```sh +$ kubectl apply -f - -o yaml +``` + +``` +Kubectl-Command: apply I thought it might be useful to send a random hash too Like a UUID? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#855 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1xrCyvnJF7J2c7aM9ID1M0pEw6082mks5vRfuqgaJpZM4bRPS9> .

pwittrock · 2019-02-27T04:49:31Z

PTAL

deads2k · 2019-02-27T16:37:58Z

keps/sig-cli/kubectl-commands-in-headers.md

+
+### Non-Goals
+
+NA


I think we should explicitly indicate that this information is not provided to be used by the apiserver to determine behavior of a request in any way. I remember there was an occasion a few years back where someone tried to make the server reply differently based on which user-agent came through.

I made this an anti-goal, which is a bit stronger.

deads2k · 2019-02-27T16:38:43Z

keps/sig-cli/kubectl-commands-in-headers.md

+Include in requests made from kubectl to the apiserver:
+
+- the kubectl subcommand
+- which flags were specified (but not the values)


will there be an opt-in for certain cases? I'd like to see the kubectl patch --type value to know which kinds of patches are actually being frequently used.

Yeah, it might be useful to have a mechanism to indicate which flag values are sensitive or not. -o for example is complex because some values can be sensitive (-o column-name=...) and some aren't (-o yaml). We could improve the flag registration mechanisms that we use in kubectl to have this sort of semantic maybe?

SGTM. We could start with a whitelisted set of flags we will include an enumerated value for (never arbitrary values supplied by the user). We can expand the list over time. That was the intention for -f and -o values.

SGTM. We could start with a whitelisted set of flags we will include an enumerated value for (never arbitrary values supplied by the user). We can expand the list over time. That was the intention for -f and -o values.

Yeah, I think that would work well.

soltysh · 2019-02-27T16:37:54Z

keps/sig-cli/kubectl-commands-in-headers.md

+- `Kubectl-Command: kubectl apply`
+- `Kubectl-Command: kubectl create secret tls` 
+- `Kubectl-Command: kubectl delete`
+- `Kubectl-Command: kubectl get`


I'd this report just Kubectl-Command: get for example, we already know it's kubectl command (from header Kubectl-command), why repeat yourself.

And I don't think we want to know how people name their kubectl command on their filesystem.

soltysh · 2019-02-27T16:38:21Z

keps/sig-cli/kubectl-commands-in-headers.md

+The `Kubectl-Flags` Header contains a list of the kubectl flags that were provided to the sub
+command.  It does *not* contain the flag values, only the names of the flags.  It
+does not normalize into short or long form.  If a flag is provided multiple times
+it will appear multiple times in the list.  Flags are sorted alpha-numerically and


Why do you want to sort them?

Will make it easier for humans to read.

soltysh · 2019-02-27T16:39:05Z

keps/sig-cli/kubectl-commands-in-headers.md

+
+Examples:
+
+- `Kubectl-Output: yaml`


This will overlap with Kubectl-flags or you're planning to exclude output-related flags?

Or this will let us differentiate between default output and requested output, like you're describing in your example?

Right. I could actually move these into flags as enums.

soltysh · 2019-02-27T16:39:25Z

keps/sig-cli/kubectl-commands-in-headers.md

+requests were made from the same kubectl command invocation.  The Session Header is generated
+once for each kubectl process.
+
+- `Kubectl-Session: 67b540bf-d219-4868-abd8-b08c77fefeca`


Very good idea.

It was @apelisse 's idea :)

soltysh · 2019-02-27T16:40:44Z

keps/sig-cli/kubectl-commands-in-headers.md

+
+### Risks and Mitigations
+
+Unintentionally including sensitive information in the request headers - such as local directory paths


We don't pass flag values, so that should not be a concern.

deads2k · 2019-02-27T16:41:30Z

keps/sig-cli/kubectl-commands-in-headers.md

+Unintentionally including sensitive information in the request headers - such as local directory paths
+or cluster names.
+
+Mitigations: only include the following non-sensative information:


spelling. thx

pwittrock · 2019-02-27T22:27:54Z

PTAL

pwittrock · 2019-02-27T22:37:00Z

Sorry, PTAL for real this time.

pwittrock · 2019-02-27T23:52:31Z

/assign @deads2k
/assign @soltysh

smarterclayton · 2019-02-28T00:11:36Z

keps/sig-cli/kubectl-commands-in-headers.md

+
+### Kubectl-Command Header
+
+The `Kubectl-Command` Header contains the kubectl sub command.  It contains


Aren’t we supposed to put X- in front of custom headers,

My understanding is that it is deprecated: link

I think the deprecation is primarily for headers that may be standardized in an RFC at some point in the future.

Good to know. Thanks.

My reading of that RFC is that X- prefixes are deprecated pretty much for all cases - hence the title Deprecating the "X-" Prefix and Similar Constructs in Application Protocols

If it's not too late to fix this before the beta, it'd be great to sort that out.

smarterclayton · 2019-02-28T00:12:53Z

keps/sig-cli/kubectl-commands-in-headers.md

+
+### Goals
+
+- Allow cluster administrators to identify how requests in the logs were generated from


That’s it? No other goals?

What did you have in mind?

Tried to add some color here. LMK if it makes sense to you.

pwittrock · 2019-03-07T04:47:34Z

PTAL. I've added and expanded a few things.

monopole · 2019-03-13T19:11:08Z

/lgtm observability is a good thing.

pwittrock · 2019-03-14T20:06:24Z

Changing assignees to move from lazy consensus to explicit approval.

/unassign @deads2k
/unassign @soltysh
/assign @monopole

monopole · 2019-03-14T20:51:57Z

/lgtm

k8s-ci-robot · 2019-03-14T20:52:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: monopole, pwittrock

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:

~~keps/sig-cli/OWNERS~~ [pwittrock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Proposal for adding headers with the command name to kubectl requests

aefe9d8

k8s-ci-robot requested review from seans3 and soltysh February 26, 2019 02:54

apelisse reviewed Feb 26, 2019

View reviewed changes

Address PR comments

a69c688

pwittrock force-pushed the headers branch from 2e71fef to a69c688 Compare February 27, 2019 04:52

deads2k reviewed Feb 27, 2019

View reviewed changes

soltysh reviewed Feb 27, 2019

View reviewed changes

deads2k reviewed Feb 27, 2019

View reviewed changes

pwittrock force-pushed the headers branch from f96203b to 281a215 Compare February 27, 2019 22:36

pwittrock mentioned this pull request Feb 27, 2019

Include kubectl command metadata in http request headers #859

Closed

pwittrock force-pushed the headers branch from 281a215 to 1f80e2b Compare February 27, 2019 23:45

Address PR comments

c3d1ebb

pwittrock force-pushed the headers branch from 1f80e2b to c3d1ebb Compare February 27, 2019 23:51

k8s-ci-robot assigned deads2k and soltysh Feb 27, 2019

smarterclayton suggested changes Feb 28, 2019

View reviewed changes

Address PR comments

3c94997

k8s-ci-robot assigned monopole and unassigned soltysh and deads2k Mar 14, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2019

k8s-ci-robot merged commit 9819ece into kubernetes:master Mar 14, 2019

pwittrock mentioned this pull request Mar 25, 2021

Kubectl command headers in requests: KEP 859 kubernetes/kubernetes#98952

Merged


		### Risks and Mitigations

		Unintentionally including sensitive information in the request headers - such as local directory paths


		### Kubectl-Command Header

		The `Kubectl-Command` Header contains the kubectl sub command. It contains


		### Goals

		- Allow cluster administrators to identify how requests in the logs were generated from

Proposal for adding headers with the command name to kubectl requests #855

Proposal for adding headers with the command name to kubectl requests #855

Conversation

pwittrock commented Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

pwittrock Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

apelisse commented Feb 27, 2019 via email

pwittrock commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Feb 27, 2019

pwittrock commented Feb 27, 2019

pwittrock commented Feb 27, 2019

Choose a reason for hiding this comment

pwittrock Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Mar 7, 2019

monopole commented Mar 13, 2019

pwittrock commented Mar 14, 2019

monopole commented Mar 14, 2019

k8s-ci-robot commented Mar 14, 2019

pwittrock commented Feb 26, 2019 •

edited

Loading

pwittrock Feb 27, 2019 •

edited

Loading

pwittrock Feb 28, 2019 •

edited

Loading