Skip to content
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

fix(ContainerAuthenticator): add sa-token as default CR token filename #241

Merged
merged 1 commit into from
May 22, 2023

Conversation

padamstx
Copy link
Member

@padamstx padamstx commented May 3, 2023

This PR modifies the ContainerAuthenticator so that it supports a second default CR token
filename.

Checklist
  • npm test passes (tip: npm run lint-fix can correct most style issues)
  • tests are included
  • documentation is changed or added

@padamstx padamstx requested a review from dpopp07 May 3, 2023 21:56
@padamstx padamstx force-pushed the add-default-filename branch from 6f9805d to 2151eb3 Compare May 3, 2023 23:12
@padamstx padamstx marked this pull request as ready for review May 3, 2023 23:13
@padamstx padamstx force-pushed the add-default-filename branch from 2151eb3 to 7c440fc Compare May 3, 2023 23:15
@padamstx padamstx self-assigned this May 3, 2023
@padamstx padamstx changed the title demonstrate linter issue fix(ContainerAuthenticator): add sa-token as default CR token filename May 3, 2023
@padamstx padamstx requested a review from pyrooka May 3, 2023 23:28
@@ -69,7 +69,9 @@ export class ContainerTokenManager extends IamRequestBasedTokenManager {
throw new Error('At least one of `iamProfileName` or `iamProfileId` must be specified.');
}

this.crTokenFilename = options.crTokenFilename || DEFAULT_CR_TOKEN_FILEPATH;
if (options.crTokenFilename) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's really no reason to store the default filename in "this" if the user didn't specify a filename, so made this change to be consistent with Go and Java.

Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@padamstx padamstx added the do-not-merge Do not merge at this time label May 5, 2023
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! And I confirmed this works with our internal cluster 👍

This commit adds support for a second default CR token filename.
If the user doesn't specify a CR token filename, the authenticator will
first try '/var/run/secrets/tokens/vault-token', and then
'/var/run/secrets/tokens/sa-token' in that order.

Signed-off-by: Phil Adams <phil_adams@us.ibm.com>
@padamstx padamstx force-pushed the add-default-filename branch from 7c440fc to 490db60 Compare May 22, 2023 20:16
@padamstx padamstx merged commit 91f9932 into main May 22, 2023
@padamstx padamstx deleted the add-default-filename branch May 22, 2023 20:33
ibm-devx-sdk pushed a commit that referenced this pull request May 22, 2023
## [4.0.7](v4.0.6...v4.0.7) (2023-05-22)

### Bug Fixes

* **ContainerAuthenticator:** add sa-token as default CR token filename ([#241](#241)) ([91f9932](91f9932))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 4.0.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Do not merge at this time released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants