Skip to content

Conversation

@jmdeal
Copy link
Contributor

@jmdeal jmdeal commented Aug 26, 2025

Fixes #N/A

Description
Drops the iam:GetRole dependency in favor of caching the result from iam:AddRoleToInstanceProfile. This isn't quite as airtight as the GetRole check since we'll still create an InstanceProfile each time the cache entry expires, so I've added a fail open attempt to delete the profile after a creation failure. If this does fail, we have the GC as a backstop.

How was this change tested?
make presubmit & /karpenter snapshot

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.

@jmdeal jmdeal requested a review from a team as a code owner August 26, 2025 19:01
@jmdeal jmdeal requested a review from DerekFrank August 26, 2025 19:01
@netlify
Copy link

netlify bot commented Aug 26, 2025

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 76fbb3d
🔍 Latest deploy log https://app.netlify.com/projects/karpenter-docs-prod/deploys/68b1d6777511050009acc415

@jmdeal jmdeal changed the title Chore/remove get role chore: remove iam:GetRole dependency Aug 26, 2025
@jmdeal jmdeal force-pushed the chore/remove-get-role branch 2 times, most recently from 56652c6 to d27bef9 Compare August 26, 2025 19:15
@jmdeal jmdeal force-pushed the chore/remove-get-role branch from d27bef9 to f528a81 Compare August 26, 2025 19:18
@github-actions
Copy link
Contributor

github-actions bot commented Aug 26, 2025

Preview deployment ready!

Preview URL: https://pr-8419.d18coufmbnnaag.amplifyapp.com

Built from commit 76fbb3dcc05733cb309985749bc53780945be639

@coveralls
Copy link

coveralls commented Aug 26, 2025

Pull Request Test Coverage Report for Build 17329251341

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 56 of 91 (61.54%) changed or added relevant lines in 5 files are covered.
  • 140 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.2%) to 67.188%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/fake/iamapi.go 2 9 22.22%
pkg/operator/operator.go 0 7 0.0%
pkg/providers/instanceprofile/instanceprofile.go 46 67 68.66%
Files with Coverage Reduction New Missed Lines %
pkg/fake/utils.go 2 95.09%
pkg/providers/instanceprofile/instanceprofile.go 2 78.49%
pkg/fake/ec2api.go 7 85.42%
pkg/fake/iamapi.go 9 57.69%
pkg/controllers/nodeclass/controller.go 11 68.55%
pkg/errors/errors.go 26 51.97%
pkg/controllers/nodeclass/validation.go 34 87.58%
test/pkg/environment/common/expectations.go 49 0.0%
Totals Coverage Status
Change from base Build 17246105559: -0.2%
Covered Lines: 7611
Relevant Lines: 11328

💛 - Coveralls

Copy link
Contributor Author

@jmdeal jmdeal left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@github-actions
Copy link
Contributor

Snapshot successfully published to oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:0-f528a81f83cc19bfcc11e331a3f85fd23c2b6a0d.
To install you must login to the ECR repo with an AWS account:

aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 021119463062.dkr.ecr.us-east-1.amazonaws.com

helm upgrade --install karpenter oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter --version "0-f528a81f83cc19bfcc11e331a3f85fd23c2b6a0d" --namespace "kube-system" --create-namespace \
  --set "settings.clusterName=${CLUSTER_NAME}" \
  --set "settings.interruptionQueue=${CLUSTER_NAME}" \
  --set controller.resources.requests.cpu=1 \
  --set controller.resources.requests.memory=1Gi \
  --set controller.resources.limits.cpu=1 \
  --set controller.resources.limits.memory=1Gi \
  --wait

DerekFrank
DerekFrank previously approved these changes Aug 28, 2025
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Nice work 🎉

jonathan-innis
jonathan-innis previously approved these changes Aug 29, 2025
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jonathan-innis jonathan-innis enabled auto-merge (squash) August 29, 2025 16:36
@jonathan-innis jonathan-innis merged commit 9cf7e6c into aws:main Aug 29, 2025
19 checks passed
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.

4 participants