From f9ff19c876b076b915429da86e6e80d2b661df7f Mon Sep 17 00:00:00 2001 From: grac3gao Date: Wed, 16 Dec 2020 11:42:44 -0800 Subject: [PATCH] address comments --- cmd/broker/fanout/main.go | 2 +- cmd/broker/retry/main.go | 2 +- pkg/broker/handler/fanout_test.go | 4 +-- pkg/broker/handler/pool_test.go | 24 ++-------------- pkg/broker/handler/retry_test.go | 4 +-- pkg/utils/authcheck/authcheck.go | 2 +- pkg/utils/authcheck/authcheck_test.go | 2 +- pkg/utils/authcheck/fake.go | 35 ++++++++++++++++++++++++ pkg/utils/authcheck/probechecker.go | 2 +- pkg/utils/authcheck/probechecker_test.go | 30 +++----------------- 10 files changed, 50 insertions(+), 57 deletions(-) create mode 100644 pkg/utils/authcheck/fake.go diff --git a/cmd/broker/fanout/main.go b/cmd/broker/fanout/main.go index aa166059fb..454932fb30 100644 --- a/cmd/broker/fanout/main.go +++ b/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)) } diff --git a/cmd/broker/retry/main.go b/cmd/broker/retry/main.go index 945a979bdb..7c653cd487 100644 --- a/cmd/broker/retry/main.go +++ b/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)) } diff --git a/pkg/broker/handler/fanout_test.go b/pkg/broker/handler/fanout_test.go index 2f528e6cfe..65275c4cbd 100644 --- a/pkg/broker/handler/fanout_test.go +++ b/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) } diff --git a/pkg/broker/handler/pool_test.go b/pkg/broker/handler/pool_test.go index 3ed03f8d64..28cb204a71 100644 --- a/pkg/broker/handler/pool_test.go +++ b/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") -} diff --git a/pkg/broker/handler/retry_test.go b/pkg/broker/handler/retry_test.go index acd370549c..13b8d6aa61 100644 --- a/pkg/broker/handler/retry_test.go +++ b/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) } diff --git a/pkg/utils/authcheck/authcheck.go b/pkg/utils/authcheck/authcheck.go index 7c69e8402b..482e65c337 100644 --- a/pkg/utils/authcheck/authcheck.go +++ b/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, diff --git a/pkg/utils/authcheck/authcheck_test.go b/pkg/utils/authcheck/authcheck_test.go index 239e96f9ca..cc28621a1e 100644 --- a/pkg/utils/authcheck/authcheck_test.go +++ b/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) { diff --git a/pkg/utils/authcheck/fake.go b/pkg/utils/authcheck/fake.go new file mode 100644 index 0000000000..9606cf7b0e --- /dev/null +++ b/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") +} diff --git a/pkg/utils/authcheck/probechecker.go b/pkg/utils/authcheck/probechecker.go index 2b1bdccbbd..aeb9a7e012 100644 --- a/pkg/utils/authcheck/probechecker.go +++ b/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), } } diff --git a/pkg/utils/authcheck/probechecker_test.go b/pkg/utils/authcheck/probechecker_test.go index 57eaef77e5..bd0aa7b367 100644 --- a/pkg/utils/authcheck/probechecker_test.go +++ b/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") -}