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

ci: Find all of instance-profiles in the CI account #6137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

engedaam
Copy link
Contributor

@engedaam engedaam commented May 3, 2024

Fixes #N/A

Description

  • Instance-profiles API is paginated, and requires us to go through all the pages. This is causing to track and clean-up only the first page of instance-profiles. This PR will allow us to go though all the pages.

How was this change tested?

  • N/A

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

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

@engedaam engedaam requested a review from a team as a code owner May 3, 2024 00:29
@engedaam engedaam requested a review from tzneal May 3, 2024 00:29
Copy link

netlify bot commented May 3, 2024

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 3b6bc84
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/6635b3794c737700080617dd
😎 Deploy Preview https://deploy-preview-6137--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@engedaam engedaam changed the title chore: Find all pages of instance-profiles ci: Find all of instance-profiles in the CI account May 3, 2024
@coveralls
Copy link

coveralls commented May 3, 2024

Pull Request Test Coverage Report for Build 8947840975

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 82.247%

Totals Coverage Status
Change from base Build 8945304949: 0.02%
Covered Lines: 5402
Relevant Lines: 6568

💛 - Coveralls

@engedaam engedaam force-pushed the pageniate-instance-profile branch 4 times, most recently from c28f317 to ecdba4f Compare May 3, 2024 03:11
@engedaam engedaam force-pushed the pageniate-instance-profile branch from ecdba4f to 3b6bc84 Compare May 4, 2024 04:03
})
if err != nil {
errs[i] = err
continue
}

clusterName, _ := lo.Find(profiles.Tags, func(tag iamtypes.Tag) bool {
clusterName, foundClusterName := lo.Find(profiles.Tags, func(tag iamtypes.Tag) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I can't remember. Maybe not a huge deal, but I recalled that we said we were fine with not doing the "found" check since the result of lo.FromPtr was going to be an empty string anyways, which isn't valid for a cluster name or for a region name

continue
}

for _, t := range profiles.Tags {
// Since we can only get the date of the instance profile (not the exact time the instance profile was created)
// we add a day to the time that it was created to account for the worst-case of the instance profile being created
// at 23:59:59 and being marked with a time of 00:00:00 due to only capturing the date and not the time
if lo.FromPtr(t.Key) == karpenterTestingTag && out.InstanceProfiles[i].CreateDate.Add(time.Hour*24).Before(expirationTime) {
names = append(names, lo.FromPtr(out.InstanceProfiles[i].InstanceProfileName))
if lo.FromPtr(t.Key) == karpenterTestingTag && instanceProfiles[i].CreateDate.Add(time.Hour*24).Before(expirationTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we're doing a bit of double-work here. We first already check whether the cluster name is in our set of excluded clusters which means we should have been able to validate that this is a "karpenter-owned" resource above, so we really don't need to check this again here. We could really avoid all of the iteration through this tags section and just reduce all of this to a few "found" checks.

func (ip *InstanceProfile) getAllInstanceProfiles(ctx context.Context) (instanceprofile []iamtypes.InstanceProfile, err error) {
var nextToken *string
for {
out, err := ip.iamClient.ListInstanceProfiles(ctx, &iam.ListInstanceProfilesInput{
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: Consider looking at aws-sdk-go-v2 pagination

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.

None yet

3 participants