"sam deploy" does not execute if a tag has a long value

[AaronChelvan]

Description:
The sam deploy command hangs if a tag with a long value (approx 30+ characters long) is provided. Tested on SAM CLI v0.43.0.

Steps to reproduce the issue:

1. Create an empty directory and navigate to it.
2. Execute sam deploy --stack-name test --tags key=012345678901234567890123456789

Observed result:
The command hangs and does not produce any output.

Expected result:

	Deploying with following values
	===============================
	Stack name                 : test
	Region                     : None
	Confirm changeset          : False
	Deployment s3 bucket       : None
	Capabilities               : null
	Parameter overrides        : {}

Initiating deployment
=====================
Error: Failed to package template: /tmpDir/template.yml. 
 [Errno 2] No such file or directory: '/tmpDir/template.yml'

However, if a shorter tag value is provided (e.g. sam deploy --stack-name test --tags key=01234567890123456789) then the correct output gets printed.

[beck3905]

This appears to be caused by this regex that was added in version 0.42.0: https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/cli/types.py#L207.

I isolated this code and tried it with a short tag and a long tag and it is the re.findall call that is hanging. Not exactly sure what the regex pattern is meant to do as it is a rather long and complex pattern. A little more commenting for that would be helpful.

[dslove]

In my case there is no any error msg showing up. It took me a day to figure out what the problem is and it was because I was lucky to google and find this page... I am using 0.44.

[iDVB]

SAM CLI, version 0.47.0
aws-cli/1.16.208 Python/3.7.7 Darwin/18.6.0 botocore/1.12.198
macOS 10.14.5

I too am hitting this issue as well.
I can provide a bit more testing cases.

Using the AWS CLI directly does work fine

aws cloudformation deploy \
  --template .aws-sam/build/packaged.yaml \
  --stack-name 'my-stack-name' \
  --capabilities CAPABILITY_NAMED_IAM \
  --tags \
      SERVICE=ggg-ggggggggg-ggggggg \
      GIT_BRANCH=master \
      GIT_REPOSITORY=gggggg-ggggggggg-ggg-gggggggggggggg

Using the same call with AWS SAM either does not work... or takes way too long

sam deploy \
 --template-file .aws-sam/build/packaged.yaml \
  --stack-name 'my-stack-name' \
  --capabilities CAPABILITY_NAMED_IAM \
--no-fail-on-empty-changeset \
  --tags \
      SERVICE=ggg-ggggggggg-ggggggg \
      GIT_BRANCH=master \
      GIT_REPOSITORY=gggggg-ggggggggg-ggg-gggggggggggggg

What I've noticed is that there seems to even be an exponential execute time increase when I increase that GIT_REPOSITORY value from 24 (4s) to 28 (49s) characters. So I didn't even want to stick around to test 35 characters.

Unfortunately, I'm in the middle of authoring a course, showcasing AWS SAM, that needs this working. 😞

[beck3905]

@sriram-mv @jfuss Has anyone looked into this issue yet? I noticed that v0.48 of the cli was just released and this has been an issue since v0.42.0. I have had to freeze on v0.41.0 because of this issue.

Why was this regex added to being with? It is not clear what it is supposed to do or look for, but the pattern is terribly non-performant.

[c2tarun]

Thanks @beck3905 for linking the code where the issue. On adding some extra logging, the regex pattern we are using is (\"(?:\\[A-Za-z0-9\"_:\.\/\+-\@=]|[^\"\\]+)*\"|(?:\\[A-Za-z0-9\"_:\.\/\+-\@=]|[^ \"\\]+)+)=(\"(?:\\[A-Za-z0-9\"_:\.\/\+-\@=]|[^\"\\]+)*\"|(?:\\[A-Za-z0-9\"_:\.\/\+-\@=]|[^ \"\\]+)+) this pattern is used to separate key and value of tag. Now, I haven't deep dived into regex but testing on regex101.com shows that we are running into Regex: Catastrophic Backtracking issue.

To add to that, I don't think this regex is completely bug free. For Eg: AWS EC2 Tagging restriction mention that you can use = in key or value. However, if we enter abc=pqr=xyz with our current regex, it'll split it as abc=pqr and xyz instead of abc and pqr=xyz. Both are correct BTW.

Usually we try to stay consistent with how AWS CLI handles things however different services across AWS have different way to handle tagging. Eg:

1. AWS S3 accepts a JSON array like here.
2. AWS EC2 accepts Key=key,Value=value type format here.

Plus we don't want to break backwards compatibility for our current users.

I'll sync with team and try to update here on what approach we want to take to fix this, but given that we are already broken with:

1. Long values.
2. = in value.
3. Multiple = in key=value format.

We can probably consider not supporting = in key or value. Please let me know your thoughts around this approach.

[beck3905]

@c2tarun Thanks for digging into this. I'm not sure about whether or not to support the = in key or value. My use case is broken due to the long values issue. My CI/CD system tags the stack with the URL of the GitHub commit that triggered the deployment. That URL is much longer than the regex can handle and causes our deployments to hang. I don't believe there are any = in that URL, but = are supported in URL's.

[c2tarun]

@beck3905 Yeah, on giving this more thought there might be people who are base64 encoding something as value. Even though we are parsing it wrong, but breaking on multiple = will downright break them. I'll try to reach out to CFN team internally to see if they have some guideline/stricter-restriction around tagging. Will update here after that.

[c2tarun]

Actually we don't have to worry about approaches taken by other AWS Services as long as we stay consistent with AWS CFN. CFN interestingly supports both approaches taken by S3 and EC2 :)

The solution I think will work is:

1. Do not support = in key and value if using tagName=tagValue format of adding tags.
2. We should start supporting Key=string,Value=string format for tagging where user can use = after Key= or Value=.
3. We should start supporting [{Key=string,Value=string},{Key=string,Value=string}] format and user can use = with no issue.

It is little more work then I thought, but should solve most of the problems. There is still a risk if breaking someone if they are already using multiple = but we will print descriptive error message to suggest to use Key=string,Value=string approach, which I think will help in the long run.

[c2tarun]

Correction in my investigation, in order to use = in key or value we can use double quotes. So we can use "abc"="pqr=xyz" and this works.

[micklove]

@c2tarun Thanks for digging into this. I'm not sure about whether or not to support the = in key or value. My use case is broken due to the long values issue. My CI/CD system tags the stack with the URL of the GitHub commit that triggered the deployment. That URL is much longer than the regex can handle and causes our deployments to hang. I don't believe there are any = in that URL, but = are supported in URL's.

FYI - I have the same problem, tagging the stack with the git commit url (using github actions), hangs the sam deploy command. (Same tagging on cloudformation deploy works fine.)
nb: The sam deploy command works in the OSX terminal.