feat: add support for new ComputeResourceAuthenticator #123
Conversation
This reverts commit ffecbde.
v5/core/cri_authenticator.go
Outdated
| ) | ||
|
|
||
| // CriAuthenticator retrieves a "compute resource token" from the local compute resource (VM) | ||
| // and uses that to obtain an IAM access token by invoking the IAM "get token" operation w/grant-type=cr-token. |
There was a problem hiding this comment.
Typo?
| // and uses that to obtain an IAM access token by invoking the IAM "get token" operation w/grant-type=cr-token. | |
| // and uses that to obtain an IAM access token by invoking the IAM "get token" operation with grant-type=cr-token. |
There was a problem hiding this comment.
it wasn't really a typo as "w/xyz" typically means "with xyz", but I changed it to be clear :)
There was a problem hiding this comment.
I know, I just thought it wasn't intentional, sorry!
|
Reviewers,
I still need to update Authentication.md and will do that as part of this PR before merging :) |
padamstx
force-pushed
the
cri-authenticator
branch
3 times, most recently
from
8ef56e3 to
967ab89
Compare
padamstx
force-pushed
the
cri-authenticator
branch
from
967ab89 to
00b02b4
Compare
|
I've updated the go core's Authentication.md file to include info about the new ComputeResourceAuthenticator, plus a few editorial changes in the other sections as well. |
padamstx
changed the title
v5/core/authenticator_factory.go
Outdated
| authType := properties[PROPNAME_AUTH_TYPE] | ||
| if authType == "" { | ||
| authType = AUTHTYPE_IAM | ||
| // If the APIKEY property is specified, then we'll guess IAM... otherwise CRI. |
There was a problem hiding this comment.
I know this is an internal comment, so not a big deal, but if we're not using "CRI" terminology anymore we may want to change this
v5/core/cr_authenticator.go
Outdated
| // crToken := authenticator.retrieveCRToken() | ||
| // if crToken == "" { | ||
| // return fmt.Errorf(ERRORMSG_UNABLE_RETRIEVE_CRTOKEN) | ||
| // } |
There was a problem hiding this comment.
I think I agree with moving this to the RequestToken method since validation only happens once and the state could theoretically change over time. I don't really think we need to verify that we can get a token until it's time to get a token. I would be in favor of removing these comments so as not to clutter the code moving forward
There was a problem hiding this comment.
Yep, I actually intended to remove that but forgot :)
# [5.6.0](v5.5.1...v5.6.0) (2021-07-26) ### Features * add support for new ComputeResourceAuthenticator ([#123](#123)) ([c7631e3](c7631e3))
|
🎉 This PR is included in version 5.6.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
|
||
| // Check to make sure that one of IAMProfileName or IAMProfileID are specified. | ||
| if authenticator.IAMProfileName == "" && authenticator.IAMProfileID == "" { | ||
| return fmt.Errorf(ERRORMSG_EXCLUSIVE_PROPS_ERROR, "IAMProfileName", "IAMProfileID") |
There was a problem hiding this comment.
@padamstx I was reading through this while working on my implementation and I noticed a small detail that I didn't pick up on in my review - I don't think this is the right error message for this situation. The IAM Profile name and ID can both be specified, right? But this error message says that exactly one must be specified
There was a problem hiding this comment.
yes, you're right... As I recall, I consciously used this pre-existing error message (rather than create a new one) for the case where neither of the properties are specified. Technically it's not 100% correct because, as you say, they can in fact be specified together (as long as they are consistent with each other).
I can open an issue to fix this by introducing a new error message that covers this case.
This PR introduces support for a new authenticator implementation which supports the "Compute Resource Identities" feature.