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

Bug: LaunchTemplate/ImageId check fails in two ways #3473

Closed
brhubbar opened this issue Jul 8, 2024 · 3 comments · Fixed by #3513
Closed

Bug: LaunchTemplate/ImageId check fails in two ways #3473

brhubbar opened this issue Jul 8, 2024 · 3 comments · Fixed by #3513
Labels
enhancement New feature or request

Comments

@brhubbar
Copy link

brhubbar commented Jul 8, 2024

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
Copy link
Contributor

kddejong commented Jul 8, 2024

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
Copy link
Contributor

kddejong commented Jul 8, 2024

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
Copy link
Author

brhubbar commented Jul 8, 2024

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.

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

Successfully merging a pull request may close this issue.

2 participants