cfn-lint v1 does not warn about missing mapping keys (nested FindInMap)

nosnilmot commented 3 days ago

CloudFormation Lint Version

1.4.2 & git (30760e20f)

What operating system are you using?

Mac

Describe the bug

Very similar to #3385. The basic case is fixed but keys missing that involve nested FindInMap are still not detected.

cfn-lint 0.87.9 can detect:

W1011 FindInMap second key "MissingKey" doesn't exist in map "Map" under "Key" at Resources/Bucket/Properties/BucketName/Fn::Sub/1/number/Fn::FindInMap
map.yaml:21:13

cfn-lint 1.4.2 / git 30760e20f accept the example template without warnings or errors

Expected behavior

E1019 'MissingKey' is not one of ['ExistingKey'] for mapping 'Map' and key 'Key' when 'Ref' is resolved
map.yaml:13:7

E1019 'MissingKey' is not one of ['ExistingKey'] for mapping 'Map' and key 'Key'
map.yaml:21:13

Reproduction template

AWSTemplateFormatVersion: '2010-09-09'
Mappings:
  Map:
    Key:
      ExistingKey: 1
  KeyMap:
    Key:
      Name: Key
Resources:
  Bucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName: !Sub
        - "bucket-${number}"
        - number: !FindInMap
          - "Map"
          - !FindInMap
            - "KeyMap"
            - "Key"
            - "Name"
          - "MissingKey"

kddejong commented 3 days ago

+1 to this I want to figure this out but also trying to scale it for larger usage. The logic isn't great in v0 but for sure provides the error you have above in the scenario you provided.

I'm going to use the docs description of map name, top level key, and second level key.

The way the v0 logic works is it requires a string for the map name and second level key and if the top level key is a function then we can validate the second level key exists for all top level keys.

Let me ask back a few questions around assumptions I keep thinking about.

Can second level keys be different between top level keys? What should the error, if any, be here? Should we say A doesn't exist when !Ref Parameter is Two? Should we not raise an error because there is a possibility of it being okay? Should there be a warning or informational error describing inconsistencies with the second level keys?

Parameters:
  Parameter:
    Type: String
Mappings:
  Map:
    One:
      A: "foo"
    Two:
      B: "bar"
Resources:
  Bucket:
    Type: AWS::S3::Bucket
    Properties:
      BucketName:  !FindInMap [Map, !Ref Parameter, A]

what if I change the parameter to

Parameters:
  Parameter:
    Type: String
    AllowedValues: ["One"]

Do we want to expand this to when we are using functions for the map name and second level key?

nosnilmot commented 2 days ago

I will take a stab at these answers but obviously I am biased towards my use-case and the specific scenario I am trying to catch. I may not have thought about this enough and others may have different expectations.

I think a general approach would be:

if there is a bounded set of function outputs, error if the template will fail to deploy for any combination of those
if there is an unbounded set of function outputs, warning if there are inconsistencies between keys that would mean some values work and others do not

Can second level keys be different between top level keys?

There might be other second level keys but if a specific second level key exists under one top level key but not under another that could raise an error if there is a bounded set of function outputs and at least one would require the missing key, and a warning if the function outputs are unbounded.

What should the error, if any, be here? Should we say A doesn't exist when !Ref Parameter is Two?

Yes, as a warning.

Should we not raise an error because there is a possibility of it being okay?

Yes, because the is no constraint on the values

Should there be a warning or informational error describing inconsistencies with the second level keys?

Yes

what if I change the parameter to

This should output no warnings or errors, but if you change it to AllowedValues: ["Two"] it should raise an error because it can't ever work and will always fail to deploy. AllowedValues: ["One", "Two"] should also error.

Do we want to expand this to when we are using functions for the map name and second level key?

In an ideal world, yes.

benbridts commented 2 days ago

I've done something like this before:

Mappings:
  Constants:
    default:
      ToolWorkspaceId: 12345
      OtherToolDomain: api.tool.com

So I could use !FindInMap [Constants, default, ToolWorkspaceId] instead of repeating the value.

If you have a lot of constants, you could change this to be

Mappings:
  Constants:
    Tool:
      WorkspaceId: 12345
    OtherTool:
      Domain: api.tool.com

I however do not think you'd use that pattern with a !Ref in a !FindInMap.

I personally think it's fine to warn / err if the First Level Key is set by a !Ref and there is a missing Second Level Key

nosnilmot commented 1 day ago

I however do not think you'd use that pattern with a !Ref in a !FindInMap.

what do you mean? I have extensive use of these 3 patterns:

Something: !FindInMap
  - Map1
  - !FindInMap
    - Map2
    - !FindInMap
      - Map3
      - !Ref AWS::Region
      - !Ref Param
    - !Ref AWS::AccountId
  - SomeString

Something: !FindInMap
  - Map1
  - !FindInMap
    - Map2
    - !Ref AWS::Region
    - !Ref Param
  - SomeString

Something: !FindInMap
  - Map
  - !Ref Param
  - SomeString

benbridts commented 1 day ago

I however do not think you'd use that pattern with a !Ref in a !FindInMap.

what do you mean? I have extensive use of these 3 patterns:

I don't think you'd use Mappings for constants, with different 2nd level keys, while also dynamically building the FindInMap

E.g, this seems unlikely:

Mappings:
  Constants:
    ToolOne:
      ApiKey: 12345
    ToolTwo:
      Domain: tool.tld

together with !FindInMap [Constants, !Ref Tool, ApiKey] (in that case every tool would have an ApiKey

aws-cloudformation / cfn-lint