Skip to content

Fixes panic on group database errors during host user reconciliation#53031

Merged
eriktate merged 1 commit intomasterfrom
eriktate/53020/fix-group-lookup
Mar 14, 2025
Merged

Fixes panic on group database errors during host user reconciliation#53031
eriktate merged 1 commit intomasterfrom
eriktate/53020/fix-group-lookup

Conversation

@eriktate
Copy link
Copy Markdown
Contributor

@eriktate eriktate commented Mar 13, 2025

Fixes #53020

This fixes an issue where error returns from user.LookupGroupByID would cause host user reconciliation to panic and crash the teleport process. A missing continue meant that we would attempt to mark a group as found by accessing the ID field on a nil pointer.

changelog: Fixed an issue causing the teleport process to crash on group database errors when host user creation was enabled

Comment thread lib/srv/usermgmt_test.go Outdated
Comment on lines +1135 to +1137
require.NotPanics(t, func() {
closer, err = users.UpsertUser("alice", userinfo)
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is how we should be testing this. You can make the argument that none of our code should panic (unless explicitly coded to do so), in which case we would have every test include a requires.NotPanics() wrapped around everything.

In this case, there was a panic due to mistaken error handling. Which says to me we need to test that the error handling is now correct, which means UpsertUser() returns an error when the group lookup returns an error. It is sufficient to just have the next line (require.Error(t, err)) and to remove the NotPanic assertion, as if it does panic, the test will still fail as panics in the code under test will always fail a test unless specifically caught and ignored.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Spot on, Cam.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great point. I included the require.NotPanic() just capture the specific regression this was guarding against, but you're right that it isn't actually testing anything valuable 👍 Updated!

Copy link
Copy Markdown
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

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

Other than that, LGTM

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ravicious March 14, 2025 08:54
@eriktate eriktate force-pushed the eriktate/53020/fix-group-lookup branch from 628f8a0 to 348f51d Compare March 14, 2025 15:53
@eriktate eriktate force-pushed the eriktate/53020/fix-group-lookup branch from 348f51d to fe8a065 Compare March 14, 2025 16:08
@eriktate eriktate enabled auto-merge March 14, 2025 16:21
@eriktate eriktate added this pull request to the merge queue Mar 14, 2025
Merged via the queue into master with commit 1fbbdd0 Mar 14, 2025
41 checks passed
@eriktate eriktate deleted the eriktate/53020/fix-group-lookup branch March 14, 2025 17:00
@public-teleport-github-review-bot
Copy link
Copy Markdown

@eriktate See the table below for backport results.

Branch Result
branch/v15 Failed
branch/v16 Failed
branch/v17 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.

Teleport Agent Panic on Failed Group Lookup with SSSD

4 participants