Skip to content

Conversation

@Amplifiyer
Copy link
Contributor

@Amplifiyer Amplifiyer commented May 29, 2025

Description of changes

Some long running e2e tests can have their creds expire after an hour. Based on recent failures, add credentials refresher to those e2e tests.

Extend credentials rotator to refresh child AWS accounts as well.

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • E2E test run linked
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Amplifiyer Amplifiyer marked this pull request as ready for review June 3, 2025 15:57
@Amplifiyer Amplifiyer requested review from a team as code owners June 3, 2025 15:57
sobolk
sobolk previously approved these changes Jun 3, 2025
throw new Error('Credentials rotator supports only tests running in parent account at this time');
}
if (process.env.USE_PARENT_ACCOUNT) {
// Attempts to refresh credentials in background every 15 minutes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion.

I'd reduce interval to something smaller (5 minutes)?

The reason is - test framework spawns subprocess that inherit env vars. So their credential validity period is how_much_time_since_last_rotation + subprocess_lifespan. Some of these processes take long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't this rotator be started again for the subprocess, specifically for the test that invokes it?

Copy link
Contributor

@sobolk sobolk Jun 3, 2025

Choose a reason for hiding this comment

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

test subprocesses yes they are covered.

what I have in mind is nspawn(gen1CLI); - this may take long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, if a specific CLI command (on any spawned process from a test) takes a long time (>45 minutes) we could have a problem. I'll update it to 10 minutes (don't want to be too aggressive until we know it to be a problem)

Copy link
Contributor

Choose a reason for hiding this comment

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

In searchable tests (in cli repo) this refresh was (is?) too sparse sometimes. (we haven't adjusted this on CLI side though).

Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

Lobbed some questions your way offline just for my own edification.

@Amplifiyer Amplifiyer merged commit c7f066a into main Jun 3, 2025
7 checks passed
@Amplifiyer Amplifiyer deleted the refresh_e2e_creds branch June 3, 2025 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants