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

Feature Request: Allow inclusive-or HTTP match conditions #123

Open
stefanprodan opened this issue Oct 15, 2019 · 11 comments
Open

Feature Request: Allow inclusive-or HTTP match conditions #123

stefanprodan opened this issue Oct 15, 2019 · 11 comments
Assignees
Labels
Roadmap: Accepted We are planning on doing this work.

Comments

@stefanprodan
Copy link

stefanprodan commented Oct 15, 2019

Allow inclusive-or HTTP match conditions. e.g. route Safari users or users with an insider cookie to the canary:

routes:
- http:
    action:
      weightedTargets:
      - virtualNodeName: podinfo-canary
        weight: 100
    match:
      - prefix: "/"
        headers:
        - name: cookie
          match:
            regex: "^(.*?;)?(type=insider)(;.*)?$"    
      - prefix: "/"
        headers:
        - name: user-agent
          match:
            regex: "(?=.*Safari)(?!.*Chrome).*$"
@jaypipes
Copy link

The above match instead of doing an OR does an AND for all conditions. It should not be the equivalent of:

  routes:
  - http:
      action:
        weightedTargets:
        - virtualNodeName: podinfo-canary
          weight: 100
      match:
        headers:
        - match:
            regex: .*blue.*
          name: blue
            regex: .*green.*
          name: green
        prefix: /

I don't believe the above is actually valid YAML? The headers field is a list of objects, and there is a single object with two fields called name. Were you instead looking for something like this?

      match:
        headers:
        matchAny:
            - regex: .*blue.*
              name: blue
            - regex: .*green.*
              name: green
        prefix: /

or, alternately, I suppose you could do an alternator regex expression?

  routes:
  - http:
      action:
        weightedTargets:
        - virtualNodeName: podinfo-canary
          weight: 100
      match:
        headers:
        - match:
            regex: .*(blue|green).*
            name: blue-or-green

@stefanprodan
Copy link
Author

The alternator doesn't work for my use case, for example I want to route Safari users or clients with an insider cookie to the canary:

With Istio this looks like this (it does an OR between conditions):

    match:
      - headers:
          cookie:
            regex: "^(.*?;)?(type=insider)(;.*)?$"
      - headers:
          user-agent:
            regex: "(?=.*Safari)(?!.*Chrome).*$"

I am trying to achieve the same thing with App Mesh but I don't see how it's possible since match accepts a single headers condition:

      match:
        headers:
        - match:
            regex: ^(.*?;)?(type=insider)(;.*)?$
          name: cookie
        - match:
            regex: (?=.*Safari)(?!.*Chrome).*$
          name: user-agent

@stefanprodan
Copy link
Author

@jaypipes I'll reword the issue and make it a feature request.

@stefanprodan stefanprodan changed the title HTTP match headers bug when using multiple conditions Allow multiple HTTP match headers conditions for a single route Oct 15, 2019
@dfezzie dfezzie transferred this issue from aws/aws-app-mesh-controller-for-k8s Oct 15, 2019
@dfezzie
Copy link

dfezzie commented Oct 15, 2019

Thanks @stefanprodan. I've gone ahead and transferred this issue to aws/app-mesh-roadmap since this is a general App Mesh feature request. In the future can you open feature requests here in this repository? Thanks again 👍

@lavignes
Copy link

lavignes commented Oct 15, 2019

Right now header matching is pretty much 1:1 with Envoy's API: https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/route/route.proto.html?highlight=strip%20headers#envoy-api-field-route-routematch-headers

It doesn't support any sort of inclusive-or matching right now. Perhaps we could expose some API to express inclusive-or matches at the route-level:

route:
  orMatch:
    - prefix: /
      headers:
      - name: foo
        match:
          value: bar
    - prefix: /
      headers:
      - name: baz
        match:
          value: qux

which could just be a short-hand for multiple identical routes. This is just me spit-balling. We'd need to think carefully about the ergonomics of this.

@stefanprodan stefanprodan changed the title Allow multiple HTTP match headers conditions for a single route Allow inclusive-or HTTP match conditions Oct 15, 2019
@stefanprodan
Copy link
Author

stefanprodan commented Oct 15, 2019

@lavignes I think making the match an array of conditions would solve the issue without introducing operators like orMatch.

E.g. target macOS users with a canary=enabled cookie or those with a X-Canary header:

routes:
- http:
    action:
      weightedTargets:
      - virtualNodeName: podinfo-canary
        weight: 100
    match:
    - prefix: /
      headers:
      - name: user-agent
        match:
          regex: ".*Macintosh.*"
      - name: cookie
        match:
          regex: "^(.*?;)?(canary=enabled)(;.*)?$"    
    - prefix: /
      headers:
      - name: x-canary
        match:
          exact: "enabled"

The above configuration would result in two Envoy routes.

@lavignes
Copy link

lavignes commented Oct 15, 2019

@stefanprodan Unfortunately, match is currently defined as an object in our API, not an array. Changing this has implications that affect our SDKs. So adding a new field is generally preferable from an api-compatibility perspective. Regardless, we appreciate this feature suggestion. I do see this as a situation that many people may run into.

@stefanprodan
Copy link
Author

If I had to make such a change without breaking backwards compatibility in App Mesh CRD, I would create a type that accepts both an object or an array of objects e.g. MatchOrMatchArray

openAPIV3Schema:
  properties:
    spec:
      properties:
        match:
          description: A HTTP match condition or array of conditions
          anyOf:
            - type: object
              properties:
                prefix:
                  description: URL prefix
                  type: string
            - type: array
              properties:
                items:
                  type: object
                  properties:
                    prefix:
                      description: URL prefix
                      type: string

I think this could easily be handled in the Virtual Service CRD, the App Mesh Kubernetes controller could take an array of conditions and create a route for each one in the App Mesh API.

@kiranmeduri kiranmeduri changed the title Allow inclusive-or HTTP match conditions Feature Request: Allow inclusive-or HTTP match conditions Oct 18, 2019
@kiranmeduri
Copy link

Given that this is generally useful, I feel App Mesh API should support this directly.

@dastbe dastbe added Phase: Researching Roadmap: Accepted We are planning on doing this work. labels Oct 23, 2019
@mic-kul
Copy link

mic-kul commented Oct 28, 2019

OR condition would be also very useful for Cloud Map tags matching. Support for this would enable to use App Mesh in the following scenario:

We are running shared integration environment, and we want to let developers test microservices they're developing. By specifying DISCOVERY_TAG, we want to enable them to deploy only microservices they're working on and fallback to defaults for other (for example: fallback to staging instances).

I'm currently spiking App Mesh with Cloud Map and ECS and ran into issue - and I'm not sure if my use case is best fit for this platform. What I'm trying to achieve is to route traffic to nodes with specified tags (attributes in Cloud Map), and define fallback nodes that will receive traffic if nothing was matched:

For service with:

  • VirtualRoute_1: weight 1, matching prefix /, VirtualNode_1 with AWS Cloud Map attributesDISCOVERY_TAG: integration

  • VirtualRoute_2: weight 2, matching prefix /, VirtualNode_2 (without any conditions on AWS Cloud Map attributes)

I'm getting no healthy upstream. It now makes total sense, that first it matched Route, evaluated Nodes and returned 'no healthy upstreams'.

Instead, I would like to see VirtualNode_2 to reply when there are no healthy services in VirtualNode_1.

@lavignes
Copy link

Hi @mic-kul. Thanks for this example. I think this is also related to another feature idea we've had: Being able to select optional Cloud Map attributes on virtual nodes (#129). I just opened the issue so feel free to chime in if you think it could also be useful for your use-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Roadmap: Accepted We are planning on doing this work.
Projects
None yet
Development

No branches or pull requests

9 participants