Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Refresh GCP tokens on retrieval by overriding client config method. #92

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

TrevorEdwards
Copy link
Contributor

For GCP auth, this overrides the method used by the swagger-generated kubernetes client to get an authorization token, allowing us to use existing python-base code to refresh the GCP auth token. This should [fix #59], as API calls that happen > 1 hour after the client is populated will have the token refreshed around 5 minutes before expiration.

For reference, https://github.com/swagger-api/swagger-codegen/blob/4607a90d7b69463a0ae8fc94fac68fc95d80965e/modules/swagger-codegen/src/main/resources/python/configuration.mustache#L215 is the swagger generator and https://github.com/kubernetes-client/python/blob/master/kubernetes/client/configuration.py#L195 is today's generated client.

I also considered using a custom dict (TrevorEdwards@d5c8cdc), but that solution seemed too hacky.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 6, 2018
@codecov-io
Copy link

codecov-io commented Oct 6, 2018

Codecov Report

Merging #92 into master will increase coverage by 0.32%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   91.72%   92.04%   +0.32%     
==========================================
  Files          13       13              
  Lines        1136     1182      +46     
==========================================
+ Hits         1042     1088      +46     
  Misses         94       94
Impacted Files Coverage Δ
config/kube_config.py 84.24% <100%> (+0.3%) ⬆️
config/kube_config_test.py 94.41% <100%> (+0.63%) ⬆️

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 4da0fcc...8f3a69e. Read the comment docs.

@TrevorEdwards
Copy link
Contributor Author

/assign @roycaihw

Copy link
Member

@roycaihw roycaihw left a comment

Choose a reason for hiding this comment

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

Awesome! I didn't come up with this idea. I suppose this will work without adding notable computing complexity to existing API calls. We just need to document it well and mention it in release notes.

A little bit hacky but LGTM in general. @mbohlool @yliaog WDYT?

P.S. I found the test names not intuitive and we should probably refactor the kube_config code to make it generalized and more readable... but I realized that it's orthogonal to this PR.

# token refresh.
def _gcp_get_api_key(*args):
return self._load_gcp_token(self._user['auth-provider'])
client_configuration.get_api_key_with_prefix = _gcp_get_api_key
if 'token' in self.__dict__:
client_configuration.api_key['authorization'] = self.token
Copy link
Member

Choose a reason for hiding this comment

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

api_key['authorization'] will still be set for GCP on config loading, but it won't be updated/persisted on refresh. Also manually setting api_key['authorization'] won't work (although I suppose currently the workarounds aren't based on doing this manually). We should document this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added comments to make that behavior more clear, unless you mean that you want this documented somewhere else.

The only existing workaround I know of will catch exceptions when an API call fails (via a wrapper) and then reload the config. I believe with this change, they will never hit that exception.

}
},
{
"name": "expired_gcp_refresh",
Copy link
Member

Choose a reason for hiding this comment

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

expired_gcp_refresh is same as expired_gcp. Is the reason for having expired_gcp_refresh that expired_gcp will be mutated during tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added a comment to note that.

@TrevorEdwards TrevorEdwards force-pushed the 59_override branch 4 times, most recently from c89ff54 to d6725e2 Compare October 20, 2018 04:44
actual = FakeConfig()
fake_config = FakeConfig()
# swagger-generated config has this, but FakeConfig does not.
self.assertFalse(hasattr(fake_config, 'get_api_key_with_prefix'))
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a test to make sure swagger-generated config has this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

# token refresh.
def _gcp_get_api_key(*args):
return self._load_gcp_token(self._user['auth-provider'])
client_configuration.get_api_key_with_prefix = _gcp_get_api_key
Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with this hack. just to make sure we are protected when things change in configuration class of generated client, can we add proper tests to make sure get_api_key_with_prefix exists and being called the way we expected it.
I can't tell from tests that we are protected against changes in configuration class. please add them if they are missing. e.g. if get_api_key_with_prefix got rename, or if it is being called but it returns something else. I see a whitebox test for get_api_key_with_prefix, but it won't protect us against get_api_key_with_prefix being removed or renamed in configuration class).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I added three new test cases which I believe cover all of the concerns that you list.

@TrevorEdwards TrevorEdwards force-pushed the 59_override branch 2 times, most recently from 8f09bac to 8d2e406 Compare November 2, 2018 17:58
@TrevorEdwards
Copy link
Contributor Author

Build is failing, but I think it's due to the changes introduced in kubernetes-client/python@e639053#diff-07d29d9544bd4183a0a05fa23257e92d. I'll look into it.

@micw523
Copy link
Contributor

micw523 commented Nov 2, 2018

Hi @TrevorEdwards, I introduced this code change. It is likely due to an out-of-date ubuntu version change when we switched to a higher version of k8s on the server. Fixing it right now #99

@TrevorEdwards
Copy link
Contributor Author

Build's fixed, thanks @micw523

@mbohlool
Copy link
Contributor

mbohlool commented Nov 2, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2018
@roycaihw
Copy link
Member

roycaihw commented Nov 2, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roycaihw, TrevorEdwards

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GCP Token is not refreshed
6 participants