From 6bdb8ed566941552c73d3b5558278f485c29f03b Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 4 Nov 2022 21:40:54 +0000 Subject: [PATCH] switch spdy round trip tests to simple http proxy github.com/elazarl/goproxy does not properly handle upgrades over HTTP proxy. The problem is this misinterpretation: https://github.com/kubernetes/kubernetes/blob/66918763265b91000482f82a7e92c68d24733542/vendor/github.com/elazarl/goproxy/proxy.go#L89-L95 These should be stripped but recalculated then added back: https://cs.opensource.google/go/go/+/refs/tags/go1.19.3:src/net/http/httputil/reverseproxy.go;l=292-297;drc=f6d844510d5f1e3b3098eba255d9b633d45eac3b Let's just stop using goproxy, and use the stdlib instead. The functionality we need is straightforward to implement and goproxy is overkill. --- staging/src/k8s.io/apimachinery/go.mod | 3 +- staging/src/k8s.io/apimachinery/go.sum | 1 + .../util/httpstream/spdy/roundtripper_test.go | 20 +-- .../apimachinery/pkg/util/net/testing/http.go | 150 ++++++++++++++++++ test/e2e/kubectl/kubectl.go | 19 +-- vendor/modules.txt | 1 + 6 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 staging/src/k8s.io/apimachinery/pkg/util/net/testing/http.go diff --git a/staging/src/k8s.io/apimachinery/go.mod b/staging/src/k8s.io/apimachinery/go.mod index 2b1a3273e3c..5a2e71e5849 100644 --- a/staging/src/k8s.io/apimachinery/go.mod +++ b/staging/src/k8s.io/apimachinery/go.mod @@ -17,6 +17,7 @@ require ( github.com/google/uuid v1.3.0 github.com/moby/spdystream v0.2.0 github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f + github.com/onsi/ginkgo/v2 v2.7.0 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.8.1 golang.org/x/net v0.4.0 @@ -35,10 +36,10 @@ require ( github.com/kr/pretty v0.3.0 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.2 // indirect - github.com/onsi/ginkgo/v2 v2.7.0 // indirect github.com/onsi/gomega v1.24.2 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + golang.org/x/sys v0.3.0 // indirect golang.org/x/text v0.5.0 // indirect google.golang.org/protobuf v1.28.1 // indirect gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect diff --git a/staging/src/k8s.io/apimachinery/go.sum b/staging/src/k8s.io/apimachinery/go.sum index 2ba5c845d6b..4a2aea98f71 100644 --- a/staging/src/k8s.io/apimachinery/go.sum +++ b/staging/src/k8s.io/apimachinery/go.sum @@ -125,6 +125,7 @@ golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5h golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.3.0 h1:w8ZOecv6NaNa/zC8944JTU3vz4u6Lagfk4RPQxv92NQ= +golang.org/x/sys v0.3.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.5.0 h1:OLmvp0KP+FVG99Ct/qFiL/Fhk4zp4QQnZ7b2U+5piUM= diff --git a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go index a0b7b169dd5..b2c2b88513a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper_test.go @@ -29,11 +29,11 @@ import ( "testing" "github.com/armon/go-socks5" - "github.com/elazarl/goproxy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/util/httpstream" + utilnettesting "k8s.io/apimachinery/pkg/util/net/testing" ) type serverHandlerConfig struct { @@ -313,6 +313,7 @@ func TestRoundTripAndNewConnection(t *testing.T) { }, )) defer server.Close() + t.Logf("Server URL: %v", server.URL) serverURL, err := url.Parse(server.URL) if err != nil { @@ -330,18 +331,20 @@ func TestRoundTripAndNewConnection(t *testing.T) { var proxyCalledWithAuth bool var proxyCalledWithAuthHeader string if testCase.proxyServerFunc != nil { - proxyHandler := goproxy.NewProxyHttpServer() - - proxyHandler.OnRequest().HandleConnectFunc(func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { - proxyCalledWithHost = host + proxyHandler := utilnettesting.NewHTTPProxyHandler(t, func(req *http.Request) bool { + proxyCalledWithHost = req.Host proxyAuthHeaderName := "Proxy-Authorization" - _, proxyCalledWithAuth = ctx.Req.Header[proxyAuthHeaderName] - proxyCalledWithAuthHeader = ctx.Req.Header.Get(proxyAuthHeaderName) - return goproxy.OkConnect, host + _, proxyCalledWithAuth = req.Header[proxyAuthHeaderName] + proxyCalledWithAuthHeader = req.Header.Get(proxyAuthHeaderName) + return true }) + defer proxyHandler.Wait() proxy := testCase.proxyServerFunc(proxyHandler) + defer proxy.Close() + + t.Logf("Proxy URL: %v", proxy.URL) spdyTransport.proxier = func(proxierReq *http.Request) (*url.URL, error) { proxierCalled = true @@ -352,7 +355,6 @@ func TestRoundTripAndNewConnection(t *testing.T) { proxyURL.User = testCase.proxyAuth return proxyURL, nil } - defer proxy.Close() } client := &http.Client{Transport: spdyTransport} diff --git a/staging/src/k8s.io/apimachinery/pkg/util/net/testing/http.go b/staging/src/k8s.io/apimachinery/pkg/util/net/testing/http.go new file mode 100644 index 00000000000..665a461859f --- /dev/null +++ b/staging/src/k8s.io/apimachinery/pkg/util/net/testing/http.go @@ -0,0 +1,150 @@ +/* +Copyright 2023 The Kubernetes 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 nettesting contains utilities for testing networking functionality. +// Don't use these utilities in production code. They have not been security +// reviewed. +package nettesting + +import ( + "io" + "net" + "net/http" + "net/http/httputil" + "sync" + "testing" + + "github.com/onsi/ginkgo/v2" +) + +type TB interface { + Logf(format string, args ...any) +} + +// NewHTTPProxyHandler returns a new HTTPProxyHandler. It accepts an optional +// hook which is called early in the handler to export request state. If the +// hook returns false, the handler returns immediately with a server error. +func NewHTTPProxyHandler(t TB, hook func(req *http.Request) bool) *HTTPProxyHandler { + // Ensure that this is only used in tests. This code has not been security + // reviewed. + switch t.(type) { + case testing.TB, ginkgo.GinkgoTInterface: + default: + panic("t is not a known test interface") + } + h := &HTTPProxyHandler{ + hook: hook, + httpProxy: httputil.ReverseProxy{ + Director: func(req *http.Request) { + req.URL.Scheme = "http" + req.URL.Host = req.Host + }, + }, + t: t, + } + return h +} + +// HTTPProxyHandler implements a simple handler for http_proxy and https_proxy +// requests for use in testing. +type HTTPProxyHandler struct { + handlerDone sync.WaitGroup + hook func(r *http.Request) bool + // httpProxy is the reverse proxy we use for standard http proxy requests. + httpProxy httputil.ReverseProxy + t TB +} + +// ServeHTTP handles an HTTP proxy request. +func (h *HTTPProxyHandler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { + h.handlerDone.Add(1) + defer h.handlerDone.Done() + + if h.hook != nil { + if ok := h.hook(req); !ok { + rw.WriteHeader(http.StatusInternalServerError) + return + } + } + + b, err := httputil.DumpRequest(req, false) + if err != nil { + h.t.Logf("Failed to dump request, host=%s: %v", req.Host, err) + } else { + h.t.Logf("Proxy Request: %s", string(b)) + } + + if req.Method != http.MethodConnect { + h.httpProxy.ServeHTTP(rw, req) + return + } + + // CONNECT proxy + + sconn, err := net.Dial("tcp", req.Host) + if err != nil { + h.t.Logf("Failed to dial proxy backend, host=%s: %v", req.Host, err) + rw.WriteHeader(http.StatusInternalServerError) + return + } + defer sconn.Close() + + hj, ok := rw.(http.Hijacker) + if !ok { + h.t.Logf("Can't switch protocols using non-Hijacker ResponseWriter: type=%T, host=%s", rw, req.Host) + rw.WriteHeader(http.StatusInternalServerError) + return + } + + rw.WriteHeader(http.StatusOK) + + conn, brw, err := hj.Hijack() + if err != nil { + h.t.Logf("Failed to hijack client connection, host=%s: %v", req.Host, err) + return + } + defer conn.Close() + + if err := brw.Flush(); err != nil { + h.t.Logf("Failed to flush pending writes to client, host=%s: %v", req.Host, err) + return + } + if _, err := io.Copy(sconn, io.LimitReader(brw, int64(brw.Reader.Buffered()))); err != nil { + h.t.Logf("Failed to flush buffered reads to server, host=%s: %v", req.Host, err) + return + } + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer h.t.Logf("Server read close, host=%s", req.Host) + defer wg.Done() + io.Copy(conn, sconn) + }() + go func() { + defer h.t.Logf("Server write close, host=%s", req.Host) + defer wg.Done() + io.Copy(sconn, conn) + }() + + wg.Wait() + h.t.Logf("Done handling CONNECT request, host=%s", req.Host) +} + +func (h *HTTPProxyHandler) Wait() { + h.handlerDone.Wait() +} diff --git a/test/e2e/kubectl/kubectl.go b/test/e2e/kubectl/kubectl.go index 3e62767ea2e..661f0f8234e 100644 --- a/test/e2e/kubectl/kubectl.go +++ b/test/e2e/kubectl/kubectl.go @@ -24,7 +24,6 @@ import ( "encoding/json" "fmt" "io" - "log" "net" "net/http" "net/http/httptest" @@ -38,7 +37,6 @@ import ( "strings" "time" - "github.com/elazarl/goproxy" openapi_v2 "github.com/google/gnostic/openapiv2" "sigs.k8s.io/yaml" @@ -52,6 +50,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" utilnet "k8s.io/apimachinery/pkg/util/net" + utilnettesting "k8s.io/apimachinery/pkg/util/net/testing" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/authentication/serviceaccount" @@ -472,8 +471,12 @@ var _ = SIGDescribe("Kubectl client", func() { framework.Failf("--host variable must be set to the full URI to the api server on e2e run.") } - ginkgo.By("Starting goproxy") - testSrv, proxyLogs := startLocalProxy() + ginkgo.By("Starting http_proxy") + var proxyLogs bytes.Buffer + testSrv := httptest.NewServer(utilnettesting.NewHTTPProxyHandler(ginkgo.GinkgoT(), func(req *http.Request) bool { + fmt.Fprintf(&proxyLogs, "Accepting %s to %s\n", req.Method, req.Host) + return true + })) defer testSrv.Close() proxyAddr := testSrv.URL @@ -2260,14 +2263,6 @@ func newBlockingReader(s string) (io.Reader, io.Closer, error) { return r, w, nil } -func startLocalProxy() (srv *httptest.Server, logs *bytes.Buffer) { - logs = &bytes.Buffer{} - p := goproxy.NewProxyHttpServer() - p.Verbose = true - p.Logger = log.New(logs, "", 0) - return httptest.NewServer(p), logs -} - // createApplyCustomResource asserts that given CustomResource be created and applied // without being rejected by kubectl validation func createApplyCustomResource(resource, namespace, name string, crd *crd.TestCrd) error { diff --git a/vendor/modules.txt b/vendor/modules.txt index 6829bd84079..6e79e3b354a 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1422,6 +1422,7 @@ k8s.io/apimachinery/pkg/util/managedfields k8s.io/apimachinery/pkg/util/mergepatch k8s.io/apimachinery/pkg/util/naming k8s.io/apimachinery/pkg/util/net +k8s.io/apimachinery/pkg/util/net/testing k8s.io/apimachinery/pkg/util/proxy k8s.io/apimachinery/pkg/util/rand k8s.io/apimachinery/pkg/util/remotecommand