feat(ec2): enabling features for ipv6 and dualstack support with corresponding unit tests

[daniilmc]

Enabling features for ipv6 and dualstack support with corresponding unit tests

Original PR: #3873

Issue #33493

Closes #33493

Reason for this change

Currently, AWS Services, i.e., Xray, Wafv2, Workmail, have started to release support for IPv6 or Dualstack for their VPC endpoints. We would like to modify our infrastructure to be able to support. these new IP address types.

Description of changes

1. Passed necessary props to the VPC Endpoint interface
2. Added necessary props that need to be passed to enable all different IP address types
3. Abstracted address types as enum to abide by L1, L2 constructs
4. Added necessary unit tests to cover all cases
5. Added necessary changes to integ tests

Added more detailed descriptions and guidlines for usage in the aws-ec2 README.md as well as very detailed comments above each code change.

Description of how you validated changes

Added unit tests and integ tests to cover all valid and invalid cases. Throws necessary errors according to documentation. Detailed descriptions of each case are outlined in comments.

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

[shikha372]

can one of these properties be specified without the other, eg. can someone pass props.idAddressType without passing props.dnsRecordIpType , i think yes, and validation makes sense in that case.
Thinking if we can model it together where if props.idAddressType is always required with optional props.dnsRecordIpType.

[AlexanderJangAMZN]

Hello! I think yes, dnsRecordIpType is only required when privateDnsOptions is enabled, but IpAddressType is always required (Pretty sure it defaults to IPv4 but we didn't want to make those assumptions and let CFN handle it).

[daniilmc]

@shikha372 good point. We wanted to stay as close to the original VPCE CFN specification as possible.

The CFN spec allows permutations of different values of dnsRecordIpType and idAddressType, so maybe it makes sense to just leave it like that and offload the validation to CFN (and we remove our validation as well)? WDYT?

Regarding your suggestion: One way to avoid a situation where dnsRecordIpType is set and idAddressType is not could be something like:

ipAddressTypeOptions?: {
   ipAddressType: VpcEndpointIpAddressType,
   dnsRecordIpType?: VpcEndpointDnsRecordIpType
   privateDnsOnlyForInboundResolverEndpoint?: VpcEndpointPrivateDnsOnlyForInboundResolverEndpoint
}

but that bundles ipAddress and dnsRecord values under a new ipAddressTypeOptions name, which seems a bit off

[shikha372]

Thanks @AlexanderJangAMZN, i would rather flatten out properties than creating them together if it doesn't make sense, i'm okay with the keeping the properties as it is.

For the validation, would rather to do it in CFN as this can become outdated with any change at handler end.

[shikha372]

can we remove these checks altogether and rely on CFN to validate, or if it is very essential to keep we modify these errors to warning instead, because :

1. its hard to keep this information updated at CDK end and a invalid check can become blocker for the customers.

[AlexanderJangAMZN]

Yes, this was also something we were discussing, we understand that incorrect checks would introduce breaking changes so we tried to limit the checks as much as possible to just the ones listed in this doc, as these seemed "safe" enough to add and good to check here. However, if you think it is unnecessary here, even the slightest chance of causing blockers doesn't seem worth the risk. What would you recommend?

[AlexanderJangAMZN]

Regarding your comment about ipAddressType and dnsRecordIpType being related, I initially created one interface for them both, but they can take in slightly different values so keeping them separate seemed like safer approach. In our case, ipAddressType allows ipv4 | ipv6 | dualstack | not-specified , while dnsRecordIpType allows ipv4 | dualstack | ipv6 | service-defined.

[shikha372]

Thanks @AlexanderJangAMZN , i meant it to model like this, where with enum you'll still have different values for ipAddressType and dnsRecordIpType, but if you prefer to flatten out that's okay .

ipAddressTypeOptions?: {
ipAddressType: VpcEndpointIpAddressType,
dnsRecordIpType?: VpcEndpointDnsRecordIpType
privateDnsOnlyForInboundResolverEndpoint?: VpcEndpointPrivateDnsOnlyForInboundResolverEndpoint
}

For validation, i would prefer to remove it altogether and rely on CFN handler for validation, if you think its still necessary to notify users, we can use warnings instead.

[shikha372]

Hi @daniilmc , Thank you for contributing this PR and implement this feature request, from the first review of implementation I feel that the two properties ipaddressType and dnsrecordIptype, are related to each other, and there seems to be an opportunity to model it under same props. let me know if you think otherwise.

``

Pull request has been modified.

[daniilmc]

Hello @shikha372, we removed all client-side validation, so now we rely on CFN to validate. Could you please have a look?

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 3912d16
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[AlexanderJangAMZN]

@shikha372 okay I think we are finally ready for the final review!

[shikha372]

LGTM.. Thank you @AlexanderJangAMZN

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.