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.
knative · Jan 14, 2021 · cf06222 · cf06222
1 parent 95c699b
commit cf06222
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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 → pkg/http/proxy.go b/pkg/activator/util/header.go → 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 → pkg/http/proxy_test.go b/pkg/activator/util/header_test.go → 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)