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

fix(cli): Fix incorrect parsing of ARNs w/ nested param names #1012

Merged
merged 4 commits into from
May 21, 2020

Conversation

bvtujo
Copy link
Contributor

@bvtujo bvtujo commented Apr 15, 2020

Addresses #1010 and #1011.

This change solves an error where we did not extract
fully qualified paths (/path/to/param) from SSM parameter ARNS
containing nested hierarchical param names. We would instead grab
"path/to/param" (excluding the leading "/") and attempt to call GetParameter via the AWS SDK,
which would result in a failure and an incorrect error message.

More information on SSM parameter name constraints [here].(https://docs.aws.amazon.com/systems-manager/latest/userguide/sysman-parameter-name-constraints.html)


Enter [N/A] in the box, if an item is not applicable to your change.

Testing

  • Unit tests passed
  • [ in progress] Integration tests passed
  • Unit tests added for new functionality

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Addresses aws#1010. This change solves an error where we did not extract
fully qualified paths (/path/to/param) from SSM parameter ARNS
containing nested hierarchical param names. We would instead grab
"path/to/param" and attempt to call GetParameter via the AWS SDK,
which would result in a failure and an incorrect error message.
@bvtujo bvtujo requested review from a team and kohidave and removed request for a team April 15, 2020 21:13
@bvtujo
Copy link
Contributor Author

bvtujo commented Apr 15, 2020

See further discussion on #1010.

@bvtujo bvtujo requested a review from a team April 15, 2020 21:40
@bvtujo bvtujo self-assigned this Apr 21, 2020
@bvtujo bvtujo added the bug label Apr 21, 2020
@Aubtin
Copy link

Aubtin commented May 12, 2020

Any timeframe on this?

Thanks.

@bvtujo
Copy link
Contributor Author

bvtujo commented May 21, 2020

Tested with the following:

ecs-params.yml

version: 1
task_definition:
  services:
    web:
      secrets:
        - value_from: arn:aws:ssm:us-west-2:<acct>:parameter/database/param
          name: MYSECRET
  task_execution_role: ecsTaskExecutionRole
  ecs_network_mode: awsvpc
  task_size:
    mem_limit: 1.0GB
    cpu_limit: 512
run_params:
  network_configuration:
    awsvpc_configuration:
      subnets:
        - "subnet-12344"
        - "subnet-456677"
      security_groups:
        - "sg-1234"
      assign_public_ip: "ENABLED"

docker-compose.yml

version: '3'
services:
  web:
    image: acct.dkr.ecr.us-west-2.amazonaws.com/otter:latest
    ports:
      - "80:80"
    logging:
      driver: awslogs
      options:
        awslogs-group: otter
        awslogs-region: us-west-2
        awslogs-stream-prefix: web

On deployment, logging the secret value via os.Getenv

2020-05-20 17:24:21 secret: extragoodgoose

@allisaurus allisaurus mentioned this pull request May 21, 2020
7 tasks
@bvtujo
Copy link
Contributor Author

bvtujo commented May 21, 2020

Tested locally:

FATA[0016] Failed to decrypt secret due to
failed to retrieve decrypted secret from arn:aws:ssm:us-west-2:898620229039:parameter/database/param due to ValidationException: Parameter name: can't be prefixed with "ssm" (case-insensitive). If formed as a path, it can consist of sub-paths divided by slash symbol; each sub-path can be formed as a mix of letters, numbers and the following 3 symbols .-_
	status code: 400, request id: a1c15c0c-7f9e-4496-b0a8-8ded4394a8ac: ValidationException: Parameter name: can't be prefixed with "ssm" (case-insensitive). If formed as a path, it can consist of sub-paths divided by slash symbol; each sub-path can be formed as a mix of letters, numbers and the following 3 symbols .-_
	status code: 400, request id: a1c15c0c-7f9e-4496-b0a8-8ded4394a8ac

now after this fix:

➜  oaas git:(ssm) ✗ ./ecs-cli local up --task-def-remote oaas:14
INFO[0000] Reading task definition from oaas:14

INFO[0000] Task Definition network mode is ignored when running containers locally. Tasks will be run in the ecs-local-network.  networkMode=awsvpc
WARN[0000] awslogs log driver is ignored when running locally. Tasks will default to json-file instead. This can be changed in your compose override file.
docker-compose.ecs-local.yml file already exists. Do you want to write over this file? [y/N]
y
INFO[0006] Successfully wrote docker-compose.ecs-local.yml
INFO[0006] docker-compose.ecs-local.override.yml already exists, skipping write.
INFO[0006] The network ecs-local-network already exists
INFO[0007] The amazon-ecs-local-container-endpoints container already exists with ID 23d0ab821cf29db8467f987d634262979c18ad4f73050d42e1c18262b0f017cf
INFO[0007] Started container with ID 23d0ab821cf29db8467f987d634262979c18ad4f73050d42e1c18262b0f017cf
INFO[0007] Using docker-compose.ecs-local.yml, docker-compose.ecs-local.override.yml files to start containers
Creating oaas_web_1 ... done

Copy link
Contributor

@SoManyHs SoManyHs left a comment

Choose a reason for hiding this comment

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

yay!!! looks great, thank you! 🎉 #extragoodgoose

@SoManyHs SoManyHs merged commit 99696d4 into aws:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants