-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): domain endpoint type, security policy and endpoint migration #17518
Conversation
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.
Wow. This looks much better. Good job on turning this PR around!
Some comments below.
packages/@aws-cdk/aws-apigatewayv2/test/http/domain-name.test.ts
Outdated
Show resolved
Hide resolved
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.
Just a few comments here.
Please address comments from previous iteration around naming the method and validations.
Pull request has been modified.
private isDuplicateEndpointType(endpointType: string | undefined) : boolean { | ||
for (let config of this.domainNameConfigurations) { | ||
if (endpointType && endpointType == config.endpointType) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} |
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.
If I'm reading this correctly, this will allow one endpoint type undefined
and one endpoint type of REGIONAL
. But this should not be allowed, right?
Can you fix this up and add a corresponding test case?
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 won't be that way. If you look at the addEndpoint()
method which is the caller for this method, if there is no EndpointType
defined by the customer, we set a default 'REGIONAL' endpoint. So this value will never be undefined. Some eslint code-check complained about it though, that's why I had to add undefined
in the parameter.
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.
You can check the last test I have added. When I call dn.addEndpoint()
, I am not giving an EndpointType
in the input, but the code still figures out that I'm trying to add a REGIONAL endpoint here and throws an error.
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.
Fantastic. Thanks for all the work here! 👏
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…t migration (aws#17518) Updating the L2 construct for `AWS::ApiGatewayV2::DomainName` resource to add support for DomainNameConfigurations. DomainNameConfigurations is a list of configurations for an API's domain name (CFN equivalent - https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigatewayv2-domainname-domainnameconfiguration.html) which includes endpoint type, security policy, ownership certificate. Changes include: - Code update to support the properties mentioned above - Unit test changes for existing tests to account for the updated `domainNameConfigurations` Lazy evaluation. - New unit tests for mutual tls with ownership certificate, and domain name migration that were not present before. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Updating the L2 construct for
AWS::ApiGatewayV2::DomainName
resource to add support for DomainNameConfigurations.DomainNameConfigurations is a list of configurations for an API's domain name (CFN equivalent - https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigatewayv2-domainname-domainnameconfiguration.html) which includes endpoint type, security policy, ownership certificate.
Changes include:
domainNameConfigurations
Lazy evaluation.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license