fix: refactor container authenticator with recent design changes #129
Conversation
This commit removes the aspects of the new ComputeResourceAuthenticator related to the VPC/VSI use-case. We're doing this due to some changes in the design of that use-case. With these changes, the ComputeResourceAuthenticator will support only the IKS file-based use-case.
| @@ -278,39 +278,12 @@ The IAM access token is added to each outbound request in the `Authorization` he | |||
| Authorization: Bearer <IAM-access-token> | |||
| ``` | |||
|
|
|||
| ### Compute Resource Token Retrieval Algorithm | |||
There was a problem hiding this comment.
No need to go into any detail about how the cr token is retreived since it's just read from a file.
| containerAuthMockCRTokenFile string = "../resources/cr-token.txt" | ||
| containerAuthMockIAMProfileName string = "iam-user-123" | ||
| containerAuthMockIAMProfileID string = "iam-id-123" | ||
| containerAuthMockClientID string = "client-id-1" | ||
| containerAuthMockClientSecret string = "client-secret-1" | ||
| containerAuthTestCRToken1 string = "cr-token-1" |
There was a problem hiding this comment.
One question: wouldn't be better to rename the cr-token to simply token or container-token?
There was a problem hiding this comment.
This is a good question. I know we've already messed with the naming plenty 🙂 but it's probably worth considering now that we've dropped the "compute resource" nomenclature from the user-facing code. This might apply to the CRTokenFilename property as well.
You do explain in the documentation that the token is a "compute resource token", so I think it's fine as is, but worth considering the simpler naming.
There was a problem hiding this comment.
I wanted to distinguish between the "cr token" and the "access token", hence the use of "cr token". Even though we're now calling this the ContainerAuthenticator, the "token" is still considered to be a "compute resource token" (and that name can also be used in the VPCInstanceAuthenticator when we add that in as well).
If you guys are ok with it, I'd like to keep the current nomenclature ("cr token", "CRTokenFilename", etc.).
There was a problem hiding this comment.
That makes sense so it's ok for me.
| containerAuthMockCRTokenFile string = "../resources/cr-token.txt" | ||
| containerAuthMockIAMProfileName string = "iam-user-123" | ||
| containerAuthMockIAMProfileID string = "iam-id-123" | ||
| containerAuthMockClientID string = "client-id-1" | ||
| containerAuthMockClientSecret string = "client-secret-1" | ||
| containerAuthTestCRToken1 string = "cr-token-1" |
There was a problem hiding this comment.
This is a good question. I know we've already messed with the naming plenty 🙂 but it's probably worth considering now that we've dropped the "compute resource" nomenclature from the user-facing code. This might apply to the CRTokenFilename property as well.
You do explain in the documentation that the token is a "compute resource token", so I think it's fine as is, but worth considering the simpler naming.
## [5.6.2](v5.6.1...v5.6.2) (2021-08-04) ### Bug Fixes * refactor container authenticator with recent design changes ([#129](#129)) ([58d4475](58d4475))
|
🎉 This PR is included in version 5.6.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR contains several changes to the authenticator formerly known as
ComputeResourceAuthenticator:ContainerAuthenticator