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

csi: add required policies for aws csi driver #1945

Merged
merged 7 commits into from
Jun 22, 2023
Merged

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Jun 20, 2023

Context

  • To use the AWS CSI driver, the corresponding storage permissions are necessary.

Proposed change(s)

  • Add the required storage permissions.

Additional info

Checklist

  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@msanft msanft added the no changelog Change won't be listed in release changelog label Jun 20, 2023
@msanft msanft added this to the v2.9.0 milestone Jun 20, 2023
@msanft msanft requested a review from katexochen as a code owner June 20, 2023 09:00
@msanft msanft requested review from daniel-weisse and removed request for katexochen June 20, 2023 09:00
@netlify
Copy link

netlify bot commented Jun 20, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit b70e304
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/6491774e5124d100085c3b0a

@msanft msanft requested a review from 3u13r June 20, 2023 09:11
@msanft msanft removed the no changelog Change won't be listed in release changelog label Jun 20, 2023
resource "aws_iam_role_policy_attachment" "csi_driver_policy_worker" {
role = aws_iam_role.worker_node_role.name
policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonEBSCSIDriverPolicy"
}

// TODO(msanft): incorporate this into the custom worker node policy
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO(msanft): incorporate this into the custom worker node policy
// TODO(msanft): incorporate this into the custom control-plane node policy

Copy link
Member

Choose a reason for hiding this comment

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

By "incorporate into custom policy", do you mean to extract the permissions from the managed AWS-CSI role and add the permissions to our role?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. I think we should add this to the minimal permission set we define for all CSPs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one that we document, that's used in the CI, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it is something worth doing because the managed role provides excess permissions, or is it just to combine permissions into one?
If its the later I would argue against it and keep the current way of attaching the role, as this guarantees the role to always have the correct permissions for CSI

Co-authored-by: Daniel Weiße <66256922+daniel-weisse@users.noreply.github.com>
@malt3 malt3 self-requested a review June 22, 2023 09:28
@msanft msanft merged commit 224c74f into main Jun 22, 2023
2 of 3 checks passed
@msanft msanft deleted the feat/csi/aws-csi-driver branch June 22, 2023 12:15
@derpsteb derpsteb changed the title csi: aws csi driver policies csi: add required policies for aws csi driver Jul 11, 2023
@derpsteb
Copy link
Member

Slight title change for changelog readability.

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

5 participants