Add Prefer: reply Header to subscriber request if spec.reply is set

[salaboy]

Fixes #4127

Proposed Changes

🎁 Add new feature

This PR adds an additional header to the subscriber destination when a reply is specified, indicating the target service that a reply is preferred.

This doesn't forward the header to the reply request, it is only added to the destination indicated in the subscriber.

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

The Header `Prefer: reply` is added to the request sent to a Subscription Subscriber if the `spec.reply` field is set. 

Docs

[codecov]

Codecov Report

Merging #5764 (adbd44a) into main (ce818dc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5764   +/-   ##
=======================================
  Coverage   82.52%   82.52%           
=======================================
  Files         203      203           
  Lines        6374     6376    +2     
=======================================
+ Hits         5260     5262    +2     
  Misses        768      768           
  Partials      346      346           

Impacted Files	Coverage Δ
pkg/channel/message_dispatcher.go	79.36% <100.00%> (+0.33%)	⬆️

---

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 ce818dc...adbd44a. Read the comment docs.

[knative-prow-robot]

@odacremolbap: changing LGTM is restricted to collaborators

In response to this:

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.

[odacremolbap]

/assign @lberk

[lionelvillard]

/hold, just for a bit. I'm not 100% sure that's what we want. The spec is ambiguous.

[evankanderson]

Can you clarify the ambiguity in the spec? I'm happy to help clean it up; the goal was that a receiving function should be able to tell whether a reply event is going to be dropped on the floor or handled further by the infrastructure.

Receiving functions aren't required to take advantage of this knowledge, but without offering that information it's hard for a programmer to defend (log, raise an alert) against message loss due to misconfiguration.

[lionelvillard]

@evankanderson it's not totally clear to me whatsenders MUST indicate support for replies exactly means. For instance, Subscription supports replies because of the reply field or because the reply field is not-empty? I think we want the former, to support the filtering use case (i.e. filtering the reply event). If Prefer: reply is not sent to the recipient, then the recipient can react by directly forwarding the reply event, bypassing the sender.

From the spec: If a sender will process a reply event it MUST include the Prefer: reply header on the POST request

process can also be interpreted as "I'm aware of the reply event but the developer didn't specify where to send reply event so I'm going assume they want the event to be filtered out".

I vaguely remember we talked about the filtering use case in the past, but I don't remember what was decided. I should go back to the spec PR.

[odacremolbap]

Subscription supports replies because of the reply field or because the reply field is not-empty? I think we want the former

Sorry, I don't follow here. Do you mean the subscription's spec.reply field, versus the Prefer: reply attribute?

I might be missing context but this is what I expect from this feature: a sink developer can check the Prefer header to produce or not a payload response back to the sender or not, avoiding flooding of messages when replies are not welcomed. For that to happen this PR does the job, if there is a reply informed at this subscription tell the subscriptor that it is worth it to reply a payload and not just an ACK/NACK.

I vaguely remember we talked about the filtering use case in the past, but I don't remember what was decided. I should go back to the spec PR.

The sink developer also needs to make sure that type, source and maybe other attributes are semantically valid, or at least append a .response to the type and avoid event loops. I wouldn't use the Prefer: reply for filtering, sounds more like a hack than a real solution. Most probably users will want to filter by type than for a Prefer attribute.

The only thing I'm wondering about this PR: should the Prefer attribute be cleaned if it is present at the incoming event but the reply is empty?

[lionelvillard]

Sorry, I don't follow here. Do you mean the subscription's spec.reply field, versus the Prefer: reply attribute?

no. I mean the existence of the spec.reply field (which implies Subscription supports reply events) vs spec.reply being left empty (i.e unspecified).

a sink developer can check the Prefer header to produce or not a payload response back to the sender or not, avoiding flooding of messages when replies are not welcomed

This is not the primary use case for Prefer: reply. From the design doc:

The Sink can detect during the request if the FE (i.e the sender) supports response events and choose how to react. The Sink is not required to respond with an error; it may be capable of using a different mechanism to deliver its event, such as another Sink endpoint.

The recipient cannot decide to not produce a reply event just because the sender does not support forwarding reply event.

I wouldn't use the Prefer: reply for filtering, sounds more like a hack than a real solution.

That's not what it is suggested here. Leaving spec.reply empty is the mechanic for filtering events.

[benmoss]

/hold

[evankanderson]

Subscription supports replies because of the reply field or because the reply field is not-empty? I think we want the former

Sorry, I don't follow here. Do you mean the subscription's spec.reply field, versus the Prefer: reply attribute?

I think the (valid) question is how should events delivered by a Subscription with an empty spec.reply be handled? Is an empty spec.reply equivalent to http://drop.events/ignored, or should a spec.subscriber be able to determine if spec.reply is set on a particular Subscription by examining the Prefer: reply header?

My gut is that we should always set the HTTP Header for spec.subscriber, but I'd be interested in other opinions / use cases / problems for either decision.

The only thing I'm wondering about this PR: should the Prefer attribute be cleaned if it is present at the incoming event but the reply is empty?

This should be an HTTP Header, not a cloudevent attribute, so it shouldn't be forwarded by Channels or Brokers.

[salaboy]

When I was looking at the issue and I was making the changes I put myself in the shoes of a developer who is consuming these requests. From that perspective, this change would help me to decide (and give me a hint) if I need to return a CloudEvent or not. I find that valuable and it follows the spec statements. I am not sure that it was the scope of this issue to review how replies should work.

My two cents..

[odacremolbap]

I'm with Mauricio, a Sink should have the chance to act different depending if a reply destination for the subscription involved in the communication is configured or not.

@evankanderson are you suggesting that we send something like a Prefer: noreply header when the subscription spec.reply is empty? If that's the case, I would up-vote that, I find it nicely explicit.

[salaboy]

@odacremolbap I also like that Prefer: noreply but I wouldn't add that change now, as it is not stated in the spec. And we have identified the misalignment with the spec blocker for GA. Maybe we can create another issue to propose that to be added to the spec.

[lionelvillard]

I think the (valid) question is how should events delivered by a Subscription with an empty spec.reply be handled? Is an empty spec.reply equivalent to http://drop.events/ignored, or should a spec.subscriber be able to determine if spec.reply is set on a particular Subscription by examining the Prefer: reply header?

We probably want both.

Prefer: reply is about the first one. It's about safely handling reply events. For instance, sources don't support forwarding events so without Prefer: reply, reply events sent back by source recipients are silently dropped (bad).

@salaboy and @odacremolbap are proposing a new header, which, if I understand correctly, is about optimizing the event delivery by dropping reply events on the recipient side when reply is empty. This is a nice optimization, specially for large responses.

My gut is that we should always set the HTTP Header for spec.subscriber

I agree.

I am not sure that it was the scope of this issue to review how replies should work.

Agree. How reply works is well-defined. It's just not safe without Prefer: reply.

this change would help me to decide (and give me a hint) if I need to return a CloudEvent or not.

Hint = optimization, meaning you can safely ignore it and no events will be silently dropped. If you ignore Prefer: reply then you risk losing events. So I think we are on the same page. Correct me if I'm wrong.

Moving forward:

• @salaboy, @odacremolbap, if you agree, this PR should be modified to always pass Prefer: reply, independently of reply's value
• @evankanderson assuming we are all on the same page, we may want to improve the description about Prefer: reply's semantic. I'm not sure yet what should be done, I'm open to suggestions. An idea is to add an example of a sender (e.g. Subscription) supporting the reply pattern. WDYT?

[odacremolbap]

For instance, sources don't support forwarding events so without Prefer: reply, reply events sent back by source recipients are silently dropped (bad).

Now yes we are on the same page, I completely overlooked the problem statement at the proposal doc.

I don't have an strong opinion on whether the response when spec.reply is empty should be Prefer: reply, but I slightly tend to think it should not, just to avoid breaking changes if we decide to introduce Prefer: noreply (or some other header) in the future when a subscription with no reply has been configured.

For the time being I'm in with what @lionelvillard suggests.

[evankanderson]

I hadn't seen the optimization case, but I agree that at this time we can't use the lack of a Prefer: reply request header to elide a response (it will require N rollouts to rely on that functionality). At the same time, requiring every Source to do extra work (add a header) to be efficient doesn't seem like a great trade-off.

Would it make sense to change the spec to require the header on all requests that will forward the reply, and then key Prefer: reply on the presence of a resolved reply destination (the Broker or spec.reply)?

[evankanderson]

To be clear, I'm fine with what y'all agree on, and will help clarify the spec to match, with this discussion as reference.

[odacremolbap]

Would it make sense to change the spec to require the header on all requests that will forward the reply, and then key Prefer: reply on the presence of a resolved reply destination (the Broker or spec.reply)?

For me that would be almost perfect, but I also understand that I am focusing on that optimization.
Would that spec change mean sending an empty Prefer: header for all requests that are able to forward the reply? Something like:

• replies accepted by FE component, reply element informed, reply destination resolved. Prefer: reply
• replies accepted by FE component, reply element informed, reply destination not resolved. Prefer:
• replies accepted by FE component, reply element not informed. Prefer:
• replies not accepted by FE component (example: sources). <no prefer header>

If that's the case, I get the impression that header with an empty value is an anti-pattern but taking a glimpse at RFCs I cannot find anything against it.

Elaborating on the meaning of headers being received at sinks it would be:

• <no prefer header>: undefined behavior, could be that replies are discarded or that a pre-GA knative release is being used.
• Prefer: <empty value, or any value without the 'reply' in it>: any produced reply would be discarded, it is up to the developer to find an alternative path for that situation. (I would rather have Prefer: no-reply)
• Prefer: reply: responses are being taken care of.

[salaboy]

@lionelvillard @evankanderson @odacremolbap to summarize, for this change we are only going to add the Prefer: reply header to all requests, then we might change the spec and we might add other options like Prefer: no-reply

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/channel/message_dispatcher.go	87.0%	87.2%	0.2

[lionelvillard]

Would it make sense to change the spec to require the header on all requests that will forward the reply, and then key Prefer: reply on the presence of a resolved reply destination (the Broker or spec.reply)?

I'm not 100% sure I understand what you mean by then key Prefer: reply. Do you mean adding parameters, for instance Prefer: reply; omitted: true ? I could also be Prefer: reply="resolved-uri"

@odacremolbap Prefer: cannot be empty. The recipient needs to know what is the sender preference. Besides that, I think you covered it all. thanks!

I wonder if Prefer is the right header, after all it's not a preference, it's an indication of capabilities. Unfortunately Expect: does not work either since its value range is limited. Anyway, not perfect but close enough :-)

[lionelvillard]

/approve
/lgtm

[lionelvillard]

@benmoss do you want to TAL, since you also put an hold on it.

[benmoss]

/hold, just for a bit. I'm not 100% sure that's what we want. The spec is ambiguous.

@benmoss do you want to TAL, since you also put an hold on it.

I was putting my hold on just because your hold failed 😄

/hold cancel
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmoss, gabo1208, lionelvillard, odacremolbap, salaboy

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 [lionelvillard]

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

[pierDipi]

IIUC, this should be forwarded by brokers too, right?

[lionelvillard]

yes

[pierDipi]

Thanks, then I'd expect this new header to be added here:

eventing/pkg/broker/filter/filter_handler.go

Lines 221 to 252 in 609741f

	func (h *Handler) sendEvent(ctx context.Context, headers http.Header, target string, event *cloudevents.Event, reporterArgs *ReportArgs) (*http.Response, error) {
	// Send the event to the subscriber
	req, err := h.sender.NewCloudEventRequestWithTarget(ctx, target)
	if err != nil {
	return nil, fmt.Errorf("failed to create the request: %w", err)
	}
	message := binding.ToMessage(event)
	defer message.Finish(nil)
	additionalHeaders := utils.PassThroughHeaders(headers)
	err = kncloudevents.WriteHTTPRequestWithAdditionalHeaders(ctx, message, req, additionalHeaders)
	if err != nil {
	return nil, fmt.Errorf("failed to write request: %w", err)
	}
	start := time.Now()
	resp, err := h.sender.Send(req)
	dispatchTime := time.Since(start)
	if err != nil {
	err = fmt.Errorf("failed to dispatch message: %w", err)
	}
	sc := 0
	if resp != nil {
	sc = resp.StatusCode
	}
	_ = h.reporter.ReportEventDispatchTime(reporterArgs, sc, dispatchTime)
	return resp, err
	}

or allow the header to pass through the filter deployment:

eventing/pkg/utils/headers.go

Lines 28 to 63 in 609741f

	var (
	// These MUST be lowercase strings, as they will be compared against lowercase strings.
	forwardHeaders = sets.NewString(
	// tracing
	"x-request-id",
	)
	// These MUST be lowercase strings, as they will be compared against lowercase strings.
	// Removing CloudEvents ce- prefixes on purpose as they should be set in the CloudEvent itself as extensions.
	// Then the SDK will set them as ce- headers when sending them through HTTP. Otherwise, when using replies we would
	// duplicate ce- headers.
	forwardPrefixes = []string{
	// knative
	"knative-",
	}
	)
	// PassThroughHeaders extracts the headers from headers that are in the `forwardHeaders` set
	// or has any of the prefixes in `forwardPrefixes`.
	func PassThroughHeaders(headers http.Header) http.Header {
	h := http.Header{}
	for n, v := range headers {
	lower := strings.ToLower(n)
	if forwardHeaders.Has(lower) {
	h[n] = v
	continue
	}
	for _, prefix := range forwardPrefixes {
	if strings.HasPrefix(lower, prefix) {
	h[n] = v
	break
	}
	}
	}
	return h
	}

[pierDipi]

Also, this new feature is missing real E2E or conformance tests.

[lionelvillard]

Indeed, thanks @pierDipi !

@salaboy wanna work on a follow-up PR?

[salaboy]

@lionelvillard @pierDipi issue created: #5771 I need help labeling the issue, as I don't have enough permissions to do so