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(router) add header regex matching method #6079

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

vanhtuan0409
Copy link
Contributor

Summary

Add header regex matching method for routing decision. Based on this issue created before

#5666

Full changelog

For avoiding behavior changes, only header with 1 value and prefixed with ~* is checked against re_find

Unit test added as

  -- 15. headers (regex)
  {
    service = service,
    route = {
      headersRegex = true,
      headers = {
        user_agent = {
          "~*windows|linux|os\\s+x\\s*[\\d\\._]+|solaris|bsd",
        },
      },
    },
  },

Issues resolved

Fix #5666

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2020

CLA assistant check
All committers have signed the CLA.

@vanhtuan0409 vanhtuan0409 changed the title Add header regex matching method feat(core) Add header regex matching method Jul 3, 2020
@vanhtuan0409 vanhtuan0409 changed the title feat(core) Add header regex matching method feat(router) add header regex matching method Jul 3, 2020
@bizhenchao1201
Copy link

Great!

@sloanshang
Copy link

@vanhtuan0409 Whether this modification supports header-based regular expressions, whether it has been tested on Kubernetes and VM, and if not, whether we have considered using plugins to handle header-based regular expressions?

@kikito kikito force-pushed the header-regex-match branch 2 times, most recently from 519a335 to 966ed86 Compare February 3, 2022 22:27
@hbagdi
Copy link
Member

hbagdi commented Feb 7, 2022

Some questions:
What if previously configured values have "*" in the value?
What if someone wants to do header matching based on the literal string "
*"?

And then finally, the Route entity has "regex_priority", historically used for distinguishing path regex matches and having a priority amongst them. Users could get confused now (renaming the property is not an option since it is a breaking change).
But, do header matches have a similar problem (as paths) where the regex_priority field could be helpful?

@kikito
Copy link
Member

kikito commented Feb 7, 2022

What if previously configured values have "*" in the value?

Those will still work. This only gets activated when there's only one rule, and the rule starts with ~*

This begs the question: what if someone has exactly that setup: someone who has a route which matches headers that exactly contain a single value that begins with ~*, like My-Header: ~*potato.

My answer to that is: we would break things for that particular person. So this change is technically backwards-breaking.

At the same time I think the odds of really impacting someone because of this are really astronomical. Matching by non-host header is already uncommon. The combination of unusual prefix + only one header really doesn't worry me.

And then finally, the Route entity has "regex_priority", historically used for distinguishing path regex matches and having a priority amongst them. Users could get confused now (renaming the property is not an option since it is a breaking change).
But, do header matches have a similar problem (as paths) where the regex_priority field could be helpful?

The place where it would he helpful would be if someone needs to prioritize one header regex over a different header regex in a different route. I think that can be added later as an extra feature, if someone requests it. Perhaps in 3.0 we will come up with something better than regex_priority.

@hbagdi
Copy link
Member

hbagdi commented Feb 9, 2022

My answer to that is: we would break things for that particular person. So this change is technically backwards-breaking.

At the same time I think the odds of really impacting someone because of this are really astronomical. Matching by non-host header is already uncommon. The combination of unusual prefix + only one header really doesn't worry me.

I get that practically this is okay today but I'll always be hesitant of introducing a change that limits the capability of the product.

The place where it would he helpful would be if someone needs to prioritize one header regex over a different header regex in a different route.

That may result in a breaking change unless we say something along the lines "in case of multiple routes with regex header values, the precedence order is undefined".

@kikito
Copy link
Member

kikito commented Feb 10, 2022

limits the capability of the product.

I understand the concern and I thank you for reviewing this. I think as it is this PR is good enough to merge.

That may result in a breaking change unless we say something along the lines "in case of multiple routes with regex header values, the precedence order is undefined".

That is a good point. I will make sure to include that in the docs.

We have run several performance tests for this branch and confirmed the changes had no significant performance impact, so I am merging it.

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.

Route match by header regex
7 participants