Enable OPTIONS and CloudEvent Webhook headers by evankanderson · Pull Request #5542 · knative/eventing

evankanderson · 2021-06-22T19:58:12Z

I'm still working on e2e testing / getting the e2e tests to run at all in my environment, but I wanted to put this out there.

Proposed Changes

🎁 Make endpoints RFC 7231 (HTTP) compliant by reporting Allow header on 405.
🎁 Add simple support for CloudEvents Webhook spec

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

/hold work-in-progress

Release Note

Adds support for HTTP OPTIONS and CloudEvents Webhook preflight

Docs

Should not have user-visible impact. Does have specs-visible impact, aligns with knative/specs#25

codecov · 2021-06-22T20:05:10Z

Codecov Report

Merging #5542 (c58bf70) into main (dcbb9af) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5542      +/-   ##
==========================================
+ Coverage   82.73%   82.76%   +0.03%     
==========================================
  Files         200      200              
  Lines        6243     6256      +13     
==========================================
+ Hits         5165     5178      +13     
  Misses        748      748              
  Partials      330      330

Impacted Files	Coverage Δ
pkg/broker/filter/filter_handler.go	`80.53% <100.00%> (+0.13%)`	⬆️
pkg/broker/ingress/ingress_handler.go	`75.92% <100.00%> (+1.41%)`	⬆️
pkg/channel/message_receiver.go	`70.51% <100.00%> (+2.45%)`	⬆️

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 dcbb9af...c58bf70. Read the comment docs.

evankanderson · 2021-06-22T23:58:14Z

It looks like I'm going to need to do some surgery on test/lib/send_event.go to be able to send an OPTIONS request; might get to this later tonight.

evankanderson · 2021-06-23T13:57:55Z

/hold cancel

matzew · 2021-06-23T15:07:21Z

 }

 func (h *Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) {
+	writer.Header().Set("Allow", "POST, OPTIONS")


should the spec say something about OPTIONS ? https://github.com/knative/specs/blob/main/specs/eventing/broker.md#ingress

http://github.com/knative/specs/pull/25, data-plane.md, line 88.

(I started working ahead of that commit landing)

matzew · 2021-06-23T15:09:39Z

 }

 func (r *MessageReceiver) ServeHTTP(response nethttp.ResponseWriter, request *nethttp.Request) {
+	response.Header().Set("Allow", "POST")


Header().Set("Allow", "POST, OPTIONS")

? or why is that different here

Because I'm an idiot.

Thanks!

(And, if I'd gotten all my e2e testing working, I'd have noticed this.

It actually turns out that my tests are all wrong, because receiverFunc is not called at all for OPTIONS requests.

Fixed.

benmoss · 2021-06-30T13:50:19Z

Looks like you need the latest go-licenses to fix the filemode changes

…ions

matzew · 2021-07-22T13:23:42Z

@evankanderson any updates on this PR ?

…ions

evankanderson · 2021-07-27T22:41:33Z

Fixed the formatting bugs; not sure how they got in there.

knative-metrics-robot · 2021-07-27T22:46:26Z

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/broker/filter/filter_handler.go	88.5%	88.5%	0.1
pkg/broker/ingress/ingress_handler.go	84.4%	85.4%	1.0
pkg/channel/message_receiver.go	75.0%	77.3%	2.3

matzew

/lgtm
/approve

knative-prow-robot · 2021-08-06T08:49:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson,matzew]

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

Enable OPTIONS and CloudEvent Webhook headers

5697ba4

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2021

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 22, 2021

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 22, 2021

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Jun 23, 2021

evankanderson force-pushed the you-leave-me-no-options branch from f1b0374 to a8319d8 Compare June 23, 2021 06:36

Add OPTIONS e2e test

40924fd

evankanderson force-pushed the you-leave-me-no-options branch from a8319d8 to 40924fd Compare June 23, 2021 06:40

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2021

matzew reviewed Jun 23, 2021

View reviewed changes

Fix Channel OPTIONS test

580ac67

matzew mentioned this pull request Jun 24, 2021

Reorganize RequestMapper and friends knative-extensions/eventing-kafka-broker#1041

Merged

Merge remote-tracking branch 'upstream/main' into you-leave-me-no-opt…

6705b9c

…ions

Evan Anderson added 2 commits July 26, 2021 10:30

Merge remote-tracking branch 'upstream/main' into you-leave-me-no-opt…

ff14468

…ions

gofmt

c58bf70

matzew approved these changes Aug 6, 2021

View reviewed changes

knative-prow-robot assigned matzew Aug 6, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2021

knative-prow-robot merged commit e7445c5 into knative:main Aug 6, 2021

pierDipi mentioned this pull request Aug 9, 2021

Enable OPTIONS and CloudEvent Webhook headers knative-extensions/eventing-kafka-broker#1131

Open

Shashankft9 mentioned this pull request Sep 17, 2021

prometheus metrics availability fixed knative-extensions/eventing-ceph#160

Merged

Conversation

evankanderson commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Pre-review Checklist

Uh oh!

codecov Bot commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evankanderson commented Jun 22, 2021

Uh oh!

evankanderson commented Jun 23, 2021

Uh oh!

matzew Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

matzew Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

evankanderson Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

benmoss commented Jun 30, 2021

Uh oh!

matzew commented Jul 22, 2021

Uh oh!

evankanderson commented Jul 27, 2021

Uh oh!

knative-metrics-robot commented Jul 27, 2021

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

evankanderson commented Jun 22, 2021 •

edited

Loading

codecov Bot commented Jun 22, 2021 •

edited

Loading