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

Long strings/refs incorrectly split over newlines causing broken user-data/etc #1309

Closed
0xdevalias opened this issue Dec 9, 2018 · 10 comments · Fixed by #1334
Closed

Long strings/refs incorrectly split over newlines causing broken user-data/etc #1309

0xdevalias opened this issue Dec 9, 2018 · 10 comments · Fixed by #1334
Assignees
Labels
bug This issue is a bug.

Comments

@0xdevalias
Copy link
Contributor

When I am building strings to use in my user-data/etc, CDK seems to incorrectly insert newlines (at long lines?), causing scripts to break.

Relevant Snippets from code:

const cfnInitCmd = `/opt/aws/bin/cfn-init -v --stack=${new cdk.AwsStackName} --region=${new cdk.AwsRegion} --resource=${asgLaunchConfig.logicalId}`;

appAsg.addUserData(
            "yum install -y aws-cfn-bootstrap;",
            `${cfnInitCmd} --configsets install;`,
            `/opt/aws/bin/cfn-signal --stack=${new cdk.AwsStackName} --region=${new cdk.AwsRegion} --resource=${asgLaunchConfig.logicalId} --exit-code \$?;`,
        );

When synthesized, this ends up like:

AppAutoScalingGroupLaunchConfig48178128:
    Type: AWS::AutoScaling::LaunchConfiguration
    Properties:
..snip..
      UserData:
        Fn::Base64:
          Fn::Join:
            - ""
            - - |-
                #!/bin/bash
                yum install -y aws-cfn-bootstrap;
                /opt/aws/bin/cfn-init -v --stack=
              - Ref: AWS::StackName
              - " --region="
              - Ref: AWS::Region
              - >-2
                 --resource=AppAutoScalingGroupLaunchConfig48178128 --configsets
                install;
                /opt/aws/bin/cfn-signal --stack=
              - Ref: AWS::StackName
              - " --region="
              - Ref: AWS::Region
              - " --resource=AppAutoScalingGroupLaunchConfig48178128 --exit-code
                $?;"

Which ends up in the end user-data as:

#!/bin/bash
yum install -y aws-cfn-bootstrap;
/opt/aws/bin/cfn-init -v --stack=TokenizedEC2 --region=ap-southeast-2 --resource=AppAutoScalingGroupLaunchConfig48178128 --configsets
install; /opt/aws/bin/cfn-signal --stack=TokenizedEC2 --region=ap-southeast-2 --resource=AppAutoScalingGroupLaunchConfig48178128 --exit-code $?;

Note how the install parameter ends up on a new line, when it shouldn't, which then breaks things.

This is also happening in my AWS::CloudFormation::Init Metadata override, breaking my /etc/cfn/cfn-hup.conf config.

@eladb
Copy link
Contributor

eladb commented Dec 9, 2018

How are you deploying this? Through the toolkit or by synthesizing a YAML output and then deploying via some other tool? I am wondering if the issue is our YAML formatter?

@0xdevalias
Copy link
Contributor Author

0xdevalias commented Dec 9, 2018 via email

@eladb
Copy link
Contributor

eladb commented Dec 9, 2018

Okay, ran a few tests. It seems like the root cause is the way our YAML formatter renders the multiline value. I reproduced the issue with YAML output and successfully deployed with JSON. Also, when I shortened the lines (by changing the logical ID of the launch configuration), the YAML template also worked (which I suspect is what happens in your FnJoin case).

@RomainMuller what do you think? This YAML output is giving us too much headache. Feels like a cat and mouse. Should we just support JSON in the toolkit and let people generate their own YAML at their own risk? Is there a configuration where our YAML output is "stupid" and doesn't do crazy things like splitting multiline outputs? (couldn't find it).

@eladb eladb added the bug This issue is a bug. label Dec 9, 2018
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 10, 2018

The maintainer of this YAML was very responsive last time around and had a patch out the next day. We can also take this to them.

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 10, 2018

But in the mean time, yeah, no reason not to use JSON internally I suppose. It's not for human consumption anyway.

@eladb
Copy link
Contributor

eladb commented Dec 10, 2018

If we allow users to synthesize YAML, then it should work. It would be an awful experience if “cdk deploy” worked and “cdk synth” didn’t. Happy to continue to try make YAML work, but I am concerned...

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 10, 2018

As far as I can tell the issue is because of the

              - >-2
                 --resource=AppAutoScalingGroupLaunchConfig48178128 --configsets
                install;
                /opt/aws/bin/cfn-signal --stack=

quoting, as opposed to

              - " --resource=AppAutoScalingGroupLaunchConfig48178128 --exit-code
                $?;"

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 10, 2018

Reported here: eemeli/yaml#55

@eemeli
Copy link

eemeli commented Dec 11, 2018

This should be fixed by the just-released version 1.1.0 of yaml. It's also now clearer how to disable line folding altogether.

@eladb
Copy link
Contributor

eladb commented Dec 12, 2018

@eemeli thanks for the amazingly quick turnaround!

rix0rrr added a commit that referenced this issue Dec 12, 2018
Fix the issue where incorrect UserData was generated by updating to a newer version of the yaml library.

Fixes #1309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants