Skip to content

Commit

Permalink
Prep for upcoming backend.Key type change (#46675)
Browse files Browse the repository at this point in the history
Adds a (Key) IsZero method to determine if a key is populated. Some
Backend implementations today validate keys before operations via
a `len(key) == 0` check, however, that will no longer work once the
key is migrated.

Starts migrating the backend.LockConfiguration away from prepopulating
the lock name to passing in a list of components. There were a few
locks constructing a portion of the name manually, which will not
work when the key type is changed.
  • Loading branch information
rosstimothy committed Oct 11, 2024
1 parent f131e47 commit 149159c
Show file tree
Hide file tree
Showing 13 changed files with 83 additions and 60 deletions.
6 changes: 3 additions & 3 deletions lib/auth/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,9 +343,9 @@ func Init(ctx context.Context, cfg InitConfig, opts ...ServerOption) (*Server, e
if err := backend.RunWhileLocked(ctx,
backend.RunWhileLockedConfig{
LockConfiguration: backend.LockConfiguration{
Backend: cfg.Backend,
LockName: domainName,
TTL: 30 * time.Second,
Backend: cfg.Backend,
LockNameComponents: []string{domainName},
TTL: 30 * time.Second,
},
RefreshLockInterval: 20 * time.Second,
}, func(ctx context.Context) error {
Expand Down
6 changes: 3 additions & 3 deletions lib/auth/machineid/machineidv1/spiffe_federation_syncer.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,9 @@ func (s *SPIFFEFederationSyncer) Run(ctx context.Context) error {
for {
err := backend.RunWhileLocked(ctx, backend.RunWhileLockedConfig{
LockConfiguration: backend.LockConfiguration{
Backend: s.cfg.Backend,
LockName: "spiffe_federation_syncer",
TTL: time.Minute,
Backend: s.cfg.Backend,
LockNameComponents: []string{"spiffe_federation_syncer"},
TTL: time.Minute,
// It doesn't matter too much if the syncer isn't running for
// a short period of time so we can take a relaxed approach to
// retrying to grab the lock.
Expand Down
19 changes: 15 additions & 4 deletions lib/backend/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,14 @@ func randomID() ([]byte, error) {
}

type LockConfiguration struct {
Backend Backend
// Backend to create the lock in.
Backend Backend
// LockName the precomputed lock name.
// TODO(tross) DELETE WHEN teleport.e is updated to use LockNameComponents.
LockName string
// LockNameComponents are subcomponents to be used when constructing
// the lock name.
LockNameComponents []string
// TTL defines when lock will be released automatically
TTL time.Duration
// RetryInterval defines interval which is used to retry locking after
Expand All @@ -65,9 +71,14 @@ func (l *LockConfiguration) CheckAndSetDefaults() error {
if l.Backend == nil {
return trace.BadParameter("missing Backend")
}
if l.LockName == "" {
return trace.BadParameter("missing LockName")
if l.LockName == "" && len(l.LockNameComponents) == 0 {
return trace.BadParameter("missing LockName/LockNameComponents")
}

if len(l.LockNameComponents) == 0 {
l.LockNameComponents = []string{l.LockName}
}

if l.TTL == 0 {
return trace.BadParameter("missing TTL")
}
Expand All @@ -83,7 +94,7 @@ func AcquireLock(ctx context.Context, cfg LockConfiguration) (Lock, error) {
if err != nil {
return Lock{}, trace.Wrap(err)
}
key := LockKey(cfg.LockName)
key := LockKey(cfg.LockNameComponents...)
id, err := randomID()
if err != nil {
return Lock{}, trace.Wrap(err)
Expand Down
51 changes: 26 additions & 25 deletions lib/backend/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,30 +38,30 @@ func TestLockConfiguration_CheckAndSetDefaults(t *testing.T) {
{
name: "minimum valid",
in: LockConfiguration{
Backend: mockBackend{},
LockName: "lock",
TTL: 30 * time.Second,
Backend: mockBackend{},
LockNameComponents: []string{"lock"},
TTL: 30 * time.Second,
},
want: LockConfiguration{
Backend: mockBackend{},
LockName: "lock",
TTL: 30 * time.Second,
RetryInterval: 250 * time.Millisecond,
Backend: mockBackend{},
LockNameComponents: []string{"lock"},
TTL: 30 * time.Second,
RetryInterval: 250 * time.Millisecond,
},
},
{
name: "set RetryAcquireLockTimeout",
in: LockConfiguration{
Backend: mockBackend{},
LockName: "lock",
TTL: 30 * time.Second,
RetryInterval: 10 * time.Second,
Backend: mockBackend{},
LockNameComponents: []string{"lock"},
TTL: 30 * time.Second,
RetryInterval: 10 * time.Second,
},
want: LockConfiguration{
Backend: mockBackend{},
LockName: "lock",
TTL: 30 * time.Second,
RetryInterval: 10 * time.Second,
Backend: mockBackend{},
LockNameComponents: []string{"lock"},
TTL: 30 * time.Second,
RetryInterval: 10 * time.Second,
},
},
{
Expand All @@ -82,9 +82,9 @@ func TestLockConfiguration_CheckAndSetDefaults(t *testing.T) {
{
name: "missing TTL",
in: LockConfiguration{
Backend: mockBackend{},
LockName: "lock",
TTL: 0,
Backend: mockBackend{},
LockNameComponents: []string{"lock"},
TTL: 0,
},
wantErr: "missing TTL",
},
Expand All @@ -111,9 +111,9 @@ func TestRunWhileLockedConfigCheckAndSetDefaults(t *testing.T) {
ttl := 1 * time.Minute
minimumValidConfig := RunWhileLockedConfig{
LockConfiguration: LockConfiguration{
Backend: mockBackend{},
LockName: lockName,
TTL: ttl,
Backend: mockBackend{},
LockNameComponents: []string{lockName},
TTL: ttl,
},
}
tests := []struct {
Expand All @@ -129,10 +129,10 @@ func TestRunWhileLockedConfigCheckAndSetDefaults(t *testing.T) {
},
want: RunWhileLockedConfig{
LockConfiguration: LockConfiguration{
Backend: mockBackend{},
LockName: lockName,
TTL: ttl,
RetryInterval: 250 * time.Millisecond,
Backend: mockBackend{},
LockNameComponents: []string{lockName},
TTL: ttl,
RetryInterval: 250 * time.Millisecond,
},
ReleaseCtxTimeout: time.Second,
// defaults to halft of TTL.
Expand All @@ -144,6 +144,7 @@ func TestRunWhileLockedConfigCheckAndSetDefaults(t *testing.T) {
input: func() RunWhileLockedConfig {
cfg := minimumValidConfig
cfg.LockName = ""
cfg.LockNameComponents = nil
return cfg
},
wantErr: "missing LockName",
Expand Down
5 changes: 5 additions & 0 deletions lib/backend/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ func (k Key) String() string {
return string(k)
}

// IsZero reports whether k represents the zero key.
func (k Key) IsZero() bool {
return len(k) == 0
}

// HasPrefix reports whether the key begins with prefix.
func (k Key) HasPrefix(prefix Key) bool {
return bytes.HasPrefix(k, prefix)
Expand Down
7 changes: 7 additions & 0 deletions lib/backend/key_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,3 +473,10 @@ func TestKeyCompare(t *testing.T) {
})
}
}

func TestKeyIsZero(t *testing.T) {
assert.True(t, backend.Key{}.IsZero())
assert.True(t, backend.NewKey().IsZero())
assert.False(t, backend.NewKey("a", "b").IsZero())
assert.False(t, backend.ExactKey("a", "b").IsZero())
}
14 changes: 7 additions & 7 deletions lib/backend/test/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ func testLocking(t *testing.T, newBackend Constructor) {
defer requireNoAsyncErrors()

// Given a lock named `tok1` on the backend...
lock, err := backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok1, TTL: ttl})
lock, err := backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok1}, TTL: ttl})
require.NoError(t, err)

// When I asynchronously release the lock...
Expand All @@ -853,7 +853,7 @@ func testLocking(t *testing.T, newBackend Constructor) {
}()

// ...and simultaneously attempt to create a new lock with the same name
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok1, TTL: ttl})
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok1}, TTL: ttl})

// expect that the asynchronous Release() has executed - we're using the
// change in the value of the marker value as a proxy for the Release().
Expand All @@ -865,7 +865,7 @@ func testLocking(t *testing.T, newBackend Constructor) {
require.NoError(t, lock.Release(ctx, uut))

// Given a lock with the same name as previously-existing, manually-released lock
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok1, TTL: ttl})
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok1}, TTL: ttl})
require.NoError(t, err)
atomic.StoreInt32(&marker, 7)

Expand All @@ -880,7 +880,7 @@ func testLocking(t *testing.T, newBackend Constructor) {
}()

// ...and simultaneously try to acquire another lock with the same name
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok1, TTL: ttl})
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok1}, TTL: ttl})

// expect that the asynchronous Release() has executed - we're using the
// change in the value of the marker value as a proxy for the call to
Expand All @@ -894,9 +894,9 @@ func testLocking(t *testing.T, newBackend Constructor) {

// Given a pair of locks named `tok1` and `tok2`
y := int32(0)
lock1, err := backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok1, TTL: ttl})
lock1, err := backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok1}, TTL: ttl})
require.NoError(t, err)
lock2, err := backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok2, TTL: ttl})
lock2, err := backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok2}, TTL: ttl})
require.NoError(t, err)

// When I asynchronously release the locks...
Expand All @@ -913,7 +913,7 @@ func testLocking(t *testing.T, newBackend Constructor) {
}
}()

lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockName: tok1, TTL: ttl})
lock, err = backend.AcquireLock(ctx, backend.LockConfiguration{Backend: uut, LockNameComponents: []string{tok1}, TTL: ttl})
require.NoError(t, err)
require.Equal(t, int32(15), atomic.LoadInt32(&y))
require.NoError(t, lock.Release(ctx, uut))
Expand Down
4 changes: 2 additions & 2 deletions lib/events/athena/consumer.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,8 @@ func (c *consumer) runContinuouslyOnSingleAuth(ctx context.Context, eventsProces
default:
err := backend.RunWhileLocked(ctx, backend.RunWhileLockedConfig{
LockConfiguration: backend.LockConfiguration{
Backend: c.backend,
LockName: "athena_lock",
Backend: c.backend,
LockNameComponents: []string{"athena_lock"},
// TTL is higher then batchMaxInterval because we want to optimize
// for low backend writes.
TTL: 5 * c.batchMaxInterval,
Expand Down
6 changes: 3 additions & 3 deletions lib/services/local/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,9 +364,9 @@ func (s *AccessService) DeleteAllLocks(ctx context.Context) error {
func (s *AccessService) ReplaceRemoteLocks(ctx context.Context, clusterName string, newRemoteLocks []types.Lock) error {
return backend.RunWhileLocked(ctx, backend.RunWhileLockedConfig{
LockConfiguration: backend.LockConfiguration{
Backend: s.Backend,
LockName: "ReplaceRemoteLocks/" + clusterName,
TTL: time.Minute,
Backend: s.Backend,
LockNameComponents: []string{"ReplaceRemoteLocks", clusterName},
TTL: time.Minute,
},
}, func(ctx context.Context) error {
remoteLocksKey := backend.ExactKey(locksPrefix, clusterName)
Expand Down
9 changes: 4 additions & 5 deletions lib/services/local/access_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ package local

import (
"context"
"strings"
"time"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -197,7 +196,7 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
action := updateAccessList
if !modules.GetModules().Features().IGSEnabled() {
action = func() error {
err := a.service.RunWhileLocked(ctx, createAccessListLimitLockName, accessListLockTTL,
err := a.service.RunWhileLocked(ctx, []string{createAccessListLimitLockName}, accessListLockTTL,
func(ctx context.Context, _ backend.Backend) error {
if err := a.VerifyAccessListCreateLimit(ctx, accessList.GetName()); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -465,7 +464,7 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc
action := reconcileMembers
if !modules.GetModules().Features().IGSEnabled() {
action = func() error {
return a.service.RunWhileLocked(ctx, createAccessListLimitLockName, 2*accessListLockTTL,
return a.service.RunWhileLocked(ctx, []string{createAccessListLimitLockName}, 2*accessListLockTTL,
func(ctx context.Context, _ backend.Backend) error {
if err := a.VerifyAccessListCreateLimit(ctx, accessList.GetName()); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -650,8 +649,8 @@ func (a *AccessListService) DeleteAllAccessListReviews(ctx context.Context) erro
return trace.Wrap(a.reviewService.DeleteAllResources(ctx))
}

func lockName(accessListName string) string {
return strings.Join([]string{"access_list", accessListName}, string(backend.Separator))
func lockName(accessListName string) []string {
return []string{"access_list", accessListName}
}

// VerifyAccessListCreateLimit ensures creating access list is limited to no more than 1 (updating is allowed).
Expand Down
10 changes: 5 additions & 5 deletions lib/services/local/generic/generic.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,14 +502,14 @@ func (s *Service[T]) MakeKey(name string) backend.Key {
}

// RunWhileLocked will run the given function in a backend lock. This is a wrapper around the backend.RunWhileLocked function.
func (s *Service[T]) RunWhileLocked(ctx context.Context, lockName string, ttl time.Duration, fn func(context.Context, backend.Backend) error) error {
func (s *Service[T]) RunWhileLocked(ctx context.Context, lockNameComponents []string, ttl time.Duration, fn func(context.Context, backend.Backend) error) error {
return trace.Wrap(backend.RunWhileLocked(ctx,
backend.RunWhileLockedConfig{
LockConfiguration: backend.LockConfiguration{
Backend: s.backend,
LockName: lockName,
TTL: ttl,
RetryInterval: s.runWhileLockedRetryInterval,
Backend: s.backend,
LockNameComponents: lockNameComponents,
TTL: ttl,
RetryInterval: s.runWhileLockedRetryInterval,
},
}, func(ctx context.Context) error {
return fn(ctx, s.backend)
Expand Down
2 changes: 1 addition & 1 deletion lib/services/local/generic/generic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ func TestGenericCRUD(t *testing.T) {
require.ErrorIs(t, err, trace.NotFound(`generic resource "doesnotexist" doesn't exist`))

// Test running while locked.
err = service.RunWhileLocked(ctx, "test-lock", time.Second*5, func(ctx context.Context, backend backend.Backend) error {
err = service.RunWhileLocked(ctx, []string{"test-lock"}, time.Second*5, func(ctx context.Context, backend backend.Backend) error {
item, err := backend.Get(ctx, service.MakeKey(r1.GetName()))
require.NoError(t, err)

Expand Down
4 changes: 2 additions & 2 deletions lib/services/local/saml_idp_service_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func (s *SAMLIdPServiceProviderService) CreateSAMLIdPServiceProvider(ctx context
return trace.Wrap(err)
}

return trace.Wrap(s.svc.RunWhileLocked(ctx, samlIDPServiceProviderModifyLock, samlIDPServiceProviderModifyLockTTL,
return trace.Wrap(s.svc.RunWhileLocked(ctx, []string{samlIDPServiceProviderModifyLock}, samlIDPServiceProviderModifyLockTTL,
func(ctx context.Context, backend backend.Backend) error {
if err := s.ensureEntityIDIsUnique(ctx, sp); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -181,7 +181,7 @@ func (s *SAMLIdPServiceProviderService) UpdateSAMLIdPServiceProvider(ctx context
return trace.Wrap(err)
}

return trace.Wrap(s.svc.RunWhileLocked(ctx, samlIDPServiceProviderModifyLock, samlIDPServiceProviderModifyLockTTL,
return trace.Wrap(s.svc.RunWhileLocked(ctx, []string{samlIDPServiceProviderModifyLock}, samlIDPServiceProviderModifyLockTTL,
func(ctx context.Context, backend backend.Backend) error {
if err := s.ensureEntityIDIsUnique(ctx, sp); err != nil {
return trace.Wrap(err)
Expand Down

0 comments on commit 149159c

Please sign in to comment.