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

[state-of-the-world reconciler] Wasm config #822

Closed
guicassolato opened this issue Aug 16, 2024 · 8 comments · Fixed by #952
Closed

[state-of-the-world reconciler] Wasm config #822

guicassolato opened this issue Aug 16, 2024 · 8 comments · Fixed by #952
Assignees

Comments

@guicassolato
Copy link
Contributor

guicassolato commented Aug 16, 2024

Reconciliation of the wasm config for the auth and RL effective policies.

Each path in the topology graph (Gateway → Gateway Listener → HTTPRoute → HTTPRouteRule) and additional top-level conditions, expressed in the form of a predicate (CEL expression), induces a call to a protection service (auth or RL) with a given payload (unique identifier of the policy/policy rules to enforce).

<predicate> → <req>

req ::= <auth-req> | <rl-req>
auth-req ::= authconfig-id context-http-req
rl-req ::= limit-id counter-qualifiers | limit-id counter-qualifiers <rl-req>

predicate ::= <http-predicate> | <grpc-predicate>

http-predicate ::= host | path | method | headers | query_string | ( <composite-http-predicate> )
composite-http-predicate ::= <http-predicate> AND <http-predicate> | <http-predicate> OR <http-predicate>

grpc-predicate ::= host | method | headers | ( <composite-grpc-predicate> )
composite-grpc-predicate ::= <composite-grpc-predicate> AND <composite-grpc-predicate> | <composite-grpc-predicate> OR <composite-grpc-predicate>

Other than host, please refer to HTTPRouteMatch and GRPCRouteMatch respectively for the matching rules to build predicate functions respectively for HTTP and GRPC.

Additionally, remove integration from Istio and OSSM regarding register of external authorization service. #814 seems no longer valid and should be closed.

Additionally, remove integration with Envoy Gateway regarding external authorization service (via SecurityPolicy).

@guicassolato
Copy link
Contributor Author

@didierofrivia @adam-cattermole @alexsnaps, I could use a sanity check on the description for this one.

@alexsnaps
Copy link
Member

@adam-cattermole wrt your comment on the current refactoring, I'm wondering if we should consider ORed Predicates in the wasm-shim just as much (if they can really be ORed in GwAPI's domain)... just to keep the mapping simpler, wdyt?

@adam-cattermole
Copy link
Member

adam-cattermole commented Sep 23, 2024

If I understood correctly my discussion with others, I was lead to believe that the conditions will become a single CEL expression where the and/or behaviour was encoded in the expression.. this was why I was suggesting to remove the nested allOf and promote the members up to the conditions level. If we do expect there to be multiple conditions though with either allOf or anyOf behaviour perhaps we should revert back to current structure

@guicassolato
Copy link
Contributor Author

guicassolato commented Sep 23, 2024

I'm wondering if we should consider ORed Predicates in the wasm-shim just as much (if they can really be ORed in GwAPI's domain)

@alexsnaps, a Gateway API HTTPRouteRule may contain multiple HTTPRouteMatches which are ORed between them of course.

This doesn't mean we necessarily have to support <predicate> OR <predicate>. In the end, I imagine a config will be a list of <predicate> → <req> pairs anyway. Instead of writing one pair per Gateway → Gateway Listener → HTTPRoute → HTTPRouteRule topological path (i.e. one pair per effective policy), we could multiply this by the number of HTTPRouteMatches.

Do you have a preference?

@eguzki
Copy link
Contributor

eguzki commented Sep 23, 2024

Actually, it is <predicate> -> [<action>].

The predicate, as a CEL expression, can express boolean operators as desired. The current design is because expression predicates in yaml is not easy, so we decided to implement something like the following to cover most use cases.

(pattern_expression_11 && pattern_expression_12 && .... && pattern_expression_1M) || 
(pattern_expression_21 && pattern_expression_22 && .... && pattern_expression_2M) || 
... ||
(pattern_expression_N1 && pattern_expression_N2 && .... && pattern_expression_NM)

That's the reason why there is only allOf. No need for anyOf. It is already covered.

conditions:
- AllOf:
  - request.path == /v1
  - request.method == GET
- AllOf:
  - request.path == /v1
  - request.method == POST

With CEL, I do not think there is a need for OR'ed predicated or anything like that. One predicate can include internal OR's and AND's.

So, maybe something like

---
extensions:
  limitador:
    type: ratelimit
    endpoint: limitador-cluster
    failureMode: deny
policies:
  - name: rlp-ns-A/rlp-name-A
    hostnames:
      - '*.toystore.com'
      - example.com
    rules:
      - predicate: CEL_PREDICATE_A
        actions:
          - extension: limitador
            scope: rlp-ns-A/rlp-name-A
            data:
              - static:
                  key: A
                  value: "1"
              - selector:
                  selector: auth.metadata.username
      - predicate: CEL_PREDICATE_B
        actions:
          - extension: limitador
            scope: rlp-ns-A/rlp-name-A
            data:
              - static:
                  key: B
                  value: "1"
              - selector:
                  selector: auth.metadata.username

The predicate can be string that can be parsed by the CEL parser or some AST.

@guicassolato
Copy link
Contributor Author

I'm OK with <predicate> -> <action>, although not with <predicate> -> [<action>]. (<action> is not optional.)

Also, as of today (with recent developments on the wasm-shim included), <action> is defined as "send a request to the auth and/or RL services". Hence the proposal <predicate> → <req>.

Semantics aside, I agree with the general behaviour described by @eguzki.

As for the exact format of the new config, I guess it's TBD.

The part that matters the most is rules. This part of the example provided seems quite sufficient to me.

This other part from the example, on the other hand, is not strictly needed:

- name: rlp-ns-A/rlp-name-A
  hostnames:
    - '*.toystore.com'
    - example.com

It may serve to a purpose of indexation inside the config, which I'm fine with should we want to keep it.

Just remember that what we call "policies" in the wasm config will no longer map 1:1 to policy resources necessarily, due to merges.

@eguzki
Copy link
Contributor

eguzki commented Sep 24, 2024

Sorry, my bad

<predicate> -> [<action>] does not mean that action is optional, it means multiple actions.

Maybe better like this? <predicate> -> <req1>, <req2>, ...., <reqN>

@guicassolato
Copy link
Contributor Author

Being implemented for Rate Limit in #893 according to Kuadrant/wasm-shim#110

@guicassolato guicassolato mentioned this issue Oct 23, 2024
15 tasks
@github-project-automation github-project-automation bot moved this from In Progress to Done in Kuadrant Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants