-
Notifications
You must be signed in to change notification settings - Fork 4k
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(apigatewayv2): http api - domain endpoint type, security policy and mTLS #17219
Conversation
Testing comments |
/** | ||
* The ARN of the public certificate issued by ACM to validate ownership of your | ||
* custom domain. Only required when configuring mutual TLS and using an ACM | ||
* imported or private CA certificate ARN as the RegionalCertificateArn. |
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.
Let's fix RegionallCertificateArn usage here. You can work with our doc writer if needed for a more accurate description
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.
ok
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.
Please find my comments below.
readonly endpointType?: EndpointType; | ||
|
||
/** | ||
* The ARN of the public certificate issued by ACM to validate ownership of your |
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 ARN of the public certificate issued by ACM to validate ownership of your | |
* The public certificate issued by ACM to validate ownership of your |
* custom domain. Only required when configuring mutual TLS and using an ACM | ||
* imported or private CA certificate ARN as the RegionalCertificateArn. |
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.
Confusing. RegionalCertificateArn
is not here AFAICT. This needs to be written in CDK terms and not just copy-pasted from CFN.
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.
Updating documentation for domain name properties and adding the same in README.
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.
some more comments
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.
Updated docs.
/** | ||
* Specifies the configuration for a an API's domain name. | ||
*/ | ||
export interface DomainNameConfiguration { |
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.
Since adding multiple domain name configuration is only during migration, and is not the dominant use case, I suggest changing the API like this -
const domain = new DomainName(stack, 'DomainName', {
domainName: 'example.com',
certificate: '...',
endpointType: '...'
});
domain.addConfiguration({
certificate: '...',
endpointType: '...',
...
});
This keeps the API clean for the majority of users, while enabling the migration use case.
More importantly, it also avoids an API breaking change.
@@ -82,13 +82,16 @@ | |||
"@aws-cdk/cdk-integ-tools": "0.0.0", | |||
"@aws-cdk/cfn2ts": "0.0.0", | |||
"@aws-cdk/pkglint": "0.0.0", | |||
"@types/jest": "^26.0.24" | |||
"@types/jest": "^26.0.24", | |||
"ts-node": "^10.4.0" |
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.
Why is this being added?
Hi @SmritiVashisth - I've just approved this PR that adds mTLS support to HTTP APIs - #17284 - since it was well contained. Hopefully, that helps you now focus on the domain configuration part. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Updating the L2 construct for
AWS::ApiGatewayV2::DomainName
resource to add support for the properties and features that ApiGateway supports. Adding the following properties:Changes include:
DomainNameProps
as a root level property, but it is supposed to be present as part ofDomainNameConfigurations
. This is a breaking change for existing customers because their old constructs will be invalid after this change.BREAKING CHANGE: Property
certificate
was present underDomainNameProps
earlier and it has been moved toDomainNameConfigurations
now, same as CFN. Existing constructs will fail as they'll be invalid now.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license