Add more tests to the logr bridge and the secret controller lifecycle

env.mk

@@ -21,7 +21,7 @@ GOLANGCI_LINT ?= github.com/golangci/golangci-lint/cmd/[email protected]
 GOSIMPORTS    ?= github.com/rinchsan/gosimports/cmd/[email protected]
 LICENSER      ?= github.com/liamawhite/[email protected]
 KIND          ?= sigs.k8s.io/[email protected]
-ENVTEST    ?= sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
+ENVTEST       ?= sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
 
 NAME    ?= authservice
 TARGETS ?= linux-amd64 linux-arm64 #darwin-amd64 darwin-arm64

internal/k8s/secret_controller.go

@@ -36,13 +36,13 @@ import (
 	"github.com/tetrateio/authservice-go/internal"
 )
 
-const (
-	clientSecretKey = "client-secret"
-)
+const clientSecretKey = "client-secret"
 
 var (
 	_ run.PreRunner      = (*SecretController)(nil)
 	_ run.ServiceContext = (*SecretController)(nil)
+
+	ErrCrossNamespaceSecretRef = errors.New("cross-namespace secret reference is not allowed")
 )
 
 // SecretController watches secrets for updates and updates the configuration with the loaded data.
@@ -116,10 +116,8 @@ func (s *SecretController) PreRun() error {
 
 			ref := oidcCfg.Oidc.GetClientSecretRef()
 			if ref.Namespace != "" && ref.Namespace != s.namespace {
-				return fmt.Errorf(
-					"cross-namespace secret reference is not allowed:"+
-						" secret reference namespace %s does not match the current namespace %s",
-					ref.Namespace, s.namespace)
+				return fmt.Errorf("%w: secret reference namespace %s does not match the current namespace %s",
+					ErrCrossNamespaceSecretRef, ref.Namespace, s.namespace)
 			}
 
 			key := secretNamespacedName(ref, s.namespace).String()
@@ -169,7 +167,7 @@ func (s *SecretController) ServeContext(ctx context.Context) error {
 	}
 
 	if err := s.manager.Start(ctx); err != nil {
-		return fmt.Errorf("error starting controller manager:%w", err)
+		return fmt.Errorf("error starting controller manager: %w", err)
 	}
 
 	return nil

internal/k8s/secret_controller_lifecycle_test.go

@@ -21,14 +21,12 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
-	"github.com/tetratelabs/log"
 	"github.com/tetratelabs/run"
 	runtest "github.com/tetratelabs/run/pkg/test"
 	"github.com/tetratelabs/telemetry"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
 
 	"github.com/tetrateio/authservice-go/internal"
 )
@@ -38,13 +36,13 @@ const (
 	defaultTick = time.Millisecond * 20
 )
 
-func TestController(t *testing.T) {
+func TestManagerStarts(t *testing.T) {
 	var (
 		g = run.Group{Logger: telemetry.NoopLogger()}
 
 		irq        = runtest.NewIRQService(func() {})
 		cfg        = internal.LocalConfigFile{}
-		logging    = internal.NewLogSystem(log.New(), &cfg.Config)
+		logging    = internal.NewLogSystem(telemetry.NoopLogger(), &cfg.Config)
 		controller = NewSecretController(&cfg.Config)
 	)
 
@@ -66,23 +64,54 @@ func TestController(t *testing.T) {
 		}, defaultWait, defaultTick, "Controller manager is not setup")
 	})
 
-	t.Run("controller is ready", func(t *testing.T) {
-		require.Eventually(t, func() bool {
-			err := controller.k8sClient.Create(context.Background(), &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-secret",
-					Namespace: "default",
-				},
-			})
-			return err == nil
-		}, defaultWait, defaultTick, "create secret failed")
+	mgrStarted := false
+	err := controller.manager.Add(manager.RunnableFunc(func(ctx context.Context) error {
+		mgrStarted = true
+		<-ctx.Done()
+		return ctx.Err()
+	}))
+	require.NoError(t, err)
+
+	t.Run("manager is started", func(t *testing.T) {
+		require.Eventually(t, func() bool { return mgrStarted },
+			defaultWait, defaultTick, "manager not started")
 	})
 
 	// signal group termination and wait for it
 	require.NoError(t, irq.Close())
 	wg.Wait()
 }
 
+func TestManagerNotInitializedIfNothingToWatch(t *testing.T) {
+	var (
+		g = run.Group{Logger: telemetry.NoopLogger()}
+
+		irq        = runtest.NewIRQService(func() {})
+		cfg        = internal.LocalConfigFile{}
+		logging    = internal.NewLogSystem(telemetry.NoopLogger(), &cfg.Config)
+		controller = NewSecretController(&cfg.Config)
+	)
+
+	controller.restConf = startEnv(t)
+	controller.namespace = "default"
+	g.Register(irq, &cfg, logging, controller)
+
+	wg := sync.WaitGroup{}
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		err := g.Run("", "--config-path", "testdata/oidc-without-secret-ref-in.json")
+		require.NoError(t, err)
+	}()
+
+	// signal group termination and wait for it
+	require.NoError(t, irq.Close())
+	wg.Wait()
+
+	// Verify that the manager was not set
+	require.Nil(t, controller.manager)
+}
+
 func startEnv(t *testing.T) *rest.Config {
 	env := &envtest.Environment{}
 	cfg, err := env.Start()

internal/k8s/secret_controller_reconcile_test.go

@@ -37,16 +37,14 @@ func TestOIDCProcessWithKubernetesSecret(t *testing.T) {
 	tests := []struct {
 		name     string
 		testFile string
-		err      string
+		err      error
 	}{
-		{"multiple secret refs", "oidc-with-multiple-secret-refs", ""},
-		{"no secret ref", "oidc-without-secret-ref", ""},
-		{"secret ref without data", "oidc-with-secret-ref-without-data", ""},
-		{"secret ref deleting", "oidc-with-secret-ref-deleting", ""},
-		{"secret ref not found", "oidc-with-secret-ref-not-found", ""},
-		{"cross namespace secret ref", "oidc-with-cross-ns-secret-ref",
-			"cross-namespace secret reference is not allowed: secret reference " +
-				"namespace test does not match the current namespace default"},
+		{"multiple secret refs", "oidc-with-multiple-secret-refs", nil},
+		{"no secret ref", "oidc-without-secret-ref", nil},
+		{"secret ref without data", "oidc-with-secret-ref-without-data", nil},
+		{"secret ref deleting", "oidc-with-secret-ref-deleting", nil},
+		{"secret ref not found", "oidc-with-secret-ref-not-found", nil},
+		{"cross namespace secret ref", "oidc-with-cross-ns-secret-ref", ErrCrossNamespaceSecretRef},
 	}
 
 	for _, tt := range tests {
@@ -63,9 +61,8 @@ func TestOIDCProcessWithKubernetesSecret(t *testing.T) {
 
 			// pre-run the controller
 			err := controller.PreRun()
-			if tt.err != "" {
-				require.EqualError(t, err, tt.err)
-			}
+			require.ErrorIs(t, err, tt.err)
+
 			// replace the k8s client with the fake client for testing
 			controller.k8sClient = kubeClient
 

internal/logr_test.go

@@ -30,6 +30,7 @@ func TestWithValues(t *testing.T) {
 	var a logr.LogSink = &logrAdapter{}
 	a = a.WithValues("one", 1, "two", 2)
 	a = a.WithValues("three")
+	a = a.WithValues()
 
 	require.Equal(t, map[string]interface{}{"one": 1, "two": 2, "three": "(MISSING)"}, a.(*logrAdapter).kvs)
 }
@@ -71,6 +72,25 @@ func TestInfo(t *testing.T) {
 	require.Contains(t, string(out), "level=info msg=\"test\" one=1 two=2")
 }
 
+func TestDebug(t *testing.T) {
+	rescueStdout := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	logger := log.New()
+	logger.SetLevel(telemetry.LevelDebug)
+	a := logrAdapter{
+		scope: logger,
+	}
+
+	a.Info(0, "test", "one", 1, "two", 2)
+
+	_ = w.Close()
+	out, _ := io.ReadAll(r)
+	os.Stdout = rescueStdout
+	require.Contains(t, string(out), "level=debug msg=\"test\" one=1 two=2")
+}
+
 func TestError(t *testing.T) {
 	rescueStdout := os.Stdout
 	r, w, _ := os.Pipe()
@@ -102,9 +122,11 @@ func TestMissingKV(t *testing.T) {
 	}
 
 	a.Info(0, "test", "one", 1, "two")
+	a.Error(errors.New("failed"), "got an error", "last")
 
 	_ = w.Close()
 	out, _ := io.ReadAll(r)
 	os.Stdout = rescueStdout
 	require.Contains(t, string(out), "level=info msg=\"test\" one=1 two=\"(MISSING)\"")
+	require.Contains(t, string(out), "level=error msg=\"got an error\" last=\"(MISSING)\" error=\"failed\"")
 }