Review changes with @doodlesbykumbi

- Remove defaultStatusUpdater singleton in favor of NewStatusUpdater factory. - Convert RepeatableSecretsProvider, which returns a RepeatableProviderFunc, to RunSecretsProvider, which runs execution logic. - ./bin/test_unit passes all args to go test command.
cyberark · Mar 30, 2023 · 28b74e0 · 28b74e0
1 parent a2f9c20
commit 28b74e0
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 119 deletions.
diff --git a/bin/test_unit b/bin/test_unit
@@ -5,12 +5,10 @@ set -eox pipefail
 junit_output_file="./junit.output"
 . bin/build_utils
 
-test_case="${1-}"
-
 function main() {
   retrieve_cyberark_ca_cert
   build_docker_ut_image
-  run_unit_tests
+  run_unit_tests $@
 }
 
 function build_docker_ut_image() {
@@ -21,20 +19,15 @@ function build_docker_ut_image() {
 function run_unit_tests() {
   echo "Running unit tests..."
 
-  test_case_cmd=""
-  if [[ "$test_case" != "" ]]; then
-    test_case_cmd="--run $test_case"
-  fi
-
   docker run --rm -t \
              --volume "$PWD"/:/secrets-provider-for-k8s/test/ \
              secrets-provider-for-k8s-test-runner:latest \
              -coverprofile="./test/c.out" \
              ./cmd/... \
              ./pkg/... \
-             $test_case_cmd \
+             $@ \
              | tee -a "$junit_output_file"
   echo "Unit test exit status: $?"
 }
 
-main
+main $@
diff --git a/pkg/entrypoint/entrypoint.go b/pkg/entrypoint/entrypoint.go
@@ -55,7 +55,7 @@ func StartSecretsProvider() {
 		defaultTemplatesBasePath,
 		conjur.NewSecretRetriever,
 		secrets.NewProviderForType,
-		secrets.DefaultStatusUpdater,
+		secrets.NewStatusUpdater,
 	)
 	os.Exit(exitCode)
 }
@@ -99,7 +99,7 @@ func startSecretsProviderWithDeps(
 		return
 	}
 
-	provideSecretsFunc, secretsConfig, err := secretsProvider(
+	provideSecrets, secretsConfig, err := secretsProvider(
 		ctx,
 		tracer,
 		secretsBasePath,
@@ -112,22 +112,26 @@ func startSecretsProviderWithDeps(
 		return
 	}
 
-	provideSecretsFunc = secrets.RetryableSecretProvider(
+	provideSecrets = secrets.RetryableSecretProvider(
 		time.Duration(secretsConfig.RetryIntervalSec)*time.Second,
 		secretsConfig.RetryCountLimit,
-		provideSecretsFunc,
-	)
-
-	provideSecrets := repeatableSecretsProvider(
-		ctx,
-		tracer,
-		secretsConfig.SecretsRefreshInterval,
-		provideSecretsFunc,
-		statusUpdaterFactory,
+		provideSecrets,
 	)
 
-	// Provide secrets
-	if err = provideSecrets(); err != nil {
+	if err = secrets.RunSecretsProvider(
+		secrets.ProviderRefreshConfig{
+			Mode:                  getContainerMode(),
+			SecretRefreshInterval: secretsConfig.SecretsRefreshInterval,
+			// Create a channel to send a quit signal to the periodic secret provider.
+			// TODO: Currently, this is just used for testing, but in the future we
+			// may want to create a SIGTERM or SIGHUP handler to catch a signal from
+			// a user / external entity, and then send an (empty struct) quit signal
+			// on this channel to trigger a graceful shut down of the Secrets Provider.
+			ProviderQuit: make(chan struct{}),
+		},
+		provideSecrets,
+		statusUpdaterFactory(),
+	); err != nil {
 		logError(err.Error())
 	}
 	return
@@ -231,36 +235,6 @@ func secretsProvider(
 	return provideSecrets, secretsConfig, nil
 }
 
-func repeatableSecretsProvider(
-	ctx context.Context,
-	tracer trace.Tracer,
-	refreshInterval time.Duration,
-	provideSecrets secrets.ProviderFunc,
-	statusUpdaterFactory secrets.StatusUpdaterFactory,
-) secrets.RepeatableProviderFunc {
-	_, span := tracer.Start(ctx, "Create repeatable secrets provider")
-	defer span.End()
-
-	// Create a channel to send a quit signal to the periodic secret provider.
-	// TODO: Currently, this is just used for testing, but in the future we
-	// may want to create a SIGTERM or SIGHUP handler to catch a signal from
-	// a user / external entity, and then send an (empty struct) quit signal
-	// on this channel to trigger a graceful shut down of the Secrets Provider.
-	providerQuit := make(chan struct{})
-
-	refreshConfig := secrets.ProviderRefreshConfig{
-		Mode:                  getContainerMode(),
-		SecretRefreshInterval: refreshInterval,
-		ProviderQuit:          providerQuit,
-	}
-
-	return secrets.RepeatableSecretProvider(
-		refreshConfig,
-		provideSecrets,
-		statusUpdaterFactory(),
-	)
-}
-
 func customEnv(key string) string {
 	if annotation, ok := envAnnotationsConversion[key]; ok {
 		if value := annotationsMap[annotation]; value != "" {

diff --git a/pkg/secrets/provide_conjur_secrets.go b/pkg/secrets/provide_conjur_secrets.go
@@ -120,71 +120,68 @@ type ProviderRefreshConfig struct {
 	ProviderQuit          chan struct{}
 }
 
-// RepeatableSecretProvider returns a new ProviderFunc, which wraps a retryable
-// ProviderFunc inside a function that operates in one of three modes:
+// RunSecretsProvider takes a retryable ProviderFunc, and runs it in one of three modes:
 //   - Run once and return (for init or application container modes)
 //   - Run once and sleep forever (for sidecar mode without periodic refresh)
 //   - Run periodically (for sidecar mode with periodic refresh)
-func RepeatableSecretProvider(
+func RunSecretsProvider(
 	config ProviderRefreshConfig,
 	provideSecrets ProviderFunc,
 	status StatusUpdater,
-) RepeatableProviderFunc {
+) error {
 
 	var periodicQuit = make(chan struct{})
 	var periodicError = make(chan error)
 	var ticker *time.Ticker
 	var err error
 
-	return func() error {
-		if err = status.CopyScripts(); err != nil {
-			return err
-		}
-		if _, err = provideSecrets(); err != nil {
-			// Return immediately upon error, regardless of operating mode
-			return err
-		}
-		err = status.SetSecretsProvided()
-		if err != nil {
-			return err
-		}
-		switch {
-		case config.Mode != "sidecar":
-			// Run once and return if not in sidecar mode
-			return nil
-		case config.SecretRefreshInterval > 0:
-			// Run periodically if in sidecar mode with periodic refresh
-			ticker = time.NewTicker(config.SecretRefreshInterval)
-			config := periodicConfig{
-				ticker:        ticker,
-				periodicQuit:  periodicQuit,
-				periodicError: periodicError,
-			}
-			go periodicSecretProvider(provideSecrets, config, status)
-		default:
-			// Run once and sleep forever if in sidecar mode without
-			// periodic refresh (fall through)
+	if err = status.CopyScripts(); err != nil {
+		return err
+	}
+	if _, err = provideSecrets(); err != nil {
+		// Return immediately upon error, regardless of operating mode
+		return err
+	}
+	err = status.SetSecretsProvided()
+	if err != nil {
+		return err
+	}
+	switch {
+	case config.Mode != "sidecar":
+		// Run once and return if not in sidecar mode
+		return nil
+	case config.SecretRefreshInterval > 0:
+		// Run periodically if in sidecar mode with periodic refresh
+		ticker = time.NewTicker(config.SecretRefreshInterval)
+		config := periodicConfig{
+			ticker:        ticker,
+			periodicQuit:  periodicQuit,
+			periodicError: periodicError,
 		}
+		go periodicSecretProvider(provideSecrets, config, status)
+	default:
+		// Run once and sleep forever if in sidecar mode without
+		// periodic refresh (fall through)
+	}
 
-		// Wait here for a signal to quit providing secrets or an error
-		// from the periodicSecretProvider() function
-		select {
-		case <-config.ProviderQuit:
-			break
-		case err = <-periodicError:
-			break
-		}
+	// Wait here for a signal to quit providing secrets or an error
+	// from the periodicSecretProvider() function
+	select {
+	case <-config.ProviderQuit:
+		break
+	case err = <-periodicError:
+		break
+	}
 
-		// Allow the periodicSecretProvider goroutine to gracefully shut down
-		if config.SecretRefreshInterval > 0 {
-			// Kill the ticker
-			ticker.Stop()
-			periodicQuit <- struct{}{}
-			// Let the go routine exit
-			time.Sleep(secretProviderGracePeriod)
-		}
-		return err
+	// Allow the periodicSecretProvider goroutine to gracefully shut down
+	if config.SecretRefreshInterval > 0 {
+		// Kill the ticker
+		ticker.Stop()
+		periodicQuit <- struct{}{}
+		// Let the go routine exit
+		time.Sleep(secretProviderGracePeriod)
 	}
+	return err
 }
 
 type periodicConfig struct {

diff --git a/pkg/secrets/provide_conjur_secrets_test.go b/pkg/secrets/provide_conjur_secrets_test.go
@@ -148,7 +148,7 @@ func TestRetryableSecretProvider(t *testing.T) {
 	}
 }
 
-func TestRepeatableSecretProvider(t *testing.T) {
+func TestRunSecretsProvider(t *testing.T) {
 	testCases := []struct {
 		description    string
 		mode           string
@@ -298,13 +298,11 @@ func TestRepeatableSecretProvider(t *testing.T) {
 				SecretRefreshInterval: tc.interval,
 				ProviderQuit:          providerQuit,
 			}
-			provideSecrets := RepeatableSecretProvider(
-				refreshConfig, tc.provider.provide, fileUpdater)
 
 			// Run the secrets provider
 			testError := make(chan error)
 			go func() {
-				err := provideSecrets()
+				err := RunSecretsProvider(refreshConfig, tc.provider.provide, fileUpdater)
 				testError <- err
 			}()
 			select {

diff --git a/pkg/secrets/provider_status.go b/pkg/secrets/provider_status.go
@@ -59,9 +59,16 @@ var stdOSFuncs = osFuncs{
 // implementation.
 type StatusUpdaterFactory func() StatusUpdater
 
-// DefaultStatusUpdater returns the default StatusUpdater.
-func DefaultStatusUpdater() StatusUpdater {
-	return defaultStatusUpdater
+// NewStatusUpdater returns a new instance of the default StatusUpdater.
+func NewStatusUpdater() StatusUpdater {
+	return fileUpdater{
+		providedFile:  "/conjur/status/CONJUR_SECRETS_PROVIDED",
+		updatedFile:   "/conjur/status/CONJUR_SECRETS_UPDATED",
+		scripts:       []string{"conjur-secrets-unchanged.sh"},
+		scriptSrcDir:  "/usr/local/bin",
+		scriptDestDir: "/conjur/status",
+		os:            stdOSFuncs,
+	}
 }
 
 // fileUpdater implements the statusUpdater interface. It records provider
@@ -75,15 +82,6 @@ type fileUpdater struct {
 	os            osFuncs
 }
 
-var defaultStatusUpdater = fileUpdater{
-	providedFile:  "/conjur/status/CONJUR_SECRETS_PROVIDED",
-	updatedFile:   "/conjur/status/CONJUR_SECRETS_UPDATED",
-	scripts:       []string{"conjur-secrets-unchanged.sh"},
-	scriptSrcDir:  "/usr/local/bin",
-	scriptDestDir: "/conjur/status",
-	os:            stdOSFuncs,
-}
-
 func (f fileUpdater) setStatus(path string) error {
 	file, err := f.os.create(path)
 	if err != nil {