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

Support for specifying the filter execution order in HTTPRoute #1598

Closed
Amila-Rukshan opened this issue Dec 13, 2022 · 24 comments · Fixed by #2687
Closed

Support for specifying the filter execution order in HTTPRoute #1598

Amila-Rukshan opened this issue Dec 13, 2022 · 24 comments · Fixed by #2687
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@Amila-Rukshan
Copy link
Contributor

What would you like to be added:

There are scenarios where the filters should be applied in a predefined order. But the filters does not support a way to order individual filter since it's an unordered list.

For example following HTTPRoute has multiple filters,

apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: http-route-webhook
  namespace: apk
spec:
  hostnames:
  - my.gw.org
  parentRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: Default
  rules:
  - backendRefs:
    - group: ""
      kind: Service
      name: web-hook-service
      port: 80
      weight: 1
    matches:
    - path:
        type: PathPrefix
        value: /my-api/7.6.0
    filters:
      - type: ExtensionRef
        extensionRef:
            group: ""
            kind: MyPolicy
            name: my-policy-2
      - type: ExtensionRef
        extensionRef:
            group: ""
            kind: MyPolicy
            name: my-policy-1
      - type: URLRewrite
        urlRewrite:
          path:
            type: ReplacePrefixMatch
            replacePrefixMatch: /2457afc0-c3c0-46a0-a389-a84025b8eabe

How can someone specify the order like my-policy-1, my-policy-2, and then urlRewrite filters are applied in order?

Docs says this is not supported yet: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteFilter

"The effects of ordering of multiple behaviours are currently unspecified.".

We would like to have the support for this.

Any thought on this?

Thanks!

@Amila-Rukshan Amila-Rukshan added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 13, 2022
@shaneutt
Copy link
Member

shaneutt commented Dec 13, 2022

Hi @Amila-Rukshan thanks for the request. Can you help us to better understand the context and use case? What implementation(s) are you particularly interested in having implement this and is this more of a user-facing request? OR what implementation(s) do you work on? Could you describe a concrete use-case (e.g. explain what my-policy-1 and my-policy-2 might actually do, and why the ordering is important in your case)?

@Amila-Rukshan
Copy link
Contributor Author

@shaneutt thanks for replying!
For example, let's say my-policy-1 checks whether some header is available in the request and adds a default header value if is not present, and the my-policy-2 is the one that does the validation of that header and allowing the request going to backend from the gateway. We should have a way to handle that kind of filter dependancies.

@shaneutt
Copy link
Member

@shaneutt thanks for replying! For example, let's say my-policy-1 checks whether some header is available in the request and adds a default header value if is not present, and the my-policy-2 is the one that does the validation of that header and allowing the request going to backend from the gateway. We should have a way to handle that kind of filter dependancies.

Understood, thank you. So is this more of a user-facing request, or what implementation(s) are you hoping to add this to?

@Amila-Rukshan
Copy link
Contributor Author

Amila-Rukshan commented Dec 15, 2022

@shaneutt what about having another optional property called order which is a number for a filter where if defined then it will be used for execution order.

For example:

apiVersion: [gateway.networking.k8s.io/v1beta1](http://gateway.networking.k8s.io/v1beta1)
kind: HTTPRoute
...
...
    filters:
      - type: ExtensionRef
        order: 2
        extensionRef:
            group: ""
            kind: MyPolicy
            name: my-policy-2
      - type: ExtensionRef
        order: 1
        extensionRef:
            group: ""
            kind: MyPolicy
            name: my-policy-1
      - type: URLRewrite
        order: 3
        urlRewrite:
          path:
            type: ReplacePrefixMatch
            replacePrefixMatch: /2457afc0-c3c0-46a0-a389-a84025b8eabe

@howardjohn
Copy link
Contributor

If we need order I would think that should be defined by the order in the list

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 15, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 14, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 2023
@sanjaypujare
Copy link
Contributor

sanjaypujare commented Dec 8, 2023

If we need order I would think that should be defined by the order in the list

I agree with this. In any case it will be good to address this in the spec. Can we reopen this issue? Or is it okay if I submit a PR to fix the spec to specify an order?

@robscott
Copy link
Member

robscott commented Dec 8, 2023

Thanks for brining this one back up @sanjaypujare! It looks like I missed this the first time, but I don't think there's anything about the existing filters list that makes it unordered. We simply haven't provided any guidance in the spec in terms of ordering. This was initially with the thought that many underlying dataplanes have a fixed order of operations that could not support this kind of custom ordering for at least some features. With that said, it probably makes sense to add some guidance that implementations should respect the order filters are defined in wherever possible. Each implementation should also likely document any implementation-specific limitations on filter ordering they have.

@sanjaypujare if you're up for making an update to the spec, I think it would be helpful to at least provide some recommendations here.

@robscott robscott reopened this Dec 8, 2023
@robscott
Copy link
Member

robscott commented Dec 8, 2023

/assign @sanjaypujare

@sanjaypujare
Copy link
Contributor

sanjaypujare commented Dec 9, 2023

Thanks for brining this one back up @sanjaypujare! It looks like I missed this the first time, but I don't think there's anything about the existing filters list that makes it unordered. We simply haven't provided any guidance in the spec in terms of ordering. This was initially with the thought that many underlying dataplanes have a fixed order of operations that could not support this kind of custom ordering for at least some features. With that said, it probably makes sense to add some guidance that implementations should respect the order filters are defined in wherever possible. Each implementation should also likely document any implementation-specific limitations on filter ordering they have.

@sanjaypujare if you're up for making an update to the spec, I think it would be helpful to at least provide some recommendations here.

So this is what I am thinking: I would like to change the text at https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/httproute_types.go#L199 (and a similar one for GRPCRoute if applicable) to say something like:

"The filters are applied in the order they are listed. In case an implementation has a fixed order of operations that cannot support the listed order, the implementation will reject the whole Route with an error."

Note that the spec already specifies list order for first marching rule in case of multiple matches:

"matching precedence MUST be granted to the FIRST matching rule (in list order) with a match"

so there is a precedent for list order of route rules in a route. We can extend the semantics to filters as well.

Let me know what you think. If okay, I'll open a PR.

@sanjaypujare
Copy link
Contributor

There are no comments so I'll take it as there are no objections and I'll open a PR to specify list order for filters.

@robscott
Copy link
Member

In case an implementation has a fixed order of operations that cannot support the listed order, the implementation will reject the whole Route with an error

I personally think rejecting the Route is a bit much in this case. We could theoretically have a standard way to communicate that filters will not be executed in the order specified, but there are some cases where order doesn't really matter and the warning message may cause unnecessary confusion.

@sanjaypujare
Copy link
Contributor

...
I personally think rejecting the Route is a bit much in this case. We could theoretically have a standard way to communicate that filters will not be executed in the order specified, but there are some cases where order doesn't really matter and the warning message may cause unnecessary confusion.

Good point about rejecting. How about indicating through RouteParentStatus.Conditions ? We can use RouteReasonIncompatibleFilters for this or I can propose a new status, say RouteReasonIncompatibleFilterOrder ? May be the latter one addresses the unnecessary confusion case?

@EItanya
Copy link
Contributor

EItanya commented Dec 13, 2023

Maybe this is unrelated/irrelevant, but I think we have to be really careful here, especially with #713 being so widely used by the community. Let's take for example the Route above:

apiVersion: [gateway.networking.k8s.io/v1beta1](http://gateway.networking.k8s.io/v1beta1)
kind: HTTPRoute
metadata:
  name: http-route-webhook
  namespace: apk
...
...
    filters:
      - type: ExtensionRef
        order: 2
        extensionRef:
            group: ""
            kind: MyPolicy
            name: my-policy-2
      - type: ExtensionRef
        order: 1
        extensionRef:
            group: ""
            kind: MyPolicy
            name: my-policy-1
      - type: URLRewrite
        order: 3
        urlRewrite:
          path:
            type: ReplacePrefixMatch
            replacePrefixMatch: /2457afc0-c3c0-46a0-a389-a84025b8eabe

Not let's add some policy which applies auth to that HTTPRoute

apiVersion: some.gvk/v1alpha1
kind: AuthPolicy
metadata:
  name: auth-policy
  namespace: apk
spec:
  targetRef:
    name: http-route-webhook
  config:
    # insert authz config here

If the user wanted to ensure that auth-policy above would be executed at a specific point in the chain, how would they accomplish that? I can think of a couple of ways, but they feel very complex in a way that may complicate the spec more than benefit it. I'd be curious to hear from implementations how often they benefit from exposing this complexity to the user rather than exposing specific use-cases via the API. @sanjaypujare and I started having this discussion in #2628 as well and I'm still not convinced this is the responsibility of the spec, but rather of any implementation that wishes to expose filter ordering to it's users.

@sanjaypujare
Copy link
Contributor

...
If the user wanted to ensure that auth-policy above would be executed at a specific point in the chain, how would they accomplish that? I can think of a couple of ways, but they feel very complex in a way that may complicate the spec more than benefit it.

Probably one of the ways is to use before or after tokens as described in Kong's plugin ordering. This can be used in filters and also in policies which indirectly/implicitly add a filter in the filter chain - so a policy can say that execute me before and/or after so-and-so.

I'd be curious to hear from implementations how often they benefit from exposing this complexity to the user rather than exposing specific use-cases via the API. @sanjaypujare and I started having this discussion in #2628 as well and I'm still not convinced this is the responsibility of the spec, but rather of any implementation that wishes to expose filter ordering to it's users.

The same link above (Kong's plugin ordering) also describes a couple of use-cases where users or implementors benefit from exposing the order.

@EItanya
Copy link
Contributor

EItanya commented Dec 15, 2023

I don't disagree with this, but I definitely think it should fall under the umbrella of implementation specific if we decide to go forward with it at all.

@sanjaypujare
Copy link
Contributor

Okay, so I suggest the following addition/change to the spec (at https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/httproute_types.go#L199). The wording will be something like this:

"This spec does not mandate any specific ordering for the execution of filters. However an implementation may decide that the filters will be executed in the listed order. In case the implementation requires a certain order of operations that cannot support the listed order, the implementation will reject the whole Route with the error RouteReasonIncompatibleFilterOrder ."

For now I am not addressing the issue of a policy's order vis-a-vis the filters' order but I will do it later.

@robscott @shaneutt @EItanya are you okay? If yes, then I'll open a PR

@robscott
Copy link
Member

SGTM, thanks @sanjaypujare! One tiny nit - there may be room to just reject an individual route rule instead of the whole route.

@sanjaypujare
Copy link
Contributor

there may be room to just reject an individual route rule

Yes, but an individual route rule doesn't have a Status like the Status an HTTPRoute has unless I am missing something.

@robscott
Copy link
Member

Yes, but an individual route rule doesn't have a Status like the Status an HTTPRoute has unless I am missing something.

Yep, you're right. You could add this as a reason to the following condition though:

// This condition indicates that the Route contains a combination of both
// valid and invalid rules.
//
// When this happens, implementations MUST take one of the following
// approaches:
//
// 1) Drop Rule(s): With this approach, implementations will drop the
// invalid Route Rule(s) until they are fully valid again. The message
// for this condition MUST start with the prefix "Dropped Rule" and
// include information about which Rules have been dropped. In this
// state, the "Accepted" condition MUST be set to "True" with the latest
// generation of the resource.
// 2) Fall Back: With this approach, implementations will fall back to the
// last known good state of the entire Route. The message for this
// condition MUST start with the prefix "Fall Back" and include
// information about why the current Rule(s) are invalid. To represent
// this, the "Accepted" condition MUST be set to "True" with the
// generation of the last known good state of the resource.
//
// Reverting to the last known good state should only be done by
// implementations that have a means of restoring that state if/when they
// are restarted.
//
// This condition MUST NOT be set if a Route is fully valid, fully invalid,
// or not accepted. By extension, that means that this condition MUST only
// be set when it is "True".
//
// Possible reasons for this condition to be True are:
//
// * "UnsupportedValue"
//
// Controllers may raise this condition with other reasons, but should
// prefer to use the reasons listed above to improve interoperability.
RouteConditionPartiallyInvalid RouteConditionType = "PartiallyInvalid"

@sanjaypujare
Copy link
Contributor

Thanks @robscott . I have opened PR #2687. PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
No open projects
8 participants