From c22ac17dfed914f5ed2aeaef3e697a1e9fda1033 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 18 Mar 2021 15:32:33 -0400 Subject: [PATCH] internal/concierge/impersonator: use http/2.0 as much as we can Signed-off-by: Monis Khan --- .../concierge/impersonator/impersonator.go | 40 ++++++++++++++++--- .../impersonator/impersonator_test.go | 9 ++++- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index 3d7ad324..311b2fc4 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -217,15 +217,14 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi return nil, fmt.Errorf("could not parse host URL from in-cluster config: %w", err) } - kubeTransportConfig, err := restConfig.TransportConfig() + http1RoundTripper, err := getTransportForProtocol(restConfig, "http/1.1") if err != nil { - return nil, fmt.Errorf("could not get in-cluster transport config: %w", err) + return nil, fmt.Errorf("could not get http/1.1 round tripper: %w", err) } - kubeTransportConfig.TLS.NextProtos = []string{"http/1.1"} // TODO huh? - kubeRoundTripper, err := transport.New(kubeTransportConfig) + http2RoundTripper, err := getTransportForProtocol(restConfig, "h2") if err != nil { - return nil, fmt.Errorf("could not get in-cluster transport: %w", err) + return nil, fmt.Errorf("could not get http/2.0 round tripper: %w", err) } return func(c *genericapiserver.Config) http.Handler { @@ -259,7 +258,26 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi return } - rt, err := getTransportForUser(userInfo, kubeRoundTripper) + reqInfo, ok := request.RequestInfoFrom(r.Context()) + if !ok { + plog.Warning("aggregated API server logic did not set request info but it is always supposed to do so", + "url", r.URL.String(), + "method", r.Method, + ) + newInternalErrResponse(w, r, c.Serializer, "invalid request info") + return + } + + // when we are running regular requests (e.g., CRUD) we should always be able to use HTTP/2.0 + // since KAS always supports that and it goes through proxies just fine. for long running + // requests (e.g., proxy, watch), we know they use http/1.1 with an upgrade to + // websockets/SPDY (this upgrade is NEVER to HTTP/2.0 as the KAS does not support that). + baseRT := http2RoundTripper + if c.LongRunningFunc(r, reqInfo) { + baseRT = http1RoundTripper + } + + rt, err := getTransportForUser(userInfo, baseRT) if err != nil { plog.WarningErr("rejecting request as we cannot act as the current user", err, "url", r.URL.String(), @@ -335,3 +353,13 @@ func newStatusErrResponse(w http.ResponseWriter, r *http.Request, s runtime.Nego gv := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} responsewriters.ErrorNegotiated(err, s, gv, w, r) } + +func getTransportForProtocol(restConfig *rest.Config, protocol string) (http.RoundTripper, error) { + transportConfig, err := restConfig.TransportConfig() + if err != nil { + return nil, fmt.Errorf("could not get in-cluster transport config: %w", err) + } + transportConfig.TLS.NextProtos = []string{protocol} + + return transport.New(transportConfig) +} diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 53efe3a7..9fccbe9e 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -5,6 +5,7 @@ package impersonator import ( "context" + "math/rand" "net" "net/http" "net/http/httptest" @@ -359,7 +360,7 @@ func TestImpersonatorHTTPHandler(t *testing.T) { ExecProvider: &api.ExecConfig{}, AuthProvider: &api.AuthProviderConfig{}, }, - wantCreationErr: "could not get in-cluster transport config: execProvider and authProvider cannot be used in combination", + wantCreationErr: "could not get http/1.1 round tripper: could not get in-cluster transport config: execProvider and authProvider cannot be used in combination", }, { name: "fail to get transport from config", @@ -369,7 +370,7 @@ func TestImpersonatorHTTPHandler(t *testing.T) { Transport: http.DefaultTransport, TLSClientConfig: rest.TLSClientConfig{Insecure: true}, }, - wantCreationErr: "could not get in-cluster transport: using a custom transport with TLS certificate options or the insecure flag is not allowed", + wantCreationErr: "could not get http/1.1 round tripper: using a custom transport with TLS certificate options or the insecure flag is not allowed", }, { name: "Impersonate-User header already in request", @@ -513,6 +514,10 @@ func TestImpersonatorHTTPHandler(t *testing.T) { metav1.AddToGroupVersion(scheme, metav1.Unversioned) codecs := serializer.NewCodecFactory(scheme) serverConfig := genericapiserver.NewRecommendedConfig(codecs) + serverConfig.Config.LongRunningFunc = func(_ *http.Request, _ *request.RequestInfo) bool { + // take the HTTP/2.0 vs HTTP/1.1 branch randomly to make sure we exercise both branches + return rand.Int()%2 == 0 //nolint:gosec // we don't care whether this is cryptographically secure or not + } w := httptest.NewRecorder() requestBeforeServe := tt.request.Clone(tt.request.Context())