Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Feature/aws ecs/add ability attaching policy for task role #1935

Merged

Conversation

psihachina
Copy link
Contributor

@psihachina psihachina commented Jul 27, 2021

This commit adds the ability to pass IAM Policy ARN for attaching to new task role, for example

app "hamster" {
 
  config {
    env = {
      test= configdynamic("aws-ssm", {
        path = "test"
      })
    }
  }
  ...
  ...

    deploy {
    use "aws-ecs" {
      region = "us-east-2"
      memory = "512"
      task_role_policy_arn = var.task_role_policy_arn
    }
  }
}

image

image

Test

asciicast

@izaaklauer
Copy link
Contributor

Hi @psihachina,

Does your use-case require that policies change from deployment to deployment, or are you looking for a way to use the same long-lived policy for every deployment for a given app?

@psihachina
Copy link
Contributor Author

psihachina commented Aug 17, 2021

Hi @izaaklauer,

The most important thing in our use is the function of automatic policy creation itself so that the user can describe the policy he needs and the waypoint created it himself.
I may have understood your point. I did not foresee the possibility of changing the policy.
I will try to fix this so that the change in the description of the policy is tracked and after the role is changed or recreated.

Please correct me if I misunderstood.

@izaaklauer
Copy link
Contributor

Hey @psihachina,

I asked because right now waypoint has a great system for creating and managing operation scoped resources (i.e. resources that need to be created and destroyed for every deployment or release), but not a great system for managing project-scoped resources (i.e. resources that need to be created once for each app and can be re-used for each deployment). We'd like to avoid waypoint managing more project-scoped resources until we can build better support for them.

The fact that it's impossible to use a custom task-role policy is definitely a problem though, and thank you for bringing it to light! Our preferred solution to this for now would be to add a config field called task_role_policy_arn field to the waypoint ECS config. Users would create this policy outside of waypoint one time, but would be able to use it for every deployment and release.

Would that work for your use-case?

@psihachina
Copy link
Contributor Author

Hey @izaaklauer,

I understand your point of view. You already have such a field as task_role_name in the config. Since it is not yet possible to create custom policies in the waypoint, I have another idea. It is necessary to somehow transfer the role name to the waypoint, for example from terraform. For example, using AWS SSM or some other secret store.
It seems like terraform has so-called data with this logic.
It would be great to have a similar waypoint, since in addition to the role name, it was planned to generate and transfer subnets in a similar way.

@AutomationD
Copy link

Hey, @izaaklauer, I see your point with project-scoped resources and a solution with passing task role task_role_policy_arn would work for me as well. Maybe there should be a documentation gist on how to create one via AWS CLI. I know it's a simple step, but a oneliner maybe super helpful for most users.

Hey, @psihachina. Thanks for the PR! Regarding the data thing, it seems like a potential solution for external data communication, but maybe we should create another issue for that to discuss it (and link it here).

@psihachina
Copy link
Contributor Author

Hi @izaaklauer ,

I added a field task_role_policy_arn and updated the PR description

Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

Thanks for changing your approach! While this does introduce some new complexity around the task role (i.e. we're now sometimes looking for a task role by a given name, and sometimes creating one with the given name), I think users being able to add capabilities to their apps without interacting too deeply with IAM is a nice addition to waypoint.

builtin/aws/ecs/platform.go Outdated Show resolved Hide resolved
builtin/aws/ecs/platform.go Outdated Show resolved Hide resolved
builtin/aws/ecs/platform.go Outdated Show resolved Hide resolved
builtin/aws/ecs/platform.go Outdated Show resolved Hide resolved
builtin/aws/ecs/platform.go Outdated Show resolved Hide resolved
builtin/aws/ecs/platform.go Show resolved Hide resolved
builtin/aws/ecs/platform.go Outdated Show resolved Hide resolved
builtin/aws/ecs/platform.go Show resolved Hide resolved
@izaaklauer
Copy link
Contributor

Could you also add a changelog entry for this change? https://github.com/hashicorp/waypoint/blob/main/.github/CHANGELOG_GUIDE.md

Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

Thanks for applying the suggestions! This looks good to me.

Just a note - we're going to be refactoring much of the structure of the ECS plugin shortly (in this pr: #2098), but we'll make sure these changes are incorporated into the restructure.

@izaaklauer izaaklauer merged commit 05bb21e into hashicorp:main Aug 27, 2021
izaaklauer added a commit that referenced this pull request Sep 1, 2021
There's a bug in the overall refactor though - intermittent argmapper errors coming out of DestroyAll
izaaklauer added a commit that referenced this pull request Sep 2, 2021
There's a bug in the overall refactor though - intermittent argmapper errors coming out of DestroyAll
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants