From cf06222923a1a214459074455a9785f8fe907d3a Mon Sep 17 00:00:00 2001 From: Julian Friedman Date: Thu, 14 Jan 2021 19:40:20 +0000 Subject: [PATCH] Remove activator/util package (#10557) Util isn't a very descriptive package name, and it now only contains NewHeaderPruningReverseProxy. This commit removes util package, moves NewHeaderPruningReverseProxy to pkg/http instead and exports the activator revision headers to remove from `activator` package. --- cmd/queue/main.go | 4 ++-- pkg/activator/activator.go | 9 +++++++++ pkg/activator/handler/handler.go | 4 ++-- .../util/header.go => http/proxy.go} | 14 +++----------- .../util/header_test.go => http/proxy_test.go} | 18 ++++++++++-------- 5 files changed, 26 insertions(+), 23 deletions(-) rename pkg/{activator/util/header.go => http/proxy.go} (74%) rename pkg/{activator/util/header_test.go => http/proxy_test.go} (85%) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index 3da8cab990d3..3ea930129b6d 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -44,7 +44,7 @@ import ( "knative.dev/pkg/tracing" tracingconfig "knative.dev/pkg/tracing/config" "knative.dev/pkg/tracing/propagation/tracecontextb3" - activatorutil "knative.dev/serving/pkg/activator/util" + "knative.dev/serving/pkg/activator" pkghttp "knative.dev/serving/pkg/http" "knative.dev/serving/pkg/http/handler" "knative.dev/serving/pkg/logging" @@ -272,7 +272,7 @@ func buildServer(ctx context.Context, env config, healthState *health.State, rp target := net.JoinHostPort("127.0.0.1", env.UserPort) - httpProxy := activatorutil.NewHeaderPruningReverseProxy(target) + httpProxy := pkghttp.NewHeaderPruningReverseProxy(target, activator.RevisionHeaders) httpProxy.Transport = buildTransport(env, logger, maxIdleConns) httpProxy.ErrorHandler = pkgnet.ErrorHandler(logger) httpProxy.BufferPool = network.NewBufferPool() diff --git a/pkg/activator/activator.go b/pkg/activator/activator.go index 4dcb201b2e45..eb8d73d18c2f 100644 --- a/pkg/activator/activator.go +++ b/pkg/activator/activator.go @@ -24,3 +24,12 @@ const ( // RevisionHeaderNamespace is the header key for revision's namespace. RevisionHeaderNamespace = "Knative-Serving-Namespace" ) + +var ( + // RevisionHeaders are the headers the activator uses to identify the + // revision. They are removed before reaching the user container. + RevisionHeaders = []string{ + RevisionHeaderName, + RevisionHeaderNamespace, + } +) diff --git a/pkg/activator/handler/handler.go b/pkg/activator/handler/handler.go index 62434e278fbf..a291675b14f2 100644 --- a/pkg/activator/handler/handler.go +++ b/pkg/activator/handler/handler.go @@ -34,7 +34,7 @@ import ( "knative.dev/pkg/tracing/propagation/tracecontextb3" "knative.dev/serving/pkg/activator" activatorconfig "knative.dev/serving/pkg/activator/config" - "knative.dev/serving/pkg/activator/util" + pkghttp "knative.dev/serving/pkg/http" "knative.dev/serving/pkg/queue" ) @@ -105,7 +105,7 @@ func (a *activationHandler) proxyRequest(logger *zap.SugaredLogger, w http.Respo r.Header.Set(network.ProxyHeaderName, activator.Name) // Set up the reverse proxy. - proxy := util.NewHeaderPruningReverseProxy(target) + proxy := pkghttp.NewHeaderPruningReverseProxy(target, activator.RevisionHeaders) proxy.BufferPool = a.bufferPool proxy.Transport = a.transport if tracingEnabled { diff --git a/pkg/activator/util/header.go b/pkg/http/proxy.go similarity index 74% rename from pkg/activator/util/header.go rename to pkg/http/proxy.go index 90ec176f1ef2..ff96d23a2819 100644 --- a/pkg/activator/util/header.go +++ b/pkg/http/proxy.go @@ -14,24 +14,16 @@ See the License for the specific language governing permissions and limitations under the License. */ -package util +package http import ( "net/http" "net/http/httputil" - - "knative.dev/serving/pkg/activator" ) -var headersToRemove = []string{ - activator.RevisionHeaderName, - activator.RevisionHeaderNamespace, -} - // NewHeaderPruningReverseProxy returns a httputil.ReverseProxy that proxies -// requests to the given targetHost. The returned reverse proxy strips -// activator-specific headers that should not reach the user container. -func NewHeaderPruningReverseProxy(targetHost string) *httputil.ReverseProxy { +// requests to the given targetHost after removing the headersToRemove. +func NewHeaderPruningReverseProxy(targetHost string, headersToRemove []string) *httputil.ReverseProxy { return &httputil.ReverseProxy{ Director: func(req *http.Request) { req.URL.Scheme = "http" diff --git a/pkg/activator/util/header_test.go b/pkg/http/proxy_test.go similarity index 85% rename from pkg/activator/util/header_test.go rename to pkg/http/proxy_test.go index 8e8ae63cb0e9..01e808b752fa 100644 --- a/pkg/activator/util/header_test.go +++ b/pkg/http/proxy_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package util +package http import ( "encoding/json" @@ -24,10 +24,9 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "knative.dev/serving/pkg/activator" ) -func TestNewProxy(t *testing.T) { +func TestNewHeaderPruningProxy(t *testing.T) { var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { if err := json.NewEncoder(w).Encode(r.Header); err != nil { panic(err) @@ -48,12 +47,12 @@ func TestNewProxy(t *testing.T) { name: "prunes activator headers, does not add user agent header", url: "http://example.com/", header: http.Header{ - "Header": []string{"value"}, - activator.RevisionHeaderName: []string{"some-value"}, - activator.RevisionHeaderNamespace: []string{"some-value"}, + "Header-Not-To-Remove": []string{"value"}, + "Header-To-Remove-1": []string{"some-value"}, + "Header-To-Remove-2": []string{"some-value"}, }, expectHeaders: http.Header{ - "Header": []string{"value"}, + "Header-Not-To-Remove": []string{"value"}, }, }, { name: "explicit user agent header not removed", @@ -68,7 +67,10 @@ func TestNewProxy(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - proxy := NewHeaderPruningReverseProxy(serverURL.Host) + proxy := NewHeaderPruningReverseProxy(serverURL.Host, []string{ + "header-to-remove-1", + "header-to-remove-2", + }) resp := httptest.NewRecorder() req := httptest.NewRequest(http.MethodPost, test.url, nil)