address comments

cmd/broker/fanout/main.go

@@ -103,7 +103,7 @@ func main() {
 	if err != nil {
 		logger.Fatal("Failed to create fanout sync pool", zap.Error(err))
 	}
-	if _, err := handler.StartSyncPool(ctx, syncPool, syncSignal, env.MaxStaleDuration, handler.DefaultProbeCheckPort, authcheck.NewDefaultAuthenticationCheck(env.AuthType)); err != nil {
+	if _, err := handler.StartSyncPool(ctx, syncPool, syncSignal, env.MaxStaleDuration, handler.DefaultProbeCheckPort, authcheck.NewDefault(env.AuthType)); err != nil {
 		logger.Fatalw("Failed to start fanout sync pool", zap.Error(err))
 	}
 

cmd/broker/retry/main.go

@@ -101,7 +101,7 @@ func main() {
 	if err != nil {
 		logger.Fatal("Failed to get retry sync pool", zap.Error(err))
 	}
-	if _, err := handler.StartSyncPool(ctx, syncPool, syncSignal, env.MaxStaleDuration, handler.DefaultProbeCheckPort, authcheck.NewDefaultAuthenticationCheck(env.AuthType)); err != nil {
+	if _, err := handler.StartSyncPool(ctx, syncPool, syncSignal, env.MaxStaleDuration, handler.DefaultProbeCheckPort, authcheck.NewDefault(env.AuthType)); err != nil {
 		logger.Fatal("Failed to start retry sync pool", zap.Error(err))
 	}
 

pkg/broker/handler/fanout_test.go

@@ -63,7 +63,7 @@ func TestFanoutWatchAndSync(t *testing.T) {
 	}
 
 	t.Run("start sync pool creates no handler", func(t *testing.T) {
-		_, err = StartSyncPool(ctx, syncPool, signal, time.Minute, p, NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, true))
+		_, err = StartSyncPool(ctx, syncPool, signal, time.Minute, p, &authcheck.FakeAuthenticationCheck{NoError: true})
 		if err != nil {
 			t.Errorf("unexpected error from starting sync pool: %v", err)
 		}
@@ -156,7 +156,7 @@ func TestFanoutSyncPoolE2E(t *testing.T) {
 		t.Fatalf("failed to get random free port: %v", err)
 	}
 
-	if _, err := StartSyncPool(ctx, syncPool, signal, time.Minute, p, NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, true)); err != nil {
+	if _, err := StartSyncPool(ctx, syncPool, signal, time.Minute, p, &authcheck.FakeAuthenticationCheck{NoError: true}); err != nil {
 		t.Errorf("unexpected error from starting sync pool: %v", err)
 	}
 

pkg/broker/handler/pool_test.go

@@ -18,7 +18,6 @@ package handler
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net"
 	"net/http"
@@ -45,7 +44,7 @@ func TestSyncPool(t *testing.T) {
 			t.Fatalf("failed to get random free port: %v", err)
 		}
 
-		_, gotErr := StartSyncPool(ctx, syncPool, make(chan struct{}), 30*time.Second, p, NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, true))
+		_, gotErr := StartSyncPool(ctx, syncPool, make(chan struct{}), 30*time.Second, p, &authcheck.FakeAuthenticationCheck{NoError: true})
 		if gotErr == nil {
 			t.Error("StartSyncPool got unexpected result")
 		}
@@ -68,7 +67,7 @@ func TestSyncPool(t *testing.T) {
 		}
 
 		ch := make(chan struct{})
-		if _, err := StartSyncPool(ctx, syncPool, ch, time.Second, p, NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, true)); err != nil {
+		if _, err := StartSyncPool(ctx, syncPool, ch, time.Second, p, &authcheck.FakeAuthenticationCheck{NoError: true}); err != nil {
 			t.Errorf("StartSyncPool got unexpected error: %v", err)
 		}
 		syncPool.verifySyncOnceCalled(t)
@@ -143,22 +142,3 @@ func GetFreePort() (int, error) {
 	defer l.Close()
 	return l.Addr().(*net.TCPAddr).Port, nil
 }
-
-type FakeAuthenticationCheck struct {
-	authType authcheck.AuthType
-	noError  bool
-}
-
-func NewFakeAuthenticationCheck(authType authcheck.AuthType, noError bool) authcheck.AuthenticationCheck {
-	return &FakeAuthenticationCheck{
-		authType: authType,
-		noError:  noError,
-	}
-}
-
-func (ac *FakeAuthenticationCheck) Check(ctx context.Context) error {
-	if ac.noError {
-		return nil
-	}
-	return errors.New("induced error")
-}

pkg/broker/handler/retry_test.go

@@ -62,7 +62,7 @@ func TestRetryWatchAndSync(t *testing.T) {
 	}
 
 	t.Run("start sync pool creates no handler", func(t *testing.T) {
-		_, err = StartSyncPool(ctx, syncPool, signal, time.Minute, p, NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, true))
+		_, err = StartSyncPool(ctx, syncPool, signal, time.Minute, p, &authcheck.FakeAuthenticationCheck{NoError: true})
 		if err != nil {
 			t.Errorf("unexpected error from starting sync pool: %v", err)
 		}
@@ -158,7 +158,7 @@ func TestRetrySyncPoolE2E(t *testing.T) {
 		t.Fatalf("failed to get random free port: %v", err)
 	}
 
-	if _, err := StartSyncPool(ctx, syncPool, signal, time.Minute, p, NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, true)); err != nil {
+	if _, err := StartSyncPool(ctx, syncPool, signal, time.Minute, p, &authcheck.FakeAuthenticationCheck{NoError: true}); err != nil {
 		t.Errorf("unexpected error from starting sync pool: %v", err)
 	}
 

pkg/utils/authcheck/authcheck.go

@@ -50,7 +50,7 @@ type DefaultAuthenticationCheck struct {
 	url      string
 }
 
-func NewDefaultAuthenticationCheck(authType AuthType) AuthenticationCheck {
+func NewDefault(authType AuthType) AuthenticationCheck {
 	return &DefaultAuthenticationCheck{
 		authType: authType,
 		client:   http.DefaultClient,

pkg/utils/authcheck/authcheck_test.go

@@ -62,12 +62,12 @@ func TestAuthenticationCheck(t *testing.T) {
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
 			if tc.setEnv {
 				os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "empty")
+				defer os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS")
 			}
 
 			server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {

pkg/utils/authcheck/fake.go

@@ -0,0 +1,35 @@
+/*
+Copyright 2020 Google LLC
+
+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 authcheck provides utilities to check authentication configuration for data plane resources.
+// File authcheck contains functions to run customized checks inside of a Pod.
+package authcheck
+
+import (
+	"context"
+	"errors"
+)
+
+type FakeAuthenticationCheck struct {
+	NoError bool
+}
+
+func (ac *FakeAuthenticationCheck) Check(ctx context.Context) error {
+	if ac.NoError {
+		return nil
+	}
+	return errors.New("induced error")
+}

pkg/utils/authcheck/probechecker.go

@@ -40,7 +40,7 @@ func NewProbeChecker(logger *zap.Logger, authType AuthType) ProbeChecker {
 	return ProbeChecker{
 		logger:    logger,
 		port:      DefaultProbeCheckPort,
-		authCheck: NewDefaultAuthenticationCheck(authType),
+		authCheck: NewDefault(authType),
 	}
 }
 

pkg/utils/authcheck/probechecker_test.go

@@ -18,15 +18,12 @@ package authcheck
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net"
 	"net/http"
 	"testing"
 	"time"
 
-	"github.com/google/go-cmp/cmp"
-
 	"github.com/google/knative-gcp/pkg/logging"
 )
 
@@ -52,6 +49,7 @@ func TestProbeCheckResult(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
 			// Get a free port.
 			addr, err := net.ResolveTCPAddr("tcp", "localhost:0")
 			if err != nil {
@@ -68,7 +66,7 @@ func TestProbeCheckResult(t *testing.T) {
 			probeChecker := ProbeChecker{
 				logger:    logger,
 				port:      port,
-				authCheck: NewFakeAuthenticationCheck("", tc.noError),
+				authCheck: &FakeAuthenticationCheck{NoError: tc.noError},
 			}
 			go probeChecker.Start(ctx)
 
@@ -86,29 +84,9 @@ func TestProbeCheckResult(t *testing.T) {
 				t.Fatal("Failed to execute probe check:", err)
 				return
 			}
-			if diff := cmp.Diff(resp.StatusCode, tc.wantStatusCode); diff != "" {
-				t.Error("unexpected probe check result (-want, +got) = ", diff)
+			if tc.wantStatusCode != resp.StatusCode {
+				t.Errorf("unexpected probe check status code want %d, got %d)", tc.wantStatusCode, resp.StatusCode)
 			}
-			cancel()
 		})
 	}
 }
-
-type FakeAuthenticationCheck struct {
-	authType AuthType
-	noError  bool
-}
-
-func NewFakeAuthenticationCheck(authType AuthType, noError bool) AuthenticationCheck {
-	return &FakeAuthenticationCheck{
-		authType: authType,
-		noError:  noError,
-	}
-}
-
-func (ac *FakeAuthenticationCheck) Check(ctx context.Context) error {
-	if ac.noError {
-		return nil
-	}
-	return errors.New("induced error")
-}