Attempt to implement exec-plugins support in kubeconfig #75

dovreshef · 2018-07-18T10:15:45Z

Hi

This is my attempt to implement exec plugin support.
I've only tested this using Python3 with EKS.
I'm also only supporting token authentication for now, since it was not clear to me how to support anything else.
I'm willing to do the necessary changes to get it to merge, but I might need help.

Thanks,

k8s-ci-robot · 2018-07-18T10:15:47Z

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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

codecov-io · 2018-07-18T11:06:21Z

Codecov Report

Merging #75 into master will decrease coverage by 0.21%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- Coverage   91.85%   91.63%   -0.22%     
==========================================
  Files          11       13       +2     
  Lines         994     1124     +130     
==========================================
+ Hits          913     1030     +117     
- Misses         81       94      +13

Impacted Files	Coverage Δ
config/kube_config_test.py	`93.64% <100%> (+0.13%)`	⬆️
config/kube_config.py	`83.77% <75%> (-0.5%)`	⬇️
config/exec_provider.py	`82.6% <82.6%> (ø)`
config/exec_provider_test.py	`98.36% <98.36%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9b3113...becae56. Read the comment docs.

dovreshef · 2018-07-21T06:23:43Z

@roycaihw What is the process to get this reviewed and (hopefully) accepted?

ltamaster · 2018-08-10T21:49:57Z

I tested this PR and worked, any plan to merge it?
Thanks
Luis

cliffpracht · 2018-08-14T15:16:00Z

This is working for me as well. 👍
@mbohlool @yliaog @roycaihw
Anything we can do to make this better? or merged in?
Thanks!

anneschuth · 2018-08-20T07:09:21Z

I'd love to see this merged too.

yliaog · 2018-08-20T18:06:06Z

thanks for the pr. has this been tested with python 2.7.12?

cliffpracht · 2018-08-20T18:08:19Z

I have tested this with 2.7.15.

yliaog · 2018-08-20T18:11:02Z

config/exec_provider.py

@@ -0,0 +1,79 @@
+# Copyright 2016 The Kubernetes Authors.


s/2016/2018/ for all copyright notices?

yliaog · 2018-08-20T19:00:03Z

config/exec_provider_test.py

+    def setUp(self):
+        self.input_ok = {
+            'command': 'aws-iam-authenticator token -i dummy',
+            'apiVersion': 'client.authentication.k8s.io/v1alpha1'


s/v1alpha1/v1beta1/ for all the tests? the feature is v1beta1 is 1.11

yliaog · 2018-08-20T19:28:21Z

config/exec_provider.py

+            'apiVersion': self.api_version,
+            'kind': 'ExecCredential',
+            'spec': {
+                'interactive': True


why is it hard coded to True?

I have changed it to be the same as the process's stdout (using sys.stdout.isatty()), which is what was done here https://github.com/kubernetes/kubernetes/blob/7b6647a418c660f2c87f183f706b297f1cb573ca/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go#L128 (I think).
But I'm still not clear whether or not I should detect interactive stdin as well. Also, if it is interactive, what else am I supposed to do.

yliaog · 2018-08-20T19:34:51Z

config/exec_provider.py

+
+class ExecProvider(object):
+    def __init__(self, exec_config):
+        for key in ['command', 'apiVersion']:


could you list explicitly what is implemented (v1beta1, right?) as listed in the proposal (https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auth/kubectl-exec-plugins.md), or better, what is left out.

AFAICT, TLS cert, caching are not implemented.

what about refreshing? what to do when token expires?

TLS cert, caching are indeed not implemented.

TLS cert

I did not implement it.
My reading of the proposal language
TLS client certificate support is orthogonal to bearer tokens, but something that we should consider supporting in the future.
lead me to think it is not implemented in the go client as well.

caching

I can try to add caching by basing it on the way it is implemented for the other auth methods, taking into account the expiry field.
But there is a problem. Right now aws-iam-authenticator does not return anexpiry field, even though the returned token is only valid for a short period (15 minutes IIRC).
The proposal on the other hand specifies:
Plugins can indicate their credentials explicit expiry using the Expiry field on the returned ExecCredentials object, otherwise credentials will be cached throughout the lifetime of a program.

I can check the return value if it is 401 and in that case re-execute the plugin, which is what the proposal also recommends. But I do not understand the code well enough for that.
Can you point me where in the code would it be best to hook up such a mechanism?

what about refreshing? what to do when token expires?

Right now I do not cache the token at all. So since the plugin always executes, the token received is always fresh.

since this is a highly desired feature, and it is orthogonal to the other auth methods. I think it is safe to merge with some missing features. people who need those features can implement them on top of this later.

so could you please add some comments about what features are missing, and probably a link
to the design (https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auth/kubectl-exec-plugins.md). then I think we can merge it. Thanks.

Added. Hope it's ok.

anneschuth · 2018-08-23T10:27:55Z

How far from getting this merged are we? Is there anything I can do to help this go through faster?

yliaog · 2018-08-23T17:13:47Z

I don't think it's far. I put in some comments, as soon as those are addressed, we can merge it.

macat · 2018-08-23T17:39:49Z

@dovreshef Do you need help fixing @yliaog's comments? Thanks for building this BTW.

dovreshef · 2018-08-23T19:03:59Z

@macat I'd be glad for help. I'll not be able to get to this before Monday.

gitfool · 2018-08-30T01:37:09Z

👍

dovreshef · 2018-09-02T07:32:56Z

Hi all. I have updated the PR.

I'm sorry about the delay, real life intervened.

Edit: Also, I don't know what This pull-request has been approved by: dovreshef above means, I did not approve it, merely updated it.

gitfool · 2018-09-02T07:55:28Z

No worries. I’m using a monkey patch with this change to tide me over. 😉

yliaog · 2018-09-04T20:58:02Z

config/kube_config.py

+        status = ExecProvider(self._user['exec']).run()
+        if 'token' not in status:
+            raise ConfigException(
+                'Missing token field in exec plugin response')


i think it's better to catch all the exceptions and return None in case there is any exception, just to give a chance to other auth methods. that is the general pattern used by other load...() methods.

Ok.
Then there is no reason throw any exception at all in exec_provider.py?
In that case how should I inform the user if the exec process return an error, or missing exec config, etc?

it's still good to have exceptions from exec_provider.py, then in kube_config.py, when it catches the exception, it could log it, and proceed.

Done. I have used logging.error since it seems to me to be more of an error than a warning. Since it implies an incorrect exec config / result.

…token only, no caching)

yliaog · 2018-09-06T17:00:42Z

thanks for the PR.

/lgtm
/approve

k8s-ci-robot · 2018-09-06T17:00:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dovreshef, yliaog

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~~ [yliaog]

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

anneschuth · 2018-09-07T09:43:47Z

thanks a lot!

Include kubernetes-client/python-base#75

timoreimann · 2019-10-02T11:40:05Z

For posterity: this one fixes kubernetes-client/python#514

dovreshef force-pushed the master branch from d39b3bb to abc5d2b Compare July 19, 2018 07:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2018

dovreshef force-pushed the master branch from abc5d2b to caec854 Compare July 19, 2018 13:08

dovreshef mentioned this pull request Jul 21, 2018

Port exec-based client auth provider to Python client kubernetes-client/python#514

Closed

dovreshef force-pushed the master branch 4 times, most recently from 999c199 to d56d8df Compare August 12, 2018 13:16

roycaihw self-assigned this Aug 14, 2018

yliaog reviewed Aug 20, 2018

View reviewed changes

dovreshef force-pushed the master branch from d56d8df to a489fde Compare September 2, 2018 07:27

dovreshef force-pushed the master branch 3 times, most recently from c02241d to c1bb5cc Compare September 3, 2018 08:01

yliaog reviewed Sep 4, 2018

View reviewed changes

Add partial support for out-of-tree client authentication providers (…

becae56

…token only, no caching)

dovreshef force-pushed the master branch from c1bb5cc to becae56 Compare September 6, 2018 09:29

k8s-ci-robot assigned yliaog Sep 6, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2018

k8s-ci-robot merged commit d68e456 into kubernetes-client:master Sep 6, 2018

tomplus mentioned this pull request Sep 6, 2018

porting exec-plugins support from kubernetes-client/python-base tomplus/kubernetes_asyncio#40

Closed

willthames added a commit to willthames/python that referenced this pull request Sep 8, 2018

Update python-base to latest for exec-based clients

1de41f1

Include kubernetes-client/python-base#75

willthames mentioned this pull request Sep 8, 2018

Update python-base to latest for exec-based clients kubernetes-client/python#619

Merged

grahamhar mentioned this pull request Sep 11, 2018

exec plugin support kubernetes-client/python#621

Closed

fabianvf mentioned this pull request Sep 19, 2018

k8s kubeconfig authorization issue with heptio-authenticator-aws ansible/ansible#45858

Closed

yuki-mt mentioned this pull request Dec 14, 2018

change to run rekcurd on EKS rekcurd/dashboard#51

Merged

jdamata mentioned this pull request Mar 4, 2019

Running powerfulseal on AWS EKS powerfulseal/powerfulseal#96

Closed

roycaihw mentioned this pull request Apr 24, 2024

ConfigExceptions aren't made visible kubernetes-client/python#2219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to implement exec-plugins support in kubeconfig #75

Attempt to implement exec-plugins support in kubeconfig #75

dovreshef commented Jul 18, 2018

k8s-ci-robot commented Jul 18, 2018

codecov-io commented Jul 18, 2018 •

edited

Loading

dovreshef commented Jul 21, 2018

ltamaster commented Aug 10, 2018

cliffpracht commented Aug 14, 2018 •

edited

Loading

anneschuth commented Aug 20, 2018

yliaog commented Aug 20, 2018

cliffpracht commented Aug 20, 2018

yliaog Aug 20, 2018

dovreshef Sep 2, 2018

yliaog Aug 20, 2018

dovreshef Sep 2, 2018

yliaog Aug 20, 2018

dovreshef Sep 2, 2018 •

edited

Loading

yliaog Aug 20, 2018

dovreshef Sep 2, 2018 •

edited

Loading

yliaog Sep 2, 2018

dovreshef Sep 3, 2018

anneschuth commented Aug 23, 2018 •

edited

Loading

yliaog commented Aug 23, 2018

macat commented Aug 23, 2018

dovreshef commented Aug 23, 2018

gitfool commented Aug 30, 2018

dovreshef commented Sep 2, 2018 •

edited

Loading

gitfool commented Sep 2, 2018

yliaog Sep 4, 2018

dovreshef Sep 5, 2018

yliaog Sep 5, 2018

dovreshef Sep 6, 2018

yliaog commented Sep 6, 2018

k8s-ci-robot commented Sep 6, 2018

anneschuth commented Sep 7, 2018

timoreimann commented Oct 2, 2019

Attempt to implement exec-plugins support in kubeconfig #75

Attempt to implement exec-plugins support in kubeconfig #75

Conversation

dovreshef commented Jul 18, 2018

k8s-ci-robot commented Jul 18, 2018

codecov-io commented Jul 18, 2018 • edited Loading

Codecov Report

dovreshef commented Jul 21, 2018

ltamaster commented Aug 10, 2018

cliffpracht commented Aug 14, 2018 • edited Loading

anneschuth commented Aug 20, 2018

yliaog commented Aug 20, 2018

cliffpracht commented Aug 20, 2018

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

dovreshef Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dovreshef Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anneschuth commented Aug 23, 2018 • edited Loading

yliaog commented Aug 23, 2018

macat commented Aug 23, 2018

dovreshef commented Aug 23, 2018

gitfool commented Aug 30, 2018

dovreshef commented Sep 2, 2018 • edited Loading

gitfool commented Sep 2, 2018

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

yliaog commented Sep 6, 2018

k8s-ci-robot commented Sep 6, 2018

anneschuth commented Sep 7, 2018

timoreimann commented Oct 2, 2019

codecov-io commented Jul 18, 2018 •

edited

Loading

cliffpracht commented Aug 14, 2018 •

edited

Loading

dovreshef Sep 2, 2018 •

edited

Loading

dovreshef Sep 2, 2018 •

edited

Loading

anneschuth commented Aug 23, 2018 •

edited

Loading

dovreshef commented Sep 2, 2018 •

edited

Loading