aws-amplify / amplify-cli

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development.
Apache License 2.0
2.81k stars 821 forks source link

Run a proper linter on the entire template #5071

Open berenddeboer opened 4 years ago

berenddeboer commented 4 years ago

I'm wasting a lot of time debugging errors in CloudFormation templates. They take minutes to process when I debug with the push -y method.

What amplify push -y should do is run an extremely accurate linter. The linter should be run over every template, or simply process the root + nested as a single file.

Integrating cfn-lint would be a good start.

SwaySway commented 4 years ago

We do run cfn-lint when pushing resources. Though as of now cfn-lint is deprecated so the full support isn't there, we are looking at other options.

Are you getting these errors from the generated templates or from editing templates for a custom use case?

berenddeboer commented 4 years ago

From editing (generated) templates.

I suppose you put a script in package.json or so? Or is there a way to run a script as part of amplify push?

berenddeboer commented 4 years ago

FYI, I'm actually using the AWS cfn-lint.

edwardfoyle commented 4 years ago

@berenddeboer If you want to run some custom logic before push, you can create a plugin that runs on the "PrePush" event. Run amplify plugin init, select util for the plugin type and select PrePush as the event to handle. This will create a boilerplate plugin for you that you can amend to your needs. You can read the docs on custom plugins here: https://docs.amplify.aws/cli/plugins/authoring

berenddeboer commented 4 years ago

I've been asked to provide some specific examples, so some comments if what I went through this morning. I wanted to add a custom alias (our own domain), and ACM HTTPS certificate. I had generated them via the GUI, to get a working version, then export it back on the template.

This is what aws cloudfront get-distribution exports:

"ViewerCertificate": {
    "ACMCertificateArn": "arn:aws:acm:us-east-1:376590287418:certificate/1ba37566-3df0-407f-a823-53d0641993e3",
    "SSLSupportMethod": "sni-only",
    "MinimumProtocolVersion": "TLSv1.2_2019",
    "Certificate": "arn:aws:acm:us-east-1:376590287418:certificate/1ba37566-3df0-407f-a823-53d0641993e3",
    "CertificateSource": "acm"
},

I started with copying this back into the template. Errors:

  1. ""ACMCertificateArn": has to be spelt as "AcmCertificateArn". Why on earth can't this be the same? Detected by cfn-lint.
  2. "MinimumProtocolVersion": cfn-lint complains about this, only goes up to "TLSv1.2_2018".
  3. I added "CloudFrontDefaultCertificate": false, forgot to run cfn-lint, so wasted minutes waiting for CloudFront to report you can't do this. cfn-lint correctly identifies this as an error.
berenddeboer commented 4 years ago

The other thing I tried to do was make this option:

"DistributionConfig":
  "Aliases": ["www.example.com"],

I first had this:

"Conditions": {
  "ShouldNotCreateAlias": {
      "Fn::Equals": [
          {
              "Ref": "DomainName"
          },
          ""
      ]
  }
}
...
"DistributionConfig":
  "Aliases": [{
      "Fn::If": [
      "ShouldNotCreateAlias",
          {"Ref": "DomainName"},
          {"Ref": "DomainName"}
      ]
  }],

cfn-lint likes this, but it does not work when DomainName is empty obviously (empty alias error). Note that at this point I didn't understand what "Fn::If" was doing, just doing copy/paste programming.

I didn't understand the error, so tried:

"DistributionConfig":
  "Aliases": [{
      "Fn::If": [
      "ShouldNotCreateAlias",
          "",
          {"Ref": "DomainName"}
      ]
  }],

cfn-lint still likes this. But trying it out givess: "The parameter CNAME contains one or more parameters that are too small."

After more tries I finally found the solution that works:

"DistributionConfig":
  "Aliases": {
      "Fn::If": [
      "ShouldNotCreateAlias",
          [],
          [{"Ref": "DomainName"}]
      ]
  },

As every upload to CloudFront takes minutes, this is just a very slow way of developing. I realise I'm spoiled with strict compilers (Elm/Eiffel), and hate the iterative debugging (make a change, run code, oops typo, back to editor) cycle. It just wastes time you could spend building useful stuff.

berenddeboer commented 4 years ago

Some more issues trying to attach a Lambda(@Edge?) path rewrite function to my CloudFront distribution.

  1. Only at push do you detect function needs to be in us-east-1.
  2. If you create a new amplify project, and push it, the function cannot have environment variables.

I suppose they all have reasons, but my main point is that you can find out most problems only by pushing (which takes minutes).

berenddeboer commented 4 years ago

And another one, created a new environment, and hit limit on name which you only find out after 20 minutes or so:

Resource Name: IndexAllLawsAccessRole (AWS::IAM::Role)
Event Type: create
Reason: 1 validation error detected: Value 'appsync-ds-lam-2msuolnmnffkbopnhyejr66tzi-IndexAllLaws-lditberend' at 'roleName' failed to satisfy constraint: Member must have length less than or equal to 64 (Service: AmazonIdentityManagement; Status Code: 400; Error Code: ValidationError; Request ID: 8d8eab66-5a6d-415e-aa14-787185dedeae)

Resource Name: RelationalDatabaseAccessRole (AWS::IAM::Role)
Event Type: create
Reason: 1 validation error detected: Value 'appsync-ds-aurora_pgsql_libya-2msuolnmnffkbopnhyejr66tzi-lditberend' at 'roleName' failed to satisfy constraint: Member must have length less than or equal to 64 (Service: AmazonIdentityManagement; Status Code: 400; Error Code: ValidationError; Request ID: 0a92e25e-d168-4f75-a87b-be39f4679a11)
PatMyron commented 2 years ago

AWS' cfn-lint replaced npm's cfn-lint in 2018: https://github.com/martysweet/cfn-lint/pull/227 https://github.com/aws-cloudformation/cfn-lint/issues/2053 This repo's now over 95% of deprecated npm's cfn-lint usage, please switch to AWS' cfn-lint

https://github.com/aws-amplify/amplify-cli/pull/10270