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

Add support for Fn::Transform #476

Open
atkinsonm opened this issue Nov 20, 2018 · 32 comments
Open

Add support for Fn::Transform #476

atkinsonm opened this issue Nov 20, 2018 · 32 comments
Labels
enhancement New feature or request

Comments

@atkinsonm
Copy link

atkinsonm commented Nov 20, 2018

cfn-lint version: 0.7.4

Description of issue.

cfn-lint does not properly evaluate Fn::Transform.

Example:

Resources:
  
  'Fn::Transform':
    Name: 'AWS::Include'
    Parameters:
      Location: "s3://mybucket/mySnippet.yaml"

  'Fn::Transform':
    Name: 'AWS::Include'
    Parameters:
      Location: "s3://mybucket/mySecondSnippet.yaml"

Returns:

Illegal cfn - missing Type: id: Fn::Transform

Additionally, multiple Fn::Transforms produce this error:

Duplicate resource found "Fn::Transform" (line 299)

The template used which generated these errors passed aws cloudformation validate-template without error, so it is valid CFN syntax.

@kddejong
Copy link
Contributor

Agreed. Transforms and Macros are an area we need to work on. @cmmeyer and @melusom what would you like to see here?

One approach would be to read the file and process it into the template just like the import would do. The other approach, sadly, would be to lint what we could. However this would require us to know what rules could break when a transform is used. Like we can't validate Refs or GetAtts to resources cause as in provided example all the resources don't exist. Basically the result would be a stripped down linting. Even harder is if you had the following template. The rules to check may change as Ref and GetAtts to other resources could now be checked.

Resources:
  Bucket:
    Type: AWS::S3::Bucket
    Properties:
      'Fn::Transform':
         Name: 'AWS::Include'
         Parameters:
            Location : "s3://mybucket/mySnippet.yaml"

That being said we have tried to not have much of a requirement on the network. Also we would have to hope the person running the cfn-lint would have read access to the template in the bucket (or have a profile setup for us to access it).

@kddejong kddejong added the enhancement New feature or request label Nov 21, 2018
@atkinsonm
Copy link
Author

atkinsonm commented Nov 21, 2018

Agreed @kddejong, it would be impossible to know whether missing Refs or other errors were legitimate without evaluating macros. Though we want to minimize network requirements, I think it is necessary in this case. I can imagine scenarios where the cfn-lint running environment has separate permissions than the one running Create|UpdateStack, but I feel that it is reasonable to require read access to the templates in this case.

Processing macros before running cfn-lint would mitigate the two errors I ran into as well as any false-positive "missing Ref or GetAtt" errors.

@cmmeyer
Copy link
Contributor

cmmeyer commented Nov 21, 2018

I would love to see the cloudformation package command extended to handle includes -- that way you could have local file paths in your template that we could evaluate pre-deploy and CFN could handle uploading the included files and deploying. But since it doesn't, I think our option is to lint around unless you can supply us a converged file to evaluate.

I would discourage doing this convergence within cfn-lint for network files, but it starts to outline a larger tool for local composition and deployment -- perhaps with pre-and-post macro evaluation linting phases. :)

@atkinsonm
Copy link
Author

@cmmeyer brings up an interesting point regarding CI. It may happen that the file used for the Include is not in the same path/repository or was even developed by the same user. We can't assume that mySnippet.yaml is available locally.

If the approach will be not to evaluate network files, we will need to suppress the warnings above and we will also need to accept that users will receive false-positive "missing Ref or GetAtt" errors.

@spockNinja
Copy link

Just adding another opinion here. 😄

I think the choice to limit network and IAM role reliance is a good one. Any sort of change that requires actually hitting AWS resources is going to create a lot of issues in this repo with people trying to diagnose IAM issues, and that shouldn't be the focus of this repository.

Since aws cloudformation package supports the Include transform for local files, I think that is a sane feature to support here as well.

If the Include transform matches a local file, I think this project should pull it in before linting.

If the Include transform does not match a local file, I think this project should not try do anything too fancy. If it does anything at all with the included block, treat it similarly to how the errors for aws cloudformation package issues work -- with a single code that can be ignored easily.

@atkinsonm
Copy link
Author

@spockNinja I can get on board with that approach. It won't fit all use cases but it makes a reasonable effort while avoiding IAM and network dependencies.

@iDVB
Copy link

iDVB commented Jul 18, 2019

Any movement on this? Last post was almost a year ago.
I'm not sure if the error I'm seeing is the same as this?

image

@kddejong
Copy link
Contributor

@iDVB I think for your scenarios we can probably work on a fix. The hard part is that Transform can complete change templates. Ideally we would be linting the resulting template from Transforms. That being said I think we should be able to not fail those two rules if we run into Fn::Transform.

@hugoduraes
Copy link

hugoduraes commented Nov 30, 2019

Another example:

Resources:
  ApiName:
    Type: AWS::Serverless::Api
    Properties:
      StageName: foo
      DefinitionBody:
        'Fn::Transform':
          Name: AWS::Include
          Parameters:
            Location: swagger.yaml
  FunctionName:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: ./dist
      Handler: FunctionNameHandler.default
      Runtime: nodejs10.x
      Events:
        FunctionNameEvent:
          Type: Api
          Properties:
            Path: /foo
            Method: post
            RestApiId:
              Ref: ApiName
            RequestModel:
              Model: RequestPayloadModel
              Required: true

The above template produces the error: E0001 Error transforming template: Resource with id [FunctionName] is invalid. Event with id [FunctionNameEvent] is invalid. Unable to set RequestModel [RequestPayloadModel] on API method [post] for path [/foo] because the related API does not define any Models.

cfn-lint is not executing the transform to include the local swagger.yaml file, which contains the RequestPayloadModel that it complains about!

While running sam validate, no error is thrown.

@Nr18
Copy link

Nr18 commented Mar 6, 2020

@hugoduraes, I sort have the same as you but I solved it differently, so:

Api:
  Type: AWS::Serverless::Api
  Properties:
    DefinitionBody:
      Fn::Transform:
        Name: AWS::Include
        Parameters:
          Location: resources/openapi.yaml

and then in your resources/openapi.yaml file you would use the x-amazon-apigateway-integration:

x-amazon-apigateway-integration:
  type: aws
  uri: 
    Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${FunctionName.Arn}/invocations
  httpMethod: POST
  credentials:
    Fn::Ref: MyRoleArn

This way you will be able to deploy the template as you intended but you need to extend your swagger definition.

@kddejong I noticed that the AWS::Include transformation does not support the shorthand syntax so that is something that you would like to catch if you would validate the imported snippet.

@hugoduraes
Copy link

Will try it. Thanks @Nr18 👍

@hugoduraes
Copy link

@Nr18 I've revisited my implementation and I've found that your suggestion is what I have at the moment. One thing you don't show in your comment is the template for your lambda function.

What causes the issue are the RestApiId and RequestModel properties on the lambda function template. Do you have these on your template?

@Nr18
Copy link

Nr18 commented Mar 12, 2020

@hugoduraes so in your example it should look like this:

  ApiName:
    Type: AWS::Serverless::Api
    Properties:
      StageName: foo
      DefinitionBody:
        'Fn::Transform':
          Name: AWS::Include
          Parameters:
            Location: swagger.yaml
  FunctionName:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: ./dist
      Handler: FunctionNameHandler.default
      Runtime: nodejs10.x

And then in your swagger.yaml file it will look like:

...rest of the file...
x-amazon-apigateway-request-validators:
  full:
    validateRequestBody: true
    validateRequestParameters: true
x-amazon-apigateway-request-validator: full

paths:
  /foo:
    post:
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/RequestPayloadModel'
      responses:
        "200":
          description: 200 Accepted
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Success'
      x-amazon-apigateway-integration:
        credentials:
          Fn::GetAtt: FunctionNameRole.Arn
        uri:
          Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${FunctionName.Arn}/invocations
        passthroughBehavior: when_no_templates
        httpMethod: POST
        type: aws
        responses:
          default:
            statusCode: "200" # Bad Request
            responseTemplates:
              application/json: |
                {
                  "Message": "Success"
                }
...rest of the file...

So you don't need the Events on your AWS::Serverless::Function resource.

@hugoduraes
Copy link

@Nr18 are the request parameters validated on API Gateway if I don't specify the Events section?

@Nr18
Copy link

Nr18 commented Mar 12, 2020

Yes, the following section in the OpenAPI definition would take care of that:

x-amazon-apigateway-request-validators:
  full:
    validateRequestBody: true
    validateRequestParameters: true
x-amazon-apigateway-request-validator: full

You could also place the x-amazon-apigateway-request-validator property on a specific method if you need to mix validators.

@hugoduraes
Copy link

I thought those two properties (RestApiId and RequestModel) were also needed for it to validate requests. I've updated my template and open api docs and everything is looking great now. Thanks @Nr18

However, the issue reported here remains.

@tkeeber
Copy link

tkeeber commented Apr 17, 2020

Is there a way to get cfn-lint to ignore this issue?

I'm suffering from this same problem;

Api:
  Type: AWS::Serverless::Api
  Properties:
    DefinitionBody:
      Fn::Transform:
        Name: AWS::Include
        Parameters:
          Location: resources/openapi.yaml

E0001 Error transforming template: Resource with id [FunctionName] is invalid. Event with id [FunctionNameEvent] is invalid. Unable to set RequestModel [RequestPayloadModel] on API method [post] for path [/foo] because the related API does not define any Models.

EDIT: I tried this

api:
    Type: AWS::Serverless::HttpApi
    Metadata:
      cfn-lint:
        config:
          ignore_checks:
          - E0001

Which does not work

@hugoduraes
Copy link

@tkeeber you can workaround this issue by not defining the RequestModel on your function resource (commented out the RequestModel bit):

Resources:
  ApiName:
    Type: AWS::Serverless::Api
    Properties:
      StageName: foo
      DefinitionBody:
        'Fn::Transform':
          Name: AWS::Include
          Parameters:
            Location: swagger.yaml
  FunctionName:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: ./dist
      Handler: FunctionNameHandler.default
      Runtime: nodejs10.x
      Events:
        FunctionNameEvent:
          Type: Api
          Properties:
            Path: /foo
            Method: post
            RestApiId:
              Ref: ApiName
            # RequestModel:
            #  Model: RequestPayloadModel
            #  Required: true

@tkeeber
Copy link

tkeeber commented Apr 17, 2020

Thanks @hugoduraes, but I do not currently have that defined

@samj

This comment has been minimized.

@iamkhalidbashir
Copy link

iamkhalidbashir commented Oct 1, 2020

 api:
     Type: AWS::Serverless::HttpApi
     Metadata:
       cfn-lint:
         config:
           ignore_checks:
           - E0001

Add this to disable the linter error at resource level:-

Resources:
  WebsiteBucket:
    Type: AWS::S3::Bucket
    Metadata:
      cfn-lint:
        config:
          ignore_checks:
          - E3001
    Fn::Transform:
      Name: AWS::Include
      Parameters:
        Location: 's3://xxxxx'
...
...

@victoru
Copy link

victoru commented Jan 5, 2021

does it make sense to at least have the option to completely ignore linting when Fn::Transform blocks are encountered?

could we support something like this?

Metadata:
  cfn-lint:
    config:
      ignore_transform: true

@qidydl
Copy link

qidydl commented Jan 11, 2021

We've been hitting this for a while, in our case it's error E3002 Invalid Property when using Fn::Transform to import container definitions for an ECS task definition.

@dave-kennedy

This comment has been minimized.

@phene
Copy link

phene commented Jan 13, 2021

That's because you do have duplicates.

- "SubnetIds": [ {"Ref": "SubnetId001", "Ref": "SubnetId002", "Ref": "SubnetId003"} ]
+ "SubnetIds": [ { "Ref": "SubnetId001" }, { "Ref": "SubnetId002" }, { "Ref": "SubnetId003" } ]

@sql-sith
Copy link

sql-sith commented Mar 11, 2021

Just want to keep kicking this along. I had my sceptre/service catalog/cloudformation templates all working dandy, with AWS::Include inside my mappings section. But when I issued a PR against our CICD system, it failed cfn-lint there. That means that, in our environment, I cannot use external mappings files and will need to hard-code the mappings section inside my templates. This is obviously both a sign of the level of respect we have for this product (we let you fail us before CICD testing starts) and also a gap that needs to be addressed in some way.

Part of the problem is that the errors generated (E7001 and E7002 are two which we see, for example) are not errors we want to ignore in CICD. If cfn-python-lint was even just modified to return warnings when it sees that a mapping is involved we could at least ignore that warning in our CICD pipeline.

Is there any foreseeable workaround for this?

@AledLewis
Copy link

Hey, ran into this today, similar use-case as @qidydl 's . As noted above in @iamkhalidbashir 's workaround, I'm disabling E0001 in my POC to get past linting.

@darrenweiner
Copy link

Would be great to see resolution on this issue. Fundamentally, synthesizing the transform is an intractable problem, ignore it, perhaps with a message if possible so cfn-lint doesn't have to be disabled.

@evin-murphy
Copy link

evin-murphy commented Oct 3, 2023

Would be great to see this fixed. Currently getting a "W2001 Parameter not used" error for parameters I am referencing in my OAS.yaml. Snippet from template.yaml;

Parameters:
    IPWhitelist:
...

Resources:
    ApiGateway:
        Type: AWS::Serverless::Api
        Properties:
            OpenApiVersion: 3.0.0
            DefinitionBody:
                'Fn::Transform':
                    Name: 'AWS::Include'
                    Parameters:
                        Location: 'oas3.yaml'

And snippet from oas3.yaml;

x-amazon-apigateway-policy:
  Version: '2012-10-17'
  Statement:
  - Action: execute-api:Invoke
    Resource: "execute-api:/*"
    Effect: Allow
    Principal: '*'
  - Action: execute-api:Invoke
    Resource: "execute-api:/*"
    Effect: Deny
    Condition:
      NotIpAddress:
        aws:SourceIp:
        - Ref: IPWhitelist
    Principal: '*'

cfn-lint is exiting with W2001 Parameter IPWhitelist not used.

I could define the IPWhitelist as Auth.ResourcePolicy.IpRangeWhitelist on the APIGateway resource in template.yaml as opposed to in oas3.yaml, but then that causes sam validate --template template.yaml to fail due to aws/aws-sam-cli#803

@kddejong
Copy link
Contributor

kddejong commented Oct 4, 2023

@evin-murphy this is a different issue. For SAM we run the template through the SAM translator and validate the output. The underlying issue with Fn::Transform is we need to have the code to execute against. Thankfully for SAM they make it available publicly and we can leverage it easily.

@kddejong
Copy link
Contributor

kddejong commented May 3, 2024

For the most part a lot of these issues should clear up with v1. A vast majority of rules are built to use json schema validation. As we hit the Fn::Transform keyword we will basically stop not validating what is underneath. While this won't help you detect an issue with the Transform it will at least prevent errors showing up in cfn-lint.

This logic isn't perfect as JSON schema can't be used for all the types of rules we have. So your mileage may very depending on how the rule was written and how you are doing the Transform. We can tackle those as one offs.

Additionally if you are doing Fn::Transform inside another function or in different parts of the template you may see an issue but those are easily fixable inside v1. I tried to add a few of these already but may have missed a couple. Again we tackle these as they arise.

For the original issue we still don't have anything to handle duplicate keys and that will continue to be an issue.

@kddejong
Copy link
Contributor

kddejong commented May 3, 2024

In relation to the uniqueness of keys its worth noting from yaml specs

Mapping

The content of a mapping node is an unordered set of key: value node pairs, with the restriction that each of the keys is unique.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests