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

Request Limiter reloadable config #25095

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Conversation

mpalmi
Copy link
Contributor

@mpalmi mpalmi commented Jan 26, 2024

This PR builds on top of #25093 to establish a reloadable break-glass config for disabling the request limiter.

Two follow-up PRs are needed:

  1. Docs
  2. Reload test

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jan 26, 2024
@mpalmi mpalmi requested review from a team and raskchanky January 26, 2024 17:42
@mpalmi mpalmi marked this pull request as ready for review January 26, 2024 17:54
@mpalmi mpalmi marked this pull request as draft January 26, 2024 18:00
Copy link

CI Results:
All Go tests succeeded! ✅

@mpalmi mpalmi added this to the 1.16.0-rc1 milestone Jan 26, 2024
Base automatically changed from request-limiter to main January 26, 2024 19:26
@mpalmi mpalmi force-pushed the request-limiter-reloadable-config branch from c97d244 to c46ca85 Compare January 26, 2024 19:28
@mpalmi mpalmi marked this pull request as ready for review January 26, 2024 19:29
@mpalmi mpalmi force-pushed the request-limiter-reloadable-config branch from ea6671b to 5c6b0f9 Compare January 26, 2024 19:37
Copy link
Collaborator

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

A few questions but nothing blocking

return ValidateUnusedFields(r.UnusedKeys, source)
}

func (r *RequestLimiter) GoString() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe for tests? Truthfully not sure, but other configs used this so I followed suit.

if result.RequestLimiter.Disable, err = parseutil.ParseBool(result.RequestLimiter.DisableRaw); err != nil {
return err
}
result.RequestLimiter.DisableRaw = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a common pattern, setting it to nil after parsing it? I haven't seen that before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for some reason we clear the raw entry after parsing. I'm not sure I understand it, but copied it from other implementations.

Copy link

Build Results:
All builds succeeded! ✅

@mpalmi mpalmi merged commit 5933768 into main Jan 26, 2024
110 checks passed
@mpalmi mpalmi deleted the request-limiter-reloadable-config branch January 26, 2024 20:01
Monkeychip pushed a commit that referenced this pull request Jan 29, 2024
This commit introduces a new reloadable stanza to the server config to allow disabling the Request Limiter.
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.

2 participants