Skip to content

Use GetRole in the user login state generator, fail if not found.#36314

Merged
mdwn merged 1 commit intomasterfrom
mike.wilson/uls-uses-get-role-instead-of-get-roles
Jan 5, 2024
Merged

Use GetRole in the user login state generator, fail if not found.#36314
mdwn merged 1 commit intomasterfrom
mike.wilson/uls-uses-get-role-instead-of-get-roles

Conversation

@mdwn
Copy link
Copy Markdown
Contributor

@mdwn mdwn commented Jan 5, 2024 •
edited
Loading

The user login state generator will use GetRole instead of GetRoles to lookup roles. GetRole has a more robust fallback when unable to find things in the cache, so using it directly will make the user login state generator more robust in general.

Additionally, the user login state generator should not allow missing roles, so if a role is missing it will now throw an error.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 5, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@mdwn mdwn force-pushed the mike.wilson/uls-uses-get-role-instead-of-get-roles branch from 23a6bf8 to dce4667 Compare January 5, 2024 02:39
@mdwn mdwn changed the title Use GetRole in the user login state generator instead of GetRoles. Use GetRole in the user login state generator, fail if not found. Jan 5, 2024
@mdwn mdwn added the no-changelog Indicates that a PR does not require a changelog entry label Jan 5, 2024
@timothyb89
Copy link
Copy Markdown
Contributor

timothyb89 commented Jan 5, 2024 •
edited
Loading

Some further discussion in slack, since I noticed this broke tbot: https://gravitational.slack.com/archives/C0DF0TPMY/p1704419253406929

This PR does fix the issue for me, though I do wonder if we're doing something incorrect while creating bot users/roles that leads to them not getting inserted properly into the cache.

It's also interesting that this doesn't affect master as badly - maybe because of @strideynet's refactoring work in #35441 changing how we interact with auth to insert the bot user/role? It does print the "Role bot-foo does not exist" message but issues certs fine at join time.

Use GetRole in the user login state generator, fail if not found.
f1cc08e 
The user login state generator will use GetRole instead of GetRoles to lookup
roles. GetRole has a more robust fallback when unable to find things in the
cache, so using it directly will make the user login state generator more
robust in general.

Additionally, the user login state generator should not allow missing roles,
so if a role is missing it will now throw an error.
@mdwn mdwn force-pushed the mike.wilson/uls-uses-get-role-instead-of-get-roles branch from dce4667 to f1cc08e Compare January 5, 2024 02:46
@mdwn mdwn enabled auto-merge January 5, 2024 02:46
@mdwn mdwn added this pull request to the merge queue Jan 5, 2024
Merged via the queue into master with commit 5c8950d Jan 5, 2024
@mdwn mdwn deleted the mike.wilson/uls-uses-get-role-instead-of-get-roles branch January 5, 2024 09:19
@public-teleport-github-review-bot
Copy link
Copy Markdown

public-teleport-github-review-bot Bot commented Jan 5, 2024

@mdwn See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR

This was referenced Jan 5, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timothyb89 timothyb89 timothyb89 approved these changes

@strideynet strideynet strideynet approved these changes

@fspmarshall fspmarshall fspmarshall approved these changes

Assignees

No one assigned

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mdwn @timothyb89 @strideynet @fspmarshall