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 timeout support to Routes #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

skiyani
Copy link
Owner

@skiyani skiyani commented Dec 9, 2020

Add support to http/http2/grpc/tpc route timeouts.

Reference links for HttpTimeout, GrpcTimeout, and TcpTimeout. (Http2 uses the HttpTimeout object).

closes aws#11643


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@sshver
Copy link

sshver commented Dec 9, 2020

This looks fine. Just one observation, I feel we could have designed route-spec more like virtual-node-listener . Avoiding the subclasses, more like Adam mentioned is this comment on the PR.
We should also update the README file. Before the pull request to the actual master branch, update the title and description as per the guidelines.

@skiyani
Copy link
Owner Author

skiyani commented Dec 9, 2020

This looks fine. Just one observation, I feel we could have designed route-spec more like virtual-node-listener . Avoiding the subclasses, more like Adam mentioned is this comment on the PR.
We should also update the README file. Before the pull request to the actual master branch, update the title and description as per the guidelines.

Can you be more specific, it looks to follow the same pattern to me.

@sshver
Copy link

sshver commented Dec 9, 2020

I was suggesting something along these line, so that we are not repeating the timeout() and the bind() function. This is just a suggestion. I have not run this code, so I am not sure this piece would work as it is. But this would reduce some lines of code.

export abstract class RouteSpec {
  /**
   * Creates an HTTP Based RouteSpec
   */
  public static http(options: HttpRouteSpecOptions): RouteSpec {
    return new RouteSpecImpl(Protocol.HTTP, options.match, undefined, options.timeout, options.weightedTargets);
  }

  /**
   * Creates an HTTP2 Based RouteSpec
   *
   */
  public static http2(options: HttpRouteSpecOptions): RouteSpec {
    return new RouteSpecImpl(Protocol.HTTP2, options.match, undefined, options.timeout, options.weightedTargets);
  }

  /**
   * Creates a TCP Based RouteSpec
   */
  public static tcp(options: TcpRouteSpecOptions): RouteSpec {
    return new RouteSpecImpl(Protocol.TCP, undefined, undefined, options.timeout, options.weightedTargets);
  }

  /**
   * Creates a GRPC Based RouteSpec
   */
  public static grpc(options: GrpcRouteSpecOptions): RouteSpec {
    return new RouteSpecImpl(Protocol.HTTP, undefined, options.match, options.timeout, options.weightedTargets);
  }

  /**
   * Called when the GatewayRouteSpec type is initialized. Can be used to enforce
   * mutual exclusivity with future properties
   */
  public abstract bind(scope: cdk.Construct): RouteSpecConfig;
}

class RouteSpecImpl extends RouteSpec {

  constructor(private readonly protocol: Protocol, 
    private readonly httpMatch: HttpRouteMatch,
    private readonly grpcMatch: GrpcRouteMatch, 
    private readonly timeout: HttpTimeout,
    public readonly weightedTargets: WeightedTarget[]) {
    super();
  }

  public bind(_scope: cdk.Construct): RouteSpecConfig {
    const prefixPath = this.httpMatch ? this.httpMatch.prefixPath : '/';
    if (prefixPath[0] != '/') {
      throw new Error(`Prefix Path must start with \'/\', got: ${prefixPath}`);
    }
    const httpConfig: CfnRoute.HttpRouteProperty = {
      action: {
        weightedTargets: renderWeightedTargets(this.weightedTargets),
      },
      match: {
        prefix: prefixPath,
      },
      timeout: this.timeout ? this.renderTimeout(this.timeout): undefined,
    };

    const tcpConfig: CfnRoute.TcpRouteProperty = {
      action: {
        weightedTargets: renderWeightedTargets(this.weightedTargets),
      },
      timeout: this.timeout ? this.renderTimeout(this.timeout): undefined,
    };

    const grpcConfig: CfnRoute.GrpcRouteProperty = {
      action: {
        weightedTargets: renderWeightedTargets(this.weightedTargets),
      },
      match: {
        serviceName: this.grpcMatch.serviceName,
      },
      timeout: this.timeout ? this.renderTimeout(this.timeout): undefined,
    };

    return {
      httpRouteSpec: this.protocol === Protocol.HTTP ? httpConfig : undefined,
      http2RouteSpec: this.protocol === Protocol.HTTP2 ? httpConfig : undefined,
      tcpRouteSpec: this.protocol === Protocol.TCP ? tcpConfig : undefined,
      grpcRouteSpec: this.protocol === Protocol.GRPC ? grpcConfig: undefined
    };
  }

  private renderTimeout(timeout: HttpTimeout): CfnRoute.HttpTimeoutProperty {
    return ({
      idle: timeout?.idle !== undefined ? {
        unit: 'ms',
        value: timeout?.idle.toMilliseconds(),
      } : undefined,
      perRequest: timeout?.perRequest !== undefined ? {
        unit: 'ms',
        value: timeout?.perRequest.toMilliseconds(),
      } : undefined,
    });
  }
}

@dfezzie
Copy link

dfezzie commented Dec 9, 2020

This looks fine. Just one observation, I feel we could have designed route-spec more like virtual-node-listener . Avoiding the subclasses, more like Adam mentioned is this comment on the PR.
We should also update the README file. Before the pull request to the actual master branch, update the title and description as per the guidelines.

I like this suggestion, but let's make it a separate PR. No need to do a refactor in a feature commit

@dfezzie
Copy link

dfezzie commented Dec 9, 2020

@sshver feel free to pick it up or create an issue for it

@skiyani skiyani changed the title Support route timeouts feat(appmesh): add timeouts support to Route Dec 10, 2020
@skiyani skiyani changed the title feat(appmesh): add timeouts support to Route feat(appmesh): add timeout support to Route Dec 10, 2020
@skiyani skiyani changed the title feat(appmesh): add timeout support to Route feat(appmesh): add timeout support to Routes Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[appmesh] implement Route Timeout feature
3 participants