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

Set AWS credential expiry window to 30 minutes #7116

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

otterley
Copy link
Contributor

@otterley otterley commented Sep 26, 2023

Description

Set the AWS credential expiry window to 30 minutes. This will force the AWS client to renew any endpoint-provided credentials (e.g. EC2 instance credentials, ECS-provided credentials) if they would expire in less than 30 minutes, and ensure the passed credentials don't expire during long CloudFormation stack operations.

This PR also includes an update to address a broken reference to pathlib retracted version v1.0.0, replacing it with v0.15.0. (The retracted version was available via the public Go module proxy, but not available directly to those of us who aren't allowed to use the proxy.)

Closes #7095

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@TiberiuGC
Copy link
Collaborator

Hi @otterley ! Thanks for opening a PR for this improvement.

I realise none of the load options set are being unit tested atm, but I think we can add a test for this one in particular, and improve the overall coverage afterwards. I'll leave a code snipped I came up with rather quickly, so feel free to think of a better way of testing if you wish. (this should be added under pkg/eks/api_test.go)

		// check that load config options were setup properly
		_, options := fakeConfigurationLoader.LoadDefaultConfigArgsForCall(0)
		lo := &config.LoadOptions{}
		for _, loadOptionsFunc := range options {
			Expect(loadOptionsFunc(lo)).NotTo(HaveOccurred())
		}

		// check that credentials cache was setup properly
		cco := &aws.CredentialsCacheOptions{}
		lo.CredentialsCacheOptions(cco)
		Expect(cco.ExpiryWindow).To(Equal(30 * time.Minute))
		Expect(cco.ExpiryWindowJitterFrac).To(Equal(float64(0)))

@otterley
Copy link
Contributor Author

Hi @TiberiuGC - I'd be happy to do this, but the testing code is a bit difficult for me to make sense of, so I don't know where this would go. Since you already have a sense of where it should go and how it should be composed, I'd welcome your patch - you can submit a PR against this branch and I'll gladly merge it.

@TiberiuGC
Copy link
Collaborator

Sure, no worries, I'll update the tests myself.

@otterley
Copy link
Contributor Author

otterley commented Oct 4, 2023

Thanks @TiberiuGC ! Are we OK to merge?

@TiberiuGC TiberiuGC merged commit 761362c into eksctl-io:main Oct 6, 2023
14 checks passed
@TiberiuGC
Copy link
Collaborator

@otterley sorry for keeping you waiting, just merged the PR so it will get caught in today's release candidate.

IdanShohamNetApp pushed a commit to spotinst/weaveworks-eksctl that referenced this pull request Oct 19, 2023
* Set AWS credential expiry window to 30 minutes

* add unit tests

* revert changes to mocks

---------

Co-authored-by: Tibi <[email protected]>
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.

[Bug] Need to refresh token before each CloudFormation stack operation
4 participants