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

RLP v2 CRD #180

Merged
merged 4 commits into from
May 24, 2023
Merged

RLP v2 CRD #180

merged 4 commits into from
May 24, 2023

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented May 5, 2023

Fixes #179

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 requested a review from a team as a code owner May 5, 2023 13:36
requests:
cpu: 200m
memory: 200Mi
- command:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

@eguzki
Copy link
Contributor Author

eguzki commented May 5, 2023

// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
type WhenCondition struct {
// Selector defines one item from the well known selectors
// TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those well known selectors need to be properly documented

// +kubebuilder:validation:MaxLength=253
type ContextSelector string

// +kubebuilder:validation:Enum:=eq;neq;incl;excl;matches
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For RLP, I thought we were going with only eq and neq for now, so nothing needs to change in Limitador.

Suggested change
// +kubebuilder:validation:Enum:=eq;neq;incl;excl;matches
// +kubebuilder:validation:Enum:=eq;neq

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not necessarily when conditions are going to be applied in Limitador. The wasm-shim can enforce those rules as well to save Limitador's traffic.

But It's ok to start with only eq and neq for now. We can extend those later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a debate for another forum, when discussing the attributions of the wasm-whim rather...

You are right about the wasm-shim technically being capable of enforcing all kinds of user-defined conditions (route related or non-route related) if we want it to. After all, it's all data the wasm-shim will need to request anyway to build the descriptors for the RL request.

I personally have always thought on route-related conditions evaluated by the wasm-shim and translated into one single descriptor to be matched by one single Limitador condition, which is the one with the identifier of the limit; and then conditions not related to route attributes, transparently passed along by the wasm-shim in the RL request and evaluated only by Limitador. But it doesn't have to be this way.

What I think you're suggesting as alternative is a bit bolder and could/would make the wasm-shim the solely evaluator of user-defined conditions effectively. Perhaps saving requests to Limitador is enough reason to do that.

Of course, requests to Limitador can only be saved when none of the limits have matching conditions, after all it takes only one single limit definition to have a matching condition for the wasm-shim to have to call Limitador. But perhaps the wasm-shim could be more selective and inject less descriptors when building requests to Limitador – i.e., only the ones for matching limit identifiers, because other conditions were all evaluated upfront by the wasm-shim itself and the limits are known to have to be triggered?

So less requests to Limitador and less conditions evaluated by Limitador in the cases where a RL request is unavoidable. 🤔

Comment on lines 35 to 38
// TODO Document properly "Well-known selector" https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
type ContextSelector string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we already have some identified, maybe we could enum them ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the list is too big and we also do not want to limit to a set of options, do we? It is all about a context with attributes that can be added dynamically.

Right, @guicassolato ? BTW, do we know if this is documented? do we have a link?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is documented, but it isn't. Perhaps the closest thing we've got is https://github.com/Kuadrant/authorino/blob/main/docs/architecture.md#the-authorization-json, combined with https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#envoy-v3-api-msg-service-auth-v3-checkrequest. What's not explicitly there is because it's really an open struct. But we do need a proper doc explaining it, with examples and all.

In the case of Authorino, the selectors include this as well: https://github.com/Kuadrant/authorino/blob/main/docs/features.md#common-feature-json-paths-valuefromauthjson

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding as well https://github.com/Kuadrant/architecture/blob/main/rfcs/0001-rlp-v2.md#well-known-selectors, which is already in a comment below and may be the best doc we had on the topic so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Envoy's Ext-Authz CheckRequest.AttributeContext type context may not be the same a wasm filter (or any filter) may access to.

This is promising doc: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes

@eguzki eguzki requested a review from a team May 9, 2023 15:09

var (
// GroupVersion is group version used to register these objects
GroupVersion = schema.GroupVersion{Group: "kuadrant.io", Version: "v2beta1"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it shouldn't be v1beta2, provided v1beta1 may never become v1. WDYT?

#179 (comment)

Selector ContextSelector `json:"selector"`

// The binary operator to be applied to the content fetched from the selector
// Possible values are: "eq" (equal to), "neq" (not equal to), "incl" (includes; for arrays), "excl" (excludes; for arrays), "matches" (regex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of these were removed.


// The value of reference for the comparison.
// If used with the "matches" operator, the value must compile to a valid Golang regex.
Value string `json:"value"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authorino conditions support only string comparisons too. I've been wondering lately if we shouldn't try to support any JSON/YAML value type. In the future, this may be helpful for numeric operators (e.g. greater than, lower than, etc), array lookups, etc. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. For now, as the condition operator is either eq or neq, I would stick to literal (string).

// Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
// +optional
Hostnames []gatewayapiv1alpha2.Hostname `json:"hostnames,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means a validation (which I like!) against the following pattern:

^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$

Only to highlight that the example provided in the description of the PR (*example.com) is not valid.

Copy link
Contributor Author

@eguzki eguzki May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! I will change it. (I think it was a typo anyway)

@eguzki
Copy link
Contributor Author

eguzki commented May 17, 2023

ready for a new review @guicassolato

@eguzki eguzki requested a review from a team May 18, 2023 15:04
Comment on lines +176 to +177
if r.Spec.TargetRef.Kind != gatewayapiv1alpha2.Kind("HTTPRoute") && r.Spec.TargetRef.Kind != gatewayapiv1alpha2.Kind("Gateway") {
return fmt.Errorf("invalid targetRef.Kind %s. The only supported kind types are HTTPRoute and Gateway", r.Spec.TargetRef.Kind)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so, we will still only cover the usage of HTTPRoute and Gateway. I thought for this v2 we were thinking on adding GatewayClass.. not sure about the VirtualService

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, it is only HTTPRoute. #156

}

if r.Spec.TargetRef.Namespace != nil && string(*r.Spec.TargetRef.Namespace) != r.Namespace {
return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be informative if we show in the error the valid namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the current one. The rate limit policy's namespace.

Anyway, this is only about the CRD we can improve that in coming PR's with actual implementation

Comment on lines 145 to 146
diff := cmp.Diff(string(currentMarshaledJSON), string(otherMarshaledJSON))
logger.V(1).Info("Conditions not equal", "difference", diff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth saving the cost of cmp.Diff if !logger.V(1).Enabled()?

}

// TODO(eastizle): reflect.DeepEqual does not work well with lists without order
if !reflect.DeepEqual(s.GatewaysRateLimits, other.GatewaysRateLimits) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if order matters. If it does matter, do we want to enforce same order? I'm guessing "yes, we do", right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the GatewaysRateLimits struct from the status section of the CRD. I guess this is still a valid feature, but needs to be refactored entirely. We can add items to the status later when we implement the feature. Does it sound good to you?

@eguzki eguzki merged commit b314383 into rlp-v2-base May 24, 2023
@eguzki eguzki deleted the crd branch May 24, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants