-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(cloudfront-origins): add ipAddressType property to Lambda Function URL origins #35458
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
feat(cloudfront-origins): add ipAddressType property to Lambda Function URL origins #35458
Conversation
…on URL origins with dualstack default
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)
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.
Thank you for your contribution! I've added some advices.
| * | ||
| * @default 'dualstack' | ||
| */ | ||
| readonly ipAddressType?: 'ipv4' | 'ipv6' | 'dualstack'; |
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.
We cannot use union type in CDK. Could you please define new enum type?
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 the quick review, and sorry for the elementary mistake.
i've added new enum.
| originProtocolPolicy: cloudfront.OriginProtocolPolicy.HTTPS_ONLY, | ||
| originReadTimeout: this.props.readTimeout?.toSeconds(), | ||
| originKeepaliveTimeout: this.props.keepaliveTimeout?.toSeconds(), | ||
| ipAddressType: this.props.ipAddressType ?? 'dualstack', |
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.
What is the cloudformation default value of ipAddressType? (If we deploy cfn with specifying ipAddressType to undefined, is the IP address type set to dual stack?)
If that default value is not 'dualstack', this modification is a breaking change and you need to implement a feature flag.
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.
What is the cloudformation default value of ipAddressType? (If we deploy cfn with specifying ipAddressType to undefined, is the IP address type set to dual stack?)
I checked in my personal account and if we don't explicitly define IpAddressType in CloudFormation, IpAddressType defaults to IPv4.
If that default value is not 'dualstack', this modification is a breaking change and you need to implement a feature flag.
I have removed the setting that makes dualstack the default and implemented only the ipAddressType property. Although I initially set dualstack as the default based on the description in #35450,
this would be a breaking change, so it might be better to limit this PR just adding the property for now.
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.
Thank you for your confirmation and I agree with your opinion.
|
Could you please add integ test? |
sorry for being late, i just added integ-test's snapshot. |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| test('Does not include ipAddressType when not specified (uses CloudFormation default)', () => { | ||
| const fn = new lambda.Function(stack, 'MyFunction', { | ||
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | ||
| handler: 'index.handler', | ||
| runtime: lambda.Runtime.NODEJS_20_X, | ||
| }); | ||
|
|
||
| const fnUrl = fn.addFunctionUrl({ | ||
| authType: lambda.FunctionUrlAuthType.NONE, | ||
| }); | ||
|
|
||
| const origin = new FunctionUrlOrigin(fnUrl); | ||
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | ||
|
|
||
| expect(stack.resolve(originBindConfig.originProperty)).toEqual({ | ||
| id: 'StackOriginLambdaFunctionURL', | ||
| domainName: { | ||
| 'Fn::Select': [2, { 'Fn::Split': ['/', { 'Fn::GetAtt': ['MyFunctionFunctionUrlFF6DE78C', 'FunctionUrl'] }] }], | ||
| }, | ||
| customOriginConfig: { | ||
| originProtocolPolicy: 'https-only', | ||
| originSslProtocols: ['TLSv1.2'], | ||
| }, | ||
| }); | ||
| }); |
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.
I think this test is not necessary.
| test('Does not include ipAddressType when not specified (uses CloudFormation default)', () => { | |
| const fn = new lambda.Function(stack, 'MyFunction', { | |
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | |
| handler: 'index.handler', | |
| runtime: lambda.Runtime.NODEJS_20_X, | |
| }); | |
| const fnUrl = fn.addFunctionUrl({ | |
| authType: lambda.FunctionUrlAuthType.NONE, | |
| }); | |
| const origin = new FunctionUrlOrigin(fnUrl); | |
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | |
| expect(stack.resolve(originBindConfig.originProperty)).toEqual({ | |
| id: 'StackOriginLambdaFunctionURL', | |
| domainName: { | |
| 'Fn::Select': [2, { 'Fn::Split': ['/', { 'Fn::GetAtt': ['MyFunctionFunctionUrlFF6DE78C', 'FunctionUrl'] }] }], | |
| }, | |
| customOriginConfig: { | |
| originProtocolPolicy: 'https-only', | |
| originSslProtocols: ['TLSv1.2'], | |
| }, | |
| }); | |
| }); |
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.
understood,i deleted this test case.
| test('Correctly sets ipAddressType to dualstack', () => { | ||
| const fn = new lambda.Function(stack, 'MyFunction', { | ||
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | ||
| handler: 'index.handler', | ||
| runtime: lambda.Runtime.NODEJS_20_X, | ||
| }); | ||
|
|
||
| const fnUrl = fn.addFunctionUrl({ | ||
| authType: lambda.FunctionUrlAuthType.NONE, | ||
| }); | ||
|
|
||
| const origin = new FunctionUrlOrigin(fnUrl, { | ||
| ipAddressType: OriginIpAddressType.DUALSTACK, | ||
| }); | ||
|
|
||
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | ||
|
|
||
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | ||
| customOriginConfig: { | ||
| ipAddressType: OriginIpAddressType.DUALSTACK, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| test('Correctly sets ipAddressType to ipv4', () => { | ||
| const fn = new lambda.Function(stack, 'MyFunction', { | ||
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | ||
| handler: 'index.handler', | ||
| runtime: lambda.Runtime.NODEJS_20_X, | ||
| }); | ||
|
|
||
| const fnUrl = fn.addFunctionUrl({ | ||
| authType: lambda.FunctionUrlAuthType.NONE, | ||
| }); | ||
|
|
||
| const origin = new FunctionUrlOrigin(fnUrl, { | ||
| ipAddressType: OriginIpAddressType.IPV4, | ||
| }); | ||
|
|
||
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | ||
|
|
||
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | ||
| customOriginConfig: { | ||
| ipAddressType: OriginIpAddressType.IPV4, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| test('Correctly sets ipAddressType to ipv6', () => { | ||
| const fn = new lambda.Function(stack, 'MyFunction', { | ||
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | ||
| handler: 'index.handler', | ||
| runtime: lambda.Runtime.NODEJS_20_X, | ||
| }); | ||
|
|
||
| const fnUrl = fn.addFunctionUrl({ | ||
| authType: lambda.FunctionUrlAuthType.NONE, | ||
| }); | ||
|
|
||
| const origin = new FunctionUrlOrigin(fnUrl, { | ||
| ipAddressType: OriginIpAddressType.IPV6, | ||
| }); | ||
|
|
||
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | ||
|
|
||
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | ||
| customOriginConfig: { | ||
| ipAddressType: OriginIpAddressType.IPV6, | ||
| }, | ||
| }); | ||
| }); |
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.
It is better to aggregate these tests like below.
| test('Correctly sets ipAddressType to dualstack', () => { | |
| const fn = new lambda.Function(stack, 'MyFunction', { | |
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | |
| handler: 'index.handler', | |
| runtime: lambda.Runtime.NODEJS_20_X, | |
| }); | |
| const fnUrl = fn.addFunctionUrl({ | |
| authType: lambda.FunctionUrlAuthType.NONE, | |
| }); | |
| const origin = new FunctionUrlOrigin(fnUrl, { | |
| ipAddressType: OriginIpAddressType.DUALSTACK, | |
| }); | |
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | |
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | |
| customOriginConfig: { | |
| ipAddressType: OriginIpAddressType.DUALSTACK, | |
| }, | |
| }); | |
| }); | |
| test('Correctly sets ipAddressType to ipv4', () => { | |
| const fn = new lambda.Function(stack, 'MyFunction', { | |
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | |
| handler: 'index.handler', | |
| runtime: lambda.Runtime.NODEJS_20_X, | |
| }); | |
| const fnUrl = fn.addFunctionUrl({ | |
| authType: lambda.FunctionUrlAuthType.NONE, | |
| }); | |
| const origin = new FunctionUrlOrigin(fnUrl, { | |
| ipAddressType: OriginIpAddressType.IPV4, | |
| }); | |
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | |
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | |
| customOriginConfig: { | |
| ipAddressType: OriginIpAddressType.IPV4, | |
| }, | |
| }); | |
| }); | |
| test('Correctly sets ipAddressType to ipv6', () => { | |
| const fn = new lambda.Function(stack, 'MyFunction', { | |
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | |
| handler: 'index.handler', | |
| runtime: lambda.Runtime.NODEJS_20_X, | |
| }); | |
| const fnUrl = fn.addFunctionUrl({ | |
| authType: lambda.FunctionUrlAuthType.NONE, | |
| }); | |
| const origin = new FunctionUrlOrigin(fnUrl, { | |
| ipAddressType: OriginIpAddressType.IPV6, | |
| }); | |
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | |
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | |
| customOriginConfig: { | |
| ipAddressType: OriginIpAddressType.IPV6, | |
| }, | |
| }); | |
| }); | |
| test.each([OriginIpAddressType.DUALSTACK, OriginIpAddressType.IPV4, OriginIpAddressType.IPV6])('Correctly sets ipAddressType to %s', (ipAddressType) => { | |
| const fn = new lambda.Function(stack, 'MyFunction', { | |
| code: lambda.Code.fromInline('exports.handler = async () => {};'), | |
| handler: 'index.handler', | |
| runtime: lambda.Runtime.NODEJS_20_X, | |
| }); | |
| const fnUrl = fn.addFunctionUrl({ | |
| authType: lambda.FunctionUrlAuthType.NONE, | |
| }); | |
| const origin = new FunctionUrlOrigin(fnUrl, { | |
| ipAddressType, | |
| }); | |
| const originBindConfig = origin.bind(stack, { originId: 'StackOriginLambdaFunctionURL' }); | |
| expect(stack.resolve(originBindConfig.originProperty)).toMatchObject({ | |
| customOriginConfig: { | |
| ipAddressType, | |
| }, | |
| }); | |
| }); |
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.
absolutely right, I aggregated testcase as you wrote.
| /** | ||
| * IP address type for CloudFront to use when connecting to the origin. | ||
| */ | ||
| export enum OriginIpAddressType { | ||
| /** | ||
| * CloudFront uses IPv4 only to connect to the origin. | ||
| */ | ||
| IPV4 = 'ipv4', | ||
|
|
||
| /** | ||
| * CloudFront uses IPv6 only to connect to the origin. | ||
| */ | ||
| IPV6 = 'ipv6', | ||
|
|
||
| /** | ||
| * CloudFront uses both IPv4 and IPv6 to connect to the origin. | ||
| */ | ||
| DUALSTACK = 'dualstack', | ||
| } |
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.
Because this enum can be used not only fn url origin but also other origins, it might be better to define this enum in cloudfront/origin.ts.
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.
I changed it to define this enum in cloudfront/origin.ts. same as your PR.
i really appreciate your specific feedbacks, I learned a lot !
|
Could you please fix this lint error and merge the newest main branch? @aws-cdk-testing/framework-integ: /codebuild/output/src419940982/src/actions-runner/_work/aws-cdk/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-cloudfront-origins/test/integ.function-url-origin-ip-address-type.ts
@aws-cdk-testing/framework-integ: 58:4 error Newline required at end of file but not found eol-last |
…ature/function-url-origin-ip-address-type
…/github.com/matoom-nomu/aws-cdk into feature/function-url-origin-ip-address-type
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!
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.
Hi @matoom-nomu,
Thanks for your contribution :)
Overall looks pretty good. Just added a comment.
| new IntegTest(app, 'FunctionUrlOriginIpAddressTypeTest', { | ||
| testCases: [stack], |
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.
Overall looks good to me. We can maybe add a assertion api call, just to be sure the resources have been created with the expected properties.
There are examples in aws-cdk that uses Integ.assertions.awsApiCall to make this type of assertion.
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 reviewing.
I added assertion tests to the integration test with Integ.assertions.awsApiCall like below.
// Assert that distributions are created with expected IP address type settings
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionIPv4.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV6Enabled', ExpectedResult.exact(false));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionIPv6.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV6Enabled', ExpectedResult.exact(true));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionDualstack.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV6Enabled', ExpectedResult.exact(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.
My suggestion to cover all the cases to make the assertion complete:
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionIPv4.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV4Enabled', ExpectedResult.exact(true));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionIPv4.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV6Enabled', ExpectedResult.exact(false));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionIPv6.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV4Enabled', ExpectedResult.exact(false));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionIPv6.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV6Enabled', ExpectedResult.exact(true));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionDualstack.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV4Enabled', ExpectedResult.exact(true));
integ.assertions.awsApiCall('CloudFront', 'getDistribution', {
Id: distributionDualstack.distributionId,
}).assertAtPath('Distribution.DistributionConfig.IsIPV6Enabled', ExpectedResult.exact(true));
Pull request has been modified.
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 good. Thanks for your contribution!
|
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). |
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
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). |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
Pull request has been modified.
|
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). |
|
@Mergifyio requeue |
☑️ This pull request is already queued |
|
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). |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |

Issue # (if applicable)
#35450
Reason for this change
Lambda Function URLs natively support dual-stack IPv4/IPv6 connectivity, but CDK's FunctionUrlOrigin class did not expose the
ipAddressTypeproperty to configure IP protocol preferences.Description of changes
OriginIpAddressTypeenum withIPV4,IPV6, andDUALSTACKoptionsipAddressTypeproperty toFunctionUrlOriginPropsinterface using the enum typeFunctionUrlOriginandFunctionUrlOriginWithOACclasses to pass through the propertyDescribe any new or updated permissions being added
N/A
Description of how you validated changes
ipAddressTypeis not specifiedOriginIpAddressType.IPV4,OriginIpAddressType.IPV6,OriginIpAddressType.DUALSTACK)Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license