-
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(cloudfront): support gRPC for distribution #32535
base: main
Are you sure you want to change the base?
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32535 +/- ##
==========================================
+ Coverage 78.80% 78.85% +0.04%
==========================================
Files 108 108
Lines 7159 7165 +6
Branches 1319 1319
==========================================
+ Hits 5642 5650 +8
+ Misses 1332 1330 -2
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more.
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
if (![HttpVersion.HTTP2, HttpVersion.HTTP2_AND_3].includes(this.httpVersion)) { | ||
throw new Error(`'httpVersion' must be HttpVersion.HTTP2 or HttpVersion.HTTP2_AND_3 if 'enableGrpc' in 'defaultBehavior' or 'additionalBehaviors' is true, got ${this.httpVersion}`); | ||
} |
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.
Otherwise CFn will fail:
Resource handler returned message: "Invalid request provided: AWS::CloudFront::Distribution: The parameter gRPC only supports HTTP/2. (Service: CloudFront, Status Code: 400, Request ID:...
private renderCacheBehaviors(): CfnDistribution.CacheBehaviorProperty[] | undefined { | ||
if (this.additionalBehaviors.length === 0) { return undefined; } | ||
return this.additionalBehaviors.map(behavior => behavior._renderBehavior()); | ||
return this.additionalBehaviors.map(behavior => { | ||
this.validateGrpc(behavior); | ||
return behavior._renderBehavior(); | ||
}); |
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 even one behavior has gRpc with invalid settings such as httpVersion: cloudfront.HttpVersion.HTTP3
, an error will occur, so check each behavior.
The example is the case for gRPC with a httpVersion not including HTTP2:
new cloudfront.Distribution(stack, 'TestDistribution', {
httpVersion: cloudfront.HttpVersion.HTTP3,
defaultBehavior: {
origin,
allowedMethods: cloudfront.AllowedMethods.ALLOW_ALL,
// enableGrpc: true,
},
additionalBehaviors: {
'/second': {
origin,
allowedMethods: cloudfront.AllowedMethods.ALLOW_ALL,
// enableGrpc: true,
},
'/third': {
origin,
allowedMethods: cloudfront.AllowedMethods.ALLOW_ALL,
enableGrpc: true,
},
},
});
if (props.edgeLambdas !== undefined && props.edgeLambdas.length > 0) { | ||
throw new Error('\'edgeLambdas\' cannot be specified if \'enableGrpc\' is 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.
Otherwise CFn will fail
Resource handler returned message: "Invalid request provided: AWS::CloudFront::Distribution: The parameter gRPC doesn't support Lambda@Edge. (Service: CloudFront, Status Code: 400, Request ID:...
if (props.allowedMethods !== AllowedMethods.ALLOW_ALL) { | ||
throw new Error('\'allowedMethods\' can only be AllowedMethods.ALLOW_ALL if \'enableGrpc\' is 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.
Otherwise CFn will fail:
Resource handler returned message: "Invalid request provided: AWS::CloudFront::Distribution: The parameter gRPC only supports the POST method. (Service: CloudFront, Status Code: 400, Request ID: ...
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 don't write like the following:
throw new Error(`'allowedMethods' can only be AllowedMethods.ALLOW_ALL if 'enableGrpc' is true, got: ${props.allowedMethods?.methods.join(', ')}`);
Because the AllowedMethods.methods
is an array type and the message will not be intuitive.
export class AllowedMethods {
/** HEAD and GET */
public static readonly ALLOW_GET_HEAD = new AllowedMethods(['GET', 'HEAD']);
/** HEAD, GET, and OPTIONS */
public static readonly ALLOW_GET_HEAD_OPTIONS = new AllowedMethods(['GET', 'HEAD', 'OPTIONS']);
/** All supported HTTP methods */
public static readonly ALLOW_ALL = new AllowedMethods(['GET', 'HEAD', 'OPTIONS', 'PUT', 'PATCH', 'POST', 'DELETE']);
/** HTTP methods supported */
public readonly methods: string[];
private constructor(methods: string[]) { this.methods = methods; }
}
Otherwise, if the user specifies AllowedMethods.ALLOW_GET_HEAD_OPTIONS
, the following message will be displayed:
'allowedMethods' can only be AllowedMethods.ALLOW_ALL if 'enableGrpc' is true, got: 'GET', 'HEAD', 'OPTIONS'
this.grpcEnabled = props.enableGrpc; | ||
|
||
if (this.grpcEnabled) { | ||
if (props.allowedMethods !== AllowedMethods.ALLOW_ALL) { |
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.
undefined
for allowedMethods
is also be bad because the default value is AllowedMethods.ALLOW_GET_HEAD
.
@@ -22,10 +22,21 @@ export interface CacheBehaviorProps extends AddBehaviorOptions { | |||
* CloudFrontWebDistribution implementation. | |||
*/ | |||
export class CacheBehavior { | |||
public readonly grpcEnabled?: boolean; |
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.
Exposed to validate settings with gRPC in Distribution
construct.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #32534.
Reason for this change
CloudFormation supports
GrpcConfig
property to enable gRPC inCacheBehavior
andDefaultCacheBehavior
.https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-grpcconfig.html
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-cachebehavior.html
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-distribution-defaultcachebehavior.html
So it would be good to enable gRPC for CloudFront Distribution using L2.
Description of changes
Add
enableGrpc
property inBehaviorOptions
.Description of how you validated changes
Both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license