Hold Auth init lock for the duration of initialization #29593

rosstimothy · 2023-07-25T18:08:52Z

Auth now uses RunWhileLocked instead of AcquireLock to ensure that the initialization lock is held until the bootstrapping process is completed. Prior, Auth only held the lock for 30s which could allow multiple Auth instances to attempt bootstrapping simultaneously. Initialization should complete prior to 30s in most cases, but it is not guaranteed, especially on first boot when CAs are being generated and a large data migration may be needed.

Auth now uses `RunWhileLocked` instead of `AcquireLock` to ensure that the initialization lock is held until the bootstrapping process is completed. Prior, Auth only held the lock for 30s which could allow multiple Auths to attempt bootstrapping simultaneously. Initialization should complete prior to 30s in most cases, but it is not guarateed, especially on first boot when CAs are being generated and a large data migration may be needed.

zmb3 · 2023-07-25T19:13:55Z

lib/auth/init.go

+				Backend:  cfg.Backend,
+				LockName: domainName,
+				TTL:      30 * time.Second,
+			},


Does RunWhileLocked release the lock as soon as the operation is completed, or only after the TTL elapses?

If the former, what's the harm in making this (for example) 60s?

Yes the lock is released before returning from RunWhileLocked:

teleport/lib/backend/helpers.go

Lines 223 to 229 in 7889f56

// lock.Release should be called with separate ctx. If someone cancels via ctx

// RunWhileLocked method, we want to at least try releasing lock.

releaseLockCtx, releaseLockCancel := context.WithTimeout(context.Background(), cfg.ReleaseCtxTimeout)

defer releaseLockCancel()

if err := lock.Release(releaseLockCtx, cfg.Backend); err != nil {

return trace.NewAggregate(fnErr, err)

}

In the event that the Auth instance holding the lock is terminated during initialization a longer TTL would block any other instances from initializing until they could acquire the lock. The longer TTL would result in less backend activity caused by lengthy migrations though.

zmb3 · 2023-07-25T19:14:43Z

lib/auth/init.go

@@ -320,7 +333,7 @@ func Init(ctx context.Context, cfg InitConfig, opts ...ServerOption) (*Server, e
 	// singletons). However, we need to keep them around while Telekube uses them.


What is Telekube?

🤷 - I have intentionally left the legacy/odd things in Init alone while focusing on improving start up time and reliability of initialization. I was planning to circle back to cfg.Roles, cfg.Authorities, cfg.ReverseTunnels and migrateLegacyResources to determine if they can be eliminated.

I think that was the name of teleport when it was running as a component of Gravity. Some deep cuts.

rosstimothy · 2023-07-27T15:16:59Z

Friendly ping @ravicious @Tener

public-teleport-github-review-bot · 2023-07-27T18:06:31Z

@rosstimothy See the table below for backport results.

Branch	Result
branch/v11	Failed
branch/v12	Failed
branch/v13	Create PR

Auth now uses `RunWhileLocked` instead of `AcquireLock` to ensure that the initialization lock is held until the bootstrapping process is completed. Prior, Auth only held the lock for 30s which could allow multiple Auths to attempt bootstrapping simultaneously. Initialization should complete prior to 30s in most cases, but it is not guarateed, especially on first boot when CAs are being generated and a large data migration may be needed.

* Add configuration with optional timeout to AcquireLock (#24559) * Add configuration with optional timeout to AcquireLock * rename to RetryInterval * backport RunWhileLocked changes from #25639 * Hold Auth init lock for the duration of initialization (#29593) Auth now uses `RunWhileLocked` instead of `AcquireLock` to ensure that the initialization lock is held until the bootstrapping process is completed. Prior, Auth only held the lock for 30s which could allow multiple Auths to attempt bootstrapping simultaneously. Initialization should complete prior to 30s in most cases, but it is not guarateed, especially on first boot when CAs are being generated and a large data migration may be needed. --------- Co-authored-by: Tobiasz Heller <14020794+tobiaszheller@users.noreply.github.com>

rosstimothy marked this pull request as ready for review July 25, 2023 19:04

github-actions bot requested review from ravicious and Tener July 25, 2023 19:04

github-actions bot added the size/sm label Jul 25, 2023

zmb3 reviewed Jul 25, 2023

View reviewed changes

zmb3 approved these changes Jul 25, 2023

View reviewed changes

fspmarshall approved these changes Jul 27, 2023

View reviewed changes

public-teleport-github-review-bot bot removed request for ravicious and Tener July 27, 2023 17:39

rosstimothy added backport/branch/v11 backport/branch/v13 labels Jul 27, 2023

rosstimothy added this pull request to the merge queue Jul 27, 2023

Merged via the queue into master with commit 6eb0551 Jul 27, 2023
27 checks passed

rosstimothy deleted the tross/auth_init_lock branch July 27, 2023 18:04

rosstimothy mentioned this pull request Jul 27, 2023

[v13] Hold Auth init lock for the duration of initialization #29706

Merged

rosstimothy mentioned this pull request Jul 27, 2023

[v12] Hold Auth init lock for the duration of initialization #29709

Merged

rosstimothy mentioned this pull request Jul 27, 2023

[v11] Hold Auth init lock for the duration of initialization #29710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hold Auth init lock for the duration of initialization #29593

Hold Auth init lock for the duration of initialization #29593

rosstimothy commented Jul 25, 2023

zmb3 Jul 25, 2023

rosstimothy Jul 25, 2023

zmb3 Jul 25, 2023

rosstimothy Jul 25, 2023

fspmarshall Jul 27, 2023

rosstimothy commented Jul 27, 2023

public-teleport-github-review-bot bot commented Jul 27, 2023

	// lock.Release should be called with separate ctx. If someone cancels via ctx
	// RunWhileLocked method, we want to at least try releasing lock.
	releaseLockCtx, releaseLockCancel := context.WithTimeout(context.Background(), cfg.ReleaseCtxTimeout)
	defer releaseLockCancel()
	if err := lock.Release(releaseLockCtx, cfg.Backend); err != nil {
	return trace.NewAggregate(fnErr, err)
	}

		@@ -320,7 +333,7 @@ func Init(ctx context.Context, cfg InitConfig, opts ...ServerOption) (*Server, e
		// singletons). However, we need to keep them around while Telekube uses them.

Hold Auth init lock for the duration of initialization #29593

Hold Auth init lock for the duration of initialization #29593

Conversation

rosstimothy commented Jul 25, 2023

zmb3 Jul 25, 2023

Choose a reason for hiding this comment

rosstimothy Jul 25, 2023

Choose a reason for hiding this comment

zmb3 Jul 25, 2023

Choose a reason for hiding this comment

rosstimothy Jul 25, 2023

Choose a reason for hiding this comment

fspmarshall Jul 27, 2023

Choose a reason for hiding this comment

rosstimothy commented Jul 27, 2023

public-teleport-github-review-bot bot commented Jul 27, 2023