Add event reply header to spec

[zhongduo]

This is one of the steps to solve #4127.

This is a follow up of the discussion in the event reply proposal to add the contract to the spec.

Proposed Changes

• Add event reply header to the spec

Release Note

- :gift: Add event reply header contract to the spec

[codecov]

Codecov Report

Merging #4560 (cd9b719) into master (3f32d79) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4560      +/-   ##
==========================================
+ Coverage   81.02%   81.10%   +0.08%     
==========================================
  Files         291      291              
  Lines        8221     8235      +14     
==========================================
+ Hits         6661     6679      +18     
+ Misses       1159     1156       -3     
+ Partials      401      400       -1     

Impacted Files	Coverage Δ
...iler/inmemorychannel/dispatcher/inmemorychannel.go	86.66% <0.00%> (ø)
...inmemorychannel/controller/resources/dispatcher.go	100.00% <0.00%> (ø)
pkg/channel/config.go
...nciler/inmemorychannel/controller/config/config.go	86.95% <0.00%> (ø)
...ciler/apiserversource/resources/receive_adapter.go	93.18% <0.00%> (+0.07%)	⬆️
...iler/inmemorychannel/controller/inmemorychannel.go	69.12% <0.00%> (+0.63%)	⬆️
...econciler/inmemorychannel/controller/controller.go	88.67% <0.00%> (+0.67%)	⬆️
...econciler/inmemorychannel/dispatcher/controller.go	87.75% <0.00%> (+8.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f32d79...cd9b719. Read the comment docs.

[zhongduo]

/cc @vaikas
/cc @lionelvillard

[aslom]

@zhongduo @grantr @evankanderson based on today WG meeting I think we need to go back to the feature doc and/or create new one to evaluate alternatives/tradeoffs and raised issues? In particular how that header will work (or not)with what async group is proposing for indicating asynchronous reply (Prefer: async)?

[zhongduo]

Copied from slack chat https://knative.slack.com/archives/C9JP909F0/p1607026424111100:

Hi, just try to resolve the ongoing discussions of Event Reply Header (https://github.com/knative/eventing/pull/4560) here from both the PR comments and WG meetings. IIUC, there are two main discussions left: 1) [@n3wscott] this can be a breaking change of the previous spec. And 2) [@aslom] we can consider using the standard Prefer header.
For 2) I will set up another thread or open meeting for discussion as it is more implementation details. Considering Scott’s loooooong vacation soon, I would like to focus on 1) and possibly new concerns of the feature itself, which I don’t find from the doc or PR yet. IMO, the versioning is more required in the preflight proposal, for this feature particular I believe it is not necessary because
  a) the reply header only requires change from the broker as a minimum and we can update all the brokers now,
  b) if a sink does not implement this feature it does not really lose or break anything,
  c) if we change the default behaviour without the header to be accepting event reply, we will have to ask all the sources to update it, which is what we want to avoid.
Since the direct connection from source to sink is officially supported, the existing reply event is already problematic IMO, so I do believe this should be solved.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhongduo

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:

• OWNERS [zhongduo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[zhongduo]

/assign @vaikas

[n3wscott]

I understand why you would want this, but I think this is the wrong direction still. I would rather use a standard header rather than K-Eventing-Http-Attr. If this header is used the likelyhood of this making its way outside of knative is near zero, meaning you can not make any of these requirements a MUST.

For this to get any traction, I think it needs to be brought up in the CloudEvents working group. The spec does not yet get into interop protocol contracts, but they might have some guidance from other standards. Likely they will say eventing should never reply but that is a tangent.

Looking at a standard header: https://tools.ietf.org/html/rfc7240

     POST / HTTP/1.1
     Host: /a/subscriber
     Content-Type: application/json
     Prefer: sink
     Prefer: reply

I could see this working, but it will never be a MUST.

From a source:

     POST / HTTP/1.1
     Host: /a/subscriber
     Content-Type: application/json
     Prefer: sink

From a Broker:

     POST / HTTP/1.1
     Host: /a/subscriber
     Content-Type: application/json
     Prefer: consumer
     Prefer: reply

From a channel sub.subscriber:

     POST / HTTP/1.1
     Host: /a/subscriber
     Content-Type: application/json
     Prefer: consumer
     Prefer: reply

From a channel sub.reply:

     POST / HTTP/1.1
     Host: /a/subscriber
     Content-Type: application/json
     Prefer: consumer

Bikeshed on the prefer value set used...

[n3wscott]

formatting is off

[n3wscott]

Please run a markdown linting tool, table format is off

[zhongduo]

Is there a linter I can use?

[n3wscott]

prettier.io is what knobots use and I use locally

[n3wscott]

s/broker/Broker/

But this is wrong, sinks and sources have no awareness of Broker, they only understand the data plane contract of their object refs or what they implement at the moment. If this was added, they would also understand what the caller is assuming the runtime contract is for the caller and this request.

[zhongduo]

For this to get any traction, I think it needs to be brought up in the CloudEvents working group. The spec does not yet get into interop protocol contracts, but they might have some guidance from other standards. Likely they will say eventing should never reply but that is a tangent.

This is an interesting point. I do remember that in one of the WG meetings, I was told that they found event reply a weird contract. But will we drop the support for event reply if they suggest so?

[zhongduo]

From a source:

     POST / HTTP/1.1
     Host: /a/subscriber
     Content-Type: application/json
     Prefer: sink

This is what we are trying to avoid actually. The reasoning is that the number of brokers and channels are limited now AFAIK and I can add support to all of them. But adding such a requirement to all the custom sources is just impossible. What really matters is the default that the sink assumes without any additional header, as long as all the existing sources remain unchanged and the sink can assume event reply not supported, that should solve the problem.

[zhongduo]

Changes after the discussion in the eventing WG meeting on 2021-01-13: 1) use standard prefer header as a hint to the sink whether event reply is supported, 2) no MUST for compatibility.

@n3wscott WDYT?

[n3wscott]

I think this looks close for me. The last bikeshed thing is what the list of valid prefer values we think are valid. We have had feedback that "callable" is not well understood and we might want to think about breaking a full broker into two Prefers, like "sink" and "reply" as I was showing in my last comment

[zhongduo]

I think this looks close for me. The last bikeshed thing is what the list of valid prefer values we think are valid. We have had feedback that "callable" is not well understood and we might want to think about breaking a full broker into two Prefers, like "sink" and "reply" as I was showing in my last comment

I guess Prefer: sink is not really necessary as all the receivers have to be a sink in the first place to be able to accept event. How about skipping the Perfer: sink and use Prefer: reply only, as in the latest commit?

[zhongduo]

@n3wscott will it be a concern if later reply becomes a standard Preference?

[n3wscott]

/lgtm

[csantanapr]

@zhongduo Where are the docs that explain this for the user?
Please link to it on this issue, I got to this issue from the release notes, but I don't see a link on this issue to the final docs for end-user consumption.