Bug: LaunchTemplate/ImageId check fails in two ways

brhubbar commented 2 weeks ago

CloudFormation Lint Version

v1.5.0

What operating system are you using?

Ubuntu

Describe the bug

This is similar to #3319 - as of upgrading to v1, I get E3014 when providing an ImageId and a LaunchTemplate for an EC2 instance. This has deployed successfully in the past, not to mention that my LaunchTemplate does not (and is not required to) define an ImageId. Per the docs:

Any additional parameters that you specify for the new instance overwrite the corresponding parameters included in the launch template.

Secondly, if I do not specify an ImageId in either spec, but provide a LaunchTemplate, I get no error, even though, per the docs:

An AMI ID is required to launch an instance and must be specified here or in a launch template

Expected behavior

ImageId may be defined on AWS::EC2::Instance regardless of whether LaunchTemplate is specified
ImageId must be defined on AWS::EC2::Instance if LaunchTemplate is not specified
ImageId must be defined on AWS::EC2::Instance if LaunchTemplate is specified, but the linked template does not specify an ImageId.
- This may need to become a warning if the LaunchTemplate is not available to cfn-lint?
Since these Xors have been raised in two issues now, may be worth reviewing any other LaunchTemplate Xors as well

Reproduction template

---
AWSTemplateFormatVersion: 2010-09-09

Parameters:
  ImageId:
    Type: AWS::EC2::Image::Id

Resources:
  MyLaunchTemplate:
    Type: AWS::EC2::LaunchTemplate
    Properties:
      LaunchTemplateName: a-template
      LaunchTemplateData:
        Monitoring:
          Enabled: true

  MyEc2:
    Type: AWS::EC2::Instance
    # All properties override values in the launch template.
    Properties:
      LaunchTemplate:
        LaunchTemplateId: !GetAtt MyLaunchTemplate.LaunchTemplateId
        Version: !GetAtt MyLaunchTemplate.LatestVersionNumber
      # Must be specified here and/or in the launch template.
      ImageId: !Ref ImageId
      # Leaving the previous line as is will raise E3014.
      # Commenting it out will pass linting, even though the LaunchTemplate does
      # not define an ImageId.

kddejong commented 2 weeks ago

Thanks for submitting. We will remove this for now. This will become a more complicated check. We may be able to follow the LaunchTemplate to the other resource to see if ImageId is specified but this isn't something we can just have in a json schema type check.

kddejong commented 2 weeks ago

This PR will remove the false positive. As mentioned above I will need to add a new rule with some cross resource logic to determine if the ImageID is specified in the LaunchTemplate. This can take a little bit of work as we need to handle conditions, etc. We will keep this issue open until we get this fully resolved.

brhubbar commented 2 weeks ago

Thanks for the quick response! I always forget my manners when I open these bug reports, so I wanted to circle back and express my gratitude for all the work you've put into cfn-lint - it's a fantastic tool and one I refer my colleagues to regularly.

aws-cloudformation / cfn-lint