From a406ee0f0e57164fae664426612598422d3d217e Mon Sep 17 00:00:00 2001 From: John ODonnell Date: Thu, 30 Mar 2023 11:57:03 -0400 Subject: [PATCH] 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. --- bin/test_unit | 13 +-- pkg/entrypoint/entrypoint.go | 62 ++++---------- pkg/secrets/provide_conjur_secrets.go | 95 +++++++++++----------- pkg/secrets/provide_conjur_secrets_test.go | 6 +- pkg/secrets/provider_status.go | 22 +++-- 5 files changed, 79 insertions(+), 119 deletions(-) diff --git a/bin/test_unit b/bin/test_unit index 809e9509..0e4abba6 100755 --- 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 index 020f0ea5..723b77c6 100644 --- 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 index d499a7bf..380101da 100644 --- 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 index 3ff8bb67..d6b9e3e2 100644 --- 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 index 4cf4a1a0..e1b398c1 100644 --- 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 {