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(appmesh): add route retry policies #13353

Merged
merged 15 commits into from
Mar 11, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/@aws-cdk/aws-appmesh/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,40 @@ router.addRoute('route-http', {
});
```

Add an http2 route with retries:

```ts
router.addRoute('route-http2-retry', {
routeSpec: appmesh.RouteSpec.http2({
weightedTargets: [{ virtualNode: node }],
retryPolicy: {
httpRetryEvents: [appmesh.HttpRetryEvent.CLIENT_ERROR],
tcpRetryEvents: [appmesh.TcpRetryEvent.CONNECTION_ERROR],
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
maxRetries: 5,
perRetryTimeout: cdk.Duration.seconds(1),
},
}),
});
```

Add a gRPC route with retries:

```ts
router.addRoute('route-grpc-retry', {
routeSpec: appmesh.RouteSpec.grpc({
weightedTargets: [{ virtualNode: node }],
match: { serviceName: 'servicename' },
retryPolicy: {
grpcRetryEvents: [appmesh.GrpcRetryEvent.DEADLINE_EXCEEDED],
httpRetryEvents: [appmesh.HttpRetryEvent.CLIENT_ERROR],
tcpRetryEvents: [appmesh.TcpRetryEvent.CONNECTION_ERROR],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here - can a GRPC Route retry on HTTP and TCP events?

Copy link
Contributor Author

@misterjoshua misterjoshua Mar 4, 2021

Choose a reason for hiding this comment

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

@skinny85 Same as above for HTTP routes. gRPC is layered on HTTP/2 which is layered on TCP, so these routes can specify retry events for all the lower layers.

maxRetries: 5,
perRetryTimeout: cdk.Duration.seconds(1),
},
}),
});
```

The _RouteSpec_ class provides an easy interface for defining new protocol specific route specs.
The `tcp()`, `http()` and `http2()` methods provide the spec necessary to define a protocol specific spec.

Expand Down
190 changes: 190 additions & 0 deletions packages/@aws-cdk/aws-appmesh/lib/route-spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as cdk from '@aws-cdk/core';
import { CfnRoute } from './appmesh.generated';
import { Protocol, HttpTimeout, GrpcTimeout, TcpTimeout } from './shared-interfaces';
import { IVirtualNode } from './virtual-node';
Expand Down Expand Up @@ -68,6 +69,76 @@ export interface HttpRouteSpecOptions {
* @default - None
*/
readonly timeout?: HttpTimeout;

/**
* The retry policy
* @default - no retry policy
*/
readonly retryPolicy?: HttpRetryPolicy;
}

/**
* HTTP retry policy
*/
export interface HttpRetryPolicy {
/**
* Specify HTTP events on which to retry
* @default - no retries for http events
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that either this, or tcpRetryEvents is required, needs to be stated in the documentation of this property.

*/
readonly httpRetryEvents?: HttpRetryEvent[];

/**
* The maximum number of retry attempts
*/
readonly maxRetries: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just calling this retryAttempts?


/**
* The timeout for each retry attempt
*/
readonly perRetryTimeout: cdk.Duration;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just calling this retryTimeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like that, but I do have a nit. The CDK design guidelines want us to use the terminology of the original service documentation. The CloudFormation calls it PerRetryTimeout (gRPC, HTTP). What do you think given this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think retryTimeout is clear enough to not cause any confusion here.


/**
* TCP events on which to retry. The event occurs before any processing of a
* request has started and is encountered when the upstream is temporarily or
* permanently unavailable.
* @default - no retries for tcp events
*/
readonly tcpRetryEvents?: TcpRetryEvent[];
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that either this, or httpRetryEvents is required, needs to be stated in the documentation of this property.

}

/**
* HTTP events on which to retry.
*/
export enum HttpRetryEvent {
/**
* HTTP status codes 500, 501, 502, 503, 504, 505, 506, 507, 508, 510, and 511
*/
SERVER_ERROR = 'server-error',

/**
* HTTP status codes 502, 503, and 504
*/
GATEWAY_ERROR = 'gateway-error',

/**
* HTTP status code 409
*/
CLIENT_ERROR = 'client-error',

/**
* Retry on refused stream
*/
STREAM_ERROR = 'stream-error',
}

/**
* TCP events on which you may retry
*/
export enum TcpRetryEvent {
/**
* A connection error
*/
CONNECTION_ERROR = 'connection-error',
}

/**
Expand Down Expand Up @@ -107,6 +178,51 @@ export interface GrpcRouteSpecOptions {
* List of targets that traffic is routed to when a request matches the route
*/
readonly weightedTargets: WeightedTarget[];

/**
* The retry policy
* @default - no retry policy
*/
readonly retryPolicy?: GrpcRetryPolicy;
}

/** gRPC retry policy */
export interface GrpcRetryPolicy extends HttpRetryPolicy {
/**
* gRPC events on which to retry
* @default - no retries for gRPC events
*/
readonly grpcRetryEvents?: GrpcRetryEvent[];
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that one of this, tcpRetryEvents or httpRetryEvents is required needs to be stated in the documentation of this property.

}

/**
* gRPC events
*/
export enum GrpcRetryEvent {
/**
* Request was cancelled
*/
CANCELLED = 'cancelled',

/**
* The deadline was exceeded
*/
DEADLINE_EXCEEDED = 'deadline-exceeded',

/**
* Internal error
*/
INTERNAL = 'internal',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call this INTERNAL_ERROR then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong objection here.


/**
* Resource was exhausted
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean? I feel like more explanation is needed here.

Copy link
Contributor Author

@misterjoshua misterjoshua Mar 4, 2021

Choose a reason for hiding this comment

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

I think the error aligns with gRPC code 8/RESOURCE_EXHAUSTED, which per gRPC documentation means, "Some resource has been exhausted, perhaps a per-user quota, or perhaps the entire file system is out of space." I'm not sure if I'm allowed to copy-paste that verbatim from the docs I just referenced - if I'm not, maybe we can say, "A resource has been exhausted." Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding that link to the gRPC documentation to the JSDocs of that field?

Copy link
Contributor Author

@misterjoshua misterjoshua Mar 4, 2021

Choose a reason for hiding this comment

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

Ok no problem. I added @see links to each of the enum values.

*/
RESOURCE_EXHAUSTED = 'resource-exhausted',

/**
* Unavailable
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "unavailable"? The resource, the server we're calling, both, either? Seems like a bigger explanation is needed here.

Copy link
Contributor Author

@misterjoshua misterjoshua Mar 4, 2021

Choose a reason for hiding this comment

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

I believe that it means, "The service is unavailable." I think it's pretty generic, in that it could be both or either, but I'm not sure here. The GrpcRetryPolicy docs don't reveal much and the CFN docs don't even mention the event. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put "service unavailable" then, and a link to that gRPC docs.

*/
UNAVAILABILE = 'unavailable',
}

/**
Expand Down Expand Up @@ -203,19 +319,43 @@ class HttpRouteSpec extends RouteSpec {
*/
public readonly weightedTargets: WeightedTarget[];

/**
* The retry policy
*/
public readonly retryPolicy?: HttpRetryPolicy;

constructor(props: HttpRouteSpecOptions, protocol: Protocol) {
super();
this.protocol = protocol;
this.match = props.match;
this.weightedTargets = props.weightedTargets;
this.timeout = props.timeout;

if (props.retryPolicy) {
if (props.retryPolicy.httpRetryEvents && props.retryPolicy.httpRetryEvents.length === 0) {
throw new Error('Specify at least one value in `httpRetryEvents` or leave it undefined');
}

if (props.retryPolicy.tcpRetryEvents && props.retryPolicy.tcpRetryEvents.length === 0) {
throw new Error('Specify at least one value in `tcpRetryEvents` or leave it undefined');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this validation, and simply return undefined if a customer provided an empty list, in renderHttpRetryPolicy.

No reason to burden the customer with unnecessary errors if we know what to do.

Copy link
Contributor Author

@misterjoshua misterjoshua Mar 4, 2021

Choose a reason for hiding this comment

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

No problemo.


const numHttpRetryEvents = (props.retryPolicy.httpRetryEvents ?? []).length;
const numTcpRetryEvents = (props.retryPolicy.tcpRetryEvents ?? []).length;
if (numHttpRetryEvents + numTcpRetryEvents === 0) {
throw new Error('You must specify one value for at least one of `httpRetryEvents` or `tcpRetryEvents`');
}

this.retryPolicy = props.retryPolicy;
}
}

public bind(_scope: Construct): RouteSpecConfig {
const prefixPath = this.match ? this.match.prefixPath : '/';
if (prefixPath[0] != '/') {
throw new Error(`Prefix Path must start with \'/\', got: ${prefixPath}`);
}

const httpConfig: CfnRoute.HttpRouteProperty = {
action: {
weightedTargets: renderWeightedTargets(this.weightedTargets),
Expand All @@ -224,6 +364,7 @@ class HttpRouteSpec extends RouteSpec {
prefix: prefixPath,
},
timeout: renderTimeout(this.timeout),
retryPolicy: this.retryPolicy ? renderHttpRetryPolicy(this.retryPolicy) : undefined,
};
return {
httpRouteSpec: this.protocol === Protocol.HTTP ? httpConfig : undefined,
Expand Down Expand Up @@ -266,11 +407,40 @@ class GrpcRouteSpec extends RouteSpec {
public readonly match: GrpcRouteMatch;
public readonly timeout?: GrpcTimeout;

/**
* The retry policy.
*/
public readonly retryPolicy?: GrpcRetryPolicy;

constructor(props: GrpcRouteSpecOptions) {
super();
this.weightedTargets = props.weightedTargets;
this.match = props.match;
this.timeout = props.timeout;

if (props.retryPolicy) {
if (props.retryPolicy.grpcRetryEvents && props.retryPolicy.grpcRetryEvents.length === 0) {
throw new Error('Specify at least one value in `grpcRetryEvents` or leave it undefined');
}

if (props.retryPolicy.httpRetryEvents && props.retryPolicy.httpRetryEvents.length === 0) {
throw new Error('Specify at least one value in `httpRetryEvents` or leave it undefined');
}

if (props.retryPolicy.tcpRetryEvents && props.retryPolicy.tcpRetryEvents.length === 0) {
throw new Error('Specify at least one value in `tcpRetryEvents` or leave it undefined');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here (get rid of this validation, and return undefined for an empty list in renderGrpcRetryPolicy ).

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. No problem.


const numGrpcRetryEvents = (props.retryPolicy.grpcRetryEvents ?? []).length;
const numHttpRetryEvents = (props.retryPolicy.httpRetryEvents ?? []).length;
const numTcpRetryEvents = (props.retryPolicy.tcpRetryEvents ?? []).length;

if (numGrpcRetryEvents + numHttpRetryEvents + numTcpRetryEvents === 0) {
throw new Error('You must specify one value for at least one of `grpcRetryEvents`, `httpRetryEvents`, `tcpRetryEvents`');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You must specify one value for at least one of 'grpcRetryEvents', 'httpRetryEvents' or 'tcpRetryEvents'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I've pushed up a fix for this.

}

this.retryPolicy = props.retryPolicy;
}
}

public bind(_scope: Construct): RouteSpecConfig {
Expand All @@ -283,6 +453,7 @@ class GrpcRouteSpec extends RouteSpec {
serviceName: this.match.serviceName,
},
timeout: renderTimeout(this.timeout),
retryPolicy: this.retryPolicy ? renderGrpcRetryPolicy(this.retryPolicy) : undefined,
},
};
}
Expand Down Expand Up @@ -323,3 +494,22 @@ function renderTimeout(timeout?: HttpTimeout): CfnRoute.HttpTimeoutProperty | un
}
: undefined;
}

function renderHttpRetryPolicy(retryPolicy: HttpRetryPolicy): CfnRoute.HttpRetryPolicyProperty {
return {
maxRetries: retryPolicy.maxRetries,
perRetryTimeout: {
unit: 'ms',
value: retryPolicy.perRetryTimeout.toMilliseconds(),
},
httpRetryEvents: retryPolicy.httpRetryEvents,
tcpRetryEvents: retryPolicy.tcpRetryEvents,
};
}

function renderGrpcRetryPolicy(retryPolicy: GrpcRetryPolicy): CfnRoute.GrpcRetryPolicyProperty {
return {
...renderHttpRetryPolicy(retryPolicy),
grpcRetryEvents: retryPolicy.grpcRetryEvents,
};
}
Loading