From 5697ba4e2064556b2112ef580744ac42dda813d5 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Fri, 18 Jun 2021 11:15:16 -0700 Subject: [PATCH 1/4] Enable OPTIONS and CloudEvent Webhook headers --- pkg/broker/filter/filter_handler.go | 1 + pkg/broker/ingress/ingress_handler.go | 7 ++++++ pkg/broker/ingress/ingress_handler_test.go | 27 ++++++++++++++-------- pkg/channel/message_receiver.go | 7 ++++++ pkg/channel/message_receiver_test.go | 20 ++++++++++++++++ 5 files changed, 53 insertions(+), 9 deletions(-) diff --git a/pkg/broker/filter/filter_handler.go b/pkg/broker/filter/filter_handler.go index 72012efcad2..215c0d3f498 100644 --- a/pkg/broker/filter/filter_handler.go +++ b/pkg/broker/filter/filter_handler.go @@ -105,6 +105,7 @@ func (h *Handler) Start(ctx context.Context) error { // 5. send event to trigger's subscriber // 6. write the response func (h *Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { + writer.Header().Set("Allow", "POST") if request.Method != http.MethodPost { writer.WriteHeader(http.StatusMethodNotAllowed) diff --git a/pkg/broker/ingress/ingress_handler.go b/pkg/broker/ingress/ingress_handler.go index 6468e888027..047b9c5c389 100644 --- a/pkg/broker/ingress/ingress_handler.go +++ b/pkg/broker/ingress/ingress_handler.go @@ -101,7 +101,14 @@ func (h *Handler) Start(ctx context.Context) error { } func (h *Handler) ServeHTTP(writer http.ResponseWriter, request *http.Request) { + writer.Header().Set("Allow", "POST, OPTIONS") // validate request method + if request.Method == http.MethodOptions { + writer.Header().Set("WebHook-Allowed-Origin", "*") // Accept from any Origin: + writer.Header().Set("WebHook-Allowed-Rate", "*") // Unlimited requests/minute + writer.WriteHeader(http.StatusOK) + return + } if request.Method != http.MethodPost { h.Logger.Warn("unexpected request method", zap.String("method", request.Method)) writer.WriteHeader(http.StatusMethodNotAllowed) diff --git a/pkg/broker/ingress/ingress_handler_test.go b/pkg/broker/ingress/ingress_handler_test.go index ab7e0a464ae..3de13349f46 100644 --- a/pkg/broker/ingress/ingress_handler_test.go +++ b/pkg/broker/ingress/ingress_handler_test.go @@ -104,20 +104,29 @@ func TestHandler_ServeHTTP(t *testing.T) { defaulter: broker.TTLDefaulter(logger, 100), }, { - name: "invalid method OPTIONS", - method: nethttp.MethodOptions, - uri: "/ns/name", - body: getValidEvent(), - statusCode: nethttp.StatusMethodNotAllowed, + name: "valid method OPTIONS", + method: nethttp.MethodOptions, + uri: "/ns/name", + body: strings.NewReader(""), + expectedHeaders: nethttp.Header{ + "Allow": []string{"PUT, OPTIONS"}, + "WebHook-Allowed-Origin": []string{"*"}, + "WebHook-Allowed-Rate": []string{"*"}, + "Content-Length": []string{"0"}, + }, + statusCode: nethttp.StatusOK, handler: handler(), reporter: &mockReporter{}, defaulter: broker.TTLDefaulter(logger, 100), }, { - name: "valid (happy path)", - method: nethttp.MethodPost, - uri: "/ns/name", - body: getValidEvent(), + name: "valid (happy path POST)", + method: nethttp.MethodPost, + uri: "/ns/name", + body: getValidEvent(), + expectedHeaders: nethttp.Header{ + "Allow": []string{"PUT, OPTIONS"}, + }, statusCode: senderResponseStatusCode, handler: handler(), reporter: &mockReporter{StatusCode: senderResponseStatusCode, EventDispatchTimeReported: true}, diff --git a/pkg/channel/message_receiver.go b/pkg/channel/message_receiver.go index 897580250bd..70160db9855 100644 --- a/pkg/channel/message_receiver.go +++ b/pkg/channel/message_receiver.go @@ -136,6 +136,13 @@ func (r *MessageReceiver) Start(ctx context.Context) error { } func (r *MessageReceiver) ServeHTTP(response nethttp.ResponseWriter, request *nethttp.Request) { + response.Header().Set("Allow", "POST") + if request.Method == nethttp.MethodOptions { + response.Header().Set("WebHook-Allowed-Origin", "*") // Accept from any Origin: + response.Header().Set("WebHook-Allowed-Rate", "*") // Unlimited requests/minute + response.WriteHeader(nethttp.StatusOK) + return + } if request.Method != nethttp.MethodPost { response.WriteHeader(nethttp.StatusMethodNotAllowed) return diff --git a/pkg/channel/message_receiver_test.go b/pkg/channel/message_receiver_test.go index 58a586626cd..abe7ac5f9ad 100644 --- a/pkg/channel/message_receiver_test.go +++ b/pkg/channel/message_receiver_test.go @@ -121,6 +121,26 @@ func TestMessageReceiver_ServeHTTP(t *testing.T) { }, expected: nethttp.StatusAccepted, }, + "OPTIONS okay": { + method: nethttp.MethodOptions, + host: "test-name.test-namespace.svc." + network.GetClusterDomainName(), + receiverFunc: func(ctx context.Context, r ChannelReference, m binding.Message, transformers []binding.Transformer, additionalHeaders nethttp.Header) error { + if r.Namespace != "test-namespace" || r.Name != "test-name" { + return fmt.Errorf("test receiver func -- bad reference: %v", r) + } + expectedHeaders := nethttp.Header{ + "Allow": []string{"POST, OPTIONS"}, + "WebHook-Allowed-Origin": []string{"*"}, + "WebHook-Allowed-Rate": []string{"*"}, + "Content-Length": []string{"0"}, + } + if diff := cmp.Diff(expectedHeaders, additionalHeaders); diff != "" { + return fmt.Errorf("test receiver func -- bad OPTION headers (-want, +got): %s", diff) + } + return nil + }, + expected: nethttp.StatusOK, + }, } reporter := NewStatsReporter("testcontainer", "testpod") for n, tc := range testCases { From 40924fdb856dd0de2ff84cf9d831028f1cdc67c4 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Tue, 22 Jun 2021 23:29:12 -0700 Subject: [PATCH 2/4] Add OPTIONS e2e test --- .../helpers/broker_data_plane_test_helper.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/test/conformance/helpers/broker_data_plane_test_helper.go b/test/conformance/helpers/broker_data_plane_test_helper.go index a88d73a8c95..178e6b26874 100644 --- a/test/conformance/helpers/broker_data_plane_test_helper.go +++ b/test/conformance/helpers/broker_data_plane_test_helper.go @@ -84,7 +84,7 @@ func BrokerDataPlaneNamespaceSetupOption(ctx context.Context, namespace string) //Supports structured or Binary mode //Respond with 2xx on good CE //Respond with 400 on bad CE -//Reject non-POST requests to publish URI +//Reject non-POST, non-OPTIONS requests to publish URI (beyond spec?!) func BrokerIngressDataPlaneTestHelper( ctx context.Context, t *testing.T, @@ -108,6 +108,22 @@ func BrokerIngressDataPlaneTestHelper( resources.WithSubscriberServiceRefForTrigger(loggerName), ) + st.Run("Ingress supports OPTIONS", func(t *testing.T) { + // TODO(evana): check "Allow" header and other options. + // This may be simpler if the setup requires a local HTTP proxy which can reach into the cluster. + method := sender.WithMethod("OPTIONS") + responseSink := "http://" + client.GetServiceHost(loggerName) + responseForward := sender.WithResponseSink(responseSink) + senderName := "options-test-sender" + client.SendRequestToAddressable(ctx, senderName, broker.Name, testlib.BrokerTypeMeta, map[string]string{}, "", method, responseForward) + client.WaitForResourceReadyOrFail(senderName, &podMeta) + eventTracker.AssertAtLeast(1, recordevents.MatchEvent(cetest.AllOf( + cetest.AnyOf( + sender.MatchStatusCode(200), sender.MatchStatusCode(405)), + cetest.HasSource("https://knative.dev/eventing/test/event-sender"), + ))) + }) + client.WaitForResourceReadyOrFail(trigger.Name, testlib.TriggerTypeMeta) st.Run("Ingress Supports CE0.3", func(t *testing.T) { eventID := "CE0.3" From 580ac67a8240ea379458ade25a2b17ede25d50b7 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Wed, 23 Jun 2021 15:22:27 -0700 Subject: [PATCH 3/4] Fix Channel OPTIONS test --- pkg/channel/message_receiver.go | 2 +- pkg/channel/message_receiver_test.go | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/channel/message_receiver.go b/pkg/channel/message_receiver.go index 70160db9855..7be4aa273fc 100644 --- a/pkg/channel/message_receiver.go +++ b/pkg/channel/message_receiver.go @@ -136,7 +136,7 @@ func (r *MessageReceiver) Start(ctx context.Context) error { } func (r *MessageReceiver) ServeHTTP(response nethttp.ResponseWriter, request *nethttp.Request) { - response.Header().Set("Allow", "POST") + response.Header().Set("Allow", "POST, OPTIONS") if request.Method == nethttp.MethodOptions { response.Header().Set("WebHook-Allowed-Origin", "*") // Accept from any Origin: response.Header().Set("WebHook-Allowed-Rate", "*") // Unlimited requests/minute diff --git a/pkg/channel/message_receiver_test.go b/pkg/channel/message_receiver_test.go index abe7ac5f9ad..4a4b38d55b0 100644 --- a/pkg/channel/message_receiver_test.go +++ b/pkg/channel/message_receiver_test.go @@ -53,6 +53,7 @@ func TestMessageReceiver_ServeHTTP(t *testing.T) { additionalHeaders nethttp.Header expected int receiverFunc UnbufferedMessageReceiverFunc + responseValidator func(r httptest.ResponseRecorder) error }{ "non '/' path": { path: "/something", @@ -124,22 +125,18 @@ func TestMessageReceiver_ServeHTTP(t *testing.T) { "OPTIONS okay": { method: nethttp.MethodOptions, host: "test-name.test-namespace.svc." + network.GetClusterDomainName(), - receiverFunc: func(ctx context.Context, r ChannelReference, m binding.Message, transformers []binding.Transformer, additionalHeaders nethttp.Header) error { - if r.Namespace != "test-namespace" || r.Name != "test-name" { - return fmt.Errorf("test receiver func -- bad reference: %v", r) - } + expected: nethttp.StatusOK, + responseValidator: func(res httptest.ResponseRecorder) error { expectedHeaders := nethttp.Header{ "Allow": []string{"POST, OPTIONS"}, - "WebHook-Allowed-Origin": []string{"*"}, - "WebHook-Allowed-Rate": []string{"*"}, - "Content-Length": []string{"0"}, + "Webhook-Allowed-Origin": []string{"*"}, + "Webhook-Allowed-Rate": []string{"*"}, } - if diff := cmp.Diff(expectedHeaders, additionalHeaders); diff != "" { + if diff := cmp.Diff(expectedHeaders, res.Header()); diff != "" { return fmt.Errorf("test receiver func -- bad OPTION headers (-want, +got): %s", diff) } return nil }, - expected: nethttp.StatusOK, }, } reporter := NewStatsReporter("testcontainer", "testpod") @@ -192,6 +189,11 @@ func TestMessageReceiver_ServeHTTP(t *testing.T) { if res.Code != tc.expected { t.Fatalf("Unexpected status code. Expected %v. Actual %v", tc.expected, res.Code) } + if tc.responseValidator != nil { + if err := tc.responseValidator(res); err != nil { + t.Errorf("Incorrect response: %v", err); + } + } }) } } From c58bf703ec9f204c8f1da32393b48e385fc15eee Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Tue, 27 Jul 2021 15:41:15 -0700 Subject: [PATCH 4/4] gofmt --- pkg/channel/message_receiver_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/channel/message_receiver_test.go b/pkg/channel/message_receiver_test.go index 4a4b38d55b0..2c9529001cb 100644 --- a/pkg/channel/message_receiver_test.go +++ b/pkg/channel/message_receiver_test.go @@ -123,8 +123,8 @@ func TestMessageReceiver_ServeHTTP(t *testing.T) { expected: nethttp.StatusAccepted, }, "OPTIONS okay": { - method: nethttp.MethodOptions, - host: "test-name.test-namespace.svc." + network.GetClusterDomainName(), + method: nethttp.MethodOptions, + host: "test-name.test-namespace.svc." + network.GetClusterDomainName(), expected: nethttp.StatusOK, responseValidator: func(res httptest.ResponseRecorder) error { expectedHeaders := nethttp.Header{ @@ -191,7 +191,7 @@ func TestMessageReceiver_ServeHTTP(t *testing.T) { } if tc.responseValidator != nil { if err := tc.responseValidator(res); err != nil { - t.Errorf("Incorrect response: %v", err); + t.Errorf("Incorrect response: %v", err) } } })