From cb0784b87f28ea1847c8d0bfd080f1ff31855c88 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Fri, 1 Sep 2023 09:07:47 -0400 Subject: [PATCH] Add config value that gives users options to skip calculating role for 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 Co-authored-by: Violet Hynes Co-authored-by: Mike Palmiotto --- changelog/22651.txt | 3 +++ command/server.go | 1 + command/server/config.go | 9 +++++++++ command/server/config_test_helpers.go | 1 + http/sys_config_state_test.go | 1 + http/util.go | 2 ++ vault/core.go | 7 +++++++ vault/request_handling.go | 14 ++++++++++---- vault/testing.go | 2 ++ website/content/docs/configuration/index.mdx | 4 ++++ 10 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 changelog/22651.txt diff --git a/changelog/22651.txt b/changelog/22651.txt new file mode 100644 index 000000000..5ca281983 --- /dev/null +++ b/changelog/22651.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core/quotas: Add configuration to allow skipping of expensive role calculations +``` \ No newline at end of file diff --git a/command/server.go b/command/server.go index 6171c60e7..fc059263b 100644 --- a/command/server.go +++ b/command/server.go @@ -2769,6 +2769,7 @@ func createCoreConfig(c *ServerCommand, config *server.Config, backend physical. LogicalBackends: c.LogicalBackends, Logger: c.logger, DetectDeadlocks: config.DetectDeadlocks, + ImpreciseLeaseRoleTracking: config.ImpreciseLeaseRoleTracking, DisableSentinelTrace: config.DisableSentinelTrace, DisableCache: config.DisableCache, DisableMlock: config.DisableMlock, diff --git a/command/server/config.go b/command/server/config.go index 2d7943147..2075b6e10 100644 --- a/command/server/config.go +++ b/command/server/config.go @@ -112,6 +112,8 @@ type Config struct { DetectDeadlocks string `hcl:"detect_deadlocks"` + ImpreciseLeaseRoleTracking bool `hcl:"imprecise_lease_role_tracking"` + EnableResponseHeaderRaftNodeID bool `hcl:"-"` EnableResponseHeaderRaftNodeIDRaw interface{} `hcl:"enable_response_header_raft_node_id"` @@ -412,6 +414,11 @@ func (c *Config) Merge(c2 *Config) *Config { result.DetectDeadlocks = c2.DetectDeadlocks } + result.ImpreciseLeaseRoleTracking = c.ImpreciseLeaseRoleTracking + if c2.ImpreciseLeaseRoleTracking { + result.ImpreciseLeaseRoleTracking = c2.ImpreciseLeaseRoleTracking + } + result.EnableResponseHeaderRaftNodeID = c.EnableResponseHeaderRaftNodeID if c2.EnableResponseHeaderRaftNodeID { result.EnableResponseHeaderRaftNodeID = c2.EnableResponseHeaderRaftNodeID @@ -1136,6 +1143,8 @@ func (c *Config) Sanitized() map[string]interface{} { "experiments": c.Experiments, "detect_deadlocks": c.DetectDeadlocks, + + "imprecise_lease_role_tracking": c.ImpreciseLeaseRoleTracking, } for k, v := range sharedResult { result[k] = v diff --git a/command/server/config_test_helpers.go b/command/server/config_test_helpers.go index 2a44d7a5f..ce33c090e 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -842,6 +842,7 @@ func testConfig_Sanitized(t *testing.T) { "add_lease_metrics_namespace_labels": false, }, "administrative_namespace_path": "admin/", + "imprecise_lease_role_tracking": false, } addExpectedEntSanitizedConfig(expected, []string{"http"}) diff --git a/http/sys_config_state_test.go b/http/sys_config_state_test.go index f6967531e..5dfdf27f5 100644 --- a/http/sys_config_state_test.go +++ b/http/sys_config_state_test.go @@ -175,6 +175,7 @@ func TestSysConfigState_Sanitized(t *testing.T) { }, "storage": tc.expectedStorageOutput, "administrative_namespace_path": "", + "imprecise_lease_role_tracking": false, } if tc.expectedHAStorageOutput != nil { diff --git a/http/util.go b/http/util.go index 19bdbf836..0353b4f11 100644 --- a/http/util.go +++ b/http/util.go @@ -71,6 +71,8 @@ func rateLimitQuotaWrapping(handler http.Handler, core *vault.Core) http.Handler NamespacePath: ns.Path, ClientAddress: parseRemoteIPAddress(r), } + + // This checks if any role based quota is required (LCQ or RLQ). requiresResolveRole, err := core.ResolveRoleForQuotas(r.Context(), quotaReq) if err != nil { core.Logger().Error("failed to lookup quotas", "path", path, "error", err) diff --git a/vault/core.go b/vault/core.go index 01370b2c5..99a11bfab 100644 --- a/vault/core.go +++ b/vault/core.go @@ -693,6 +693,9 @@ type Core struct { // if populated, the callback is called for every request // for testing purposes requestResponseCallback func(logical.Backend, *logical.Request, *logical.Response) + + // If any role based quota (LCQ or RLQ) is enabled, don't track lease counts by role + impreciseLeaseRoleTracking bool } // c.stateLock needs to be held in read mode before calling this function. @@ -754,6 +757,9 @@ type CoreConfig struct { // Use the deadlocks library to detect deadlocks DetectDeadlocks string + // If any role based quota (LCQ or RLQ) is enabled, don't track lease counts by role + ImpreciseLeaseRoleTracking bool + // Disables the trace display for Sentinel checks DisableSentinelTrace bool @@ -1018,6 +1024,7 @@ func CreateCore(conf *CoreConfig) (*Core, error) { experiments: conf.Experiments, pendingRemovalMountsAllowed: conf.PendingRemovalMountsAllowed, expirationRevokeRetryBase: conf.ExpirationRevokeRetryBase, + impreciseLeaseRoleTracking: conf.ImpreciseLeaseRoleTracking, } c.standbyStopCh.Store(make(chan struct{})) diff --git a/vault/request_handling.go b/vault/request_handling.go index 743dad62c..adffba177 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1687,10 +1687,16 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // Attach the display name, might be used by audit backends req.DisplayName = auth.DisplayName - // 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 { + requiresLease := resp.Auth.TokenType != logical.TokenTypeBatch + + // If role was not already determined by http.rateLimitQuotaWrapping + // and a lease will be generated, calculate a role for the leaseEntry. + // We can skip this step if there are no pre-existing role-based quotas + // for this mount and Vault is configured to skip lease role-based lease counting + // until after they're created. This effectively zeroes out the lease count + // for new role-based quotas upon creation, rather than counting old leases toward + // the total. + if reqRole == nil && requiresLease && !c.impreciseLeaseRoleTracking { role = c.DetermineRoleFromLoginRequest(ctx, req.MountPoint, req.Data) } diff --git a/vault/testing.go b/vault/testing.go index f1fb433ed..c86ca20ae 100644 --- a/vault/testing.go +++ b/vault/testing.go @@ -220,6 +220,7 @@ func TestCoreWithSealAndUINoCleanup(t testing.T, opts *CoreConfig) *Core { conf.Experiments = []string{experiments.VaultExperimentEventsAlpha1} conf.CensusAgent = opts.CensusAgent conf.AdministrativeNamespacePath = opts.AdministrativeNamespacePath + conf.ImpreciseLeaseRoleTracking = opts.ImpreciseLeaseRoleTracking if opts.Logger != nil { conf.Logger = opts.Logger @@ -1533,6 +1534,7 @@ func NewTestCluster(t testing.T, base *CoreConfig, opts *TestClusterOptions) *Te coreConfig.DisableAutopilot = base.DisableAutopilot coreConfig.AdministrativeNamespacePath = base.AdministrativeNamespacePath coreConfig.ServiceRegistration = base.ServiceRegistration + coreConfig.ImpreciseLeaseRoleTracking = base.ImpreciseLeaseRoleTracking if base.BuiltinRegistry != nil { coreConfig.BuiltinRegistry = base.BuiltinRegistry diff --git a/website/content/docs/configuration/index.mdx b/website/content/docs/configuration/index.mdx index 337a3dc01..1ae7f8d89 100644 --- a/website/content/docs/configuration/index.mdx +++ b/website/content/docs/configuration/index.mdx @@ -221,6 +221,10 @@ a negative effect on performance due to the tracking of each lock attempt. the `VAULT_EXPERIMENTS` environment variable as a comma-separated list, or via the [`-experiment`](/vault/docs/commands/server#experiment) flag. +- `imprecise_lease_role_tracking` `(bool: "false")` - Skip lease counting by role if there are no role based quotas enabled. + When `imprecise_lease_role_tracking` is set to true and a new role-based quota is enabled, subsequent lease counts start from 0. + `imprecise_lease_role_tracking` affects role-based lease count quotas, but reduces latencies when not using role based quotas. + ### High availability parameters The following parameters are used on backends that support [high availability][high-availability].