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

Populate token expiry for GCE credentials #473

Merged
merged 1 commit into from Jun 10, 2016

Conversation

Projects
None yet
7 participants
@bendemaree
Contributor

bendemaree commented Mar 24, 2016

Populates the token_expiry property for GCE App Assertion credentials. The token responses from the metadata service have a expires_in value in the response that can be leveraged to make things work better on the base OAuth2Credentials class, like access_token_expired.

I largely followed the implementation in the base client though less defensively; I don't see any reason why expires_in wouldn't be present though perhaps someone knows better. I did note that the existing tests actually assert this value is not set in the mocked token response so perhaps that provides evidence against my assumption.

@googlebot googlebot added the cla: yes label Mar 24, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 24, 2016

Coverage Status

Coverage increased (+0.003%) to 97.569% when pulling c96c404 on bendemaree:gce-app-assertion-expiry into 3ca2ca7 on google:master.

coveralls commented Mar 24, 2016

Coverage Status

Coverage increased (+0.003%) to 97.569% when pulling c96c404 on bendemaree:gce-app-assertion-expiry into 3ca2ca7 on google:master.

Show outdated Hide outdated oauth2client/contrib/gce.py Outdated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 24, 2016

Coverage Status

Coverage increased (+0.002%) to 97.569% when pulling bffa989 on bendemaree:gce-app-assertion-expiry into 3ca2ca7 on google:master.

coveralls commented Mar 24, 2016

Coverage Status

Coverage increased (+0.002%) to 97.569% when pulling bffa989 on bendemaree:gce-app-assertion-expiry into 3ca2ca7 on google:master.

@@ -73,6 +78,8 @@ def _refresh_success_helper(self, bytes_response=False):
self.assertEquals(None, credentials.access_token)
credentials.refresh(http)
self.assertEquals(access_token, credentials.access_token)
self.assertFalse(credentials.access_token_expired)
self.assertTrue(credentials.token_expiry > datetime.utcnow())

This comment has been minimized.

@dhermes

dhermes Mar 24, 2016

Contributor

FYI you could do something like this to control the output of utcnow:

@mock.patch('datetime.datetime.utcnow', return_value=123456789)
def _refresh_success_helper(self, utcnow, bytes_response=False):
    # Make sure the value returned was 12345678
    ...
    utcnow.assert_called_once_with()
@dhermes

dhermes Mar 24, 2016

Contributor

FYI you could do something like this to control the output of utcnow:

@mock.patch('datetime.datetime.utcnow', return_value=123456789)
def _refresh_success_helper(self, utcnow, bytes_response=False):
    # Make sure the value returned was 12345678
    ...
    utcnow.assert_called_once_with()

This comment has been minimized.

@bendemaree

bendemaree Mar 25, 2016

Contributor

Yep, certainly...this is an older habit for me to prevent myself from accidentally returning a mocked value that is not the right type: utcnow actually returns a datetime.datetime but I had to check the stdlib docs because it seemed possible it would return a Unix time int. Happy to update if you'd prefer the mock, though!

@bendemaree

bendemaree Mar 25, 2016

Contributor

Yep, certainly...this is an older habit for me to prevent myself from accidentally returning a mocked value that is not the right type: utcnow actually returns a datetime.datetime but I had to check the stdlib docs because it seemed possible it would return a Unix time int. Happy to update if you'd prefer the mock, though!

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes
Member

theacodes commented Mar 25, 2016

/cc @elibixby

@@ -135,6 +136,8 @@ def _refresh(self, http_request):
raise HttpAccessTokenRefreshError(str(e),
status=response.status)
self.access_token = token_content['access_token']
delta = datetime.timedelta(seconds=int(token_content['expires_in']))

This comment has been minimized.

@elibixby

elibixby Mar 25, 2016

Contributor

This is unnecessary. The metadata server returns an "expires_at" field along with the access token.

@elibixby

elibixby Mar 25, 2016

Contributor

This is unnecessary. The metadata server returns an "expires_at" field along with the access token.

This comment has been minimized.

@elibixby

elibixby Mar 25, 2016

Contributor

This comment has been minimized.

@bendemaree

bendemaree Mar 25, 2016

Contributor

Oh. Is the 0.1 metadata service more recent than v1? Or a different API? I don't have it in front of me but IIRC the v1 server only returned the seconds until expiration.

Edit: Just noticed the commit comment. Still curious, though.

@bendemaree

bendemaree Mar 25, 2016

Contributor

Oh. Is the 0.1 metadata service more recent than v1? Or a different API? I don't have it in front of me but IIRC the v1 server only returned the seconds until expiration.

Edit: Just noticed the commit comment. Still curious, though.

This comment has been minimized.

@dhermes

dhermes Mar 25, 2016

Contributor

@Galabar001 might be able to answer your question

@dhermes

dhermes Mar 25, 2016

Contributor

@Galabar001 might be able to answer your question

This comment has been minimized.

@theacodes

theacodes Jun 6, 2016

Member

So which do we prefer? expires_in or expires_at?

@theacodes

theacodes Jun 6, 2016

Member

So which do we prefer? expires_in or expires_at?

This comment has been minimized.

@theacodes

theacodes Jun 10, 2016

Member

Historical note: it seems we use expires_in in this library consistently, so no need to rock the boat.

@theacodes

theacodes Jun 10, 2016

Member

Historical note: it seems we use expires_in in this library consistently, so no need to rock the boat.

@elibixby

This comment has been minimized.

Show comment
Hide comment
@elibixby

elibixby Mar 25, 2016

Contributor

Hi bendemaree@ thanks for the contribution, but I'm actually working on a larger update to GCE credentials that includes this.

Still writing tests but I'll go ahead and submit a pull request so more work doesn't get duplicated =(

Contributor

elibixby commented Mar 25, 2016

Hi bendemaree@ thanks for the contribution, but I'm actually working on a larger update to GCE credentials that includes this.

Still writing tests but I'll go ahead and submit a pull request so more work doesn't get duplicated =(

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Mar 25, 2016

Contributor

@elibixby smaller PRs are much easier to review FWIW.

Contributor

dhermes commented Mar 25, 2016

@elibixby smaller PRs are much easier to review FWIW.

@elibixby

This comment has been minimized.

Show comment
Hide comment
@elibixby

elibixby Mar 25, 2016

Contributor

See #476

I rewrote the wrapper function for the metadata server so that it can be used by lots of common calls that will need to be made, so I would like the first PR to contain that. However, if after that we want to rewrite the interface in a bunch of small PRs that's fine with me. Still, I think all these changes are quite related and should be reviewed together. Just my .02

(Also FYI, I AM splitting up the PR with the GCE credential updates, and the PR to add IAM blob signing, as IAM blob signing could potentially be used in other credential types)

Contributor

elibixby commented Mar 25, 2016

See #476

I rewrote the wrapper function for the metadata server so that it can be used by lots of common calls that will need to be made, so I would like the first PR to contain that. However, if after that we want to rewrite the interface in a bunch of small PRs that's fine with me. Still, I think all these changes are quite related and should be reviewed together. Just my .02

(Also FYI, I AM splitting up the PR with the GCE credential updates, and the PR to add IAM blob signing, as IAM blob signing could potentially be used in other credential types)

@bendemaree

This comment has been minimized.

Show comment
Hide comment
@bendemaree

bendemaree Mar 25, 2016

Contributor

Ah, alright, thanks for the review anyway! Looking forward to the update; we're trying out some optimistic credential refresh logic since the OAuth dance takes a bit and could cause a request to hang while the token is implicitly refreshed.

Closing out; feel free to reopen if needed.

Contributor

bendemaree commented Mar 25, 2016

Ah, alright, thanks for the review anyway! Looking forward to the update; we're trying out some optimistic credential refresh logic since the OAuth dance takes a bit and could cause a request to hang while the token is implicitly refreshed.

Closing out; feel free to reopen if needed.

@bendemaree bendemaree closed this Mar 25, 2016

@bendemaree

This comment has been minimized.

Show comment
Hide comment
@bendemaree

bendemaree Apr 25, 2016

Contributor

@elibixby Given that #476 was closed out and there aren't followup PRs yet (I may have missed them though), would it be possible to put this back on the table for review?...

Contributor

bendemaree commented Apr 25, 2016

@elibixby Given that #476 was closed out and there aren't followup PRs yet (I may have missed them though), would it be possible to put this back on the table for review?...

@elibixby

This comment has been minimized.

Show comment
Hide comment
@elibixby

elibixby Apr 25, 2016

Contributor

I'm going to split #476 into smaller PRs and submit those. It's on my plate for the next couple weeks.

I think the right thing to do here is write a metadata module that wraps the metadata server and caches requests. Which should make follow up PRs easy. If you want to take that on you are welcome to. I can't promise I will get to it this week.

Contributor

elibixby commented Apr 25, 2016

I'm going to split #476 into smaller PRs and submit those. It's on my plate for the next couple weeks.

I think the right thing to do here is write a metadata module that wraps the metadata server and caches requests. Which should make follow up PRs easy. If you want to take that on you are welcome to. I can't promise I will get to it this week.

@bendemaree

This comment has been minimized.

Show comment
Hide comment
@bendemaree

bendemaree Jun 3, 2016

Contributor

@elibixby @jonparrott Sorry to bring this back to your attention again, but is there any chance we can merge this in? I don't think it's more than a minor bugfix, and at this point it's been 2 months without the major refactor needed to get this behavioral fix. 😢 I don't have the familiarity or bandwidth to re-wrap the metadata server myself, unfortunately.

Contributor

bendemaree commented Jun 3, 2016

@elibixby @jonparrott Sorry to bring this back to your attention again, but is there any chance we can merge this in? I don't think it's more than a minor bugfix, and at this point it's been 2 months without the major refactor needed to get this behavioral fix. 😢 I don't have the familiarity or bandwidth to re-wrap the metadata server myself, unfortunately.

@bendemaree bendemaree reopened this Jun 3, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jun 6, 2016

Member

@dhermes are you fine with this being merged?

Member

theacodes commented Jun 6, 2016

@dhermes are you fine with this being merged?

@elibixby

This comment has been minimized.

Show comment
Hide comment
@elibixby

elibixby Jun 6, 2016

Contributor

@bendemaree Sorry about taking so long. I will have some time in the next week to work on my PRs, but if you'd like to go ahead and submit this for merge. That's understandable.

Contributor

elibixby commented Jun 6, 2016

@bendemaree Sorry about taking so long. I will have some time in the next week to work on my PRs, but if you'd like to go ahead and submit this for merge. That's understandable.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jun 7, 2016

Contributor

https://coveralls.io/builds/5540724

What happened to 100% coverage?

Contributor

dhermes commented Jun 7, 2016

https://coveralls.io/builds/5540724

What happened to 100% coverage?

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jun 7, 2016

Contributor

Ahhh I know. This needs to be rebased against HEAD in master

Contributor

dhermes commented Jun 7, 2016

Ahhh I know. This needs to be rebased against HEAD in master

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Jun 7, 2016

Collaborator

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Collaborator

googlebot commented Jun 7, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 7, 2016

Ben Demaree
Populate token expiry for GCE credentials
Populates the token_expiry property for GCE App Assertion credentials
(thus enabling access_token_expired). This corrects assumptions like the
one in the access_token_expired property on GCE specifically: it's stated
there "If the token_expiry isn't set, we assume the token doesn't expire"
which seems to be incorrect for tokens retrieved from the GCE Metadata service.

Remove usage of _UTCNOW
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Jun 7, 2016

Collaborator

CLAs look good, thanks!

Collaborator

googlebot commented Jun 7, 2016

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jun 7, 2016

@bendemaree

This comment has been minimized.

Show comment
Hide comment
@bendemaree

bendemaree Jun 7, 2016

Contributor

@dhermes Squashed and rebased (eventually!).

@elibixby @jonparrott Thanks a lot for circling back on this; much obliged!

Contributor

bendemaree commented Jun 7, 2016

@dhermes Squashed and rebased (eventually!).

@elibixby @jonparrott Thanks a lot for circling back on this; much obliged!

@@ -135,6 +136,8 @@ def _refresh(self, http_request):
raise HttpAccessTokenRefreshError(str(e),
status=response.status)
self.access_token = token_content['access_token']
delta = datetime.timedelta(seconds=int(token_content['expires_in']))
self.token_expiry = delta + datetime.datetime.utcnow()

This comment has been minimized.

@ericbuehl

ericbuehl Jun 7, 2016

should this use oauth2client.client._UTCNOW for consistency?

@ericbuehl

ericbuehl Jun 7, 2016

should this use oauth2client.client._UTCNOW for consistency?

This comment has been minimized.

@theacodes

theacodes Jun 7, 2016

Member

Please

@theacodes

theacodes Jun 7, 2016

Member

Please

This comment has been minimized.

@bendemaree

bendemaree Jun 7, 2016

Contributor

That suggestion conflicts with prior review.

@bendemaree

bendemaree Jun 7, 2016

Contributor

That suggestion conflicts with prior review.

This comment has been minimized.

@theacodes

theacodes Jun 7, 2016

Member

Gotcha. No worries then. 👍

@theacodes

theacodes Jun 7, 2016

Member

Gotcha. No worries then. 👍

@bendemaree

This comment has been minimized.

Show comment
Hide comment
@bendemaree

bendemaree Jun 10, 2016

Contributor

Bump. 😁

Contributor

bendemaree commented Jun 10, 2016

Bump. 😁

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jun 10, 2016

Member

This LGTM.

Member

theacodes commented Jun 10, 2016

This LGTM.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jun 10, 2016

Member

@dhermes if you have any post-merge objections, let me know and I'll fix myself.

Member

theacodes commented Jun 10, 2016

@dhermes if you have any post-merge objections, let me know and I'll fix myself.

@theacodes theacodes merged commit 54d7dce into googleapis:master Jun 10, 2016

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bendemaree

This comment has been minimized.

Show comment
Hide comment
@bendemaree

bendemaree Jul 10, 2016

Contributor

@jonparrott Is there an upcoming release planned that will include this?

Contributor

bendemaree commented Jul 10, 2016

@jonparrott Is there an upcoming release planned that will include this?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Jul 11, 2016

Member

Yes, it will be 3.0.0. I'm waiting for a few outstanding PRs to be resolved first.

Member

theacodes commented Jul 11, 2016

Yes, it will be 3.0.0. I'm waiting for a few outstanding PRs to be resolved first.

@theacodes theacodes referenced this pull request Jul 28, 2016

Merged

Release 3.0.0 #575

@bendemaree bendemaree deleted the bendemaree:gce-app-assertion-expiry branch Sep 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment