From 89bca5244dc39cf3c48c41771d28b0036f133260 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/fake.go | 43 ++++++++++++++++++++++++ pkg/utils/authcheck/probechecker.go | 2 +- pkg/utils/authcheck/probechecker_test.go | 28 ++------------- 9 files changed, 56 insertions(+), 55 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..4eb7fe0eff 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.NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, 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.NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, 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..ad2673a4a8 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.NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, 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.NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, 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..8464a0ff6d 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.NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, 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.NewFakeAuthenticationCheck(authcheck.WorkloadIdentity, 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/fake.go b/pkg/utils/authcheck/fake.go new file mode 100644 index 0000000000..64ab5cb8d6 --- /dev/null +++ b/pkg/utils/authcheck/fake.go @@ -0,0 +1,43 @@ +/* +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 { + 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") +} 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..c39ffa91d6 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" ) @@ -86,29 +83,10 @@ 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() + defer 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") -}