feat: add support for new ContainerAuthenticator #151
Conversation
| * limitations under the License. | ||
| */ | ||
|
|
||
| import { ContainerTokenManager } from '../token-managers'; |
There was a problem hiding this comment.
Note that most of the "meat" of this change is in the token manager, not the authenticator
| @@ -24,6 +24,6 @@ | |||
| */ | |||
|
|
|||
| export * from './helpers'; | |||
| export * from './read-credentials-file'; | |||
| export * from './file-reading-helpers'; | |||
There was a problem hiding this comment.
I changed the name of this file to be more accurate. Even though this file is importable by users, it is not part of our "official" API so I don't consider it a breaking change. We wouldn't expect any users to be using this file, it only contains internal utilities.
| @@ -67,7 +67,6 @@ describe('IAM Authenticator', () => { | |||
| // verify the scope param wasn't set | |||
| expect(authenticator.scope).toBeUndefined(); | |||
| expect(authenticator.tokenManager.scope).toBeUndefined(); | |||
| done(); | |||
| }); | |||
There was a problem hiding this comment.
I didn't make anything more than superficial changes to these tests to ensure the refactoring maintains compatibility
There was a problem hiding this comment.
LGTM
Initially, I was a little surprised to not find much testing of the requestToken() method in terms of the various scenarios with token expiration, etc. but after poking around it looks like the jwt-token-manager.test.js script contains tests for those types of scenarios in sort of a generic sense (i.e. not specific to cp4d, iam or container authenticators). Is that right?
|
|
||
| /** | ||
| * Setter for the filename of the compute resource token. | ||
| * @param {string} scope A string containing a path to the CR token file |
There was a problem hiding this comment.
| * @param {string} scope A string containing a path to the CR token file | |
| * @param {string} crTokenFilename A string containing a path to the CR token file |
copy/paste faux pas??? :)
other instances of this below as well
| throw new Error(`File does not exist: ${filepath}`); | ||
| } | ||
|
|
||
| if (token === '') { |
There was a problem hiding this comment.
In the other SDKs we don't check for empty file. I think we should be consistent across the languages, so I will happily add this to the Python core if you wish.
There was a problem hiding this comment.
That's fair - I saw @padamstx comment on your PR about having one error to capture the "can't read token" scenario. Perhaps I will update this code to only throw one generic error, but leave in the more specific log messages to help the user in the case of an issue.
What do you think about that?
auth/utils/file-reading-helpers.ts
Outdated
| logger.error(`Expected to find CR token file but the file does not exist: ${filepath}`); | ||
| throw new Error(`File does not exist: ${filepath}`); |
There was a problem hiding this comment.
Does the fileExistsAtPath catch permission issues? If not, these messages could be misleading. What do you think?
There was a problem hiding this comment.
No, permission issues will throw an error within the actual readFileSync method and the error shown will say "permission denied". So there shouldn't be an issue with confusing error messages here
That's right - everything but the actual request for the token is abstracted away into the shared parent, the JWT token manager. So, that behavior is all captured in the JWT token manager tests. |
Co-authored-by: Phil Adams <padamstx@gmail.com>
# [2.12.0](v2.11.3...v2.12.0) (2021-08-10) ### Features * add support for new ContainerAuthenticator ([#151](#151)) ([b01c011](b01c011))
|
🎉 This PR is included in version 2.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR adds an authenticator and token manager for the new "container" flavor of authentication. This style retrieves a compute resource token from the file system and uses that to retrieve an IAM access token, which is used to authenticate with the service.
Note: I realized that I refactored the IAM token manager but not the authenticator. It really should be both, so as part of this PR I refactored the authenticator to match the refactoring in the token manager.