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

Allow kubens to work with client-go credential plugins #156

Closed
wants to merge 4 commits into from

Conversation

mnussbaum
Copy link

Client-go, and by extension, kubectl, has the ability to run configurable commands to interactively prompt a user to authenticate. This system is known as client-go credential plugins.

Kubectl will only run credential plugins when it believes its STDOUT is an interactive TTY. Since kubens invokes kubectl get namespaces in a subshell, by default it causes kubectl to bypass its credential plugins.

Since kubens uses the STDOUT of kubectl get namespaces we also can't blindly redirect its STDOUT to that of the parent process.

The solution to allow kubens to work correctly with credential plugins is two part:

  1. Check if the get namespaces command will succeed, if so, run it like normal
  2. If the get namespaces command will fail, then run it again, interactively, by redirecting its inputs and outputs to those of the parent process. After the user successfully authenticates then run the get namespaces command one final time without any output redirection

Client-go, and by extension, kubectl, has the ability to run
configurable commands to interactively prompt a user to authenticate.
This system is known as client-go credential plugins.

Kubectl will only run credential plugins when it believes its STDOUT is
an interactive TTY. Since kubens invokes `kubectl get namespaces` in a
subshell, by default it causes kubectl to bypass its credential plugins.

Since kubens uses the STDOUT of `kubectl get namespaces` we also can't
blindly redirect its STDOUT to that of the parent process.

The solution to allow kubens to work correctly with credential plugins
is two part:

1. Check if the get namespaces command will succeed, if so, run it like
   normal

2. If the get namespaces command will fail, then run it again,
   interactively, by redirecting its inputs and outputs to those of the
   parent process. After the user successfully authenticates then run the
   get namespaces command one final time without any output redirection
@ahmetb
Copy link
Owner

ahmetb commented Jun 7, 2019

Have you tested with the fzf integration we have? It completely relies on TTY detection and we use [ -t check in a few other places, too.

2. If the get namespaces command will fail, then run it again

This makes the tool much slower for 90% users who I believe don't use interactive auth plugins. We hold off similar fixes (#83) due to these reasons.

@mnussbaum
Copy link
Author

mnussbaum commented Jun 7, 2019

Indeed it does work with the fzf integration. When the user is already authenticated the fzf integration behaves exactly as it did before. I've just pushed a commit that prevents a flash of unexpected kubectl get namespaces output after a user auths and before fzf kicks in. So now the fzf integration works totally correctly either if the user is already auth'd or if the credential plugin is triggered.

Regarding the extra network requests. I could examine the kubeconfig to see if any credential plugins are configured for the current context, and only invoke the new commands if so. What do you think about that approach?

@mnussbaum
Copy link
Author

I've just updated this PR again, it now only makes additional kubectl network calls if the initial attempt to get namespaces fails. This should leave performance completely unchanged in the happy path case where a user is authenticated and able to reach their cluster.

If the attempt to get namespaces initially fails then we attempt to trigger the credential plugin, and then fetch namespaces again if the user actually has the permissions to do so. Do you think that adding the two additional network calls in just the situation where a user can't fetch namespaces is an acceptable trade-off for the additional functionality?

Copy link
Owner

@ahmetb ahmetb left a comment

Choose a reason for hiding this comment

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

Sorry for the silence here. This fell of my plate. I just reviewed.
Overall I'm still worried that this increases code complexity for future maintainers. If you can address some comments explaining what’s going on, maybe that will ease that.

@@ -24,6 +24,10 @@ IFS=$'\n\t'
SELF_CMD="$0"
KUBENS_DIR="${XDG_CACHE_HOME:-$HOME/.kube}/kubens"

exec {STDIN}>&0
Copy link
Owner

Choose a reason for hiding this comment

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

can you please add a comment about what do these do? I'm not sure...

Copy link
Author

@mnussbaum mnussbaum Jun 27, 2019

Choose a reason for hiding this comment

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

Just pushed those comments you requested. Happy to clarify them further if possible!

These three lines assign the main process' standard streams to variables so that they can be used to invoke kubectl auth can-i get namespaces in a function, but with the main process' standard streams as its STDOUT, STDERR and STDIN. Using the main process' standard streams makes the kubectl auth call interactive, so client-go auth plugins are triggered

kubens Outdated
@@ -58,7 +62,19 @@ current_context() {
}

get_namespaces() {
$KUBECTL get namespaces -o=jsonpath='{range .items[*].metadata.name}{@}{"\n"}{end}'
name_jsonpath='{range .items[*].metadata.name}{@}{"\n"}{end}'
namespaces=$($KUBECTL get namespaces -o=jsonpath="${name_jsonpath}" 2>/dev/null)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm suspecting that having a " around $() is generally a good idea.

return 0
fi

$KUBECTL auth can-i get namespaces --quiet 1>&$STDOUT 2>&$STDERR <&$STDIN
Copy link
Owner

Choose a reason for hiding this comment

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

can you add a comment here explaining the fallback etc?

return $?
fi

$KUBECTL get namespaces -o=jsonpath="${name_jsonpath}"
Copy link
Owner

Choose a reason for hiding this comment

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

what's the point of repeating this? (making the stderr visible to the user?)

Copy link
Author

Choose a reason for hiding this comment

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

If the kubectl auth can-i get namespaces successfully triggered an auth plugin above then this kubectl get namespaces call should now succeed and return the namespace output that get_namespaces is supposed to provide

Copy link
Owner

@ahmetb ahmetb left a comment

Choose a reason for hiding this comment

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

Oops, sorry didn't mean to approve just yet. Undoing.

@mnussbaum mnussbaum force-pushed the fix-credential-plugins branch from 55519d1 to bdeaeb9 Compare June 27, 2019 06:30
@2rs2ts
Copy link

2rs2ts commented May 20, 2020

Is this PR not necessary now that kubectx is written in Go?

@ahmetb
Copy link
Owner

ahmetb commented May 20, 2020

Sounds about right.

@ahmetb ahmetb closed this May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants