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

"when 'Ref' is resolved" to a parameter #3378

Open
kddejong opened this issue Jun 22, 2024 · 9 comments
Open

"when 'Ref' is resolved" to a parameter #3378

kddejong opened this issue Jun 22, 2024 · 9 comments
Labels
question Further information is requested

Comments

@kddejong
Copy link
Contributor

cfn-lint Version

v1.3.0

cfn-lint will validate your template parameters against the resource provider schemas. To do this we use any values that are provided in the template including Default and AllowedValues. AllowedValues will be used if provided and if not we use the Default value.

The result can be confusing so I want to discuss how some of the expectations are and to use this issue to track this issue to see if it needs to be changed.

Basic example

To represent the issue we will use this basic template

Parameters:
  MyImageId:
    Type: String
    Default: ""
Resources:
  MyInstance:
    Type: AWS::EC2::Instance
    Properties:
      ImageId: !Ref MyImageId
      InstanceType: t2.micro

returns the below error because when we resolve the Default value we do not end up with a valid AMI ID

E1152 {'Ref': 'MyImageId'} is not a 'AWS::EC2::Image.Id' when 'Ref' is resolved

Resolutions

Conditions

Sometimes we use the default parameter to represent an optional parameter and we wrap it in a condition. The following template will be error free as cfn-lint can now determine the value will not be "" when ImageId is validated.

Parameters:
  MyImageId:
    Type: String
    Default: ""
Conditions:
  IsImageId: !Not [!Equals [!Ref MyImageId, ""]]
Resources:
  MyInstance:
    Condition: IsImageId
    Type: AWS::EC2::Instance
    Properties:
      ImageId: !Ref MyImageId
      InstanceType: t2.micro

No Default

If we require the template implementer to provide a valid value remove the Default property. If we remove Default we can use other parameter properties (AllowedPattern) to better validate the parameter value. We do this because we are expecting the template user to provide a value when they are deploying the template.

Parameters:
  MyImageId:
    Type: String
    AllowedPattern: "ami-.+"  # not meant to be perfect
Resources:
  MyInstance:
    Type: AWS::EC2::Instance
    Properties:
      ImageId: !Ref MyImageId
      InstanceType: t2.micro

"Valid" Default

For this we will provide a "valid" value as the Default value.

Parameters:
  MyImageId:
    Type: String
    Default: "ami-1234567890abcdef0"
Resources:
  MyInstance:
    Type: AWS::EC2::Instance
    Properties:
      ImageId: !Ref MyImageId
      InstanceType: t2.micro

You can also use Metadata to provide hints to the user that are using the console to deploy the template.

Metadata:
  AWS::CloudFormation::Interface:
    ParameterLabels:
      MyImageId:
        default: Provide a valid image ID (ami-1234567890abcdef0)
@kddejong kddejong added the question Further information is requested label Jun 22, 2024
@kddejong kddejong pinned this issue Jun 22, 2024
@randybasrs
Copy link

Thank you for being proactive about this. After the 1.0 update we have a large number of lint errors we're working through and this represents a decent portion of them, especially with our code modules. For those errors, I think removing the default for the parameters (which are blank) we expect from a user will be a great solution. I have not tested the module deployment or usage yet but the linting errors are gone after removing the blank defaults and I plan to add the proper validation to the schema so we can lint things like the ImageId and SecurityGroupIds list.

The documentation for figuring out how to use custom schemas, override specs, and custom rules could be better but I don't have any specific advice other than it could be more cohesive with, perhaps, a few more examples of using .cfnlintrc in conjuction with other external files.

@kddejong
Copy link
Contributor Author

kddejong commented Jul 2, 2024

@randybasrs If you are wrapping those Default: "" usages by a condition and we are still showing an error please let me know. We have put in fixes to account for condition logic meaning we shouldn't validate the "" against a pattern, enum, etc. if a condition wouldn't allow us to reach that part of the code.

Heard on the documentation let me see what we can do to improve that documentation. I'll start tackling that next week.

@randybasrs
Copy link

randybasrs commented Jul 3, 2024

To confirm, using Default: "" with some of our non required parameters did function properly as long as there was a condition for that particular property. Works like a champ!

Ex:

  UserData:
    !If
      - hasUserData
      - !Ref userData
      - !Ref "AWS::NoValue"
  PrivateIpAddress:
    !If
      - useDHCP
      - !Ref "AWS::NoValue"
      - !Ref staticIP

Both of these EC2 properties have a defulat of '' which expectedly allows a user to specify the static IP or userdata or not.

@animaxcg
Copy link

animaxcg commented Jul 10, 2024

I have an error related to this but it is really a mis-match between cfn-lint and aws cloudformation deploy linting will complain like mentioned above when the param default is "" aws cloudformation deploy will complain if there isn't a provided value for those params even though the conditions mean it won't be evaluated. this causes the deploy to fail if the linting passes:

easiest example is when you are deploying a multi-regioned resource using the same template.

example:

Parameters:
  paramDynamoStreamArn:
    Type: String
    Description: StreamArn used for region that does NOT own the dynamo Resource
Conditions:
   conditionIsDbOwningRegion: !Equals [!Ref AWS::Region, us-east-1]
Resources:
  myLambda:
    Type: AWS::Serverless::Function
    Properties:
      FunctionName: myLambda
      CodeUri: somewhere
      Handler: handler.hello
      Role: !GetAtt myRole.Arn
      Events:
        stream:
          Type: DynamoDB
          Properties:
            Enabled: true
            StartingPosition: LATEST
            Stream: !If
              - conditionIsDbOwningRegion
              - !GetAtt resDynamoTable.StreamArn
              - !Ref paramDynamoStreamArn
...

without paramDynamoStreamArn having a default when we try to deploy in the DbOwner region aws cloudformation deploy fails due to no value provided for paramDynamoStreamArn despite passing linting. add the default value to pass deploy and it fails linting. cfn-lint should lint to what aws cloudformation deploy implements not the other way around.

@kddejong
Copy link
Contributor Author

@animaxcg What is the logic for conditionIsDbOwningRegion? If its possible for conditionIsDbOwningRegion to be false we will validate the default value of "" which is invalid.

@animaxcg
Copy link

animaxcg commented Jul 10, 2024

@kddejong
conditionIsDbOwningRegion: !Equals [!Ref AWS::Region, us-east-1]
so yes it is possible to fail from a linting perspective but from the deploy perspective it isn't, based on the region that is passed into the aws cloudformation deploy command but that creates the case where when the deploy works the linting fails and vice versa

@kddejong
Copy link
Contributor Author

@animaxcg to summarize... If the Default is added to the parameter the template will work in us-east-1 and fail in any other region unless a valid parameter value is provided. cfn-lint will fail relating to the fact that this template will fail in a region other than us-east-1. If Default is removed you will have to provide a parameter when deploying to us-east-1 even if the value is just "" because no Default is provided.

The current resolution that cfn-lint would stir you towards is updating the Condition to be something like whats below. This would allow you to keep the Default and then would require the resource to have a non-default value during provisioning time.

conditionIsDbOwningRegion: !And [!Equals [!Ref AWS::Region, us-east-1], !Not [!Equals [!Ref paramDynamoStreamArn, ""]]]

@animaxcg
Copy link

animaxcg commented Jul 10, 2024

@kddejong
the whole point of linting it to validate your code will execute when you do a aws cloudformation deploy the above code does execute when aws cloudformation deploy adding additional condition booleans to the condition is an over complication making the code harder to read than it should be. I think you have made the linting go one step too far here where it is assume what you are passing in the deploy the cfn which is not something you can determine at the linting stage nor is it something you should...

Also adding that to the conditional now means I have 2 places where the same logic exists in different languages (in the template and in the code that deploys the cfn) which means modifying that logic in the future means double the work which also not desirable and will likely lead to an unclear headache for people in the future

@emes4
Copy link

emes4 commented Aug 14, 2024

What is the proposed solution for cases when the Default value is a valid string representation of the object, but that validation fails?

Parameters:
  MyRole:
    Type: String
    Description: My Role ARN
    Default: "arn:aws:iam::111111111111:role/my-role"
Resources:
  MyBucketPolicy:
    Type: 'AWS::S3::BucketPolicy'
    Properties:
      Bucket: !FindInMap [MyBucket, !Ref "AWS::Region", S3Bucket]
      PolicyDocument:
        Statement:
          - Action:
              - s3:GetObject
              - s3:PutObject
            Effect: Allow
            Principal:
              AWS: !Ref MyRole

The above set up fails with E3512 {'Ref': 'MyRole'} does not match '^\\d{12}$' when 'Ref' is resolved. Using !Sub "${MyRole}" solves the issue, but ideally the validation wouldn't fail for a valid string. The Principal fields accepts role ARNs as well as AWS account numbers. Is this expected behaviour? Happy to open a new thread if this isn't the correct place for it, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants