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

update iam policy #16

Closed
wants to merge 2 commits into from
Closed

update iam policy #16

wants to merge 2 commits into from

Conversation

nabuskey
Copy link

Description of your changes

Updates the IAM policy used by Karpenter. I was trying to use this for karpenter v0.33.1, but it does not have enough permissions.

Policies are taken from here: https://github.com/aws/karpenter-provider-aws/blob/main/website/content/en/preview/getting-started/getting-started-with-karpenter/cloudformation.yaml

#15 should probably be merged first or do it all in that PR.

Fixes #

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Signed-off-by: Manabu Mccloskey <[email protected]>
@nabuskey nabuskey marked this pull request as ready for review January 18, 2024 23:15
Signed-off-by: Manabu Mccloskey <[email protected]>
@ytsarev
Copy link
Member

ytsarev commented Jan 22, 2024

Hey @nabuskey , thanks a lot for your contribution! Can you please rebase your PR after we merged #15 ?

Please take #15 (comment) into account on reasoning why we didn't immediately include instanceProfile related policy configuration.

@nabuskey
Copy link
Author

It is already rebased. I included instanceProfile stuff for two reasons:

  1. I was getting errors when trying to use Karpenter without these permissions.
  2. I think it's a good idea for this composition to be in sync with the official reference IAM permissions defined in the Karpenter repo for a particular Karpenter version.

Let me know what you think!

@ytsarev
Copy link
Member

ytsarev commented Jan 27, 2024

@nabuskey my only concern here is that we will add instanceProfile permissions without actually utilizing them... We are controlling associated instance profile by Crossplane Configuration itself so there is no need to give this permission to Karpenter until we switch to this model.

I would suggest removing instanceProfile related permissions from this PR and then work on a separate one where we might add them and simplify the Composition.

@haarchri
Copy link
Member

thanks for fixing this - we added this change in #34 - and we bump karpenter to v1

@haarchri haarchri closed this Aug 16, 2024
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