aws / aws-app-mesh-controller-for-k8s

A controller to help manage App Mesh resources for a Kubernetes cluster.
Apache License 2.0
187 stars 109 forks source link

GrpcRouteMatch port match error despite defining it #715

Closed conorevans closed 1 year ago

conorevans commented 1 year ago

Describe the bug

Despite defining route.grpcRoute.match.port, I get the reconciler error BadRequestException: Route must define a port match criteria if the Virtual Router has multiple listeners

✗ kubectl describe virtualrouter conor
Name:         conor
Namespace:    mesh
Labels:       <none>
Annotations:  <none>
API Version:  appmesh.k8s.aws/v1beta2
Kind:         VirtualRouter
Metadata:
  Creation Timestamp:  2023-07-13T14:25:28Z
  Finalizers:
    finalizers.appmesh.k8s.aws/aws-appmesh-resources
  Generation:        10
  Resource Version:  73605192
  UID:               09909b96-03d7-4f7b-a079-72626cf2744f
Spec:
  Aws Name:  conor_mesh
  Listeners:
    Port Mapping:
      Port:      63001
      Protocol:  grpc
    Port Mapping:
      Port:      64001
      Protocol:  http
  Mesh Ref:
    Name:  mesh
    UID:   96d2dcce-b5c5-4b59-8ef8-51bb3eaaf008
  Routes:
    Grpc Route:
      Action:
        Weighted Targets:
          Port:  63001
          Virtual Node Ref:
            Name:  conor
          Weight:  100
      Match:
        Port:          63001
        Service Name:  srv.conor.Conor
    Name:              conor-grpc
    Priority:          2
Status:
  Conditions:
    Last Transition Time:  2023-07-13T14:25:28Z
    Status:                True
    Type:                  VirtualRouterActive
  Observed Generation:     7
  Virtual Router ARN:      arn:aws:appmesh:<redacted>:<redacted>:mesh/mesh/virtualRouter/conor_mesh
Events:
  Type     Reason          Age                From           Message
  ----     ------          ----               ----           -------
  Warning  ReconcileError  6s (x36 over 31m)  VirtualRouter  BadRequestException: Route must define a port match criteria if the Virtual Router has multiple listeners.

Steps to reproduce:

Define a router similar to

apiVersion: appmesh.k8s.aws/v1beta2
kind: VirtualRouter
metadata:
  namespace: mesh
  name: conor
spec:
  listeners:
    - portMapping:
        port: 63001
        protocol: grpc
    - portMapping:
        port: 64001
        protocol: http
  routes:
    # - name: conor-http
    #   priority: 1
    #   httpRoute:
    #     action:
    #       weightedTargets:
    #         - virtualNodeRef:
    #             name: conor
    #           weight: 100
    #           port: 64001
    #     match:
    #       prefix: /conor/
    #       port: 64001
    - name: conor-grpc
      priority: 2
      grpcRoute:
        action:
          weightedTargets:
            - virtualNodeRef:
                name: conor
              weight: 100
              port: 63001
        match:
          port: 63001
          serviceName: srv.conor.Conor

and try to apply. Note that if you uncomment the HTTP route that one is created correctly.

Expected outcome The gRPC Route configures successfully

Environment: EKS 1.26

Additional Context:

conorevans commented 1 year ago

@ysdongAmazon I took a look through the codebase here to see if it's not building up the SDK request properly but all looked good to me. I simply can't see why there's an issue here. I can't really debug further as the SDK calls aren't logged - maybe worth adding a flag / env var to the controller to allow for setting the LogLevel of the AWS Config for the AWS Session used to create the AppMesh SDK client

ysdongAmazon commented 1 year ago

you need to create listener ports first and then routes to match all the defined ports. The order is essential. This is necessary because the listener is where the VirtualRouter is looking for incoming connections, while the route specifies how to handle those connections. A strong testament to this is that after I deployed the grpc 63001 port and the corresponding route, when I try to define an additional http 64001 listener without setting the route first, this also fails with: All Routes for this Virtual Router must define a port match before adding multiple listeners to this Virtual Router. 1 Route(s) are missing port. Listing up to 5 route names: [conor-grpc] Listener ports must be defined first and all must be satisfied.

conorevans commented 1 year ago

you need to create listener ports first and then routes to match all the defined ports. The order is essential. This is necessary because the listener is where the VirtualRouter is looking for incoming connections, while the route specifies how to handle those connections. A strong testament to this is that after I deployed the grpc 63001 port and the corresponding route, when I try to define an additional http 64001 listener without setting the route first, this also fails with: All Routes for this Virtual Router must define a port match before adding multiple listeners to this Virtual Router. 1 Route(s) are missing port. Listing up to 5 route names: [conor-grpc] Listener ports must be defined first and all must be satisfied.

I don't disagree with anything you say, but I have a port match.

✗ cat router.yaml
---
apiVersion: appmesh.k8s.aws/v1beta2
kind: VirtualRouter
metadata:
  namespace: mesh
  name: conor
spec:
  listeners:
    - portMapping:
        port: 63001
        protocol: grpc
    # - portMapping:
    #     port: 64001
    #     protocol: http
  routes:
    # - name: conor-http
    #   priority: 1
    #   httpRoute:
    #     action:
    #       weightedTargets:
    #         - virtualNodeRef:
    #             name: conor
    #           weight: 100
    #           port: 64001
    #     match:
    #       path:
    #         regex: /conor.*
    #       port: 64001
    - name: conor-grpc
      priority: 2
      grpcRoute:
        action:
          weightedTargets:
            - virtualNodeRef:
                name: conor
              weight: 100
              port: 63001
        match:
          port: 63001
          serviceName: srv.conor.Conor

So I have one port mapping (63001:grpc) and one route which does define a match.port (it also defines a match.serviceName). See that in kube

✗ k describe virtualrouter conor
Name:         conor
Namespace:    mesh
Labels:       <none>
Annotations:  <none>
API Version:  appmesh.k8s.aws/v1beta2
Kind:         VirtualRouter
Metadata:
  Creation Timestamp:  2023-07-31T14:59:07Z
  Finalizers:
    finalizers.appmesh.k8s.aws/aws-appmesh-resources
  Generation:        1
  Resource Version:  76628516
  UID:               bc3ee383-fd06-410a-891e-cc01f687f00f
Spec:
  Aws Name:  conor_mesh
  Listeners:
    Port Mapping:
      Port:      63001
      Protocol:  grpc
  Mesh Ref:
    Name:  mesh
    UID:   96d2dcce-b5c5-4b59-8ef8-51bb3eaaf008
  Routes:
    Grpc Route:
      Action:
        Weighted Targets:
          Port:  63001
          Virtual Node Ref:
            Name:  conor
          Weight:  100
      Match:
        Port:          63001
        Service Name:  srv.conor.Conor
    Name:              conor-grpc
    Priority:          2
Status:
  Conditions:
    Last Transition Time:  2023-07-31T14:59:08Z
    Status:                True
    Type:                  VirtualRouterActive
  Observed Generation:     1
  Route AR Ns:
    Conor - Grpc:      arn:aws:appmesh:<redacted>:<redacted>:mesh/mesh/virtualRouter/conor_mesh/route/conor-grpc
  Virtual Router ARN:  arn:aws:appmesh:<redacted>:<redacted>:mesh/mesh/virtualRouter/conor_mesh
Events:                <none>

The corresponding listener and route can be seen here

Screenshot 2023-07-31 at 15 59 23 Screenshot 2023-07-31 at 15 59 30

Note in the route definition, that match.serviceName is present, but match.port is empty - this is where I think the bug is in this operator, not properly setting the port in the request sent to AWS

Screenshot 2023-07-31 at 15 59 37

As you said, if I then add a listener, I get that error

✗ k describe virtualrouter conor
Name:         conor
Namespace:    mesh
Labels:       <none>
Annotations:  <none>
API Version:  appmesh.k8s.aws/v1beta2
Kind:         VirtualRouter
Metadata:
  Creation Timestamp:  2023-07-31T14:59:07Z
  Finalizers:
    finalizers.appmesh.k8s.aws/aws-appmesh-resources
  Generation:        2
  Resource Version:  76629151
  UID:               bc3ee383-fd06-410a-891e-cc01f687f00f
Spec:
  Aws Name:  conor_mesh
  Listeners:
    Port Mapping:
      Port:      63001
      Protocol:  grpc
    Port Mapping:
      Port:      64001
      Protocol:  http
  Mesh Ref:
    Name:  mesh
    UID:   96d2dcce-b5c5-4b59-8ef8-51bb3eaaf008
  Routes:
    Grpc Route:
      Action:
        Weighted Targets:
          Port:  63001
          Virtual Node Ref:
            Name:  conor
          Weight:  100
      Match:
        Port:          63001
        Service Name:  srv.conor.Conor
    Name:              conor-grpc
    Priority:          2
Status:
  Conditions:
    Last Transition Time:  2023-07-31T14:59:08Z
    Status:                True
    Type:                  VirtualRouterActive
  Observed Generation:     1
  Route AR Ns:
    Conor - Grpc:      arn:aws:appmesh:<redacted>:<redacted>:mesh/mesh/virtualRouter/conor_mesh/route/conor-grpc
  Virtual Router ARN:  arn:aws:appmesh:<redacted>:<redacted>:mesh/mesh/virtualRouter/conor_mesh
Events:
  Type     Reason          Age               From           Message
  ----     ------          ----              ----           -------
  Warning  ReconcileError  0s (x11 over 6s)  VirtualRouter  BadRequestException: All Routes for this Virtual Router must define a port match before adding multiple listeners to this Virtual Router. 1 Route(s) are missing port. Listing up to 5 route names: [conor-grpc]

but again.. I have defined grpcRoute.match.port so I shouldn't be getting it.

conorevans commented 1 year ago

oh @ysdongAmazon it seems to be same as https://github.com/aws/aws-app-mesh-controller-for-k8s/issues/710

BennettJames commented 1 year ago

Conor - I just published that release as the default (v1.12.2). Let us know if it resolves the issue.

conorevans commented 1 year ago

Thanks @BennettJames that sorted it 👌