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

*No* merging of policies #10

Closed
wants to merge 2 commits into from
Closed

*No* merging of policies #10

wants to merge 2 commits into from

Conversation

alexsnaps
Copy link
Member

@alexsnaps alexsnaps commented Feb 28, 2023

This aims at providing a much simpler model than what #4 is proposing. It comes pretty close to using HTTPRouteFilter, as it lets you target the same granular "resource". It gets rid of merging policies, so that default & overrides are merely a way to stack policies and use the one that ends up "on top" of the stack, according to all the resolve rules of the Policy Attachment hierarchy.

@alexsnaps alexsnaps marked this pull request as draft February 28, 2023 13:49
@alexsnaps alexsnaps added RFC Request For Comments target/current labels Feb 28, 2023
@alexsnaps alexsnaps self-assigned this Feb 28, 2023
guicassolato added a commit that referenced this pull request Feb 28, 2023
# Motivation
[motivation]: #motivation

As per the specification, a given resource will always need to support multiple policies targeting it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to this description, clear and concise

If two policies target the same `HTTPRouteRule`, one would take precedence over the other according to
the Gateway API's merging rules.

With these rules, Kuadrant can "easily" identify what policies apply to an `HTTPRoute`, possibly
Copy link
Collaborator

Choose a reason for hiding this comment

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

one thing that occurred to me might be to actually reflect this in the status of the HTTPRoute in a condition

for `color` precedes and as such `blue` will be enforced for _any_ `HTTPRoute` that ends up being wired
to this `Gateway`.

That policy tho has no traffic to police yet. Unless an `HTTPRoute` binds that `Gateway` to a
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1


## What actual use-cases does this address?

- An admin restricts all traffic using a `default` at the gateway to force application developers to
Copy link
Collaborator

Choose a reason for hiding this comment

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

It also ensure no unlimited endpoints are exposed


So there are a couple of things to consider here:

- status reporting on "not yet" existing `HTTPRouteRule`, e.g. a Policy declares a set of `matches` that
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think reporting status in the RLP and in the HTTPRoute / Gateway is worth consideration
example on a condition of the HTTPRoute status we could add something like GVK name namspace enforced

# Drawbacks
[drawbacks]: #drawbacks

- The use cases this approach doesn't address…
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think linking to kubernetes-sigs/gateway-api#1489 would make sense here also

would be an alternative and provide finer grained control over how a policy applies, i.e. at the
`HTTPRouteRule` level, or even on a per `HTTPBackendRef` within an `HTTPRouteRule`. But today there is
no way (that I know of) for us to add a `HTTPRouteFilterType` to the list of supported
`HTTPRouteFilter` by a Gateway API implementor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

if there was a way to do this (for example if we proposed a GEP around third party filters), would this compliment the policy side? Does a filter at the gateway level make sense? I don't see how filters would solve the use case of wanting to set a default limit for all new endpoints


- What use cases are we making "overly" complex, because of the duplication of `HTTPRoute`'s
`HTTPRouteMatch` ?
- Is it fine for the `matches` section to be at the same level as `default`, `override` and `targetRef`?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the fact it is an optional complexity is the perfect balance at least until we hear otherwise via new use cases. You have the "target all of a resource by default" and given a way to cover more advanced use cases and targeting. I also like the fact that the default section stays focused on the actual domain (RL) and leaves the targeting as a separate piece

apply to other matches. So that traffic to `bar-svc` on port `8080` would be colored `green` as per the
`PaintPolicy` attached to `my-gw`.

The resolving of _Policies_ to enforce occurs at the `HTTPRoute` or within individual `HTTPRouteRule`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that we can describe this relatively simply. That said I thnk we would want a good range of "example" policies to cover common use cases similar to how it is done for https://github.com/kubernetes-sigs/gateway-api/tree/main/examples/standard

Copy link
Collaborator

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

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

I really like the balance found here. I see this as a good way forward with RLP to solve some of the use cases we want to solve for gateway admins and developers. I would encourage us to do a PoC ASAP to boil this down to practice

@eguzki
Copy link
Contributor

eguzki commented Mar 2, 2023

Looking good, indeed.

@guicassolato
Copy link
Contributor

I'd suggest trying the same Policy Attachment exercise based on this approach as well, i.e. without merges. My guts say that it's going to be significantly easier... at least for Kuadrant 🙂

@guicassolato
Copy link
Contributor

My guts say that it's going to be significantly easier... at least for Kuadrant 🙂

Update: My guts were wrong! I'm trying to solve use cases 1-5 of the exercise without merges and my head hurts a little.

In the first example here, the PaintPolicy was changed from override: { color: blue } to default: { color: blue }. (That allowed later creating an HTTPRoute that inherited blue by default, and then replaced it with green by defining its own route-level PaintPolicy.)

What if I didn't want to change from an override to a default in the first PaintPolicy targeting the Gateway, but to create an additional default: { color: yellow } for a special case (before any route is created)? I suppose I would modify the gw-level policy like this:

kind: PaintPolicy
spec:
  override:
    color: blue
  default:
    color: yellow
    when:
    - special case
  targetRef:
    kind: Gateway
    name: my-gw

I would expect then yellow at all routes where special case matches (which might still be overridden by a route-level policy) and blue for all other cases, i.e. where special case does not match.

What happens if I create that route-level policy?

kind: PaintPolicy
spec:
  default:
    color: green
  targetRef:
    kind: HTTPRoute
    name: foo-route

I imagine it would be a noop, because of the gw-level override still holding?

So to be able to get green at least at special case on foo-route, what should I do? Maybe...

kind: PaintPolicy
spec:
  default:
    color: green
    when:
    - special case
  targetRef:
    kind: HTTPRoute
    name: foo-route

So the owner of the HTTPRoute needs to be aware of the details in the first PaintPolicy attached to the GW, so it can be reasoned that in fact what's going to happen is:

kind: PaintPolicy
spec:
  override: # the part that cannot be changed as enforced by the gw override policy
    color: blue
  default: # the part that the gw default policy allows to be overridden
    color: green
    when:
    - special case
  targetRef:
    kind: HTTPRoute
    name: foo-route

?

@alexsnaps
Copy link
Member Author

alexsnaps commented Mar 3, 2023

@guicassolato ... to your yellow/green question: yes.
These people writing these Policies would have to sync in this world. Most importantly tho, the "when" there, would need to specified within the same policy for the catch all to not apply to it. And eventually that "when" would need to be present as an HTTPRouteRule, i.e. a HTTPRouteMatch clause of an actual HTTPRoute.

@guicassolato
Copy link
Contributor

@alexsnaps, I'm probably overthinking this but I still have to go through... Apologies in advance.


What step 5 of the Policy Attachment exercise asks is actually even simpler (tho possibly harder to solve) than what I exemplified here with the PaintPolicy, because it does not involve conditions.

At steps 1 and 2, an override policy is set at the level of the Gateway. No conditions there. Then, at step 5, a specific policy targeting an entire HTTPRoute is set. There's no conditions there either.

I still don't know how to fix this.

It looks like, without merging and without conditions, once you set an override at the level of the Gateway, there's no point in setting anything else at the level of the HTTPRoute, yes?

And if there, for an entire HTTPRoute, a legit exception to an override set at the level of a Gateway, that exception needs to be specified in the policy that targets the Gateway. We don't want to remove the override, so it won't fall apart for the other HTTPRoutes where the override still holds.

This in itself already means the problem cannot be solved without conditions... Some sort of condition.

Because it's going to be, for a given route, either the policy that targets the Gateway or the policy that targets the HTTPRoute – i.e. all in, no merging –, it won't suffice a condition that says "this override, except in case X" and then specify what the policy is when "case X" in a separate policy targeting the HTTPRoute. What to apply for "case X" needs to go in the policy targeting Gateway as well, thus not leveraging hierarchy.

To be fair, this is not a problem inherent of merging or not merging, but a problem inherent of how we identify the bits of a policy that can be overridden and the bits that cannot. No merging of policies I guess only means less granularity to those bits, maybe?

Anyway, this thing that identify the bits seems to be pointing so far to the "conditions", but not any kind of condition; only the ones the control plane can reason about, practically ruling out what we've been calling "soft conditions".


Back to the case of the default set at the level of the Gateway and the attempt to override it with a policy targeting the HTTPRoute, which seems a problem easier to solve than the problem of the override with one exception without condition...

To me, it doesn't make sense to have a default and an override block in the same policy targeting the Gateway without any conditions at all. So, imagine the default is for a special case and the override is for all the rest (exactly like in my example with the PaintPolicies).

You said:

the "when" there, would need to specified within the same policy for the catch all to not apply to it.

Do you mean adding a kind of !special case condition to the part that goes in the override block?

kind: PaintPolicy
spec:
  override:
    color: blue
    when:
    - !special case
  default:
    color: yellow
    when:
    - special case
  targetRef:
    kind: Gateway
    name: my-gw

Then you said:

And eventually that "when" would need to be present as an HTTPRouteRule

There are two possibilities here: a "hard" (route-related) condition and a "soft" (non route-related) condition.

If special case is route-related, I don't think negating it (i.e. !special case) is supported by Gateway API, is it? Maybe this invalidates the previous recommendation? (If I even understood the recommendation correctly in the first place.)

If special case is NOT route-related, then likely the two rules there can hardly be reasoned in terms of the "identity of the rule" by the control plane, i.e. if they mean yellow and blue for the same context or for two different contexts. Therefore better if the conditions are processed as what we've been calling "soft conditions" by whatever ultimately enforces the policy in the data plane (Limitador, Authorino, or the "Painter" apparently).

This raises a hypothesis that, for all that matters, to the control plane, soft conditions are just like having no conditions at all.

Imagine the HTTPRoute exists but no policy was attached to it; it's only that one policy targeting the Gateway, with one override and one default and the two "soft conditions" (!special case and special case, respectively). Is blue deemed to apply to all cases because it's an override and we cannot reason about the relation it has relatively to yellow based on soft conditions? Or, on the other hand, if yellow or blue are really to be decided based on soft conditions – and therefore both bits of the policy need to be pushed to the data plane –, then the override and the default keywords there mean nothing at all?

I imagine it's the latter case, i.e. overrides and defaults only having meaning when dealing with conflicts between policies, but nothing between them within a same policy.

So, now, if we do create the second policy green targeting the HTTPRoute, while the conditions are "soft conditions", there's nothing the control plane can do for working out the relations and therefore traffic will always be either blue or yellow but never green?

(And from here I refer back to the beginning of my comment where override with no "hard" conditions equals to no leveraging of hierarchy.)

art-tapin pushed a commit that referenced this pull request Mar 22, 2023
* rfc: RateLimitPolicy v2

* add spec.limits.matches

* Added drawbacks

* Added implementation details for each example

* Example section headers moved one level up

* Future possibilities

* Prior art

* Added date and issue refs

* Added pr #10 to the watchlist

* fix: header name :host -> :authority

* remove the concept of increment from the rfc - it deserves its own proposal to be discussed separately

* fix unique hash route matcher identifiers

* Atomic targeting of HTTPRouteRules

* Back to matching any subset of HTTPRouteMatches as the way to select the HTTPRouteRules to be bound to a limit + Limit's name becomes a first-class citizen

* Soft conditions do not accept expressions related to attributes of the HTTP request

* Update the example of the Mechanics of generating RL descriptor actions for one that does not involve HTTP attributes used in 'soft' conditions

* Fix example 1 so it's clear the HTTPRouteMatch does not have to entirely stated within the trigger matches, but only a portion of it will do (if that portion is enough to scope the limit)
@alexsnaps
Copy link
Member Author

superseded by #58

@alexsnaps alexsnaps closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments target/current
Projects
No open projects
Status: Needs refinement
Development

Successfully merging this pull request may close these issues.

4 participants