-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(ec2): network interface definitions for launch templates #34327
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
base: main
Are you sure you want to change the base?
feat(ec2): network interface definitions for launch templates #34327
Conversation
…ations as part of Launch Template definitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
/** | ||
* InterfaceType | ||
*/ | ||
export enum InterfaceType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason why the EFA-only interface type wasn't included?
* | ||
* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-networkinterface.html | ||
*/ | ||
export interface Ipv6Add extends CfnLaunchTemplate.Ipv6AddProperty { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In AWS CDK, interface and class names typically use full, descriptive words rather than abbreviations. The naming Ipv6Add
appears to be shortened from what would more conventionally be named Ipv6Address
.
/** | ||
* Specifies the parameters for a Launch Template network interface definition. | ||
*/ | ||
export interface NetworkInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing. The current approach in this PR is simply taking the CloudFormation (L1) properties and exposing them directly with minimal changes:
- The
NetworkInterface
interface is almost identical toCfnLaunchTemplate.NetworkInterfaceProperty
- The mutually exclusive options (like
ipv4PrefixCount
vsipv4Prefixes
) are carried over without any type safety improvements - It doesn't provide the higher-level abstractions that make L2 constructs easier to use
This approach doesn't leverage TypeScript's type system to prevent configuration errors, which is one of the key benefits of using L2 constructs in CDK. Users could still provide mutually exclusive options and only discover the error at runtime.
A proper L2 implementation would:
- Use TypeScript's type system to prevent invalid configurations
- Provide more intuitive property names and structures
- Add validation logic to catch errors early
- Include helper methods for common operations
- Abstract away CloudFormation-specific details
if ( props.versionDescription && !Token.isUnresolved(props.versionDescription) && props.versionDescription.length > 255) { | ||
throw new ValidationError(`versionDescription must be less than or equal to 255 characters, got ${props.versionDescription.length}`, this ); | ||
} | ||
|
||
if ( | ||
props.versionDescription && | ||
!Token.isUnresolved(props.versionDescription) && | ||
props.versionDescription.length > 255 | ||
) { | ||
throw new ValidationError(`versionDescription must be less than or equal to 255 characters, got ${props.versionDescription.length}`, this ); | ||
} | ||
|
||
if (props.versionDescription && !Token.isUnresolved(props.versionDescription) && props.versionDescription.length > 255) { | ||
throw new ValidationError(`versionDescription must be less than or equal to 255 characters, got ${props.versionDescription.length}`, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three identical validation blocks for versionDescription length. This appears to be a mistake.
/** | ||
* efa | ||
* | ||
* @see https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/efa.html | ||
*/ | ||
EFA = 'efa', | ||
|
||
/** | ||
* interface | ||
*/ | ||
INTERFACE = 'interface', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for InterfaceType enum is minimal and doesn't fully explain what each option means. Enhance the documentation to better explain what each interface type represents
* | ||
* @see https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/security-group-connection-tracking.html#connection-tracking-timeouts | ||
*/ | ||
export interface ConnectionTrackingSpecification extends CfnLaunchTemplate.ConnectionTrackingSpecificationProperty {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These interfaces are simply extending CloudFormation types without adding any L2 construct value. We should avoid "leaking" the details or types of the CFN layer when defining construct APIs
* To improve the reliability of network packet delivery, ENA Express reorders network packets on the receiving end by default. However, some UDP-based applications are designed to handle network packets that are out of order to reduce the overhead for packet delivery at the network layer. When ENA Express is enabled, you can specify whether UDP network traffic uses it. | ||
*/ | ||
export interface EnaSrdSpecification | ||
extends CfnLaunchTemplate.EnaSrdSpecificationProperty { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above: leaking CFN layer
/** | ||
* Specifies a secondary private IPv4 address for a network interface. | ||
*/ | ||
export interface PrivateIpAdd extends CfnLaunchTemplate.PrivateIpAddProperty { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above: leaking CFN layer
/** | ||
* List of subnets to which to place the interface in | ||
* | ||
* @default No subnets are defined. | ||
*/ | ||
readonly subnet?: ISubnet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property name subnet is singular but the documentation refers to "List of subnets". This is confusing.
* | ||
* Note: If defined, overrides settings `associatePublicIpAddress` and `securityGroup`. | ||
* | ||
* @default: A single network interface using `associatePublicIpAddress` and `securityGroup`is created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the default documentation - missing space between "securityGroup" and "is".
new ec2.LaunchTemplate(stack, 'LTWithNetworkInterfaces', { | ||
networkInterfaces: [ | ||
{ associatePublicIpAddress: false, deviceIndex: 0, deleteOnTermination: true }, | ||
{ associatePublicIpAddress: false, deviceIndex: 1, deleteOnTermination: true }, | ||
], | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration test only covers a basic case with minimal properties. It doesn't test more complex scenarios like using security groups, subnets, or other network interface properties.
@@ -2479,6 +2479,35 @@ const launchTemplate = new ec2.LaunchTemplate(this, 'LaunchTemplate', { | |||
}); | |||
``` | |||
|
|||
Following demonstrates how to add multiple network interface cards to launch template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment explaining the real-world use case this example addresses, such as multi-homed instances or instances with multiple public IPs.
ipv4Prefixes: networkInterface.ipv4Prefixes?.map(p => ({ ipv4Prefix: p })), | ||
ipv6Prefixes: networkInterface.ipv6Prefixes?.map(p => ({ ipv6Prefix: p })), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arrow functions don't use parentheses around the parameter, which is inconsistent with other arrow functions in the file that use parentheses.
if ( props.versionDescription && !Token.isUnresolved(props.versionDescription) && props.versionDescription.length > 255) { | ||
throw new ValidationError(`versionDescription must be less than or equal to 255 characters, got ${props.versionDescription.length}`, this ); | ||
} | ||
|
||
if ( | ||
props.versionDescription && | ||
!Token.isUnresolved(props.versionDescription) && | ||
props.versionDescription.length > 255 | ||
) { | ||
throw new ValidationError(`versionDescription must be less than or equal to 255 characters, got ${props.versionDescription.length}`, this ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is a duplicate check here.
This PR implements basic functionality for defining network interfaces as part of Launch Template.
I'm just reviving #29875 to try and get those changes merged. All the work was done by @pasiorovuo, I made some modifications to try and address the feedback from his original PR.
Issue # (if applicable)
Fixes #14494
Reason for this change
Current Launch Template L2 is missing the ability to define multiple interfaces, their subnets etc. This functionality is required in advanced scenarios where instances need multiple public IP addresses or need to be multi-homed etc.
Description of changes
Multiple new types have been introduced in packages/aws-cdk-lib/aws-ec2/lib/launch-template.ts. Many of them are just renamed extends of the ec2-generated types. This made sense as the Cloudformation type definition is suitable, but I'm a bit uncertain if this violates the design requirement Do not “leak” the details or types of the CFN layer when defining your construct API.
Main type is NetworkInterface which defines the attributes for the interfaces. Additional changes are in LaunchTemplate constructor to accommodate the new functionality.
README has been updated to include a simple usage example.
Description of how you validated changes
A single test has been added. I have also verified the implementation by creating stacks in different configurations and deployed them. There too many combinations to check them all, so only a subset has been tested, unfortunately.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license