From 43d44580fb90f82c6786f13625dce6bb4a30ef2a Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 22 Nov 2018 13:44:30 -0500 Subject: [PATCH 01/18] Use http.DefaultTransport dialer settings in h2c.DefaultTransport --- pkg/http/h2c/h2c.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/http/h2c/h2c.go b/pkg/http/h2c/h2c.go index 8bab9d981309..a522bb051439 100644 --- a/pkg/http/h2c/h2c.go +++ b/pkg/http/h2c/h2c.go @@ -14,6 +14,7 @@ limitations under the License. package h2c import ( + "context" "crypto/tls" "net" "net/http" @@ -42,6 +43,6 @@ func ListenAndServe(addr string, h http.Handler) error { var DefaultTransport http.RoundTripper = &http2.Transport{ AllowHTTP: true, DialTLS: func(netw, addr string, cfg *tls.Config) (net.Conn, error) { - return net.Dial(netw, addr) + return http.DefaultTransport.(*http.Transport).DialContext(context.Background(), netw, addr) }, } From 0e2e7c073d407884c0c8a4789d601028ce666a84 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 22 Nov 2018 18:02:54 -0500 Subject: [PATCH 02/18] Support streaming in activator/util.Rewinder --- pkg/activator/util/rewinder.go | 50 ++++++++++++------- pkg/activator/util/rewinder_test.go | 74 ++++++++++++++++++++--------- 2 files changed, 83 insertions(+), 41 deletions(-) diff --git a/pkg/activator/util/rewinder.go b/pkg/activator/util/rewinder.go index 243e17a23337..9052c061aaf3 100644 --- a/pkg/activator/util/rewinder.go +++ b/pkg/activator/util/rewinder.go @@ -16,44 +16,58 @@ package util import ( "bytes" "io" - "io/ioutil" "sync" ) type rewinder struct { sync.Mutex - rc io.ReadCloser - rs io.ReadSeeker + original io.ReadCloser + current io.Reader + next io.ReadWriter + eof bool } -// rewinder wraps a single-use `ReadCloser` into a `ReadCloser` that can be read multiple times +// rewinder wraps a single-use `ReadCloser` into a `ReadCloser` +// that can be read multiple times func NewRewinder(rc io.ReadCloser) io.ReadCloser { - return &rewinder{rc: rc} + r := &rewinder{original: rc} + + r.next = new(bytes.Buffer) + r.current = r.original + + return r } func (r *rewinder) Read(b []byte) (int, error) { r.Lock() defer r.Unlock() - // On the first `Read()`, the contents of `rc` is read into a buffer `rs`. - // This buffer is used for all subsequent reads - if r.rs == nil { - buf, err := ioutil.ReadAll(r.rc) - if err != nil { - return 0, err - } - r.rc.Close() - - r.rs = bytes.NewReader(buf) + + // Buffer everything we read + n, err := io.TeeReader(r.current, r.next).Read(b) + + // Keep track of when we reach the end + if err == io.EOF { + r.eof = true } - return r.rs.Read(b) + return n, err } func (r *rewinder) Close() error { r.Lock() defer r.Unlock() - // Rewind the buffer on `Close()` for the next call to `Read` - r.rs.Seek(0, io.SeekStart) + + // Close the original ReadCloser if we have read it to the end + if r.eof && r.original != nil { + r.original.Close() + r.original = nil + } + + // Rewind back to the beginning + start := io.MultiReader(r.next, r.current) + + r.next = new(bytes.Buffer) + r.current = start return nil } diff --git a/pkg/activator/util/rewinder_test.go b/pkg/activator/util/rewinder_test.go index 22a4c52c5016..e9c26b8b6ac4 100644 --- a/pkg/activator/util/rewinder_test.go +++ b/pkg/activator/util/rewinder_test.go @@ -13,37 +13,65 @@ limitations under the License. package util import ( - "bytes" + "bufio" + "io" "io/ioutil" "testing" ) func TestRewinder(t *testing.T) { - str := "test string" - rc := &spyReadCloser{Reader: bytes.NewBufferString(str)} - rewinder := NewRewinder(rc) - - b1, err := ioutil.ReadAll(rewinder) - if err != nil { - t.Errorf("Unexpected error reading b1: %v", err) + expectStr := func(want, got string, err error) { + if err != nil { + t.Fatalf("Expected to read string, got error: %v", err) + } + if want != got { + t.Errorf("Unexpected string, want %q, got %q", want, got) + } } - rewinder.Close() - b2, err := ioutil.ReadAll(rewinder) - if err != nil { - t.Errorf("Unexpected error reading b2: %v", err) - } - rewinder.Close() + cont := make(chan bool) + rp, wp := io.Pipe() // Note: wp.Write() blocks until rp.Read() - if string(b1) != str { - t.Errorf("Unexpected str b1. Want %q, got %q", str, b1) - } + rewinder := NewRewinder(rp) + out := bufio.NewReader(rewinder) - if string(b2) != str { - t.Errorf("Unexpected str b2. Want %q, got %q", str, b2) - } + go func() { + t.Log("Writing chunk #1") + wp.Write([]byte("s1:")) - if !rc.Closed { - t.Error("Expected ReadCloser to be closed") - } + t.Log("Writing chunk #2") + wp.Write([]byte("s2:")) + + t.Log("Rewinding") + rewinder.Close() + + cont <- true + + t.Log("Writing chunk #3") + wp.Write([]byte("s3:")) + + t.Log("Closing stream") + wp.Close() + }() + + got, err := out.ReadString(byte(':')) + t.Log("Checking chunk #1") + expectStr("s1:", got, err) + + got, err = out.ReadString(byte(':')) + t.Log("Checking chunk #2") + expectStr("s2:", got, err) + + <-cont + + gotAll, err := ioutil.ReadAll(out) + t.Log("Checking chunks #1-3") + expectStr("s1:s2:s3:", string(gotAll), err) + + t.Log("Rewinding again") + rewinder.Close() + + gotAll, err = ioutil.ReadAll(out) + t.Log("Checking chunks #1-3 again") + expectStr("s1:s2:s3:", string(gotAll), err) } From 3fdab18605905d84bbfed2198f0901403d96feed Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 22 Nov 2018 18:27:01 -0500 Subject: [PATCH 03/18] Use custom timeout handler in queue-proxy The http.TimeoutHandler will buffer the response body in memory until either the request completes or the request times out. This works well for HTTP, but is a problem for HTTP2 and gRPC streaming requests, where responses should be written as each sub-request is processed. This commit enforces the timeout by processing the request in a separate goroutine and panicing with http.ErrAbortHandler if the timeout is reached. The http.ErrAborthandler is a canary error used by the net/http and x/net/http2 packages to gracefully end the connection without dumping the stack trace. --- cmd/queue/main.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index 22761599e59a..fa498c228335 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -164,6 +164,25 @@ func isProbe(r *http.Request) bool { return strings.HasPrefix(r.Header.Get("User-Agent"), "kube-probe/") } +func timeoutHandler(h http.Handler, d time.Duration) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + done := make(chan bool) + + go func() { + h.ServeHTTP(w, r) + + done <- true + }() + + select { + case <-done: + return + case <-time.After(d): + panic(http.ErrAbortHandler) + } + }) +} + func handler(w http.ResponseWriter, r *http.Request) { proxy := proxyForRequest(r) @@ -189,6 +208,7 @@ func handler(w http.ResponseWriter, r *http.Request) { } else { proxy.ServeHTTP(w, r) } + } // Sets up /health and /quitquitquit endpoints. From b2254d921b65daa6ab35f297f59740f0bc171738 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 22 Nov 2018 18:48:04 -0500 Subject: [PATCH 04/18] Use http2 instead of http as the k8s service port names Istio uses port names in k8s services to determine protocols supported by the service. This change allows kservices to support HTTP/2 and gRPC traffic. --- config/400-activator-service.yaml | 2 +- pkg/reconciler/v1alpha1/revision/resources/constants.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/400-activator-service.yaml b/config/400-activator-service.yaml index 95614ad5559e..c9f8e98b063a 100644 --- a/config/400-activator-service.yaml +++ b/config/400-activator-service.yaml @@ -24,7 +24,7 @@ spec: selector: app: activator ports: - - name: http + - name: http2 protocol: TCP port: 80 targetPort: 8080 diff --git a/pkg/reconciler/v1alpha1/revision/resources/constants.go b/pkg/reconciler/v1alpha1/revision/resources/constants.go index a4bf504973d4..e519752f9fe6 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/constants.go +++ b/pkg/reconciler/v1alpha1/revision/resources/constants.go @@ -39,7 +39,7 @@ const ( autoscalerPort = 8080 // ServicePortName is the name of the external port of the service - ServicePortName = "http" + ServicePortName = "http2" // ServicePort is the external port of the service ServicePort = int32(80) // MetricsPortName is the name of the external port of the service for metrics From 79278f3a3ff879c7beb02e50c7a9bbb92e694186 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Fri, 23 Nov 2018 14:03:33 -0500 Subject: [PATCH 05/18] Enforce max content length for streaming requests in activator --- .../handler/enforce_length_handler.go | 22 +++++++ .../handler/enforce_length_handler_test.go | 65 +++++++++++++++++-- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/pkg/activator/handler/enforce_length_handler.go b/pkg/activator/handler/enforce_length_handler.go index b4b2da84bae3..91cb580b9622 100644 --- a/pkg/activator/handler/enforce_length_handler.go +++ b/pkg/activator/handler/enforce_length_handler.go @@ -14,9 +14,27 @@ limitations under the License. package handler import ( + "io" "net/http" ) +func limitReadCloser(rc io.ReadCloser, l int64) io.ReadCloser { + return &readCloser{io.LimitReader(rc, l), rc} +} + +type readCloser struct { + Reader io.Reader + Closer io.Closer +} + +func (lrc *readCloser) Read(b []byte) (int, error) { + return lrc.Reader.Read(b) +} + +func (lrc *readCloser) Close() error { + return lrc.Closer.Close() +} + // EnforceMaxContentLengthHandler prevents uploads larger than `MaxContentLengthBytes` type EnforceMaxContentLengthHandler struct { NextHandler http.Handler @@ -24,10 +42,14 @@ type EnforceMaxContentLengthHandler struct { } func (h *EnforceMaxContentLengthHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // If we have a ContentLength, we can fail early if r.ContentLength > h.MaxContentLengthBytes { w.WriteHeader(http.StatusRequestEntityTooLarge) return } + // Enforce MaxContentLengthBytes + r.Body = limitReadCloser(r.Body, h.MaxContentLengthBytes) + h.NextHandler.ServeHTTP(w, r) } diff --git a/pkg/activator/handler/enforce_length_handler_test.go b/pkg/activator/handler/enforce_length_handler_test.go index b579bc3a54b9..9718b0a9f813 100644 --- a/pkg/activator/handler/enforce_length_handler_test.go +++ b/pkg/activator/handler/enforce_length_handler_test.go @@ -13,51 +13,104 @@ limitations under the License. package handler import ( - "bytes" + "io" + "io/ioutil" "net/http" "net/http/httptest" + "strings" "testing" ) func TestEnforceMaxContentLengthHandler(t *testing.T) { payload := "SAMPLE PAYLOAD" + // httptest.NewRequest will set ContentLength for strings.NewReader + fixed := func(p string) io.Reader { return strings.NewReader(p) } + stream := func(p string) io.Reader { return ioutil.NopCloser(strings.NewReader(p)) } + examples := []struct { label string maxUpload int + request io.Reader + response string status int }{ { - label: "under", + label: "under with ContentLength", maxUpload: len(payload) + 1, + request: fixed(payload), status: http.StatusOK, + response: payload, }, { - label: "equal", + label: "equal with ContentLength", maxUpload: len(payload), + request: fixed(payload), status: http.StatusOK, + response: payload, }, { - label: "over", + label: "over with ContentLength", maxUpload: len(payload) - 1, + request: fixed(payload), status: http.StatusRequestEntityTooLarge, + response: "", + }, + { + label: "under without ContentLength", + maxUpload: len(payload) + 1, + request: stream(payload), + status: http.StatusOK, + response: payload, + }, + { + label: "equal without ContentLength", + maxUpload: len(payload), + request: stream(payload), + status: http.StatusOK, + response: payload, + }, + { + label: "over without ContentLength", + maxUpload: len(payload) - 1, + request: stream(payload), + status: http.StatusOK, + response: payload[0 : len(payload)-1], }, } for _, e := range examples { t.Run(e.label, func(t *testing.T) { + // Return 200 and request body baseHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) + + if r.Body != nil { + body, err := ioutil.ReadAll(r.Body) + if err != nil { + t.Fatalf("Error request reading body") + } + w.Write(body) + } }) handler := EnforceMaxContentLengthHandler{NextHandler: baseHandler, MaxContentLengthBytes: int64(e.maxUpload)} resp := httptest.NewRecorder() - req := httptest.NewRequest("POST", "http://example.com", bytes.NewBufferString(payload)) + req := httptest.NewRequest("POST", "http://example.com", e.request) handler.ServeHTTP(resp, req) + gotBody, err := ioutil.ReadAll(resp.Body) + if err != nil { + t.Fatalf("Error request reading body") + } + + if e.response != string(gotBody) { + t.Errorf("Unexpected response body. Want %q, got %q", e.response, gotBody) + } + if resp.Code != e.status { - t.Errorf("Unexpected response status for payload %q. Want %d, got %d", payload, e.status, resp.Code) + t.Errorf("Unexpected response status. Want %d, got %d", e.status, resp.Code) } }) } From bee2aa4e024eec8a011243d8d7729e6433a39523 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Fri, 23 Nov 2018 19:43:59 -0500 Subject: [PATCH 06/18] Don't read original reader again after rewinder is closed --- pkg/activator/util/io_test.go | 6 +++--- pkg/activator/util/rewinder.go | 13 ++++++++++--- pkg/activator/util/rewinder_test.go | 28 +++++++++++++++++++++++++++ pkg/activator/util/transports_test.go | 4 ++-- 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/pkg/activator/util/io_test.go b/pkg/activator/util/io_test.go index 6c4ed61b1568..f72cffa3057c 100644 --- a/pkg/activator/util/io_test.go +++ b/pkg/activator/util/io_test.go @@ -15,7 +15,7 @@ package util import "io" type spyReadCloser struct { - io.Reader + io.ReadCloser Closed bool ReadAfterClose bool } @@ -25,11 +25,11 @@ func (s *spyReadCloser) Read(b []byte) (n int, err error) { s.ReadAfterClose = true } - return s.Reader.Read(b) + return s.ReadCloser.Read(b) } func (s *spyReadCloser) Close() error { s.Closed = true - return nil + return s.ReadCloser.Close() } diff --git a/pkg/activator/util/rewinder.go b/pkg/activator/util/rewinder.go index 9052c061aaf3..67c73ad3cded 100644 --- a/pkg/activator/util/rewinder.go +++ b/pkg/activator/util/rewinder.go @@ -57,15 +57,22 @@ func (r *rewinder) Close() error { r.Lock() defer r.Unlock() + // Start = what we've read already + what we haven't read yet + start := io.MultiReader(r.next, r.current) + // Close the original ReadCloser if we have read it to the end if r.eof && r.original != nil { + + if r.current == r.original { + // Start = only what we've read already + start = r.next + } + r.original.Close() r.original = nil } - // Rewind back to the beginning - start := io.MultiReader(r.next, r.current) - + // Rewind back to the start r.next = new(bytes.Buffer) r.current = start diff --git a/pkg/activator/util/rewinder_test.go b/pkg/activator/util/rewinder_test.go index e9c26b8b6ac4..7fe46302b4fc 100644 --- a/pkg/activator/util/rewinder_test.go +++ b/pkg/activator/util/rewinder_test.go @@ -16,6 +16,7 @@ import ( "bufio" "io" "io/ioutil" + "strings" "testing" ) @@ -75,3 +76,30 @@ func TestRewinder(t *testing.T) { t.Log("Checking chunks #1-3 again") expectStr("s1:s2:s3:", string(gotAll), err) } + +func TestRewinder_Cleanup(t *testing.T) { + want := "foo" + + in := &spyReadCloser{ReadCloser: ioutil.NopCloser(strings.NewReader(want))} + rewinder := NewRewinder(in) + + got, err := ioutil.ReadAll(rewinder) + if err != nil { + t.Fatalf("Expected to read string, got error: %v", err) + } + if want != string(got) { + t.Errorf("Unexpected string, want %q, got %q", want, got) + } + + rewinder.Close() + + if !in.Closed { + t.Error("Input ReadCloser not closed") + } + + ioutil.ReadAll(rewinder) + + if in.ReadAfterClose { + t.Error("Read() after Close() in input ReadCloser") + } +} diff --git a/pkg/activator/util/transports_test.go b/pkg/activator/util/transports_test.go index 7370e9998839..5bfc737bf5d2 100644 --- a/pkg/activator/util/transports_test.go +++ b/pkg/activator/util/transports_test.go @@ -78,7 +78,7 @@ func TestRetryRoundTripper(t *testing.T) { req := &http.Request{Header: http.Header{}} resp := func(status int) *http.Response { - return &http.Response{StatusCode: status, Body: &spyReadCloser{}} + return &http.Response{StatusCode: status, Body: &spyReadCloser{ReadCloser: ioutil.NopCloser(strings.NewReader(""))}} } someErr := errors.New("some error") @@ -213,7 +213,7 @@ func TestRetryRoundTripperRewind(t *testing.T) { conditions..., ) - spy := &spyReadCloser{Reader: strings.NewReader(bodyContent)} + spy := &spyReadCloser{ReadCloser: ioutil.NopCloser(strings.NewReader(bodyContent))} req, _ := http.NewRequest("POST", "http://test.domain", spy) rt.RoundTrip(req) From a1ca7b164d8d4ec501891e9b5f48c398ebe001df Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 26 Nov 2018 14:58:09 -0500 Subject: [PATCH 07/18] Use chan struct{} instead of chan bool in queue-proxy timeoutHandler --- cmd/queue/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index fa498c228335..8793e1b68a0b 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -166,12 +166,12 @@ func isProbe(r *http.Request) bool { func timeoutHandler(h http.Handler, d time.Duration) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - done := make(chan bool) + done := make(chan struct{}) go func() { - h.ServeHTTP(w, r) + defer close(done) - done <- true + h.ServeHTTP(w, r) }() select { From 5239099905e5a86d2054404c93b20db7a21ebd9c Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 26 Nov 2018 15:57:07 -0500 Subject: [PATCH 08/18] Move LimitReadCloser from activator handler to activator util and add test coverage --- .../handler/enforce_length_handler.go | 22 ++----------- pkg/activator/util/io.go | 33 +++++++++++++++++++ pkg/activator/util/io_test.go | 30 ++++++++++++++++- 3 files changed, 65 insertions(+), 20 deletions(-) create mode 100644 pkg/activator/util/io.go diff --git a/pkg/activator/handler/enforce_length_handler.go b/pkg/activator/handler/enforce_length_handler.go index 91cb580b9622..6ef6156963c4 100644 --- a/pkg/activator/handler/enforce_length_handler.go +++ b/pkg/activator/handler/enforce_length_handler.go @@ -14,26 +14,10 @@ limitations under the License. package handler import ( - "io" "net/http" -) - -func limitReadCloser(rc io.ReadCloser, l int64) io.ReadCloser { - return &readCloser{io.LimitReader(rc, l), rc} -} - -type readCloser struct { - Reader io.Reader - Closer io.Closer -} -func (lrc *readCloser) Read(b []byte) (int, error) { - return lrc.Reader.Read(b) -} - -func (lrc *readCloser) Close() error { - return lrc.Closer.Close() -} + "github.com/knative/serving/pkg/activator/util" +) // EnforceMaxContentLengthHandler prevents uploads larger than `MaxContentLengthBytes` type EnforceMaxContentLengthHandler struct { @@ -49,7 +33,7 @@ func (h *EnforceMaxContentLengthHandler) ServeHTTP(w http.ResponseWriter, r *htt } // Enforce MaxContentLengthBytes - r.Body = limitReadCloser(r.Body, h.MaxContentLengthBytes) + r.Body = util.LimitReadCloser(r.Body, h.MaxContentLengthBytes) h.NextHandler.ServeHTTP(w, r) } diff --git a/pkg/activator/util/io.go b/pkg/activator/util/io.go new file mode 100644 index 000000000000..67df5424df66 --- /dev/null +++ b/pkg/activator/util/io.go @@ -0,0 +1,33 @@ +/* +Copyright 2018 The Knative Authors +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package util + +import "io" + +func LimitReadCloser(rc io.ReadCloser, l int64) io.ReadCloser { + return &readCloser{io.LimitReader(rc, l), rc} +} + +type readCloser struct { + Reader io.Reader + Closer io.Closer +} + +func (lrc *readCloser) Read(b []byte) (int, error) { + return lrc.Reader.Read(b) +} + +func (lrc *readCloser) Close() error { + return lrc.Closer.Close() +} diff --git a/pkg/activator/util/io_test.go b/pkg/activator/util/io_test.go index f72cffa3057c..a54d056d961b 100644 --- a/pkg/activator/util/io_test.go +++ b/pkg/activator/util/io_test.go @@ -12,7 +12,12 @@ limitations under the License. */ package util -import "io" +import ( + "io" + "io/ioutil" + "strings" + "testing" +) type spyReadCloser struct { io.ReadCloser @@ -33,3 +38,26 @@ func (s *spyReadCloser) Close() error { return s.ReadCloser.Close() } + +func TestLimitReadCloser(t *testing.T) { + want := "test" + r := strings.NewReader(want + " foo") + rc := &spyReadCloser{ReadCloser: ioutil.NopCloser(r)} + + lrc := LimitReadCloser(rc, int64(len(want))) + + got, err := ioutil.ReadAll(lrc) + lrc.Close() + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if string(got) != want { + t.Fatalf("Unexpected body. Want %q, got %q", want, got) + } + + if !rc.Closed { + t.Fatalf("Expected ReadCloser to be closed.") + } +} From 4d50862c133fe955558e8f358ad7b85ed2fd96e2 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 26 Nov 2018 19:11:30 -0500 Subject: [PATCH 09/18] Add/fix godoc comments for activator/util --- pkg/activator/util/io.go | 1 + pkg/activator/util/rewinder.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/activator/util/io.go b/pkg/activator/util/io.go index 67df5424df66..d0c87b858f2c 100644 --- a/pkg/activator/util/io.go +++ b/pkg/activator/util/io.go @@ -15,6 +15,7 @@ package util import "io" +// LimitReadCloser returns a ReadCloser wrapped with an `io.LimitReader` func LimitReadCloser(rc io.ReadCloser, l int64) io.ReadCloser { return &readCloser{io.LimitReader(rc, l), rc} } diff --git a/pkg/activator/util/rewinder.go b/pkg/activator/util/rewinder.go index 67c73ad3cded..723c4372cb95 100644 --- a/pkg/activator/util/rewinder.go +++ b/pkg/activator/util/rewinder.go @@ -27,7 +27,7 @@ type rewinder struct { eof bool } -// rewinder wraps a single-use `ReadCloser` into a `ReadCloser` +// NewRewinder wraps a single-use `ReadCloser` into a `ReadCloser` // that can be read multiple times func NewRewinder(rc io.ReadCloser) io.ReadCloser { r := &rewinder{original: rc} From 816afa86436729949baccc0a6028de6517438e40 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 26 Nov 2018 19:17:46 -0500 Subject: [PATCH 10/18] Explictly set Dialer config defaults in h2c.DefaultTransport --- pkg/http/h2c/h2c.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/http/h2c/h2c.go b/pkg/http/h2c/h2c.go index a522bb051439..fe1a52024b71 100644 --- a/pkg/http/h2c/h2c.go +++ b/pkg/http/h2c/h2c.go @@ -14,10 +14,10 @@ limitations under the License. package h2c import ( - "context" "crypto/tls" "net" "net/http" + "time" "golang.org/x/net/http2" "golang.org/x/net/http2/h2c" @@ -43,6 +43,12 @@ func ListenAndServe(addr string, h http.Handler) error { var DefaultTransport http.RoundTripper = &http2.Transport{ AllowHTTP: true, DialTLS: func(netw, addr string, cfg *tls.Config) (net.Conn, error) { - return http.DefaultTransport.(*http.Transport).DialContext(context.Background(), netw, addr) + d := &net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + DualStack: true, + } + + return d.Dial(netw, addr) }, } From 183ca2bda06eb478685702ca19b87a0a5448c708 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 21 Jan 2019 17:12:13 -0500 Subject: [PATCH 11/18] Remove timeoutHandler from queue-proxy --- cmd/queue/main.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index 8793e1b68a0b..3ab290fe448c 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -164,25 +164,6 @@ func isProbe(r *http.Request) bool { return strings.HasPrefix(r.Header.Get("User-Agent"), "kube-probe/") } -func timeoutHandler(h http.Handler, d time.Duration) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - done := make(chan struct{}) - - go func() { - defer close(done) - - h.ServeHTTP(w, r) - }() - - select { - case <-done: - return - case <-time.After(d): - panic(http.ErrAbortHandler) - } - }) -} - func handler(w http.ResponseWriter, r *http.Request) { proxy := proxyForRequest(r) From 9258466d32dd120744d1e1657b7503e5fe955d09 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 21 Jan 2019 17:19:41 -0500 Subject: [PATCH 12/18] Revert k8s Service port name from http2 to http Changing it to http2 for all services breaks services which only support http1. Support for http2 will require selectively setting the port name to http2 only for services that explicitly support it. --- pkg/reconciler/v1alpha1/revision/resources/constants.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/v1alpha1/revision/resources/constants.go b/pkg/reconciler/v1alpha1/revision/resources/constants.go index e519752f9fe6..a4bf504973d4 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/constants.go +++ b/pkg/reconciler/v1alpha1/revision/resources/constants.go @@ -39,7 +39,7 @@ const ( autoscalerPort = 8080 // ServicePortName is the name of the external port of the service - ServicePortName = "http2" + ServicePortName = "http" // ServicePort is the external port of the service ServicePort = int32(80) // MetricsPortName is the name of the external port of the service for metrics From a25b30ad3421df716e6b91cf1ba023237310f745 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Tue, 22 Jan 2019 12:00:28 -0500 Subject: [PATCH 13/18] Run activator on two ports, to support activating both http1 and h2c targets --- cmd/activator/main.go | 14 +++++++++++--- config/400-activator-service.yaml | 6 +++++- config/activator.yaml | 4 +++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/cmd/activator/main.go b/cmd/activator/main.go index 099533a2a3f1..8a8c164c577e 100644 --- a/cmd/activator/main.go +++ b/cmd/activator/main.go @@ -184,14 +184,22 @@ func main() { logger.Fatalw("Failed to start configuration manager", zap.Error(err)) } - srv := h2c.NewServer(":8080", ah) + http1Srv := h2c.NewServer(":8080", ah) go func() { - if err := srv.ListenAndServe(); err != nil { + if err := http1Srv.ListenAndServe(); err != nil { + logger.Errorw("Error running HTTP server", zap.Error(err)) + } + }() + + h2cSrv := h2c.NewServer(":8081", ah) + go func() { + if err := h2cSrv.ListenAndServe(); err != nil { logger.Errorw("Error running HTTP server", zap.Error(err)) } }() <-stopCh a.Shutdown() - srv.Shutdown(context.TODO()) + http1Srv.Shutdown(context.TODO()) + h2cSrv.Shutdown(context.TODO()) } diff --git a/config/400-activator-service.yaml b/config/400-activator-service.yaml index c9f8e98b063a..c928bdeb06a4 100644 --- a/config/400-activator-service.yaml +++ b/config/400-activator-service.yaml @@ -24,11 +24,15 @@ spec: selector: app: activator ports: - - name: http2 + - name: http protocol: TCP port: 80 targetPort: 8080 nodePort: # empty removing existing value to avoid upgrade error # TODO delete after v0.4 + - name: http2 + protocol: TCP + port: 81 + targetPort: 8081 - name: metrics protocol: TCP port: 9090 diff --git a/config/activator.yaml b/config/activator.yaml index a858d1d51916..7f4281affd16 100644 --- a/config/activator.yaml +++ b/config/activator.yaml @@ -41,8 +41,10 @@ spec: # and substituted here. image: github.com/knative/serving/cmd/activator ports: - - name: activator-port + - name: http1-port containerPort: 8080 + - name: h2c-port + containerPort: 8081 - name: metrics-port containerPort: 9090 args: From e8200a5793cd03d1963ad4838ce952c5fec160fd Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 24 Jan 2019 10:33:02 -0500 Subject: [PATCH 14/18] Add RevisionProtocolType to API --- pkg/apis/serving/v1alpha1/revision_types.go | 20 ++++++++ .../serving/v1alpha1/revision_types_test.go | 51 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 9fc303be2a29..bd556331ee4d 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -118,6 +118,17 @@ const ( RevisionContainerConcurrencyMax RevisionContainerConcurrencyType = 1000 ) +// RevisionProtocolType is an enumeration of the supported application-layer protocols +// See also: https://github.com/knative/serving/blob/master/docs/runtime-contract.md#protocols-and-ports +type RevisionProtocolType string + +const ( + // HTTP/1.1 + RevisionProtocolHTTP1 RevisionProtocolType = "http1" + // HTTP/2 with Prior Knowledge + RevisionProtocolH2C RevisionProtocolType = "h2c" +) + const ( // UserPortName is the name that will be used for the Port on the // Deployment and Pod created by a Revision. This name will be set regardless of if @@ -316,6 +327,15 @@ func (r *Revision) BuildRef() *corev1.ObjectReference { return nil } +func (r *Revision) GetProtocol() RevisionProtocolType { + ports := r.Spec.Container.Ports + if len(ports) > 0 && ports[0].Name == "h2c" { + return RevisionProtocolH2C + } + + return RevisionProtocolHTTP1 +} + // IsReady looks at the conditions and if the Status has a condition // RevisionConditionReady returns true if ConditionStatus is True func (rs *RevisionStatus) IsReady() bool { diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go index 002673052250..95a54e19bdd1 100644 --- a/pkg/apis/serving/v1alpha1/revision_types_test.go +++ b/pkg/apis/serving/v1alpha1/revision_types_test.go @@ -603,6 +603,57 @@ func TestRevisionBuildRefNil(t *testing.T) { } } +func TestRevisionGetProtocol(t *testing.T) { + containerWithPortName := func(name string) corev1.Container { + return corev1.Container{Ports: []corev1.ContainerPort{{Name: name}}} + } + + tests := []struct { + name string + container corev1.Container + protocol RevisionProtocolType + }{ + { + name: "undefined", + container: corev1.Container{}, + protocol: RevisionProtocolHTTP1, + }, + { + name: "http1", + container: containerWithPortName("http1"), + protocol: RevisionProtocolHTTP1, + }, + { + name: "h2c", + container: containerWithPortName("h2c"), + protocol: RevisionProtocolH2C, + }, + { + name: "unknown", + container: containerWithPortName("whatever"), + protocol: RevisionProtocolHTTP1, + }, + { + name: "empty", + container: containerWithPortName(""), + protocol: RevisionProtocolHTTP1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Revision{Spec: RevisionSpec{Container: tt.container}} + + got := r.GetProtocol() + want := tt.protocol + + if got != want { + t.Errorf("got: %#v, want: %#v", got, want) + } + }) + } +} + func TestRevisionGetLastPinned(t *testing.T) { cases := []struct { name string From 2987952a6214c4d53830c866cadce34c185a4d3a Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 24 Jan 2019 12:24:00 -0500 Subject: [PATCH 15/18] Dynamically select 'http' or 'http2' for k8s service port name --- pkg/activator/revision.go | 2 +- pkg/activator/revision_test.go | 3 +- .../v1alpha1/revision/resources/constants.go | 4 ++- .../v1alpha1/revision/resources/service.go | 34 +++++++++++-------- .../revision/resources/service_test.go | 32 +++++++++++++++-- .../v1alpha1/route/resources/service.go | 2 +- 6 files changed, 56 insertions(+), 21 deletions(-) diff --git a/pkg/activator/revision.go b/pkg/activator/revision.go index 9bfa33e9e7c4..db5e5ecdecec 100644 --- a/pkg/activator/revision.go +++ b/pkg/activator/revision.go @@ -123,7 +123,7 @@ func (r *revisionActivator) revisionEndpoint(revision *v1alpha1.Revision) (end E // Search for the correct port in all the service ports. port := int32(-1) for _, p := range svc.Spec.Ports { - if p.Name == revisionresources.ServicePortName { + if p.Name == revisionresources.ServicePortName(revision) { port = p.Port break } diff --git a/pkg/activator/revision_test.go b/pkg/activator/revision_test.go index 5ce78afb2475..1f8c0d908743 100644 --- a/pkg/activator/revision_test.go +++ b/pkg/activator/revision_test.go @@ -289,6 +289,7 @@ func newRevisionBuilder(labels map[string]string) *revisionBuilder { Spec: v1alpha1.RevisionSpec{ Container: corev1.Container{ Image: "gcr.io/repo/image", + Ports: []corev1.ContainerPort{{Name: "h2c"}}, }, }, Status: v1alpha1.RevisionStatus{ @@ -335,7 +336,7 @@ func newServiceBuilder() *serviceBuilder { }, Spec: corev1.ServiceSpec{ Ports: []corev1.ServicePort{{ - Name: revisionresources.ServicePortName, + Name: revisionresources.ServicePortNameH2C, Port: v1alpha1.DefaultUserPort, }, { Name: "anotherport", diff --git a/pkg/reconciler/v1alpha1/revision/resources/constants.go b/pkg/reconciler/v1alpha1/revision/resources/constants.go index a4bf504973d4..c1d6c7a4cc2f 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/constants.go +++ b/pkg/reconciler/v1alpha1/revision/resources/constants.go @@ -39,7 +39,9 @@ const ( autoscalerPort = 8080 // ServicePortName is the name of the external port of the service - ServicePortName = "http" + ServicePortNameHTTP1 = "http" + ServicePortNameH2C = "http2" + // ServicePort is the external port of the service ServicePort = int32(80) // MetricsPortName is the name of the external port of the service for metrics diff --git a/pkg/reconciler/v1alpha1/revision/resources/service.go b/pkg/reconciler/v1alpha1/revision/resources/service.go index 29ef67c4fb45..a7fa14c94071 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/service.go +++ b/pkg/reconciler/v1alpha1/revision/resources/service.go @@ -28,20 +28,6 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" ) -var ( - servicePorts = []corev1.ServicePort{{ - Name: ServicePortName, - Protocol: corev1.ProtocolTCP, - Port: ServicePort, - TargetPort: intstr.FromString(v1alpha1.RequestQueuePortName), - }, { - Name: MetricsPortName, - Protocol: corev1.ProtocolTCP, - Port: MetricsPort, - TargetPort: intstr.FromString(v1alpha1.RequestQueueMetricsPortName), - }} -) - // MakeK8sService creates a Kubernetes Service that targets all pods with the same // serving.RevisionLabelKey label. Traffic is routed to queue-proxy port. func MakeK8sService(rev *v1alpha1.Revision) *corev1.Service { @@ -56,10 +42,28 @@ func MakeK8sService(rev *v1alpha1.Revision) *corev1.Service { OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(rev)}, }, Spec: corev1.ServiceSpec{ - Ports: servicePorts, + Ports: []corev1.ServicePort{{ + Name: ServicePortName(rev), + Protocol: corev1.ProtocolTCP, + Port: ServicePort, + TargetPort: intstr.FromString(v1alpha1.RequestQueuePortName), + }, { + Name: MetricsPortName, + Protocol: corev1.ProtocolTCP, + Port: MetricsPort, + TargetPort: intstr.FromString(v1alpha1.RequestQueueMetricsPortName), + }}, Selector: map[string]string{ serving.RevisionLabelKey: rev.Name, }, }, } } + +func ServicePortName(rev *v1alpha1.Revision) string { + if rev.GetProtocol() == v1alpha1.RevisionProtocolH2C { + return ServicePortNameH2C + } + + return ServicePortNameHTTP1 +} diff --git a/pkg/reconciler/v1alpha1/revision/resources/service_test.go b/pkg/reconciler/v1alpha1/revision/resources/service_test.go index 40d447cfc785..fe5408f2acbc 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/service_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/service_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" "github.com/knative/serving/pkg/apis/autoscaling" "github.com/knative/serving/pkg/apis/serving" @@ -63,7 +64,17 @@ func TestMakeK8sService(t *testing.T) { }}, }, Spec: corev1.ServiceSpec{ - Ports: servicePorts, + Ports: []corev1.ServicePort{{ + Name: ServicePortNameHTTP1, + Protocol: corev1.ProtocolTCP, + Port: ServicePort, + TargetPort: intstr.FromString(v1alpha1.RequestQueuePortName), + }, { + Name: MetricsPortName, + Protocol: corev1.ProtocolTCP, + Port: MetricsPort, + TargetPort: intstr.FromString(v1alpha1.RequestQueueMetricsPortName), + }}, Selector: map[string]string{ serving.RevisionLabelKey: "bar", }, @@ -77,6 +88,13 @@ func TestMakeK8sService(t *testing.T) { Name: "baz", UID: "1234", }, + Spec: v1alpha1.RevisionSpec{ + Container: corev1.Container{ + Ports: []corev1.ContainerPort{ + {Name: "h2c"}, + }, + }, + }, }, want: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -99,7 +117,17 @@ func TestMakeK8sService(t *testing.T) { }}, }, Spec: corev1.ServiceSpec{ - Ports: servicePorts, + Ports: []corev1.ServicePort{{ + Name: ServicePortNameH2C, + Protocol: corev1.ProtocolTCP, + Port: ServicePort, + TargetPort: intstr.FromString(v1alpha1.RequestQueuePortName), + }, { + Name: MetricsPortName, + Protocol: corev1.ProtocolTCP, + Port: MetricsPort, + TargetPort: intstr.FromString(v1alpha1.RequestQueueMetricsPortName), + }}, Selector: map[string]string{ serving.RevisionLabelKey: "baz", }, diff --git a/pkg/reconciler/v1alpha1/route/resources/service.go b/pkg/reconciler/v1alpha1/route/resources/service.go index 58768a376b9d..61f1e8959fed 100644 --- a/pkg/reconciler/v1alpha1/route/resources/service.go +++ b/pkg/reconciler/v1alpha1/route/resources/service.go @@ -88,7 +88,7 @@ func makeServiceSpec(ingress *netv1alpha1.ClusterIngress) (*corev1.ServiceSpec, return &corev1.ServiceSpec{ Type: corev1.ServiceTypeClusterIP, Ports: []corev1.ServicePort{{ - Name: revisionresources.ServicePortName, + Name: revisionresources.ServicePortNameHTTP1, Port: revisionresources.ServicePort, }}, }, nil From 7ce391f378bbae5340b286fc2b95cf47c4198460 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 24 Jan 2019 15:30:20 -0500 Subject: [PATCH 16/18] Dynamically route to the http1 or h2c activator service port based on the revision protocol --- pkg/activator/activator.go | 17 ++ .../route/resources/cluster_ingress.go | 2 +- .../v1alpha1/route/traffic/traffic.go | 8 +- .../v1alpha1/route/traffic/traffic_test.go | 167 +++++++++++------- 4 files changed, 132 insertions(+), 62 deletions(-) diff --git a/pkg/activator/activator.go b/pkg/activator/activator.go index 12c401de3481..f24a69108aea 100644 --- a/pkg/activator/activator.go +++ b/pkg/activator/activator.go @@ -16,6 +16,8 @@ limitations under the License. package activator +import "github.com/knative/serving/pkg/apis/serving/v1alpha1" + const ( // K8sServiceName is the name of the activator service K8sServiceName = "activator-service" @@ -25,6 +27,11 @@ const ( RevisionHeaderName string = "knative-serving-revision" // RevisionHeaderNamespace is the header key for revision's namespace RevisionHeaderNamespace string = "knative-serving-namespace" + + // ServicePortHTTP1 is the port number for activating HTTP1 revisions + ServicePortHTTP1 int32 = 80 + // ServicePortHTTP1 is the port number for activating H2C revisions + ServicePortH2C int32 = 81 ) // Activator provides an active endpoint for a revision or an error and @@ -53,3 +60,13 @@ type ActivationResult struct { ConfigurationName string Error error } + +// ServicePort returns the activator service port for the given Revision protocol. +// Default is `ServicePortHTTP1`. +func ServicePort(protocol v1alpha1.RevisionProtocolType) int32 { + if protocol == v1alpha1.RevisionProtocolH2C { + return ServicePortH2C + } + + return ServicePortHTTP1 +} diff --git a/pkg/reconciler/v1alpha1/route/resources/cluster_ingress.go b/pkg/reconciler/v1alpha1/route/resources/cluster_ingress.go index 8ab651b350f0..425559a84b05 100644 --- a/pkg/reconciler/v1alpha1/route/resources/cluster_ingress.go +++ b/pkg/reconciler/v1alpha1/route/resources/cluster_ingress.go @@ -147,7 +147,7 @@ func addInactive(r *v1alpha1.HTTPClusterIngressPath, ns string, inactive traffic ClusterIngressBackend: v1alpha1.ClusterIngressBackend{ ServiceNamespace: system.Namespace(), ServiceName: activator.K8sServiceName, - ServicePort: intstr.FromInt(int(revisionresources.ServicePort)), + ServicePort: intstr.FromInt(int(activator.ServicePort(maxInactiveTarget.Protocol))), }, Percent: totalInactivePercent, }) diff --git a/pkg/reconciler/v1alpha1/route/traffic/traffic.go b/pkg/reconciler/v1alpha1/route/traffic/traffic.go index c61757489bd9..5e74b0fe538d 100644 --- a/pkg/reconciler/v1alpha1/route/traffic/traffic.go +++ b/pkg/reconciler/v1alpha1/route/traffic/traffic.go @@ -27,10 +27,12 @@ import ( // DefaultTarget is the unnamed default target for the traffic. const DefaultTarget = "" -// A RevisionTarget adds the Active/Inactive state of a Revision to a flattened TrafficTarget. +// A RevisionTarget adds the Active/Inactive state and the transport protocol of a +// Revision to a flattened TrafficTarget. type RevisionTarget struct { v1alpha1.TrafficTarget - Active bool + Active bool + Protocol v1alpha1.RevisionProtocolType } // RevisionTargets is a collection of revision targets. @@ -208,6 +210,7 @@ func (t *configBuilder) addConfigurationTarget(tt *v1alpha1.TrafficTarget) error target := RevisionTarget{ TrafficTarget: *tt, Active: !rev.Status.IsActivationRequired(), + Protocol: rev.GetProtocol(), } target.TrafficTarget.RevisionName = rev.Name t.addFlattenedTarget(target) @@ -225,6 +228,7 @@ func (t *configBuilder) addRevisionTarget(tt *v1alpha1.TrafficTarget) error { target := RevisionTarget{ TrafficTarget: *tt, Active: !rev.Status.IsActivationRequired(), + Protocol: rev.GetProtocol(), } t.revisions[tt.RevisionName] = rev if configName, ok := rev.Labels[serving.ConfigurationLabelKey]; ok { diff --git a/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go b/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go index 3a78c672cd8e..4289d51936de 100644 --- a/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go +++ b/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go @@ -131,7 +131,8 @@ func TestBuildTrafficConfiguration_Vanilla(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -140,7 +141,8 @@ func TestBuildTrafficConfiguration_Vanilla(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig}, Revisions: map[string]*v1alpha1.Revision{goodNewRev.Name: goodNewRev}, @@ -164,9 +166,9 @@ func TestPartitionTargets(t *testing.T) { "skip 0 percent", RevisionTargets( []RevisionTarget{ - {v1alpha1.TrafficTarget{RevisionName: "implicit"}, true}, - {v1alpha1.TrafficTarget{RevisionName: "explicit", Percent: 0}, true}, - {v1alpha1.TrafficTarget{RevisionName: "passive", Percent: 0}, false}, + {v1alpha1.TrafficTarget{RevisionName: "implicit"}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "explicit", Percent: 0}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "passive", Percent: 0}, false, ""}, }, ), make(RevisionTargets, 0), @@ -174,49 +176,49 @@ func TestPartitionTargets(t *testing.T) { }, { "1 active", - RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, true}}), - RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, true}}), + RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, true, ""}}), + RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, true, ""}}), make(RevisionTargets, 0), }, { "1 passive", - RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, false}}), + RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, false, ""}}), make(RevisionTargets, 0), - RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, false}}), + RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "a", Percent: 1}, false, ""}}), }, { "1 active, 1 passive, 1 0 percent", RevisionTargets( []RevisionTarget{ - {v1alpha1.TrafficTarget{RevisionName: "zero"}, true}, - {v1alpha1.TrafficTarget{RevisionName: "fiver", Percent: 5}, true}, - {v1alpha1.TrafficTarget{RevisionName: "sixer", Percent: 6}, false}, + {v1alpha1.TrafficTarget{RevisionName: "zero"}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "fiver", Percent: 5}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "sixer", Percent: 6}, false, ""}, }, ), - RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "fiver", Percent: 5}, true}}), - RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "sixer", Percent: 6}, false}}), + RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "fiver", Percent: 5}, true, ""}}), + RevisionTargets([]RevisionTarget{{v1alpha1.TrafficTarget{RevisionName: "sixer", Percent: 6}, false, ""}}), }, { "2 of each", RevisionTargets( []RevisionTarget{ - {v1alpha1.TrafficTarget{RevisionName: "one", Percent: 1}, true}, - {v1alpha1.TrafficTarget{RevisionName: "two", Percent: 2}, false}, - {v1alpha1.TrafficTarget{RevisionName: "three", Percent: 3}, true}, - {v1alpha1.TrafficTarget{RevisionName: "four", Percent: 4}, false}, - {v1alpha1.TrafficTarget{RevisionName: "fiver", Percent: 0}, true}, + {v1alpha1.TrafficTarget{RevisionName: "one", Percent: 1}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "two", Percent: 2}, false, ""}, + {v1alpha1.TrafficTarget{RevisionName: "three", Percent: 3}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "four", Percent: 4}, false, ""}, + {v1alpha1.TrafficTarget{RevisionName: "fiver", Percent: 0}, true, ""}, }, ), RevisionTargets( []RevisionTarget{ - {v1alpha1.TrafficTarget{RevisionName: "one", Percent: 1}, true}, - {v1alpha1.TrafficTarget{RevisionName: "three", Percent: 3}, true}, + {v1alpha1.TrafficTarget{RevisionName: "one", Percent: 1}, true, ""}, + {v1alpha1.TrafficTarget{RevisionName: "three", Percent: 3}, true, ""}, }, ), RevisionTargets( []RevisionTarget{ - {v1alpha1.TrafficTarget{RevisionName: "two", Percent: 2}, false}, - {v1alpha1.TrafficTarget{RevisionName: "four", Percent: 4}, false}, + {v1alpha1.TrafficTarget{RevisionName: "two", Percent: 2}, false, ""}, + {v1alpha1.TrafficTarget{RevisionName: "four", Percent: 4}, false, ""}, }, ), }, @@ -246,7 +248,8 @@ func TestBuildTrafficConfiguration_NoNameRevision(t *testing.T) { ConfigurationName: goodConfig.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -255,7 +258,8 @@ func TestBuildTrafficConfiguration_NoNameRevision(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig}, Revisions: map[string]*v1alpha1.Revision{goodNewRev.Name: goodNewRev}, @@ -281,7 +285,8 @@ func TestBuildTrafficConfiguration_VanillaScaledToZero(t *testing.T) { RevisionName: inactiveRev.Name, Percent: 100, }, - Active: false, + Active: false, + Protocol: v1alpha1.RevisionProtocolHTTP1, }}, }, revisionTargets: []RevisionTarget{{ @@ -290,7 +295,8 @@ func TestBuildTrafficConfiguration_VanillaScaledToZero(t *testing.T) { RevisionName: inactiveRev.Name, Percent: 100, }, - Active: false, + Active: false, + Protocol: v1alpha1.RevisionProtocolHTTP1, }}, Configurations: map[string]*v1alpha1.Configuration{inactiveConfig.Name: inactiveConfig}, Revisions: map[string]*v1alpha1.Revision{inactiveRev.Name: inactiveRev}, @@ -319,13 +325,16 @@ func TestBuildTrafficConfiguration_TwoConfigs(t *testing.T) { RevisionName: niceNewRev.Name, Percent: 90, }, - Active: true}, { + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, + }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, Percent: 10, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -334,13 +343,16 @@ func TestBuildTrafficConfiguration_TwoConfigs(t *testing.T) { RevisionName: niceNewRev.Name, Percent: 90, }, - Active: true}, { + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, + }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, Percent: 10, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig, niceConfig.Name: niceConfig}, Revisions: map[string]*v1alpha1.Revision{goodNewRev.Name: goodNewRev, niceNewRev.Name: niceNewRev}, @@ -370,14 +382,16 @@ func TestBuildTrafficConfiguration_Canary(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 90, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, Percent: 10, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -386,14 +400,16 @@ func TestBuildTrafficConfiguration_Canary(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 90, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, Percent: 10, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig}, Revisions: map[string]*v1alpha1.Revision{goodOldRev.Name: goodOldRev, goodNewRev.Name: goodNewRev}, @@ -430,7 +446,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 49, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "two", @@ -438,7 +455,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 51, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, "one": {{ TrafficTarget: v1alpha1.TrafficTarget{ @@ -447,7 +465,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }}, "two": {{ TrafficTarget: v1alpha1.TrafficTarget{ @@ -456,7 +475,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, "also-two": {{ TrafficTarget: v1alpha1.TrafficTarget{ @@ -465,7 +485,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -475,7 +496,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 49, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "two", @@ -483,7 +505,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 50, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "also-two", @@ -491,7 +514,8 @@ func TestBuildTrafficConfiguration_Consolidated(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 1, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig}, Revisions: map[string]*v1alpha1.Revision{goodOldRev.Name: goodOldRev, goodNewRev.Name: goodNewRev}, @@ -520,14 +544,16 @@ func TestBuildTrafficConfiguration_TwoFixedRevisions(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 90, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, Percent: 10, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -536,14 +562,16 @@ func TestBuildTrafficConfiguration_TwoFixedRevisions(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 90, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, Percent: 10, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig}, Revisions: map[string]*v1alpha1.Revision{goodNewRev.Name: goodNewRev, goodOldRev.Name: goodOldRev}, @@ -572,14 +600,16 @@ func TestBuildTrafficConfiguration_TwoFixedRevisionsFromTwoConfigurations(t *tes RevisionName: goodNewRev.Name, Percent: 40, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: niceConfig.Name, RevisionName: niceNewRev.Name, Percent: 60, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -588,14 +618,16 @@ func TestBuildTrafficConfiguration_TwoFixedRevisionsFromTwoConfigurations(t *tes RevisionName: goodNewRev.Name, Percent: 40, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }, { TrafficTarget: v1alpha1.TrafficTarget{ ConfigurationName: niceConfig.Name, RevisionName: niceNewRev.Name, Percent: 60, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig, niceConfig.Name: niceConfig}, Revisions: map[string]*v1alpha1.Revision{goodNewRev.Name: goodNewRev, niceNewRev.Name: niceNewRev}, @@ -627,19 +659,24 @@ func TestBuildTrafficConfiguration_Preliminary(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 100, }, - Active: true}, { + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, + }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "beta", ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, }, - Active: true}, { + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, + }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "alpha", ConfigurationName: niceConfig.Name, RevisionName: niceNewRev.Name, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, "beta": {{ TrafficTarget: v1alpha1.TrafficTarget{ @@ -648,7 +685,9 @@ func TestBuildTrafficConfiguration_Preliminary(t *testing.T) { RevisionName: goodNewRev.Name, Percent: 100, }, - Active: true}}, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, + }}, "alpha": {{ TrafficTarget: v1alpha1.TrafficTarget{ Name: "alpha", @@ -656,7 +695,8 @@ func TestBuildTrafficConfiguration_Preliminary(t *testing.T) { RevisionName: niceNewRev.Name, Percent: 100, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, }, revisionTargets: []RevisionTarget{{ @@ -665,19 +705,24 @@ func TestBuildTrafficConfiguration_Preliminary(t *testing.T) { RevisionName: goodOldRev.Name, Percent: 100, }, - Active: true}, { + Active: true, + Protocol: v1alpha1.RevisionProtocolHTTP1, + }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "beta", ConfigurationName: goodConfig.Name, RevisionName: goodNewRev.Name, }, - Active: true}, { + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, + }, { TrafficTarget: v1alpha1.TrafficTarget{ Name: "alpha", ConfigurationName: niceConfig.Name, RevisionName: niceNewRev.Name, }, - Active: true, + Active: true, + Protocol: v1alpha1.RevisionProtocolH2C, }}, Configurations: map[string]*v1alpha1.Configuration{goodConfig.Name: goodConfig, niceConfig.Name: niceConfig}, Revisions: map[string]*v1alpha1.Revision{goodOldRev.Name: goodOldRev, goodNewRev.Name: goodNewRev, niceNewRev.Name: niceNewRev}, @@ -973,6 +1018,10 @@ func getTestReadyConfig(name string) (*v1alpha1.Configuration, *v1alpha1.Revisio Status: corev1.ConditionTrue, }}, }) + + // rev1 will use http1, rev2 will use h2c + config.Spec.RevisionTemplate.Spec.Container.Ports = []corev1.ContainerPort{{Name: "h2c"}} + rev2 := testRevForConfig(config, name+"-revision-2") rev2.Status.MarkResourcesAvailable() rev2.Status.MarkContainerHealthy() From c7f6743acd90355ac78aa58fe365ecf17a26cf06 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Thu, 24 Jan 2019 16:18:50 -0500 Subject: [PATCH 17/18] Add test coverage for activator.ServicePort --- pkg/activator/activator_test.go | 49 +++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 pkg/activator/activator_test.go diff --git a/pkg/activator/activator_test.go b/pkg/activator/activator_test.go new file mode 100644 index 000000000000..d15da7e6b367 --- /dev/null +++ b/pkg/activator/activator_test.go @@ -0,0 +1,49 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package activator + +import ( + "testing" + + "github.com/knative/serving/pkg/apis/serving/v1alpha1" +) + +func TestServicePort(t *testing.T) { + tests := []struct { + name string + protocol v1alpha1.RevisionProtocolType + port int32 + }{{ + name: "h2c", + protocol: v1alpha1.RevisionProtocolH2C, + port: ServicePortH2C, + }, { + name: "http1", + protocol: v1alpha1.RevisionProtocolHTTP1, + port: ServicePortHTTP1, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + want := tt.port + got := ServicePort(tt.protocol) + + if want != got { + t.Errorf("unexpected port. want %d, got %d", want, got) + } + }) + } +} From 1700fbc04bbed883ae58492dec5c26aa02e1b037 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 30 Jan 2019 16:34:22 -0500 Subject: [PATCH 18/18] Print errors in EnforceLengthHandler tests --- pkg/activator/handler/enforce_length_handler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/activator/handler/enforce_length_handler_test.go b/pkg/activator/handler/enforce_length_handler_test.go index 9718b0a9f813..166b04f0f8e3 100644 --- a/pkg/activator/handler/enforce_length_handler_test.go +++ b/pkg/activator/handler/enforce_length_handler_test.go @@ -88,7 +88,7 @@ func TestEnforceMaxContentLengthHandler(t *testing.T) { if r.Body != nil { body, err := ioutil.ReadAll(r.Body) if err != nil { - t.Fatalf("Error request reading body") + t.Fatalf("Error reading request body: %v", err) } w.Write(body) } @@ -102,7 +102,7 @@ func TestEnforceMaxContentLengthHandler(t *testing.T) { gotBody, err := ioutil.ReadAll(resp.Body) if err != nil { - t.Fatalf("Error request reading body") + t.Fatalf("Error reading response body: %v", err) } if e.response != string(gotBody) {