default dynamic wasm rule data to empty string

[guicassolato]

Sets the empty string as the default value for the counter qualifiers ("wasm data items") in the wasm config, to prevent the selectors that fail to select a value to invalidate the limit.

Ref.: Kuadrant/wasm-shim#39 (comment)

[codecov]

Codecov Report

Merging #232 (9da6e08) into main (0adfe0c) will decrease coverage by 0.81%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
- Coverage   64.01%   63.21%   -0.81%     
==========================================
  Files          33       33              
  Lines        3224     3224              
==========================================
- Hits         2064     2038      -26     
- Misses        990     1009      +19     
- Partials      170      177       +7     

Flag	Coverage Δ
integration	68.48% <ø> (-1.70%)	⬇️
unit	58.41% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	∅ <ø> (∅)
pkg/common (u)	76.99% <ø> (ø)
pkg/istio (u)	29.69% <ø> (ø)
pkg/log (u)	31.81% <ø> (ø)
pkg/reconcilers (u)	33.68% <ø> (ø)
pkg/rlptools (u)	57.63% <100.00%> (ø)
controllers (i)	68.48% <ø> (-1.70%)	⬇️

Files Changed	Coverage Δ
pkg/rlptools/wasm_utils.go	62.81% <100.00%> (ø)

... and 8 files with indirect coverage changes

[eguzki]

What if I set as counter request.headers.My-Custom-Header-01 ? All the requests not having that header will contribute to a single (limitador's internal hit) counter

[guicassolato]

Requests that do not have that header are treated as requests that have that header with an empty value, therefore sharing the same counter.

If what you want is to trigger or not trigger a limit based on the presence or absence of a header, use a condition instead; not a counter qualifier.

[eguzki]

Indeed, what you say is correct, but I do not fully agree on this UX: if you set a counter based on a header, add a condition on the header existence.

For me, adding a counter implicitly tells I only want to rate limit traffic that is qualified by the counter.

Let's see what @didierofrivia and @alexsnaps have to say about this

Let's say we have this RLP

apiVersion: kuadrant.io/v2beta1
kind: RateLimitPolicy
metadata: {}
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: <HTTPROUTE-NAME>
  limits:
    some-name:
      routeSelectors:           
        matches:                
        - path:                    
            type: PathPrefix
            value: "/toys"
      rates:
      - limit: 50
        duration: 1
        unit: minute
      counters:
      - auth.identity.username

What would you expect from this RLP? rate limiting traffic authenticated (hence, the auth.identify.username exists in the metadata) is expected. So far so good. What about traffic not authenticated (hence, the auth.identify.username does not exist in the metadata)?? With this PR approved, the traffic which is not authenticated will be rate limited because the username es "". Without the changes of this PR, if the request is not authenticated, rate limiting will not be triggered.

[guicassolato]

@eguzki, coincidently, this was exactly the example that made us¹ want to default to an empty string in the wasm-shim (what Kuadrant/wasm-shim#39 wants to revert now).

Footnotes

1. Discussed offline with @alexsnaps and @didierofrivia on 2023-07-27, and implemented by @alexsnaps on 2023-07-28. ↩

[eguzki]

Kuadrant/wasm-shim#39 does not revert entirely that commit. Only reverts the behavior when the descriptor is not found in the module generating RLS descriptors. For the conditions, Kuadrant/wasm-shim#39 keeps the changes to allow operators to be meaningful.

[guicassolato]

Kuadrant/wasm-shim#39 does not revert entirely that commit.

I understand that. But, most importantly, I also understand that setting the default value to "", so some RL is enforced, is one approach. In itself, it is nor better, nor worse than setting no default at all. If most people prefers the contrary of what's proposed here, I'm good with that too obviously.

[guicassolato]

So, after some discussion we've decided NOT to move on with this change. The reasoning:

1. Setting an empty string loses information; by neglecting the distinction between empty and null, we're dismissing the meaning of missing data – changing that in the future may be hard
2. Missing a value is an exception case, therefore users should NOT design with that in mind; instead, the behaviour for such case MUST be different – e.g. define other limits for that case, using the conditions, etc
3. Defaulting to a general RL case cannot be the solution to protect against failures (e.g. typos), but to improve the visibility instead (e.g. better logging, debugging tools, etc) – Improve traffic visibility in the data plane wasm-shim#40
4. Consistency with Limitador - in Limitador, a limit is only enforced when the required data is complete; the same principle can apply to the wasm-shim. While the default field of the wasm config is not exposed to users, completeness in the case is controlled by the RLP controller. Let's not make "complete" something that was not set as complete by the user
5. In the future, we can provide an option (e.g. expose the default field to users) for users to define their own desired (safe) intended behaviours when data is missing, e.g. blocking the request altogether (in contrast to sending the traffic to Limitador and let Limitador decide) – Make possible for RLP owners to define defaults when missing well-known values #238