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

Skip unneeded resolve role #22597

Merged
merged 7 commits into from
Aug 30, 2023
Merged

Skip unneeded resolve role #22597

merged 7 commits into from
Aug 30, 2023

Conversation

mpalmi
Copy link
Contributor

@mpalmi mpalmi commented Aug 28, 2023

This PR skips ResolveRoleOperation calls for requests if the namespace/mount doesn't have role-based quotas.

We later perform the operation on-demand for lease creation if it has not already been done.

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 28, 2023
@github-actions
Copy link

github-actions bot commented Aug 28, 2023

CI Results:
All Go tests succeeded! ✅

vault/core.go Outdated Show resolved Hide resolved
@mpalmi mpalmi force-pushed the skip-unneeded-resolve-role branch 2 times, most recently from 771d162 to 0f2ab8e Compare August 30, 2023 01:38
@mpalmi mpalmi marked this pull request as ready for review August 30, 2023 01:45
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

// ns would have been made non-empty during insertion. Use non-empty
// value during query as well.
if req.NamespacePath == "" {
req.NamespacePath = "root"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere upstream? We are changing the request downstream and I am wondering what effect could it have for the upstream caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought. Note that this is specifically for the quotas.Request, so I don't think it will have any outside impacts (this is just used internally to query memdb). This is consistent with the way its done for other queries if you look at queryQuota.

// If this is not a role-based quota, we still need to associate the
// login role with this lease for later lease-count quotas to be
// accurate.
if reqRole == nil && resp.Auth.TokenType != logical.TokenTypeBatch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this how we determine if a request generates a lease? just make sure it's not a batch token?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, for login requests. In general it's more complicated.

@mpalmi mpalmi merged commit c4a8b23 into main Aug 30, 2023
99 of 100 checks passed
@mpalmi mpalmi deleted the skip-unneeded-resolve-role branch August 30, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants