refactor(auth): update auth classes #34
Conversation
ghost
requested a review
from 
gcloud/rest/auth/token.py
Outdated
| with open(service, 'r') as f: | ||
| data = json.loads(f.read()) | ||
| return data | ||
| except FileNotFoundError: |
There was a problem hiding this comment.
FileNotFoundError isn't defined in Py2 which raises IOError on a file that wasn't found:
>>> open('foobar', 'r')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
IOError: [Errno 2] No such file or directory: 'foobar'FileNotFoundError inherits from OSError, so I think you'll have to check for both IOError and OSError to make this portable.
There was a problem hiding this comment.
yes, I realized that too after the lint/hook failures. Have a change in place to fix this in the next update to this PR.
gcloud/rest/auth/iam.py
Outdated
| SCOPES = ['https://www.googleapis.com/auth/iam'] | ||
|
|
||
|
|
||
| class IamClient: |
There was a problem hiding this comment.
yup, a lint error caught this too. Thought I had fixed it, will be fixed in the next update to this PR.
gcloud/rest/auth/iam.py
Outdated
| class IamClient: | ||
| def __init__(self, | ||
| service_file=None, # type: Optional[str] | ||
| session=None, # type: requests.Session |
There was a problem hiding this comment.
Add Optional to all of these
There was a problem hiding this comment.
so my understanding was that there won't be a need to add Optional to these based on gcloud-aio's Type annotations - https://github.com/talkiq/gcloud-aio/blob/master/auth/gcloud/aio/auth/token.py#L65.
There was a problem hiding this comment.
Looks like these are incorrect in gcloud-aio as well; these would only be accepted if we have the --no-strict-optional flag set in mypy, which we should avoid since it tends to help mask missing null check issues. We should similarly make this change in -aio, since it'd be great to be more accurate about these things.
gcloud/rest/auth/iam.py
Outdated
| return self.token.service_data.get('client_email') | ||
|
|
||
| # https://cloud.google.com/iam/reference/rest/v1/projects.serviceAccounts.keys/get | ||
| # pylint: disable=too-many-arguments |
There was a problem hiding this comment.
Let's just throw this into our .pre-commit-config.yaml
gcloud/rest/auth/token.py
Outdated
| # to throw this error at load time rather than lazily during normal operations, | ||
| # where plumbing this error through will require several changes to otherwise- | ||
| # good error handling. | ||
| import cryptography # pylint: disable=unused-import |
There was a problem hiding this comment.
Let's keep this (and the comment explaining why we're keeping it!)
There was a problem hiding this comment.
Ahh! good point! will do!
gcloud/rest/auth/token.py
Outdated
| return jwt.encode(payload, credentials['private_key'], | ||
| algorithm='RS256') | ||
|
|
||
| def acquire(self): |
There was a problem hiding this comment.
Similarly, lets keep around the old public API aliased to the new version along with DeprecationWarnings, something like:
def acquire(self):
warnings.warn('Token.acquire() is deprecated, please use Token.get()', DeprecationWarning)There was a problem hiding this comment.
Cool, will do. Should acquire() then also call self.get() in the deprecation period?
There was a problem hiding this comment.
Basically, we want to match behavior as much as possible. Looks like acquire() is closest approximated by acquire_access_token(), ensure() is roughly equivalent to ensure_token(), etc. Basically, we want to make sure we have a few more releases where users aren't forced to change their API so they have time to properly migrate before we do a major version bump with breaking changes.
gcloud/rest/kms/client.py
Outdated
| @@ -1,3 +1,4 @@ | |||
| # pylint: disable=duplicate-code | |||
There was a problem hiding this comment.
Huh, I thought this was disabled globally?
There was a problem hiding this comment.
I got flagged for this when trying to commit. Will take a look once I commit again
gcloud/rest/auth/token.py
Outdated
| return self.session.post(self.token_uri, data=payload, | ||
| headers=REFRESH_HEADERS, timeout=timeout) | ||
|
|
||
| @backoff.on_exception(backoff.expo, Exception, max_tries=1) # type: ignore |
There was a problem hiding this comment.
Why change this to 1 instead of 5 tries? Not much point of having a backoff handler unless it has at least one retry
There was a problem hiding this comment.
Ahh, this is my bad. When running the integration tests, I was getting a '429 Client Error: Too Many Requests for url' error, which I pinged you about. So this was just me trying to see if the max_tries was causing this. Will revert this back to 5
Updates to auth module based on gcloud-aio using requests.Session