Remove double invocations to responseWriter.WriteHeader in filter handler

[slinkydeveloper]

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Fixes #4464

Proposed Changes

• Removed double invocations to responseWriter.WriteHeader in filter handler
• Testing in filter_handler_test.go if WriteHeader is invoked more than once

[codecov]

Codecov Report

Merging #4466 into master will increase coverage by 0.07%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #4466      +/-   ##
==========================================
+ Coverage   81.19%   81.27%   +0.07%     
==========================================
  Files         281      282       +1     
  Lines        7981     8004      +23     
==========================================
+ Hits         6480     6505      +25     
  Misses       1112     1112              
+ Partials      389      387       -2     

Impacted Files	Coverage Δ
pkg/mtbroker/filter/filter_handler.go	79.51% <80.00%> (+0.99%)	⬆️
pkg/kncloudevents/message_sender.go	78.00% <0.00%> (-1.67%)	⬇️
pkg/channel/message_dispatcher.go	77.31% <0.00%> (-0.24%)	⬇️
...econciler/inmemorychannel/dispatcher/controller.go	78.26% <0.00%> (ø)
pkg/kncloudevents/http_client.go	100.00% <0.00%> (ø)
...iler/inmemorychannel/dispatcher/inmemorychannel.go	89.39% <0.00%> (+0.86%)	⬆️

---

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 7bf5d1c...db64ed6. Read the comment docs.

[tommyreddad]

/assign

[tommyreddad]

I'm just a bit concerned that some of the error cases which this is intended to fix don't really have coverage. So I'm not sure the extent to which this is fully tested. Still, to my eyes this looks good!

/lgtm

[slinkydeveloper]

I'm just a bit concerned that some of the error cases which this is intended to fix don't really have coverage. So I'm not sure the extent to which this is fully tested. Still, to my eyes this looks good!

@tommyreddad Can you be more specific on which error case? I can try to cover them all, although the real issue was that the writeResponse method wasn't writing the response in some cases. And in the caller of writeResponse, we were trying anyway to write the headers

/hold

[tommyreddad]

I'm just a bit concerned that some of the error cases which this is intended to fix don't really have coverage. So I'm not sure the extent to which this is fully tested. Still, to my eyes this looks good!

@tommyreddad Can you be more specific on which error case? I can try to cover them all, although the real issue was that the writeResponse method wasn't writing the response in some cases. And in the caller of writeResponse, we were trying anyway to write the headers

/hold

Just examining the coverage report, the following cases don't seem covered:

• eventing/pkg/mtbroker/filter/filter_handler.go

  Lines 285 to 289 in 5447329

  	// Like in the above case, we could just use StatusInternalServerError, but to distinguish
  	// between the failure cases, we use a different code here.
  	writer.WriteHeader(http.StatusBadGateway)
  	// Malformed event, reply with err
  	return resp.StatusCode, err

• eventing/pkg/mtbroker/filter/filter_handler.go

  Lines 294 to 295 in 5447329

  	writer.WriteHeader(http.StatusInternalServerError)
  	return http.StatusInternalServerError, fmt.Errorf("failed to reset TTL: %w", err)

• eventing/pkg/mtbroker/filter/filter_handler.go

  Line 302 in 5447329

  return http.StatusInternalServerError, fmt.Errorf("failed to write response event: %w", err)

[slinkydeveloper]

@tommyreddad I covered only the case 1, the case 2 is infallible and for the case 3 failure is very hard to simulate (a failure that comes to my mind is a broken i/o pipe to simulate), not really worth it. I think now we're well covered

/unhold

[pierDipi]

/lgtm

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, slinkydeveloper

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 [pierDipi,slinkydeveloper]

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

[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/mtbroker/filter/filter_handler.go	87.1%	87.2%	0.1

[matzew]

little bit late w/ the reviews, but most has been covered already

LGTM

[slinkydeveloper]

FYI @cardil https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_eventing/4466/pull-knative-eventing-upgrade-tests/1324390060111433728#1:build-log.txt%3A2292

/retest

[slinkydeveloper]

Porting to 0.17 and 0.18

[slinkydeveloper]

0.17 port: #4470
0.18 port: #4471