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.

Sign up for GitHub

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

Jump to bottom

Add config value that gives users options to skip calculating role for each lease #22651

Merged
merged 24 commits into from
Sep 1, 2023
Merged

Add config value that gives users options to skip calculating role for each lease #22651

merged 24 commits into from
Sep 1, 2023

Conversation

elliesterner
Copy link
Contributor

Skipping calculation of role significantly reduces latency for cloud provider logins.

@elliesterner elliesterner requested a review from mpalmi August 30, 2023 17:10
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 30, 2023
@elliesterner elliesterner added this to the 1.14.3 milestone Aug 30, 2023
ncabatoff
ncabatoff reviewed Aug 30, 2023
View reviewed changes
vault/core.go Outdated Show resolved Hide resolved
ncabatoff
ncabatoff reviewed Aug 30, 2023
View reviewed changes
command/server/config.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 30, 2023 •
edited
Loading

CI Results:
All Go tests succeeded! ✅

@elliesterner elliesterner marked this pull request as ready for review August 30, 2023 20:40
VioletHynes
VioletHynes reviewed Aug 30, 2023
View reviewed changes
vault/core.go Outdated Show resolved Hide resolved
VioletHynes
VioletHynes reviewed Aug 30, 2023
View reviewed changes
vault/core.go Outdated Show resolved Hide resolved
VioletHynes
VioletHynes reviewed Aug 30, 2023
View reviewed changes
vault/core.go Outdated Show resolved Hide resolved
VioletHynes
VioletHynes reviewed Aug 30, 2023
View reviewed changes
command/server/config.go
@@ -112,6 +112,8 @@ type Config struct {

DetectDeadlocks string `hcl:"detect_deadlocks"`

ImpreciseLeaseRoleTracking bool `hcl:"imprecise_lease_role_tracking"`
Copy link
Contributor

@VioletHynes VioletHynes Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Though it's more verbose, how about imprecise_lease_count_quota_role_tracking?

I worry that "lease role" won't mean a lot to customers, and changing the name in the future if it's confusing will be difficult. I also want to be very explicit that it's lease count quota specific, and doesn't affect lease behaviour otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with this change. Although, I think documentation would help here the most. A lot of vault config names/env vars names are confusing to me until I read the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow I keep going back and forth on this one. The only concern I have is that imprecise_lease_count_quota_role_tracking is a bit of a mouthful. @ncabatoff @mpalmi do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to go this route, perhaps role_tracking is too much of an implementation detail and we could get away with skip_pre_quota_lease_tracking, or skip_old_lease_counts_on_quota_creation?

I'm honestly cool with any name we land on. This is a tricky one to pin down and I tend to agree that the documentation is the important part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm going to stick with what we have. we have pretty good documentation on the option now which makes me feel better about it.

VioletHynes
VioletHynes reviewed Aug 30, 2023
View reviewed changes
changelog/22651.txt Outdated Show resolved Hide resolved
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@elliesterner elliesterner marked this pull request as draft August 30, 2023 21:09
schavis
schavis reviewed Aug 31, 2023
View reviewed changes
Copy link
Contributor

@schavis schavis left a comment

Choose a reason for hiding this comment

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

The suggested text change LGTM with one small change.

website/content/docs/configuration/index.mdx Outdated Show resolved Hide resolved
@elliesterner @mpalmi
Update http/util.go
5537c96 
Co-authored-by: Mike Palmiotto <[email protected]>
mpalmi
mpalmi reviewed Aug 31, 2023
View reviewed changes
vault/request_handling.go Outdated Show resolved Hide resolved
elliesterner
elliesterner commented Aug 31, 2023
View reviewed changes
vault/core.go Outdated Show resolved Hide resolved
elliesterner
elliesterner commented Aug 31, 2023
View reviewed changes
vault/core.go Outdated Show resolved Hide resolved
mpalmi
mpalmi reviewed Aug 31, 2023
View reviewed changes
vault/request_handling.go Outdated Show resolved Hide resolved
@mpalmi mpalmi self-requested a review August 31, 2023 22:26
@elliesterner elliesterner merged commit bd36e66 into main Sep 1, 2023
@elliesterner elliesterner deleted the add-disable-lease-role-tracking branch September 1, 2023 12:01
This was referenced Sep 1, 2023
Closed
Merged
Merged
elliesterner added a commit that referenced this pull request Sep 1, 2023
@elliesterner @VioletHynes @mpalmi
Add config value that gives users options to skip calculating role fo…
7ee30e6 
…r each lease (#22651)

* Add config value that gives users options to skip calculating role for each lease

* add changelog

* change name

* add config for testing

* Update changelog/22651.txt

Co-authored-by: Violet Hynes <[email protected]>

* update tests, docs and reorder logic in conditional

* fix comment

* update comment

* fix comment again

* Update comments and change if order

* change comment again

* add other comment

* fix tests

* add documentation

* edit docs

* Update http/util.go

Co-authored-by: Mike Palmiotto <[email protected]>

* Update vault/core.go

* Update vault/core.go

* update var name

* udpate docs

* Update vault/request_handling.go

Co-authored-by: Mike Palmiotto <[email protected]>

* 1 more docs change

---------

Co-authored-by: Violet Hynes <[email protected]>
Co-authored-by: Mike Palmiotto <[email protected]>
elliesterner added a commit that referenced this pull request Sep 1, 2023
@elliesterner @VioletHynes @mpalmi
Add config value that gives users options to skip calculating role fo…
b820e80 
…r each lease (#22651)

* Add config value that gives users options to skip calculating role for each lease

* add changelog

* change name

* add config for testing

* Update changelog/22651.txt

Co-authored-by: Violet Hynes <[email protected]>

* update tests, docs and reorder logic in conditional

* fix comment

* update comment

* fix comment again

* Update comments and change if order

* change comment again

* add other comment

* fix tests

* add documentation

* edit docs

* Update http/util.go

Co-authored-by: Mike Palmiotto <[email protected]>

* Update vault/core.go

* Update vault/core.go

* update var name

* udpate docs

* Update vault/request_handling.go

Co-authored-by: Mike Palmiotto <[email protected]>

* 1 more docs change

---------

Co-authored-by: Violet Hynes <[email protected]>
Co-authored-by: Mike Palmiotto <[email protected]>
elliesterner added a commit that referenced this pull request Sep 1, 2023
@hc-github-team-secure-vault-core @elliesterner
@VioletHynes @mpalmi
Backport of Add config value that gives users options to skip calcula…
9042812 
…ting role for each lease into release/1.13.x (#22729)

* Add config value that gives users options to skip calculating role for each lease (#22651)

* Add config value that gives users options to skip calculating role for each lease

* add changelog

* change name

* add config for testing

* Update changelog/22651.txt

Co-authored-by: Violet Hynes <[email protected]>

* update tests, docs and reorder logic in conditional

* fix comment

* update comment

* fix comment again

* Update comments and change if order

* change comment again

* add other comment

* fix tests

* add documentation

* edit docs

* Update http/util.go

Co-authored-by: Mike Palmiotto <[email protected]>

* Update vault/core.go

* Update vault/core.go

* update var name

* udpate docs

* Update vault/request_handling.go

Co-authored-by: Mike Palmiotto <[email protected]>

* 1 more docs change

---------

Co-authored-by: Violet Hynes <[email protected]>
Co-authored-by: Mike Palmiotto <[email protected]>

* remove wrong part of cherry-pick

---------

Co-authored-by: Ellie <[email protected]>
Co-authored-by: Violet Hynes <[email protected]>
Co-authored-by: Mike Palmiotto <[email protected]>
elliesterner added a commit that referenced this pull request Sep 1, 2023
@hc-github-team-secure-vault-core @elliesterner
@VioletHynes @mpalmi
Add config value that gives users options to skip calculating role fo…
429c693 
…r each lease (#22651) (#22730)

* Add config value that gives users options to skip calculating role for each lease

* add changelog

* change name

* add config for testing

* Update changelog/22651.txt



* update tests, docs and reorder logic in conditional

* fix comment

* update comment

* fix comment again

* Update comments and change if order

* change comment again

* add other comment

* fix tests

* add documentation

* edit docs

* Update http/util.go



* Update vault/core.go

* Update vault/core.go

* update var name

* udpate docs

* Update vault/request_handling.go



* 1 more docs change

---------

Co-authored-by: Ellie <[email protected]>
Co-authored-by: Violet Hynes <[email protected]>
Co-authored-by: Mike Palmiotto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpalmi mpalmi mpalmi approved these changes

@VioletHynes VioletHynes VioletHynes approved these changes

@schavis schavis Awaiting requested review from schavis

@ncabatoff ncabatoff Awaiting requested review from ncabatoff

Assignees
No one assigned
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Milestone
1.14.3
Development

Successfully merging this pull request may close these issues.
5 participants
@elliesterner @VioletHynes @ncabatoff @mpalmi @schavis