feat: Parameter store and secrets manager handling

[Andrew-Chen-Wang]

Issue #, if available:

Attempting to solve #20

Description of changes:

Disclaimer: I have barely used AWS as I'm a beginner, so I've got a some questions below... Anyways.

Added GitHub input for turning on handling parameter store and secretsmanager valueFrom. You can have a base set of secrets in the task definition (or only those) and an external JSON file for injecting secrets values without showing the AWS account ID (by using the output from configure AWS credential action).

But I'm wondering about the current implementation of "substituting the task definition with parameters/secrets" (this is also my first time with node JS... so I haven't really gotten the hang of writing the tests with the mock JSON as my initial shot at it with jest failed spectacularly).

1. Is the arn format even ok? I omitted the account ID and the parameter path expected must start with a "/". I'm pretty sure it's fine to not begin with a slash in parameter store, so if the parameter does not begin with a slash, should I instead insert a ":"? Nevermind. Added the / at the beginning.
2. Is it even bad if the arn is exposed in a public repository in the task definition? I'm just most concerned about an exposed account ID, if that's even dangerous to expose... I don't know, beginner, confused AWS person. Do you need the account ID? Some parts of the AWS docs have it, other parts don't (maybe I'm confusing regular parameters with public parameters for AMIs?). README now includes how to include the account ID.
3. Not sure where the executionRoleArn is supposed to be. Also not sure if I should update the JSON minimum role in the README.rst... mostly because I have NO clue what is actually needed... I'm struggling successful with ECS deployment myself here...

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[Andrew-Chen-Wang]

@michaelhelmick Is this PR what you're looking for? It allows for base secrets from parameter store and secretsmanager in the task definition and a secondary, external secrets JSON file in which both can be combined.

Also, do you mind answering this question: where is the executionRoleArn supposed to be? I'm just a beginner in knowing about AWS and even worse with JS since I've never used it before, but the newly created tests pass and the last part I need to know is the answer to that question. Thanks!

[Andrew-Chen-Wang]

Again, not sure which IAM roles are actually needed for this feature. Still having trouble myself as I keep using role/ARandomRole (ARandomRole is the role name I gave, not the actual role...). Basically... So the IAM role is an actual IAM role. Make sure the Trust Relationship for the specified account ID has the service "ecs-tasks.amazonaws.com" (noted below in TODO).

DOC TODO (not for this repo)

• Update the Required permissions in https://github.com/aws-actions/amazon-ecs-render-task-definition and https://github.com/aws-actions/amazon-ecs-deploy-task-definition
• Also make sure that people follow this: https://stackoverflow.com/questions/48997463/ecs-unable-to-assume-role so that the IAM user linked to the account ID has the trust relationship with the service "ecs-tasks.amazonaws.com" in order to use the secrets feature.

[Andrew-Chen-Wang]

@clareliguori Any thoughts? My JS isn't very good... so sorry for some weird programming.

[allisaurus]

Hi @Andrew-Chen-Wang , thanks for taking the time to submit this! My understanding is we're trying to accomplish 2 things here:

1. obscure the AWS account ID
2. merge environment-specific values into a rendered task definition

For the second one (task def merging), I'd prefer if we built a generic merge functionality, vs. one specific to secrets, so that this action can accommodate more use cases (such as overriding CPU, mentioned in the original issue).

For the first one, account IDs are not strictly sensitive, but it is a common practice to obscure them. If you'd prefer to keep it out of your public repository but want to add SSM parameters to your task definition, we have a couple options re: using this action:

• if the parameter exists in the same region where you want to deploy your service, you can refer to it by name instead of the full ARN
• if you still need to refer to it by the full ARN, you can interpolate a Github secret or the output of the configure-credentials action into the task-defintion.json file prior to calling this action:

[...configure aws credentials...]

 - name: Render AWS Account ID
        uses: nowactions/envsubst@v1   // example envsubst action
        with:
          input: task-definition.json  // contains '${AWS_ACCOUNT_ID}' in ARNs
          output: ./updated/task-definition.json
        env:
          AWS_ACCOUNT_ID: ${{ steps.config-creds.outputs.aws-account-id }}

[...amazon-ecs-render-task-definition action...]

Coupled with generic "merge" capability, your workflow might then look like this:

  - name: Configure AWS credentials
      id: config-creds
      uses: aws-actions/configure-aws-credentials@v1
      with:
        aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
        aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
        aws-region: us-east-1

    - name: Login to Amazon ECR
      id: login-ecr
      uses: aws-actions/amazon-ecr-login@v1

    - name: Build, tag, and push image to Amazon ECR
      id: build-image
      {...build & output image params...}

     - name: Render AWS Account ID
        uses: nowactions/envsubst@v1   // example envsubst action
        with:
          input: prod-vars.json  // contains prod-specific values, uses '${AWS_ACCOUNT_ID}' in ARNs
          output: ./updated/prod-vars.json
        env:
          AWS_ACCOUNT_ID: ${{ steps.config-creds.outputs.aws-account-id }}

    - name: Render Amazon ECS task definition
      id: task-def
      uses: aws-actions/amazon-ecs-render-task-definition@v1
      with:
        task-definition: task-definition.json
        container-name: sample-app
        image: ${{ steps.build-image.outputs.image }}
        merge: ./updated/prod-vars.json  // merge 'fragment' which now contains account ID

    - name: Deploy Amazon ECS task definition
      uses: aws-actions/amazon-ecs-deploy-task-definition@v1
      with:
        task-definition: ${{ steps.task-def.outputs.task-definition }}
        service: ecs-demo
        cluster: ecs-demo
        wait-for-service-stability: true

Would one of these options work for your case? Let me know if you have any questions re: the above or if you would like to take a stab at a more generic "merge" feature.

[allisaurus]

FYI the execution role may not always have this name, it's valid to use a custom one.

[Andrew-Chen-Wang]

Huh the more you know... I think the generic merge might be necessary.

[Andrew-Chen-Wang]

Thanks for taking a look at this @allisaurus!

I was mostly scared about leaking an account ID. I've been reading around that it was a faux pas to let it into the wild. I'm definitely getting busier with college coming around, and my JS is still very... the skills are lacking.

Regarding the full Arn, I looked up how to do the shorter one, but nothing really came up. If I'm not mistaken, the Name attribute vs. the valueFrom attribute have two separate purposes: Name belonging to the environment variable name on the system and valueFrom being the ARN in SSM or Secrets manager. Do you mind sharing an example of the newer version? Thanks!

Lastly, for the generic merger functionality, I was thinking we could use the "with" part of GitHub actions. I don't really like piling up my GitHub secrets since those should truly be secrets whereas something as public as CPU core count doesn't really need to go in there. But, for sure, a generic merger functionality seems more favorable.

I think the way Claire did it was pretty clean, though, and that might be the correct way to go.
Having two different JSON files for staging and production is smart, although I'm still slightly confused about the merging sequence. Her method would work for something as public as CPU core count but what about more secretive things? Do people even typically put secretive info in their task definition???

Then again, I haven't touched JS or this PR in awhile and school's coming up, so I'm not entirely sure if I can even get around to this.

[michaelhelmick]

Having two different JSON files for staging and production is smart, although I'm still slightly confused about the merging sequence. Her method would work for something as public as CPU core count but what about more secretive things? Do people even typically put secretive info in their task definition???

FWIW, I throw mine in AWS secrets manager and just paste the secret ARN as a valueFrom

[allisaurus]

@Andrew-Chen-Wang you should be able to reference an SSM parameter by name in the valueFrom field of the secret if that parameter exists in the same region as the task you are launching. i.e., this...

"secrets": [{
      "name": "environment_variable_name",
      "valueFrom": "arn:aws:ssm:region:aws_account_id:parameter/my/parameter_name"  <---- full ARN
    }]

should also work as:

"secrets": [{
      "name": "environment_variable_name",
      "valueFrom": "my/parameter_name"   <----- name of in-region secret
    }]

You may have to play with it (for example, I'm not sure if the preceding slash is needed), but that would eliminate the need to reveal the account number.

re: this:

Do people even typically put secretive info in their task definition???

@michaelhelmick 's approach is what we normally see and what I would recommend; keep sensitive values out of the task def and instead reference their parameters (by name or ARN).

[allisaurus]

Closing due to lack of activity. @Andrew-Chen-Wang feel free to reopen or resubmit if you'd like to make updates, or submit an issue if you have other questions about using this action.

[Andrew-Chen-Wang]

Hi @allisaurus getting slightly busy lately. I will re-open this when I have time. I also tried out the short format for all these ARNs. I added the slash at the beginning of valueFrom, but I don't know if it's necessary. executionRoleArn can also just be the ECS instance role's name. Thanks again!