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

…#64)

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,14 @@ import (
 	"github.com/tetrateio/authservice-go/internal"
 )
 
-const (
-	clientSecretKey = "client-secret"
-)
+const clientSecretKey = "client-secret"
 
 var (
 	_ run.PreRunner      = (*SecretController)(nil)
 	_ run.ServiceContext = (*SecretController)(nil)
+
+	ErrLoadingConfig           = errors.New("error loading kube config")
+	ErrCrossNamespaceSecretRef = errors.New("cross-namespace secret reference is not allowed")
 )
 
 // SecretController watches secrets for updates and updates the configuration with the loaded data.
@@ -104,27 +105,8 @@ func (s *SecretController) PreRun() error {
 	}
 
 	// Collect the k8s secrets that are used in the configuration
-	s.secrets = make(map[string][]*oidcv1.OIDCConfig)
-	for _, c := range s.config.Chains {
-		for _, f := range c.Filters {
-			oidcCfg, isOIDCConf := f.Type.(*configv1.Filter_Oidc)
-			if !isOIDCConf ||
-				oidcCfg.Oidc.GetClientSecretRef() == nil ||
-				oidcCfg.Oidc.GetClientSecretRef().GetName() == "" {
-				continue
-			}
-
-			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)
-			}
-
-			key := secretNamespacedName(ref, s.namespace).String()
-			s.secrets[key] = append(s.secrets[key], oidcCfg.Oidc)
-		}
+	if err = s.loadSecrets(); err != nil {
+		return err
 	}
 
 	// Load the k8s configuration from in-cluster environment
@@ -169,12 +151,37 @@ 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
 }
 
+// loadSecrets loads the secrets from the configuration and stores them in the secrets map.
+func (s *SecretController) loadSecrets() error {
+	s.secrets = make(map[string][]*oidcv1.OIDCConfig)
+	for _, c := range s.config.Chains {
+		for _, f := range c.Filters {
+			oidcCfg, isOIDCConf := f.Type.(*configv1.Filter_Oidc)
+			if !isOIDCConf ||
+				oidcCfg.Oidc.GetClientSecretRef() == nil ||
+				oidcCfg.Oidc.GetClientSecretRef().GetName() == "" {
+				continue
+			}
+
+			ref := oidcCfg.Oidc.GetClientSecretRef()
+			if ref.Namespace != "" && 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()
+			s.secrets[key] = append(s.secrets[key], oidcCfg.Oidc)
+		}
+	}
+	return nil
+}
+
 func secretNamespacedName(secretRef *oidcv1.OIDCConfig_SecretReference, currentNamespace string) types.NamespacedName {
 	return types.NamespacedName{
 		Namespace: currentNamespace,

internal/k8s/secret_controller_lifecycle_test.go

@@ -21,35 +21,42 @@ 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"
 )
 
 const (
-	defaultWait = time.Second * 10
-	defaultTick = time.Millisecond * 20
+	defaultWait      = time.Second * 10
+	defaultTick      = time.Millisecond * 20
+	defaultNamespace = "default"
 )
 
-func TestController(t *testing.T) {
+func TestErrorLoadingConfig(t *testing.T) {
+	t.Setenv("KUBECONFIG", "non-existent-file")
+	sc := NewSecretController(loadTestConf(t, "testdata/oidc-with-secret-ref.json"))
+	sc.namespace = defaultNamespace
+
+	require.ErrorIs(t, sc.PreRun(), ErrLoadingConfig)
+}
+
+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)
 	)
 
 	controller.restConf = startEnv(t)
-	controller.namespace = "default"
+	controller.namespace = defaultNamespace
 	g.Register(irq, &cfg, logging, controller)
 
 	wg := sync.WaitGroup{}
@@ -66,27 +73,59 @@ 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 = defaultNamespace
+	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()
 	require.NoError(t, err)
+
 	t.Cleanup(func() {
 		require.NoError(t, env.Stop())
 	})

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 {
@@ -60,26 +58,20 @@ func TestOIDCProcessWithKubernetesSecret(t *testing.T) {
 			kubeClient := fake.NewClientBuilder().WithLists(secrets).Build()
 			controller := NewSecretController(originalConf)
 			controller.namespace = "default"
-
-			// pre-run the controller
-			err := controller.PreRun()
-			if tt.err != "" {
-				require.EqualError(t, err, tt.err)
-			}
-			// replace the k8s client with the fake client for testing
-			controller.k8sClient = kubeClient
+			controller.k8sClient = kubeClient // set the k8s client with the fake client for testing
+			require.ErrorIs(t, controller.loadSecrets(), tt.err)
 
 			// reconcile the secrets
 			for _, secret := range secrets.Items {
-				_, err = controller.Reconcile(context.Background(), ctrl.Request{
+				_, err := controller.Reconcile(context.Background(), ctrl.Request{
 					NamespacedName: types.NamespacedName{
 						Namespace: secret.Namespace,
 						Name:      secret.Name,
 					},
 				})
 				require.NoError(t, err)
 			}
-			_, err = controller.Reconcile(context.Background(), ctrl.Request{
+			_, err := controller.Reconcile(context.Background(), ctrl.Request{
 				NamespacedName: types.NamespacedName{
 					Namespace: "default",
 					Name:      "non-existing-secret",