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

CUMULUS-3580 Use the correct variable for lzards launchpad secret value #3574

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Jkovarik
Copy link
Member

@Jkovarik Jkovarik commented Feb 5, 2024

Summary: Summary of changes

Addresses CUMULUS-3580

Changes

  • **CUMULUS-3580
    • Fixed secret value for lzards_launchpad_passphrase which was getting the value from var.launchpad_passphrase instead of var.lzards_launchpad_passphrase.

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Ad-hoc testing - Deploy changes and test manually
  • Integration tests

@Jkovarik Jkovarik changed the title Use the correct variable for lzards launchpad secret value CUMULUS-3580 Use the correct variable for lzards launchpad secret value Feb 5, 2024
@paulpilone
Copy link
Contributor

I resolved the CL merge conflict and waiting on the new build to pass. Otherwise this looks good to me.

@paulpilone paulpilone self-requested a review February 19, 2024 17:55
@paulpilone paulpilone removed their assignment Feb 19, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jennyhliu jennyhliu left a comment

Choose a reason for hiding this comment

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

Currently launchpad token is cached in s3 https://github.com/nasa/cumulus/blob/v18.2.0/packages/launchpad-auth/src/LaunchpadToken.ts. More changes are needed if we cache more than one token. Also, we need to have a separate token service account to verify the change.

@Jkovarik
Copy link
Member Author

@jennyhliu nods that's a good point, this code has evolved, and there's a caching assumption. I'll update the ticket with this commentary, but at a minimum we'd need to update getLaunchpadToken.

I'm not sure I agree we have to have another token service account - we can configure the same TSA with seperate cache files and that with units should cover this case sufficiently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants