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: use stdin to detect user interaction #99654
exec credential provider: use stdin to detect user interaction #99654
Conversation
We are not sure why this was stdout, since stdin is what the user uses to pass information to the exec plugin. There is a question of backwards compatibility here. Our take is that this is a bug, and so we are ameliorating behavior instead of breaking behavior. There are 2 main cases to consider with respect to backwards compatibility: 1. an existing exec plugin depended on stdin being hooked up to them if stdout was a terminal (e.g., echo foo | client-go-command-line-tool); we believe this is an anti-pattern, since the client-go-command-line-tool could be using stdin elsewhere (e.g., echo foo | kubectl apply -f -) 2. an existing exec plugin depended on stdin not being hooked up to them if stdout was not a terminal (e.g., client-go-command-line-tool >/dev/null); hopefully there are very few plugins that have tried to base logic off of whether stdin returned EOF immediately, since this could also happen when something else is wrong with stdin We hope to apply a stronger fix to this exec plugin user interaction stuff in a future release. Signed-off-by: Andrew Keesler <akeesler@vmware.com>
Hi @ankeesler. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/triage accepted |
/assign @liggitt |
@@ -198,7 +198,7 @@ func newAuthenticator(c *cache, config *api.ExecConfig, cluster *clientauthentic | |||
|
|||
stdin: os.Stdin, | |||
stderr: os.Stderr, | |||
interactive: term.IsTerminal(int(os.Stdout.Fd())), | |||
interactive: term.IsTerminal(int(os.Stdin.Fd())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the rationale here was that unless stdout was visible to the user, the exec plugin couldn't reasonably prompt for input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
talked with @enj, who pointed out that even if that were the case, this check wouldn't make sense, since the plugins prompted on stderr
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankeesler, liggitt 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 |
Hello! I am from the Bug Triage team! Friendly reminder that code freeze is starting March 9th, 2021 (about 1 week from now). We want to ensure that each PR has a chance to be merged before code freeze. This looks close to being merged (pending some failed tests), but commenting for awareness of the code freeze date. |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind bug
What this PR does / why we need it:
We are not sure why this was stdout, since stdin is what the user uses to pass
information to the exec plugin.
There is a question of backwards compatibility here. Our take is that this is a
bug, and so we are ameliorating behavior instead of breaking behavior. There are
2 main cases to consider with respect to backwards compatibility:
an existing exec plugin depended on stdin being hooked up to them if stdout
was a terminal (e.g., echo foo | client-go-command-line-tool); we believe
this is an anti-pattern, since the client-go-command-line-tool could be using
stdin elsewhere (e.g., echo foo | kubectl apply -f -)
an existing exec plugin depended on stdin not being hooked up to them if
stdout was not a terminal (e.g., client-go-command-line-tool >/dev/null);
hopefully there are very few plugins that have tried to base logic off of
whether stdin returned EOF immediately, since this could also happen when
something else is wrong with stdin
We hope to apply a stronger fix to this exec plugin user interaction stuff in a
future release.
Which issue(s) this PR fixes:
Fixes #98451
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: