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

Add extra rules for role to support ECS #113

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Feb 15, 2024

Motivation

When running ECS tasks, the service role needs more permissions than what is specified in the template. Specifically, in addition to the existing rules we need:

- apiGroups: [""]
  resources: ["pods/log"]
  verbs: ["get"]
- apiGroups: [""]
  resources: ["services"]
  verbs: ["get", "list"]

Changes

  • Add extra role rules to support running ECS tasks

@simonrw simonrw added the type: enhancement Improvement to usability or performance label Feb 15, 2024
@simonrw simonrw self-assigned this Feb 15, 2024
@simonrw simonrw requested a review from alexrashed February 15, 2024 15:23
@simonrw simonrw marked this pull request as ready for review February 15, 2024 15:25
@simonrw simonrw requested a review from lakkeger as a code owner February 15, 2024 15:25
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

The change is looking good, but it feels like this should be the default in order to have a working configuration of LocalStack. With the currently suggested change, I would have to explicitly configure these roles to fix ECS tasks in LocalStack, right?
Maybe we should just add them? Or set them as default with the possibility of removing them?

@lakkeger
Copy link
Contributor

@alexrashed I agree, however at the same time it's harder to remove something than adding rules. But perhaps time boxed it might worth to look into it @simonrw. I can live with either one.

@simonrw
Copy link
Contributor Author

simonrw commented Feb 16, 2024

Perhaps something like:

(values.toml)

role:
  defaultRules: true
  extraRules: []

and we default to these rules in the PR. Then the user can opt out of whatever we add with defaultRules: true and add their own with extraRules?

(inspired by Rust's handling of "features": https://doc.rust-lang.org/cargo/reference/features.html#the-default-feature and the "default" feature)

@alexrashed
Copy link
Member

I would actually just add it to the roles without having it configurable. If it gets requested to be configurable (i.e. the possibility to remove roles while risking to break LocalStack), then we can extract the whole content of the roles (including the already existing ones) to the default value of a newly introduced variable.

@simonrw simonrw changed the title Support extra rules for the role Add extra rules for role to support ECS Feb 16, 2024
@simonrw simonrw requested a review from alexrashed February 16, 2024 09:22
@simonrw simonrw merged commit 7d7004f into main Feb 16, 2024
2 checks passed
@simonrw simonrw deleted the feat/extra-role-rules branch February 16, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement to usability or performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants