Conditionally redirect stderr for exec auth provider

[bianyifan]

External command authentication provider may prompt user interaction in stderr (e.g. Azure/kubelogin device code flow).
If stderr is redirected, users won't be able to see the prompt in the console.

Globally searching for ExecStdError didn't yield any meaningful result, so I would just remove that event handler and disable stderr redirection.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bianyifan / name: Yifan Bian (0c409cc)

[k8s-ci-robot]

Welcome @bianyifan!

It looks like this is your first PR to kubernetes-client/csharp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/csharp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[IvanJosipovic]

See this PR on why this was added, #1267 (comment)

[bianyifan]

I can understand back in time the stderr messages are catched and being used in exception messages, but that got changed in #1267; nothing reads redirected stderr stream now and prompts in stderr got lost.
I've also checked golang impl, golang is directly passing stderr to os.stderr.

And for error handling, I believe we should check process.ExitCode after process.WaitForExit, before trying to deserialize stdout. I could send another PR for that.

[IvanJosipovic]

I can understand back in time the stderr messages are catched and being used in exception messages, but that got changed in #1267; nothing reads redirected stderr stream now and prompts in stderr got lost. I've also checked golang impl, golang is directly passing stderr to os.stderr.

And for error handling, I believe we should check process.ExitCode after process.WaitForExit, before trying to deserialize stdout. I could send another PR for that.

It has been a little time since that PR, however, I believe that approach would cause the process to hang as Kubelogin waits for you to log into a http based url and won't exit until the process is killed or the login has occurred.

Grab kubelogin and try https://github.com/Azure/kubelogin/releases

kubelogin get-token --environment AzurePublicCloud --server-id 6dae42f8-4368-4678-94ff-3960e28e3630 --client-id 80faf920-1908-4b52-b5ef-a8e7bedfc67a --tenant-id b82075ce-f897-4df8-8624-47b71a1fd251 --login devicecode

Maybe we could set RedirectStandardError to true only if ExecStdError is not null?

[tg123]

seems a customizable exec auth provider would be better

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (master@f7450f9). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##             master    #1377   +/-   ##
=========================================
  Coverage          ?   70.51%           
=========================================
  Files             ?       89           
  Lines             ?     2683           
  Branches          ?        0           
=========================================
  Hits              ?     1892           
  Misses            ?      791           
  Partials          ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[bianyifan]

It has been a little time since that PR, however, I believe that approach would cause the process to hang as Kubelogin waits for you to log into a http based url and won't exit until the process is killed or the login has occurred.

Exactly, I am not able to login without a prompt and kubelogin keeps waiting for user interaction.

Maybe we could set RedirectStandardError to true only if ExecStdError is not null?
seems a customizable exec auth provider would be better

Let me try summarize:

• Users can opt into capturing stderr with ExecStdError.
• If user opted into capturing stderr, we would redirect the stderr to ExecStdError. Otherwise, send stderr directly to console.
• Also add exit code check right after if (!process.WaitForExit((int)(ExecTimeout.TotalMilliseconds))), so users could get a rough idea of what happened even without stderr.

Thanks for your input and I will make an update!

[bianyifan]

Updated.
However I don't think using a global event handler to process stderr message is a great design: if I am reading multiple different kubeconfig files at the same time, it would be tricky to identify where each log comes from.

[tg123]

lets do a proposal to remove static?

#1267 (comment)

[bianyifan]

lets do a proposal to remove static?

I'd like to understand the purpose of capturing stderr. If the purpose is just to include stderr output into exception messages in non-user-interactive scenarios, ideally we could just use System.Environment.UserInteractive as a switch, if UserInteractive = false, capture the stderr and include it in the thrown exception; if UserInteractive = true, disable RedirectStandardError.
Python impl is doing it this way.
Unfortunately this only works on Windows. On UNIX or browser UserInteractive is always true.

So, currently I am thinking:

1. Remove ExecuteExternalCommand from KubernetesClientConfiguration. The initial token retrieval can be handled by ExecTokenProvider as well.
2. Currently the call chain is BuildConfigFrom* -> GetKubernetesClientConfiguration -> InitializeContext -> SetUserDetails -> new ExecTokenProvider. We could add an optional parameter bool? allowInteractiveAuthentication = null at each level. If allowInteractiveAuthentication ?? (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && Environment.UserInteractive), stderr will be redirected and included in exception message.

If this plan looks good to you, I can start another PR for that.

[IvanJosipovic]

One more thing to note, when the exception occurs, we need to keep the kubelogin process around if it doesn't exit on its own, otherwise the login process will not complete. However there is a concern that we could spawn many zombie processes.

[bianyifan]

One more thing to note, when the exception occurs, we need to keep the kubelogin process around if it doesn't exit on its own, otherwise the login process will not complete. However there is a concern that we could spawn many zombie processes.

I believe this is only a concern in the try-block around process.Start and the if-block of WaitForExit(int), right?
For try-block around process.Start, if exception happens there, it could be process didn't start at all, or failed to redirect stderr to event handler.
If process didn't even start, we have nothing to clean up.
If stderr redirection failed, or the process didn't exit within the timeout, I believe we should terminate the process since we already considered the authentication attempt as failed, there's no use keeping it around.

[tg123]

any additional comments? @IvanJosipovic

[tg123]

good to merge @IvanJosipovic ?

[tg123]

/LGTM

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bianyifan, tg123

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:

• OWNERS [tg123]

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

[bianyifan]

Thanks @tg123 ! I will work on removing static stderr handler as we have discussed before soon.