-
Notifications
You must be signed in to change notification settings - Fork 618
add GEP-1323: Response Header Filter #1326
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
Conversation
|
Hi @aryan9600. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
shaneutt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good proposal. I particularly like the usage examples, thanks for taking the time to write all this out.
robscott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aryan9600!
| Adding support for this unlocks a lot of real world use cases. Let’s review a couple of them: | ||
|
|
||
| * For example, if a team has a frontend web app that along with two different versions of their backends exposed as Kubernetes services, and the frontend needs to know which backend it’s talking to, this can be easily achieved without any modifications to the application code. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you describe how this might work if the backend application needs to deliver a custom value in the response? E.g. the backend destination determines a cookie value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I don't understand fully. If the backend application needs to deliver a custom cookie that it determined using some business logic, it'd need to set it in while returning the response itself. If I'm not answering your question clearly here, could you kindly elaborate a bit more? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like maybe you're describing something like the backend app setting a cookie, and then the Gateway implementation needing to do something with that value, @candita?
If that is correct, I know some proxies support setting a header value using the value from another, but I don't feel like that's common enough to be included in the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aryan9600 for example, this HTTPRoute has two hostnames, and the header response depends on the value of the hostname that delivered this traffic. If response1.header.example was used, return header value 1, but if response2.header.example was used, return header value 2. The backend gets to decide what value is sent back in the response header. Can this be set up to hold a variable name in the value of the add stanza?
| filters: | |
| - type: ResponseHeaderModifier | |
| responseHeaderModifier: | |
| add: | |
| name: number1or2 | |
| value: $someVariableSetAtTheBackEnd |
I guess I'm aware of more response headers that are set after arriving at the backend, and based on what the application determines. This proposal is based more on a hard-coded response header value, which is less challenging, but perhaps also less useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, so from what I understand, you're suggesting parsing with logical conditioning? this is a very cool feature, but i doubt if it's generic enough to be included into the API. could you please point me to a couple of service meshes/ingresses that have support for something similar today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would setting this proposed ResponseHeaderModifier filter with different values on two separate HTTPBackendRefs be sufficient to enable some of the desired functionality here? The configuration would be static still and a bit more verbose than specifying a filter with a dynamic value at the HTTPRouteRule level, but maybe enough for some needs, such as determining if a response was served by a canary backend?
For the hostname matching specifically, I suppose two separate HTTPRouteRules (identical except for the hostnames field) could be configured as one way to implement that instead of relying on a value from the backend - that feels like it could be a use case that could be simplified with config templating, which may be best achieved by third-party tools at a layer above Gateway API.
Finally, for header manipulation (add or modify a header based on an existing header value in the response, which feels like the most direct way to implement this without touching the backend app itself), it feels like we might have some of the scaffolding to build from by combining type: RegularExpression from HTTPHeaderMatch and the add/set fields in HTTPRequestHeaderFilter if we were to extend the RegEx implementation from just matching to allow capture groups for setting a value, but then you're already in Custom conformance due to the variance in RegEx implementations, so I'm not sure how much value there is to spec that, although HTTPPathModifier could possibly benefit from a similar implementation. This last option feels beyond the scope of this current proposal (it could build on top of it though!) but may be worth exploring if there's sufficient interest, or first experiment by building something as an implementation-specific filter using the extensionRef field of HTTPRouteFilter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was some confusion on this thread, but after discussing in more detail at today's community meeting, it looks like Envoy provides a good example of this kind of capability. Essentially allowing users to intersperse variables in the responses using % special chars: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers#custom-request-response-headers.
Nothing in our API for either request, or in this case response, header modification prevents that from working, but we should likely add a note to the spec that while this kind of functionality may be supported by the underlying implementation, it is unlikely to be portable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 that we should have something in the implemented spec that says we're not excluding metacharacter use, but with a warning about portability. I think that can be left as an exercise for the implementer though.
|
|
||
| ## Goals | ||
| * Provide a way to modify HTTP response headers. | ||
| * Reuse existing types as much as possible to reduce boilerplate code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the admin wanted to add the same response header to all HTTPRoutes (e.g. the same HSTS response for all of them) - can you find a way for the admin to override response headers at a higher level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that's possible with the current structure of the API. Adding a response header to all HTTPRoutes is possible if the Gateway object had a way of modifying the responses that are handled by HTTPRoutes attached to it. From what I understand going through https://gateway-api.sigs.k8s.io/concepts/api-overview/#gateway, a Gateway is not responsible for actually modifying or forwarding requests/responses.
If we want to introduce this behavior, I'd guess that it's out of scope here and needs a GEP of it's own. cc: @robscott @shaneutt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That use case (admin wanting to force the addition of something) is one of the things we had in mind with Policy Attachment - I could see a Policy Resource that replicated the same structure used here in an override block, that could be attached to a Gateway resource. That would imply that these settings should Override any settings on the HTTPRoutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aryan9600 if we're going to go straight to implementable with this, then I think adding a section for this and pointing to policy attachment sounds correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youngnick added a section on Policy Attachment, please take a look, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a policy that overrides a filter creates an implicit policy for a filter in an HTTPRoute :) . It also means we now have two different ways to configure the same thing - through a policy and through a filter. I wonder if it is a good thing.
Perhaps another way is to have a policy that can override all filters rather than a specific one? So it is more general-purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pleshakov yes we have two different ways to configure the same thing, but they are meant to be used in a different context. The example in the Policy Attachment section describes a real world use case that highlights this.
I'm definitely okay with having a multi-purpose policy instead of this but please keep in mind that this is just a suggestion. As far as I understand, implementations are free to design their policies as they chose and like. In https://gateway-api.sigs.k8s.io/references/policy-attachment/, I can see policies like TimeoutPolicy and RetryPolicy being used throughout, although they are technically each a filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing this at today's community meeting, it seems like we're all OK to leave policy attachment out of the scope of this GEP. As long as the GEP mentions that this may be further expanded by policy attachment in the future, I don't think we need to get into the details of what that would look like here.
youngnick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty great to me, just a few small things.
|
|
||
| ## Goals | ||
| * Provide a way to modify HTTP response headers. | ||
| * Reuse existing types as much as possible to reduce boilerplate code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That use case (admin wanting to force the addition of something) is one of the things we had in mind with Policy Attachment - I could see a Policy Resource that replicated the same structure used here in an override block, that could be attached to a Gateway resource. That would imply that these settings should Override any settings on the HTTPRoutes.
| Adding support for this unlocks a lot of real world use cases. Let’s review a couple of them: | ||
|
|
||
| * For example, if a team has a frontend web app that along with two different versions of their backends exposed as Kubernetes services, and the frontend needs to know which backend it’s talking to, this can be easily achieved without any modifications to the application code. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like maybe you're describing something like the backend app setting a cookie, and then the Gateway implementation needing to do something with that value, @candita?
If that is correct, I know some proxies support setting a header value using the value from another, but I don't feel like that's common enough to be included in the API.
| } | ||
| ``` | ||
|
|
||
| Given the fact that this functionality is offered by only a few projects that currently implement Gateway API when using their own traffic routing CRDs, it’s better to support `ResponseHeaderModifier` as an _Extended_ feature, unlike `RequestHeaderModifier` which is a _Core_ feature. This will also not increase the difficulty of implementing Gateway API for any future ingress or service mesh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, we can use the same HTTPHeaderModifier struct to represent the actual config, but using the field that the struct appears in inside the Route can have different support, and that support can be documented on those fields.
Not sure if we need that in this GEP or not though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we could add a line in the HTTPRouteFilter API above the ResponseHeaderModifier field mentioning the support similar to this?
gateway-api/apis/v1beta1/httproute_types.go
Line 571 in bb549d0
| // Support: Core |
If yes, I didn't mention it in the proposal, because I thought it's implied that the support will be documented in the API docs. But if you think, we need to be explicit about it in the proposal, I'll add something accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that when this is implemented, any new fields should have their support explicitly documented.
site-src/geps/gep-1323.md
Outdated
|
|
||
| // Default defines default policy configuration for the targeted resource. | ||
| // +optional | ||
| Default *HTTPHeaderModifier `json:"default,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Default is needed here. If so, how is the Default different than the Override? What happens if both are specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example below describes the usage of Default and Override sufficiently. About both being specified, I don't think that makes any sense, so maybe I should add something like only one of Default and Override should be present in a Policy object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://gateway-api.sigs.k8s.io/references/policy-attachment/#policy-boilerplate does explicitly describe allowing both Default and Override to be set ("at least one"), although I think that often makes more sense when they are used for aggregate config objects and modify different values, such as the AcmeServicePolicy example in https://gateway-api.sigs.k8s.io/references/policy-attachment/#hierarchy
For this case, it could make sense to add a few headers by default, and only enforce adding one important one with an override.
site-src/geps/gep-1323.md
Outdated
| ## Policy Attachment | ||
|
|
||
| This feature can be further extended via [Policy Attachment](../references/policy-attachment.md). Implementations can introduce a new Policy, `ResponseHeaderPolicy`, which can be used to ensure that certain headers are present/absent in a response, regardless of the `responseHeaderModifier` field in the `HTTPRoute` associated with that response. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you don't need to hash all of this out in this GEP, and just a reference to future work could be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean by "a reference to future work"? Do you mean state something like "This API can be extended with Policy Attachment, which will be discussed in a future GEP"?
site-src/geps/gep-1323.md
Outdated
|
|
||
| `.spec.default` provides a way to modify headers of responses being returned from the resource specified in `.spec.targetRef`. This resource can be a part of the Gateway API like `Gateway` or a core Kubernetes resource like `Namespace`. Using a `Namespace` as the `.spec.targetRef` would result in any response being returned from any resource in that namespace having its headers modified according to the policy. | ||
| `.spec.override` can be used to modify the headers of responses being returned from a resource that is already covered by `.spec.default` of a `ResponseHeaderPolicy`. Let's walk through an example: | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good example. If the community decides to keep these details within this GEP, then I'll offer a suggestion for a different way to implement a global policy with the possibility of override. The policy is set at the Gateway to apply to all *Routes globally as you described, but then each *Route could override the global default if needed, using the ResponseHeaderModifier mechanism you described earlier. The policy would then need to include some way to describe whether it is optional or not, i.e. whether it can be overridden or not. To me, this approach is clearer, but I still think it could be discussed in a different GEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we allow a Route to override the override set by a Gateway using its ResponseHeaderModifier, how would admins set headers alongside the headers specified in the Route's ResponseHeaderModifier?
About, moving the Policy Attachment section to its own GEP, I'm neither for or against it, so some consensus from the maintainers on this would be helpful.
wdyt? @shaneutt @robscott @youngnick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this proposed approach and think it's sufficiently clear. Policy Attachment already has the structure for separating defaults and overrides - if a header was added or modified by an Default config a filter on the Route object should be able to further modify or remove it, but if added or modified by an Override config, a filter on the Route should not be able to modify or remove it. @candita does this fully address your proposed use case?
The main reason this section may warrant consideration as a separate GEP is that it contradicts the current warning in https://gateway-api.sigs.k8s.io/references/policy-attachment/#interaction-with-custom-route-filters against [using] multiple mechanisms for representing the same fields, although that does explicitly state:
Note that this guidance may change in the future as we gain a better understanding of how extension mechanisms of the Gateway API can interoperate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikemorris having gone through the text, I understand that we disallow overlaps between Policy and custom Route filters. Since this proposed filter is not custom I feel like the current proposal is more or less okay.
I'll update it with your suggestion about Routes being able to modify Default config. @candita I now realize that I misread your initial comment 🤦 so yes, a Route should be able to modify the gloabal default
|
|
||
| ## Goals | ||
| * Provide a way to modify HTTP response headers. | ||
| * Reuse existing types as much as possible to reduce boilerplate code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a policy that overrides a filter creates an implicit policy for a filter in an HTTPRoute :) . It also means we now have two different ways to configure the same thing - through a policy and through a filter. I wonder if it is a good thing.
Perhaps another way is to have a policy that can override all filters rather than a specific one? So it is more general-purpose.
| # set cookies for all requests being forwarded to this service | ||
| responseHeaderModifier: | ||
| set: | ||
| name: Set-Cookie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie, "To send multiple cookies, multiple Set-Cookie headers should be sent in the same response." Will it be possible to set multiple headers with the same name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use add instead of set:
gateway-api/apis/v1beta1/httproute_types.go
Line 730 in 30f2983
| Add []HTTPHeader `json:"add,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the validation will prevent it though - it will reject any attempt to set or add two headers with the same name
// +listType=map
// +listMapKey=name
at the same time, it is probably will be very uncommon requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not 100% sure, but i think that'll happen when you have the same header name (the key) present in both set and add. This conclusion of mine stems from https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
map: X + Y performs a merge where the array positions of all keys in X are preserved but the values are overwritten by values in Y when the key sets of X and Y intersect. Elements in Y with non-intersecting keys are appended, retaining their partial order.
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Add []HTTPHeader `json:"add,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to always add headers? For example, what if the backend returns 500 response or the backend doesn't exist so that the proxy generates 500.
For example, in NGINX, it is possible to add headers for a subset of 2xx and 3xx codes only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pleshakov I think what you described is more like the "conditions" for running the filter. I think it can be implemented as a common option for all filters, i.e. check out the condition before running the filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pleshakov you seem to be suggesting similar to #1326 (comment)
i agree with @tokers that this could be made more generic. furthermore, i believe it exists outside the scope of this GEP, as deciding how to conditional logic on filters is a GEP of it's own.
site-src/geps/gep-1323.md
Outdated
| * Status: Implementable | ||
|
|
||
| ## TLDR | ||
| Similar to how we have `RequestHeaderModifier` in `HTTPRouteFilter`, which lets users modify request headers before the request is forwarded to a backend (or a group of backends), it’d be helpful to have a `ResponseHeaderModifier` field which would let users modify response headers before they are forwarded to its destination. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Similar to how we have `RequestHeaderModifier` in `HTTPRouteFilter`, which lets users modify request headers before the request is forwarded to a backend (or a group of backends), it’d be helpful to have a `ResponseHeaderModifier` field which would let users modify response headers before they are forwarded to its destination. | |
| Similar to how we have `RequestHeaderModifier` in `HTTPRouteFilter`, which lets users modify request headers before the request is forwarded to a backend (or a group of backends), it’d be helpful to have a `ResponseHeaderModifier` field which would let users modify response headers before they are returned to the client. |
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Add []HTTPHeader `json:"add,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pleshakov I think what you described is more like the "conditions" for running the filter. I think it can be implemented as a common option for all filters, i.e. check out the condition before running the filter.
|
Thanks for all the work on this @aryan9600! We talked about this at today's community meeting. I left some follow up comments above, but I think the major themes are:
/ok-to-test |
|
Thanks @aryan9600! This LGTM but will defer to someone else for the final approval. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aryan9600, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Sanskar Jaiswal <[email protected]>
|
Yup, I think this is good to go. /lgtm |
Signed-off-by: Sanskar Jaiswal [email protected]
What type of PR is this?
/kind gep
What this PR does / why we need it:
Adds a GEP proposing a way of modifying response headers via a new field
responseHeaderModifier.Which issue(s) this PR fixes:
Fixes #1323
Does this PR introduce a user-facing change?: