Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NewHeadlessAuthenticationWatcher prevents Auth initialization when the backend is unavailable #27059

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented May 27, 2023

Only wait for headless authentication watcher initialization in tests.

Closes #26444

@Joerger Joerger requested a review from rosstimothy May 27, 2023 02:39
@github-actions github-actions bot requested review from AntonAM and zmb3 May 27, 2023 02:39
Copy link
Contributor

@rosstimothy rosstimothy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause inconsistent behavior outside of tests now? Is it possible to attempt a headless login just after Auth has been started but prior to the watcher being initialized?

@Joerger
Copy link
Contributor Author

Joerger commented May 30, 2023

Will this cause inconsistent behavior outside of tests now?

This won't really be a problem. The Headless Authentication Watcher system is immune to race conditions due to the stale check logic. The problem is that the tests are trying to test specific cases (ex: watcher catches initial backend put, not stale check), so we need a way to prevent such race conditions within the tests themselves. Let me know if you think there is a better way to go about this.

Is it possible to attempt a headless login just after Auth has been started but prior to the watcher being initialized?

To be more specific, yes, this is possible. But the watcher will successfully allocate a subscriber, and do an initial backend check. Once the watcher is fully initialized, it will notify any existing subscribers:

// Notify any subscribers initiated before the new watcher initialized.
headlessAuthns, err := h.identityService.GetHeadlessAuthentications(ctx)
if err != nil {
return trace.Wrap(err)
}
h.notify(headlessAuthns...)

@Joerger Joerger requested a review from rosstimothy May 30, 2023 19:30
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from zmb3 June 2, 2023 16:55
@Joerger Joerger added this pull request to the merge queue Jun 2, 2023
Merged via the queue into master with commit b5c31b7 Jun 2, 2023
22 checks passed
@Joerger Joerger deleted the joerger/fix-headless-watcher-initialization branch June 2, 2023 18:16
@public-teleport-github-review-bot

@Joerger See the table below for backport results.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NewHeadlessAuthenticationWatcher prevents Auth initialization when the backend is unavailable
3 participants