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

matches in HTTPRouteRule should allow more than 8 entries #1485

Closed
Amila-Rukshan opened this issue Oct 28, 2022 · 7 comments
Closed

matches in HTTPRouteRule should allow more than 8 entries #1485

Amila-Rukshan opened this issue Oct 28, 2022 · 7 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@Amila-Rukshan
Copy link
Contributor

What would you like to be added:

When defining more than 8 HTTPRouteMatch for the matches field in HTTPRouteRule, it returns the following error message. We would like to have the support for allowing more than 8 items for a match.

The HTTPRoute "petstore" is invalid: spec.rules[0].matches: Too many: 11: must have at most 8 items

Why this is needed:

It is possible to have more than 8 matches goes to the same backend service and they should be defined under a single HTTPRouteRule. Consider the example given below:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: demo-petstore
  labels:
    version: 1.0.1
spec:
  parentRefs:
  - name: eg
  hostnames:
  - www.example.com
  rules:
  - matches:
    - path:
        type: Exact
        value: /api/v3/pet/{petId}/uploadImage
      method: POST
    - path:
        type: Exact
        value: /api/v3/pet
      method: POST
    - path:
        type: Exact
        value: /api/v3/pet
      method: PUT   
    - path:
        type: Exact
        value: /api/v3/pet/findByStatus
      method: GET
    - path:
        type: Exact
        value: /api/v3/pet/findByTags
      method: GET
    - path:
        type: Exact
        value: /api/v3/pet/{petId}
      method: GET
    - path:
        type: Exact
        value: /api/v3/pet/{petId}
      method: POST
    - path:
        type: Exact
        value: /api/v3/pet/{petId}
      method: DELETE
    - path:
        type: Exact
        value: /api/v3/store/order
      method: POST
    - path:
        type: Exact
        value: /api/v3/store/order/{orderId}
      method: GET
    - path:
        type: Exact
        value: /api/v3/store/order/{orderId}
      method: DELETE
    - path:
        type: Exact
        value: /api/v3/store/inventory
      method: GET
    - path:
        type: Exact
        value: /api/v3/user/createWithArray
      method: POST
    - path:
        type: Exact
        value: /api/v3/user/createWithList
      method: POST
    - path:
        type: Exact
        value: /api/v3/user/{username}
      method: PUT
    backendRefs:
        - name: foo-svc
          port: 8080
@gyohuangxin
Copy link
Member

Thanks for creating this issue, I think it caused by the limitation here:
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/httproute_types.go#L178

I also find other items also have the limitations, why did we set them in the first place?

@danehans
Copy link
Contributor

why did we set them in the first place

Applying validation rules to the API schema is good practice and consistent with Kube API conventions. The validation rule's value was determined based on general consensus from the community (xref). Maybe the maintainers can reconsider the current maxItems value. In the meantime, can you create multiple matching rules or collapse the rules using RegularExpression or PathPrefix match types? Maybe the community would consider pluralizing method to help reduce duplicative rules.

@youngnick
Copy link
Contributor

youngnick commented Nov 7, 2022

@danehans is correct about the reasons for having a limit - not having a limit means that you can get weird behavior once you have a large object, if the object gets big enough, it will not be saved in etcd correctly, so we limit the number of items in each field.

@Amila-Rukshan, a couple of things about the YAML you provided:

  • Exact path matching means what it says, so from what I can see, the bracketed variables (?) should not be captured, those paths should match a literal {variable}, not capture the variable.
  • I agree that it seems reasonable for us to consider if having a methods field as well as method would be a good idea (or doing a migration and deprecation of the singular method field maybe).
  • Are you considering autogenerating HTTPRoute resources from Swagger sources? These look like they match up with the https://petstore3.swagger.io/ example. We haven't really looked at this style of more API Gateway/WAF usage before.

@youngnick youngnick added the triage/needs-information Indicates an issue needs more information in order to work on it. label Nov 8, 2022
@Amila-Rukshan
Copy link
Contributor Author

Amila-Rukshan commented Nov 10, 2022

@youngnick,
yes, this example from https://petstore3.swagger.io/.

thanks for the clarification!

@Amila-Rukshan
Copy link
Contributor Author

@youngnick, can I know how these limits are calculated / decided? any reference for that?

I'm thinking the possibility to adjust this value to some higher value.

Thanks!

@youngnick
Copy link
Contributor

youngnick commented Nov 14, 2022

They're an arbitrary choice, and we chose a small number because the matches list is already part of another list (the rules list), so a small increase in each has a big combinatorial impact on the size.

I'm worried that once we start increasing it, it will get harder to say no to further increases, and we will end up in a bad spot.

I'd really like work to try and make some guidelines for autogenerating Routes from Swagger entries so that maybe we can limit the size of each HTTPRoute. (Maybe each endpoint gets a HTTPRoute? That would already cut down on the number of matches substantially.)

@robscott
Copy link
Member

robscott commented Dec 2, 2022

Hey @Amila-Rukshan, as Nick mentioned, we really need to enforce some kind of max limit here otherwise the potential size of these resources could get pretty overwhelming. These matches are already within a list with a max length of 16:

// +kubebuilder:validation:MaxItems=16
// +kubebuilder:default={{matches: {{path: {type: "PathPrefix", value: "/"}}}}}
Rules []HTTPRouteRule `json:"rules,omitempty"`

That means that each HTTPRoute can specify 128 (16*8) unique matches. Within each of those match structs, we can configure as many as 16 header matches:

// +kubebuilder:validation:MaxItems=16
Headers []HTTPHeaderMatch `json:"headers,omitempty"`

That means each Route can configure 2048 (16*128) unique header matches. If we were to double the number of matches we allow per Route rule, we'd also double all these other values that are already quite high. Implementations need some kind of upper limit to test against here.

In retrospect it likely would have been better to have a list of method matches instead of an individual string, but that's a complicated/messy change to make at this point. I believe these kinds of situations where >8 matches are required are likely to be uncommon edge cases that can already be solved by adding an additional Route rule. I'd recommend starting there. Unfortunately I don't think we can justify increasing any limits here.

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. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

5 participants