-
Notifications
You must be signed in to change notification settings - Fork 296
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
Out-of-tree client authentication providers (UserCredentials exec option) for asp.net core applications #359
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @talboren! |
/assign @krabhishek8260 |
Signed the CLA |
Seems like Process and Environment.UserInteractive are not supported by all dependencies. |
new file: src/KubernetesClient/KubeConfigModels/ExecCredentialResponse.cs new file: src/KubernetesClient/KubeConfigModels/ExternalExecution.cs modified: src/KubernetesClient/KubeConfigModels/UserCredentials.cs modified: src/KubernetesClient/KubernetesClientConfiguration.ConfigFile.cs
…hentication in case it is not a asp.net core app
Thanks for the PR. This looks great to me, but we need unit tests for this before we merge it. Thanks! |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
…ication) extension
@brendandburns I've added 2 tests, let me know if that's OK. |
@brendandburns I'm having trouble making the tests pass with OSX due to some issues I am experiencing with 'echo' (different behavior than Linux?), any chance you have any idea? |
I looked at the error, the JSON parse error is:
You might consider using base64 encoding (e.g. But I'm not sure what bit is having a problem. I have an old Mac at home, I'll try to repro sometime if you still can't figure it out. |
/// Environment variables to set when executing the plugin. Optional. | ||
/// </summary> | ||
[YamlMember(Alias = "env")] | ||
public Dictionary<string, string> EnvironmentVariables { get; set; } |
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.
IDictionary if possible
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 did not quite catch about why this is related to asp.net core. did not see anything related to asp.net
/// Arguments to pass when executing the plugin. Optional. | ||
/// </summary> | ||
[YamlMember(Alias = "args")] | ||
public List<string> Arguments { get; set; } |
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.
IList?
@@ -365,6 +390,83 @@ public static string RenewAzureToken(string tenantId, string clientId, string ap | |||
throw new KubeConfigException("Refresh not supported."); | |||
} | |||
|
|||
#if NETCOREAPP |
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.
why this is specific to netcore, can it be netstandard?
if (string.IsNullOrWhiteSpace(stderr) == false) | ||
throw new KubeConfigException($"external exec failed due to: {stderr}"); | ||
|
||
process.WaitForExit(); |
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.
timeout?
Changed OSX testing command to printf to try and solve the JSON parsing errors
….0 according to Microsoft documentation
I am working on migrating the codebase to .netstandard2.0 |
@tg123 @brendandburns still waiting for your approval. |
@talboren any chance to take a look at my comments? |
@tg123 please note my latest commits. I’ve made changes according to your changes. What are you talking about? |
@talboren sorry forget to click publish :( I forget to do so many times |
@tg123 are you ok with merging this? Any other comments need addressing? Thanks! |
/LGTM lets make timeout configurable later if necessary |
/approve thanks for the PR (and the patience) |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, talboren 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 |
after facing the same problem described in: #340
I've tried to create a version that will allow me to receive the token using an external command execution (as kubeconfig supports).
Implemented according to:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auth/kubectl-exec-plugins.md
and with inspiration from the kubernetes python implementation (https://github.com/kubernetes-client/python-base/blob/master/config/exec_provider.py)
1. I haven't written tests
2. Token refreshment hasn't been implemented meaning that once the token is expired a new token has to be generated, I am currently using my kubernetes "access layer" from the DI with a scoped life time cycle.
3. I've tested it running on Linux (Amazon Linux 2) only, not sure if it works properly on Windows/any other creature ;)
Would appreciate help finishing and wrapping this up for usage.