Skip to content

Conversation

@danmactough
Copy link
Contributor

@danmactough danmactough commented Oct 4, 2020

Adds the ability to use YAML formatted Cloudformation templates, even if they use short form syntax for intrinsic functions.

Parsing plain old YAML is fairly straightforward, but out of the box, usage of short form syntax for things like !Ref and !Sub breaks the YAML parser. Luckily, js-yaml has a facility for providing a custom schema for handling custom YAML tags (which is what that short form syntax is). That schema is provided by js-yaml-cloudformation-schema.

Note that this doesn't change the output of templates (such as when they are saved to S3), which keeps things simple -- the diffing and whatnot should not be affected by this at all. (In other words, we basically perform an on-the-fly cfn-flip and from that point forward, the data is the data, and it doesn't matter if it was ingested from YAML or JSON or JS.)

That being said, I'm not sure if there's anything else that would require testing, so please let me know if there is.

cc @rclark

@danmactough
Copy link
Contributor Author

For an end-to-end test, I used the following simple YAML template:

AWSTemplateFormatVersion: '2010-09-09'
Description: Example to demonstrate NoEcho parameter bug
Parameters:
  DeploymentType:
    Type: String
    Default: staging
    Description: Is this a staging, production, or dev or whatever deployment?
  NotSecret:
    Type: String
    Description: This value is not secret
  Secret:
    Type: String
    NoEcho: true
    Description: '[secure] This value is secret'
Resources:
  SecretsManagerSecret:
    Type: AWS::SecretsManager::Secret
    Properties:
      Name: !Sub '${DeploymentType}/${AWS::StackName}/config'
      Description: Example to demonstrate NoEcho parameter bug
      SecretString: !Sub '{ "NotSecret": "${NotSecret}", "Secret": "${Secret}" }'

I successfully create a stack with this template, and updated it. During update, I changed a parameter and got the expected JSON formatted diff to accept parameter changes:

image

@danmactough
Copy link
Contributor Author

I'm not sure if there's anything else that would require testing, so please let me know if there is.

Found one more thing in 68c60eb -- when performing a stack update on a stack having a template stored in YAML (meaning the preceding stack creation or update was performed with a YAML template), we need to load that YAML, as well.

@rclark
Copy link
Contributor

rclark commented Oct 6, 2020

The !Ref !Sub stuff is tough to grapple with. I am wary of modules that are built to navigate those substitutions that aren't directly built/maintained by AWS. And in fact I'm wary of some such modules that ARE by AWS (check out the dependency tree that covers YAML parsing in https://github.com/awslabs/goformation -- forks of forks of forks).

CloudFormation schema is a poorly-defined, moving target, and I'm nervous that the schema you're using is a fork of a module that's used to support an Atom linter that isn't maintained by AWS. Could you take a look at https://www.npmjs.com/package/@aws-cdk/yaml-cfn? Can this be used in the cfn-config context?

@danmactough
Copy link
Contributor Author

in fact I'm wary of some such modules that ARE by AWS

🤣

Switching to https://www.npmjs.com/package/@aws-cdk/yaml-cfn was no sweat. See b539156 Just needed to make the invalid yaml even more invalid for the tests to pass.

@ian29
Copy link
Contributor

ian29 commented Oct 21, 2020

Thanks for this @danmactough! Swooping in to say I don't think we should merge this until we've figured out our long-term plan for cfn-config. Currently yaml templates are not supported by internal mapbox tooling, and while there's some chatter about doing so, we haven't planned out what that would look like.

We should keep this work in a branch until @mapbox/release-engineering decides what the future of cfn-config holds.

@danmactough
Copy link
Contributor Author

I don't think we should merge this until we've figured out our long-term plan for cfn-config.

This makes me sad.

@purew purew requested review from a team, JCasera and oielbanna and removed request for a team September 29, 2022 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants