aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.45k stars 593 forks source link

E3032 for keyschema in dynamodb table false positive #3619

Closed AyoubOukh closed 2 months ago

AyoubOukh commented 2 months ago

CloudFormation Lint Version

1.10.3

What operating system are you using?

ubuntu

Describe the bug

E3032 [{'AttributeName': {'Ref': 'HashKeyName'}, 'KeyType': 'HASH'}] is too short (2)
Error: table-template.yml:215:7

the e3032 is raised even if there is the minimum required hash key for keyschema in dynamodb table

Expected behavior

It shouldn't fail for e3032

Reproduction template

  Table:
    Type: AWS::DynamoDB::Table
    Properties:
      TableName: !Ref TableName
      BillingMode: !Ref BillingMode
      AttributeDefinitions:
        - AttributeName: !Ref HashKeyName
          AttributeType: !Ref HashKeyType
        - !If
          - HasRangeKey
          - AttributeName: !Ref RangeKeyName
            AttributeType: !Ref RangeKeyType
          - Ref: AWS::NoValue
        - !If
          - HasAttribute0
          - AttributeName: !Ref Attribute0Name
            AttributeType: !Ref Attribute0Type
          - Ref: AWS::NoValue
      KeySchema:
        - AttributeName: !Ref HashKeyName
          KeyType: HASH
        - !If
          - HasRangeKey
          - AttributeName: !Ref RangeKeyName
            KeyType: RANGE
          - Ref: AWS::NoValue
 ...
kddejong commented 2 months ago

Can you provide the condition logic for HasRangeKey also are you using LocalSecondaryIndexes in your resource?

If you have LocalSecondaryIndexes under a condition also provide that conditions logic. My guess is there are scenarios in which the parameters provided can create a scenario where this is a valid error.

AyoubOukh commented 2 months ago

Thanks for your reply 🙏🏼

Conditions:
  HasRangeKey: !Not [!Equals [!Ref RangeKeyName, '']]
  HasLocalSecondaryIndex0: !Not [!Equals [!Ref LocalSecondaryIndex0Name, '']]
  HasLocalSecondaryIndex1: !Not [!Equals [!Ref LocalSecondaryIndex1Name, '']]

Yes we do have them but also on a condition:

      LocalSecondaryIndexes:
        - !If
          - HasLocalSecondaryIndex0
          - IndexName: !Ref LocalSecondaryIndex0Name
            KeySchema:
              - AttributeName: !Ref HashKeyName
                KeyType: HASH
              - AttributeName: !Ref LocalSecondaryIndex0RangeKey
                KeyType: RANGE
            Projection:
              ProjectionType: ALL
          - Ref: AWS::NoValue
        - !If
          - HasLocalSecondaryIndex1
          - IndexName: !Ref LocalSecondaryIndex1Name
            KeySchema:
              - AttributeName: !Ref HashKeyName
                KeyType: HASH
              - AttributeName: !Ref LocalSecondaryIndex1RangeKey
                KeyType: RANGE
            Projection:
              ProjectionType: ALL
          - Ref: AWS::NoValue

If I rely solely on the table deployment success, then I don't think it's a valid error.

kddejong commented 2 months ago

An attribute referenced in a KeySchema element is not defined in AttributeDefinitions and One or more parameter values were invalid: Table KeySchema does not have a range key, which is required when specifying a LocalSecondaryIndex

are both errors that we should be covering and looking for.

So I'm guessing your actual AttributeDefinitions has an additional element for LocalSecondaryIndex1RangeKey?

But the issue here is in relation to the conditions. So if I specify a value for Parameter LocalSecondaryIndex1RangeKey but not for RangeKeyName the template will fail because the conditions don't account for this scenario. You can of course ignore this error if you would like.

Ideally we would have something like a template rule that says you have to specify the range key when specifying the LSI. Another option is adding to the HasLocalSecondaryIndex0 condition with an AND that also requires the HasRangeKey be none empty. This would look like !And [!Not [!Equals [!Ref LocalSecondaryIndex0Name, '']], !Condition HasRangeKey]

For the Rules approach this issue should be considered #1888

That being said our logic could use some improvement. I'm updating the schema logic to include minItems to 1.

"if": {
    "properties": {
     "LocalSecondaryIndexes": {
      "minItems": 1,
      "type": "array"
     }
    },
    "required": [
     "LocalSecondaryIndexes"
    ],
    "type": "object"
   },
AyoubOukh commented 2 months ago

Thank you!