Enhance Broker Filter to proxy Response Headers#5946
Conversation
knative-prow-robot
added
the
size/M
|
The following is the coverage report on the affected files.
|
Codecov Report
@@ Coverage Diff @@
## main #5946 +/- ##
=======================================
Coverage 82.23% 82.24%
=======================================
Files 220 220
Lines 7572 7575 +3
=======================================
+ Hits 6227 6230 +3
Misses 911 911
Partials 434 434
Continue to review full report at Codecov.
|
| writer.WriteHeader(http.StatusBadGateway) | ||
| return http.StatusBadGateway, errors.New("received a non-empty response not recognized as CloudEvent. The response MUST be either empty or a valid CloudEvent") | ||
| } | ||
| proxyHeaders(resp.Header, writer) // Proxy original Response Headers for downstream use |
There was a problem hiding this comment.
I was wondering if we call this function once at line 262, so that we cover every error or success path.
There was a problem hiding this comment.
Yeah, I can certainly do that. I did consider it, and the reason I only changed the "success" paths is that I wasn't sure whether it made sense to return certain headers in cases where we are changing the response code. I still have "Retry-After" on my brain, and that header only "relates" to 429/503... so would it make sense to return with 500 or 502. I suppose in that example the header might not make sense but wouldn't cause a problem?
I will rework the PR (tests mostly) as suggested ; )
There was a problem hiding this comment.
I see, thanks for the clarification!
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierDipi, travis-minke-sap The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
4 participants
Fixes #5935
I'm new to the Broker / Trigger / Filter implementations (first PR) so someone with more familiarity should confirm whether the proposed changes are acceptable in the context of the larger ecosystem. I'm hoping this is an easy net-win, but don't know whether there could be downstream impacts on various channels. I've tested with the IMC to verify it's working as expected.
The fact that the Filter was not already returning the headers was surprising to me, as it is effectively placing limitations and assumptions on Channel implementations ability to consider headers in a non-broker use case. Seems very presumptuous to do so - unless it's somewhere in the Channel Spec that I've not seen? (something like - "...when you build a Channel you are not allowed to use the response headers or your channel will not be compatible with Brokers / Triggers...")
Proposed Changes
Pre-review Checklist
Release Note
Docs