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

HTTPRoute matching should support custom match types #832

Closed
youngnick opened this issue Aug 26, 2021 · 3 comments · Fixed by #850
Closed

HTTPRoute matching should support custom match types #832

youngnick opened this issue Aug 26, 2021 · 3 comments · Fixed by #850
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@youngnick
Copy link
Contributor

What would you like to be added:

See the discussion here for more detail (#822 (comment)), but in short:

In GEP-820 (#820 and #822) we are removing the extensionRef in the HTTPRoute matches section. This issue covers work to add something back in that allows implementations to try custom matching of some sort.

@bowei suggested:
Modify HTTPPathMatch.Type:

  • Allow qualified names (e.g. acme.io/my-custom-match). These represent custom match types
  • The standard match types (Exact, Prefix, RegularExpression) stay
  • Remove ImplementationSpecific

Why this is needed:
We agreed to go ahead with #822 on the proviso we come back, this is to make sure this doesn't get lost.

@youngnick youngnick added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 26, 2021
@robscott
Copy link
Member

This was also suggested by @danwinship in #780 (comment).

@hbagdi
Copy link
Contributor

hbagdi commented Aug 30, 2021

While I understand the motivation here, I think we should refrain from adding this. Here is why:

  • Prefix, Exact, Regex, ImplementationSpecific are 4 types. That's enough of a burden we are putting on our users.
  • From a product view, being clever with matches is asking for trouble. I frequently advise users to stay away from fine-grained matching semantics because mistakes are easily made and the blast radius is often larger than one can anticipate. Use another dumber matching criteria when possible.
  • IME, asking the user to further select fine-grained differences at this level of detail doesn't go well. It becomes really hard to communicate the differences and the overall UX suffers quite a bit. At Kong, we introduced one such field and we failed. Even the maintainers have a hard time distinguishing the differences.
  • Although on the surface it may feel wrong to be more opinionated and be restrictive, I think opting for simplicity and clarity is better in this case. Optimizing for esoteric cases at the cost of harming UX for 99% of the users rarely (if ever) gives any positive returns.
  • AFAIK, we do not even have use-cases for this. There should be very good reasons to justify adding this.

@hbagdi
Copy link
Contributor

hbagdi commented Aug 30, 2021

/assign

hbagdi added a commit to hbagdi/service-apis that referenced this issue Sep 3, 2021
ImplementationSpecific is trappy match type: it provides an escape hatch
to do more but it doesn't take into account that multiple
implementation-specific match types could be possible. Use-cases exist
in this area but we have not thought through them deeply.
Being conservative and dropping this feature in v1alpha2 to avoid any potential
breaking change in future. We may revisit this in future as we gain a
better understanding.

For kubernetes-sigs#832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants