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

New RLP CRD version #179

Closed
Tracked by #156
eguzki opened this issue May 3, 2023 · 5 comments
Closed
Tracked by #156

New RLP CRD version #179

eguzki opened this issue May 3, 2023 · 5 comments
Assignees
Labels
area/api Changes user facing APIs kind/enhancement New feature or request size/small

Comments

@eguzki
Copy link
Contributor

eguzki commented May 3, 2023

From RFC 0001

Group Version Kind
kuadrant.io v2beta1 RateLimitPolicy

Example with all fields:

apiVersion: kuadrant.io/v2beta1
kind: RateLimitPolicy
metadata: {}
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: <HTTPROUTE-NAME>
  limits:
    <limit-name>:
      routeSelectors:           
      - hostnames:
         - "*example.com"
        matches:                # []HTTPRouteMatch
        - path:                      # HTTPPathMatch 
            type: PathPrefix
            value: "/toys"
          method: GET          # HTTPMethod 
          headers:                # []HTTPHeaderMatch 
          - type: Exact
            name: "X-Canary"
            value: "1"
          queryParams:      # []HTTPQueryParamMatch 
          - type: Exact
            name: "q"
            value: "1"
      rates:
      - limit: 50
        duration: 1
        unit: minute
      counters:
      - auth.identity.username
      - context.request.http.path
      when:
      - selector: auth.identity.group
        operator: neq
        value: admin
@eguzki eguzki self-assigned this May 3, 2023
@eguzki eguzki added kind/enhancement New feature or request target/current size/small area/api Changes user facing APIs labels May 3, 2023
@eguzki eguzki moved this to In Progress in Kuadrant Service Protection May 3, 2023
@guicassolato
Copy link
Contributor

In RFC 0001, we exemplified hostnames at the same level as matches. I think the idea was that matches could be typed as or identical to GWAPI's []HTTPRouteMatch.

I have nothing against bringing hostnames as new field of brought inside of matches, side-by-side with the other fields inherited from GWAPI. However, in that case, we probably don't need the matches level at all. routeSelectors could of type []Selector, where:

type Selector struct {
  Hostnames:   []Hostname            `json:"hostnames,omitempty"`
  Path:        []HTTPPathMatch       `json:"paths,omitempty"`
  Method:      HTTPMethod            `json:"method,omitempty"`
  Headers:     []HTTPHeaderMatch     `json:"headers,omitempty"`
  QueryParams: []HTTPQueryParamMatch `json:"queryParams,omitempty"`
}

Any advantage on keeping matches []HTTPRouteMatch or let's go with the custom Selector type above?

@eguzki
Copy link
Contributor Author

eguzki commented May 3, 2023

. However, in that case, we probably don't need th

That sounds good, because I also believe that routeSelectors matches should be the type of []HTTPRouteMatch

@guicassolato
Copy link
Contributor

I also believe that routeSelectors should be the type of []HTTPRouteMatch

But it wouldn't, right? The two options pitched are:

  1. Using HTTPRouteMatch (like in the RFC):
type RouteSelector struct {
  Hostnames []Hostname
  Matches   []HTTPRouteMatch
}
  1. Without HTTPRouteMatch:
type RouteSelector struct {
  Hostnames:   []Hostname            `json:"hostnames,omitempty"`
  Path:        []HTTPPathMatch       `json:"paths,omitempty"`
  Method:      HTTPMethod            `json:"method,omitempty"`
  Headers:     []HTTPHeaderMatch     `json:"headers,omitempty"`
  QueryParams: []HTTPQueryParamMatch `json:"queryParams,omitempty"`
}

@eguzki
Copy link
Contributor Author

eguzki commented May 3, 2023

Correct, updated previous comment

@eguzki
Copy link
Contributor Author

eguzki commented May 3, 2023

Do we want v2beta1 or v1beta2?

In RFC 0001 v2beta1 is specified. But as we are skipping v1 it seems a long hop IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Changes user facing APIs kind/enhancement New feature or request size/small
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants