From e8559d142a7caecfc103ad0f2d31a52112798624 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 24 May 2023 23:33:51 -0400 Subject: [PATCH] Fix worker auth rotation test (#3243) Most of the changes here are changes or enhancements that are useful for tests. The crux of the issue was that I had pulled up one var to the package level so it could be read from tests, but that var was not being reset between tests. It's not even used in tests anymore, so I put it back to function-local. --- go.mod | 2 +- go.sum | 4 +-- internal/cmd/config/options.go | 22 +++++++++++++-- internal/daemon/controller/testing.go | 8 ++++++ internal/daemon/worker/auth_rotation.go | 28 +++++++++----------- internal/daemon/worker/auth_rotation_test.go | 15 ++++++----- internal/daemon/worker/testing.go | 1 + internal/daemon/worker/worker.go | 4 +++ 8 files changed, 57 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index 7930cb3b9..c6e09391b 100644 --- a/go.mod +++ b/go.mod @@ -95,7 +95,7 @@ require ( github.com/hashicorp/cap/ldap v0.0.0-20230420150311-6d1e00a6c5e0 github.com/hashicorp/go-kms-wrapping/extras/kms/v2 v2.0.0-20221122211539-47c893099f13 github.com/hashicorp/go-version v1.6.0 - github.com/hashicorp/nodeenrollment v0.2.3 + github.com/hashicorp/nodeenrollment v0.2.4 github.com/jimlambrt/gldap v0.1.6 github.com/kelseyhightower/envconfig v1.4.0 github.com/mikesmitty/edkey v0.0.0-20170222072505-3356ea4e686a diff --git a/go.sum b/go.sum index ed17cf32d..ff53f4d00 100644 --- a/go.sum +++ b/go.sum @@ -755,8 +755,8 @@ github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= -github.com/hashicorp/nodeenrollment v0.2.3 h1:Lx1g5QBHQ/DyorfSp6W61Q0SLsmIsQyjjd3EUcmElhI= -github.com/hashicorp/nodeenrollment v0.2.3/go.mod h1:Do2FvaFHgnnMaqhdSTBX+TkQP0ep8VVQ0rOr+ZjUbJU= +github.com/hashicorp/nodeenrollment v0.2.4 h1:Rhdgtdd26Ng+r2+m6CDVWT8+wwQzN8y+6kApoc5Q91U= +github.com/hashicorp/nodeenrollment v0.2.4/go.mod h1:Do2FvaFHgnnMaqhdSTBX+TkQP0ep8VVQ0rOr+ZjUbJU= github.com/hashicorp/vault/api v1.9.1 h1:LtY/I16+5jVGU8rufyyAkwopgq/HpUnxFBg+QLOAV38= github.com/hashicorp/vault/api v1.9.1/go.mod h1:78kktNcQYbBGSrOjQfHjXN32OhhxXnbYl3zxpd2uPUs= github.com/hashicorp/vault/sdk v0.1.13/go.mod h1:B+hVj7TpuQY1Y/GPbCpffmgd+tSEwvhkWnjtSYCaS2M= diff --git a/internal/cmd/config/options.go b/internal/cmd/config/options.go index ad7ce0187..2a50d8da6 100644 --- a/internal/cmd/config/options.go +++ b/internal/cmd/config/options.go @@ -2,7 +2,10 @@ package config -import "testing" +import ( + "os" + "testing" +) // getOpts - iterate the inbound Options and return a struct func getOpts(opt ...Option) options { @@ -25,7 +28,22 @@ type options struct { } func getDefaultOptions() options { - return options{} + opts := options{} + + if os.Getenv("BOUNDARY_ENABLE_TEST_SYS_EVENTS") != "" { + opts.withSysEventsEnabled = true + } + if os.Getenv("BOUNDARY_ENABLE_TEST_AUDIT_EVENTS") != "" { + opts.withAuditEventsEnabled = true + } + if os.Getenv("BOUNDARY_ENABLE_TEST_OBSERVATIONS") != "" { + opts.withObservationsEnabled = true + } + if os.Getenv("BOUNDARY_ENABLE_TEST_ERROR_EVENTS") != "" { + opts.testWithErrorEventsEnabled = true + } + + return opts } // WithSysEventsEnabled provides an option for enabling system events diff --git a/internal/daemon/controller/testing.go b/internal/daemon/controller/testing.go index fcf9f118b..c6fd591da 100644 --- a/internal/daemon/controller/testing.go +++ b/internal/daemon/controller/testing.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "testing" "time" @@ -476,6 +477,9 @@ type TestControllerOpts struct { // The time to use for CA certificate lifetime for worker auth WorkerAuthCaCertificateLifetime time.Duration + + // Toggle worker auth debugging + WorkerAuthDebuggingEnabled *atomic.Bool } func NewTestController(t testing.TB, opts *TestControllerOpts) *TestController { @@ -494,6 +498,7 @@ func NewTestController(t testing.TB, opts *TestControllerOpts) *TestController { shutdownDoneCh: make(chan struct{}), shutdownOnce: new(sync.Once), } + t.Cleanup(tc.Shutdown) conf := TestControllerConfig(t, ctx, tc, opts) var err error @@ -546,6 +551,7 @@ func TestControllerConfig(t testing.TB, ctx context.Context, tc *TestController, ContextCancel: cancel, ShutdownCh: make(chan struct{}), }) + tc.b.WorkerAuthDebuggingEnabled = opts.WorkerAuthDebuggingEnabled // Get dev config, or use a provided one var err error @@ -804,6 +810,8 @@ func (tc *TestController) AddClusterControllerMember(t testing.TB, opts *TestCon PublicClusterAddr: opts.PublicClusterAddr, WorkerStatusGracePeriodDuration: opts.WorkerStatusGracePeriodDuration, LivenessTimeToStaleDuration: opts.LivenessTimeToStaleDuration, + WorkerAuthCaCertificateLifetime: tc.c.conf.TestOverrideWorkerAuthCaCertificateLifetime, + WorkerAuthDebuggingEnabled: tc.c.conf.WorkerAuthDebuggingEnabled, } if opts.Logger != nil { nextOpts.Logger = opts.Logger diff --git a/internal/daemon/worker/auth_rotation.go b/internal/daemon/worker/auth_rotation.go index 7dcda19a7..8241b449f 100644 --- a/internal/daemon/worker/auth_rotation.go +++ b/internal/daemon/worker/auth_rotation.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "math/rand" - "sync/atomic" "time" berrors "github.com/hashicorp/boundary/internal/errors" @@ -23,14 +22,6 @@ import ( // we can't get a better reset time const defaultAuthRotationResetDuration = 5 * time.Second -// AuthRotationResetDuration allows us to view it from tests, which allows us to -// get test timing right. It'll start at 0 which will cause us to run -// immediately. -var AuthRotationResetDuration time.Duration - -// AuthRotationNextRotation is useful in tests to understand how long to sleep -var AuthRotationNextRotation atomic.Pointer[time.Time] - func (w *Worker) startAuthRotationTicking(cancelCtx context.Context) { const op = "worker.(Worker).startAuthRotationTicking" @@ -39,9 +30,14 @@ func (w *Worker) startAuthRotationTicking(cancelCtx context.Context) { return } + event.WriteSysEvent(cancelCtx, op, "starting auth rotation ticking") + + // This will start at 0 which will cause us to run immediately + var authRotationResetDuration time.Duration + // Get a valid value in startNow := time.Now() - AuthRotationNextRotation.Store(&startNow) + w.AuthRotationNextRotation.Store(&startNow) timer := time.NewTimer(defaultAuthRotationResetDuration) lastRotation := time.Time{}.UTC() @@ -57,9 +53,9 @@ func (w *Worker) startAuthRotationTicking(cancelCtx context.Context) { } if w.TestOverrideAuthRotationPeriod != 0 { - AuthRotationResetDuration = w.TestOverrideAuthRotationPeriod + authRotationResetDuration = w.TestOverrideAuthRotationPeriod } - timer.Reset(AuthRotationResetDuration) + timer.Reset(authRotationResetDuration) select { case <-cancelCtx.Done(): @@ -70,7 +66,7 @@ func (w *Worker) startAuthRotationTicking(cancelCtx context.Context) { // Add some jitter in case there is some issue to prevent thundering // herd jitter := time.Duration(rand.Intn(6)) * time.Second - AuthRotationResetDuration = defaultAuthRotationResetDuration + jitter + authRotationResetDuration = defaultAuthRotationResetDuration + jitter // Check if it's time to rotate and if not don't do anything currentNodeCreds, err := types.LoadNodeCredentials( @@ -188,7 +184,7 @@ func (w *Worker) startAuthRotationTicking(cancelCtx context.Context) { // See if we don't need to do anything, and if so calculate reset // duration and loop back if !lastRotation.IsZero() && now.Before(nextRotation) { - AuthRotationResetDuration = lastRotation.Add(rotationInterval / 2).Sub(now) + authRotationResetDuration = lastRotation.Add(rotationInterval / 2).Sub(now) continue } @@ -199,8 +195,8 @@ func (w *Worker) startAuthRotationTicking(cancelCtx context.Context) { } lastRotation = now nextRotation = lastRotation.Add(newRotationInterval / 2) - AuthRotationNextRotation.Store(&nextRotation) - AuthRotationResetDuration = nextRotation.Sub(now) + w.AuthRotationNextRotation.Store(&nextRotation) + authRotationResetDuration = nextRotation.Sub(now) event.WriteSysEvent(cancelCtx, op, "worker credentials rotated", "next_rotation", nextRotation) } } diff --git a/internal/daemon/worker/auth_rotation_test.go b/internal/daemon/worker/auth_rotation_test.go index 3d9e2097d..0545994cc 100644 --- a/internal/daemon/worker/auth_rotation_test.go +++ b/internal/daemon/worker/auth_rotation_test.go @@ -4,6 +4,7 @@ package worker_test import ( + "sync/atomic" "testing" "time" @@ -35,12 +36,14 @@ func TestRotationTicking(t *testing.T) { conf, err := config.DevController() require.NoError(err) + workerAuthDebugEnabled := new(atomic.Bool) + workerAuthDebugEnabled.Store(true) c := controller.NewTestController(t, &controller.TestControllerOpts{ Config: conf, Logger: logger.Named("controller"), WorkerAuthCaCertificateLifetime: rotationPeriod, + WorkerAuthDebuggingEnabled: workerAuthDebugEnabled, }) - t.Cleanup(c.Shutdown) // names should not be set when using pki workers wConf, err := config.DevWorker() @@ -48,9 +51,10 @@ func TestRotationTicking(t *testing.T) { wConf.Worker.Name = "" wConf.Worker.InitialUpstreams = c.ClusterAddrs() w := worker.NewTestWorker(t, &worker.TestWorkerOpts{ - InitialUpstreams: c.ClusterAddrs(), - Logger: logger.Named("worker"), - Config: wConf, + InitialUpstreams: c.ClusterAddrs(), + Logger: logger.Named("worker"), + Config: wConf, + WorkerAuthDebuggingEnabled: workerAuthDebugEnabled, }) t.Cleanup(w.Shutdown) @@ -107,8 +111,7 @@ func TestRotationTicking(t *testing.T) { // Now we wait and check that we see new values in the DB and different // creds on the worker after each rotation period for i := 2; i < 5; i++ { - t.Log("iteration", i) - nextRotation := worker.AuthRotationNextRotation.Load() + nextRotation := w.Worker().AuthRotationNextRotation.Load() time.Sleep((*nextRotation).Sub(time.Now()) + 5*time.Second) // Verify we see 2- after credentials have rotated, we should see current and previous diff --git a/internal/daemon/worker/testing.go b/internal/daemon/worker/testing.go index 2ef891a95..cc4fe4814 100644 --- a/internal/daemon/worker/testing.go +++ b/internal/daemon/worker/testing.go @@ -251,6 +251,7 @@ func NewTestWorker(t testing.TB, opts *TestWorkerOpts) *TestWorker { shutdownDoneCh: make(chan struct{}), shutdownOnce: new(sync.Once), } + t.Cleanup(tw.Shutdown) if opts == nil { opts = new(TestWorkerOpts) diff --git a/internal/daemon/worker/worker.go b/internal/daemon/worker/worker.go index 0f7804b67..791349155 100644 --- a/internal/daemon/worker/worker.go +++ b/internal/daemon/worker/worker.go @@ -139,6 +139,10 @@ type Worker struct { successfulStatusGracePeriod *atomic.Int64 statusCallTimeoutDuration *atomic.Int64 + // AuthRotationNextRotation is useful in tests to understand how long to + // sleep + AuthRotationNextRotation atomic.Pointer[time.Time] + // Test-specific options (and possibly hidden dev-mode flags) TestOverrideX509VerifyDnsName string TestOverrideX509VerifyCertPool *x509.CertPool