Skip to content
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

Closed
wants to merge 7 commits into from
Closed

Conversation

SmritiVashisth
Copy link
Contributor

@SmritiVashisth SmritiVashisth commented Oct 28, 2021

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:

  • Code update to support the properties mentioned above
  • Unit test changes for existing tests to account for the updated property - certificate. It was present in DomainNameProps as a root level property, but it is supposed to be present as part of DomainNameConfigurations. This is a breaking change for existing customers because their old constructs will be invalid after this change.
  • New unit tests for mutual tls that were not present before.

BREAKING CHANGE: Property certificate was present under DomainNameProps earlier and it has been moved to DomainNameConfigurations 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

@gitpod-io
Copy link

gitpod-io bot commented Oct 28, 2021

@github-actions github-actions bot added the @aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2 label Oct 28, 2021
@awsperi2
Copy link

awsperi2 commented Nov 2, 2021

Testing comments

packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
/**
* 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.
Copy link

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

nija-at
nija-at previously requested changes Nov 4, 2021
Copy link
Contributor

@nija-at nija-at left a 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.

packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
readonly endpointType?: EndpointType;

/**
* The ARN of the public certificate issued by ACM to validate ownership of your
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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

Comment on lines 120 to 121
* custom domain. Only required when configuring mutual TLS and using an ACM
* imported or private CA certificate ARN as the RegionalCertificateArn.
Copy link
Contributor

@nija-at nija-at Nov 4, 2021

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.

@mergify mergify bot dismissed nija-at’s stale review November 4, 2021 21:57

Pull request has been modified.

Copy link
Contributor Author

@SmritiVashisth SmritiVashisth left a 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.

Copy link

@awsperi2 awsperi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some more comments

Copy link
Contributor Author

@SmritiVashisth SmritiVashisth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated docs.

@nija-at nija-at changed the title feat: add support for apigatewayv2 domain-name feat(apigatewayv2): http api - domain endpoint type, security policy and mTLS Nov 5, 2021
nija-at
nija-at previously requested changes Nov 5, 2021
packages/@aws-cdk/aws-apigatewayv2/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/README.md Show resolved Hide resolved
/**
* Specifies the configuration for a an API's domain name.
*/
export interface DomainNameConfiguration {
Copy link
Contributor

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"
Copy link
Contributor

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?

packages/@aws-cdk/aws-apigatewayv2/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigatewayv2/README.md Outdated Show resolved Hide resolved
@nija-at
Copy link
Contributor

nija-at commented Nov 5, 2021

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.

@mergify mergify bot dismissed nija-at’s stale review November 9, 2021 22:03

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: ada1f20
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@nija-at nija-at closed this Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigatewayv2 Related to Amazon API Gateway v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants