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

Add proper GCP config loader and refresher #22

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

mbohlool
Copy link
Contributor

This is based on #21. I added expiration mechanism as well as persisting config back so we don't need to refresh the token every time. The way it persisted back should be the same as kubectl.
Tests will fail for this PR unless kubernetes-client/python#302 is merged.

@gabrielgbim
Copy link

Thanks!

@mbohlool
Copy link
Contributor Author

@gabrielgbim @pokoli This is a release blocker. Can you review it please.

@mbohlool mbohlool force-pushed the gce_config branch 2 times, most recently from f4da0c7 to 253d937 Compare July 23, 2017 10:15
@codecov-io
Copy link

codecov-io commented Jul 23, 2017

Codecov Report

Merging #22 into master will decrease coverage by 1.05%.
The diff coverage is 87.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   94.69%   93.64%   -1.06%     
==========================================
  Files           9       11       +2     
  Lines         698      803     +105     
==========================================
+ Hits          661      752      +91     
- Misses         37       51      +14
Impacted Files Coverage Δ
config/dateutil_test.py 100% <100%> (ø)
config/kube_config.py 93.17% <77.77%> (-3.41%) ⬇️
config/dateutil.py 88.88% <88.88%> (ø)
config/kube_config_test.py 92.49% <92.3%> (-0.07%) ⬇️

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 00d2417...824c03c. Read the comment docs.

@@ -54,6 +59,17 @@ def _create_temp_file_with_content(content):
return name


def _is_expired(expiry):
tf = tf_from_timestamp(expiry)
n = time.time()
Copy link

Choose a reason for hiding this comment

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

I don't think you need to store n as a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -308,21 +353,32 @@ def load_kube_config(config_file=None, context=None,
from config file will be used.
:param client_configuration: The kubernetes.client.ConfigurationObject to
set configs to.
:param persist_config: If True and config changed (e.g. GCP token refresh)
Copy link

Choose a reason for hiding this comment

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

Maybe better: "IF True config file will be updated when changed (e.g GCP token refresh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

"""

if config_file is None:
config_file = os.path.expanduser(KUBE_CONFIG_DEFAULT_LOCATION)

config_persister = None
if persist_config:
config_persister = lambda config_map, config_file=config_file: (
Copy link

Choose a reason for hiding this comment

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

Why is required a lambda function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to pass the config_file to _save_kube_config. If there is a better way to do it, I am open to it.

Copy link

Choose a reason for hiding this comment

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

AFAIU it should work with:
config_persister = _save_kube_config

Copy link
Contributor Author

@mbohlool mbohlool Jul 25, 2017

Choose a reason for hiding this comment

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

the key here is the config_file=config_file: part that passes the local variable config_file as one of the parameters.config_persisteraccepts one parameter,_save_kube_config` needs two.

Copy link

Choose a reason for hiding this comment

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

Ok, I didn't understand at all the code.

Then you should probably use the partial function from functools standard library.

This will make the code clearer.

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 inline'ed _save_kube_config. it should be cleaner. is it good or do you still suggest partial function?

Copy link

Choose a reason for hiding this comment

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

Ok, now the usage it's much clearer.

actual = FakeConfig()
KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="gcp",
client_configuration=actual,
get_google_credentials=lambda: TEST_ANOTHER_DATA_BASE64) \
get_google_credentials=lambda: "SHOULD NOT BE CALLED") \
Copy link

Choose a reason for hiding this comment

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

maybe you can put a raise to ensure it's not called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this with some other 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.

Done.

@@ -0,0 +1 @@
The (rfc3339.py)[rfc3339.py] file is copied from [this site](http://home.blarg.net/~steveha/pyfeed.html) because PyFeed is not available in PyPi.
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like they only do one side of conversion? https://bitbucket.org/henry/rfc3339/issues/3/datetime-to-rfc-3339

I see a parse method but it is not exported.

Copy link

Choose a reason for hiding this comment

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

Then probably it's worth to contribute one to the library :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. This is, however, a release blocker and I don't want to wait for that contribution to go through. I suggest we proceed with this and either contribute to the that one or even release the one I copied as a pypi package for all to use (I have a feeling by looking at the code that this one is better, but we can decide on that later).

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've looked at different options and decided to implement my own. Some options are too restrictive for our use and the others are too admissive. I've implemented a simple balanced one in dateutil.py.

Copy link

Choose a reason for hiding this comment

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

Ok, as it's a release blocker I think the unique option we have is to include the code in the client.

Copy link
Contributor Author

@mbohlool mbohlool Jul 25, 2017

Choose a reason for hiding this comment

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

For the record, I didn't copy code from any of those libraries. I've implemented it again by reading rfc. Of course my implementation can be influenced by those libraries, but I stick to the rules of rfc3339. None of the libraries I've checked were like that, they were either relaxer or stricter. Also our implementation forces a UTC timezone.

@mbohlool
Copy link
Contributor Author

@pokoli I am going to add some tests for dateutil but this is basically ready for review and I already tested it for GKE. Please take another look. I would like to merge this as soon as possible to cut a new release.

@mbohlool mbohlool force-pushed the gce_config branch 2 times, most recently from 5d0b367 to 50f717d Compare July 25, 2017 03:54
Copy link

@pokoli pokoli left a comment

Choose a reason for hiding this comment

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

We should probably need some test for the dateutil module.

"""

if config_file is None:
config_file = os.path.expanduser(KUBE_CONFIG_DEFAULT_LOCATION)

config_persister = None
if persist_config:
config_persister = lambda config_map, config_file=config_file: (
Copy link

Choose a reason for hiding this comment

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

AFAIU it should work with:
config_persister = _save_kube_config

@mbohlool mbohlool force-pushed the gce_config branch 3 times, most recently from c4b8c87 to bd507f4 Compare July 25, 2017 09:28
@mbohlool
Copy link
Contributor Author

Thanks for the review @pokoli I think I addressed all of your comments and tests are passing. Can you take a final look please?

Copy link

@pokoli pokoli left a comment

Choose a reason for hiding this comment

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

Despite two minor issues its a LGTM

@@ -308,21 +341,35 @@ def load_kube_config(config_file=None, context=None,
from config file will be used.
:param client_configuration: The kubernetes.client.ConfigurationObject to
set configs to.
:param persist_config: IF True, config file will be updated when changed
Copy link

Choose a reason for hiding this comment

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

IF -> If

@@ -36,6 +38,10 @@ def _base64(string):
return base64.encodestring(string.encode()).decode()


def _raise_exception(st):
Copy link

Choose a reason for hiding this comment

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

Why not directly raising the exception?

Copy link
Contributor Author

@mbohlool mbohlool Jul 25, 2017

Choose a reason for hiding this comment

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

Why do you mean? I am passing this function as a function pointer to config loader and expect the function pointer (that suppose to update GCE token) never been called in the test. I was using lambda syntax to return a dummy token before, but you cannot raise exception in lambda syntax.

Copy link

Choose a reason for hiding this comment

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

Ok, I didn't now that you can not raise inside lambda (always learning from codereview).

Forget about it.

@pokoli
Copy link

pokoli commented Jul 25, 2017

@mbohlool for me you can merge once fixed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants