From bd36e66ea6b2ebdcde9ccfd077ca47d9e170c456 Mon Sep 17 00:00:00 2001 From: Ellie Date: Fri, 1 Sep 2023 07:01:41 -0500 Subject: [PATCH] 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 * 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 * Update vault/core.go * Update vault/core.go * update var name * udpate docs * Update vault/request_handling.go Co-authored-by: Mike Palmiotto * 1 more docs change --------- 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 0000000000..5ca2819837 --- /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 5758b3d13e..264ccbf9fd 100644 --- a/command/server.go +++ b/command/server.go @@ -2998,6 +2998,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 fc9ab7cfd6..da96040062 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 @@ -1144,6 +1151,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 441ae68e21..e0de7271ba 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -849,6 +849,7 @@ func testConfig_Sanitized(t *testing.T) { "add_mount_point_rollback_metrics": 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 16db800bf3..0d28c9a1c2 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 fadac4f4ad..c01f093708 100644 --- a/http/util.go +++ b/http/util.go @@ -67,6 +67,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 5238b8e824..971346cb7a 100644 --- a/vault/core.go +++ b/vault/core.go @@ -699,6 +699,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. @@ -760,6 +763,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 @@ -1032,6 +1038,7 @@ func CreateCore(conf *CoreConfig) (*Core, error) { pendingRemovalMountsAllowed: conf.PendingRemovalMountsAllowed, expirationRevokeRetryBase: conf.ExpirationRevokeRetryBase, rollbackMountPathMetrics: conf.MetricSink.TelemetryConsts.RollbackMetricsIncludeMountPoint, + impreciseLeaseRoleTracking: conf.ImpreciseLeaseRoleTracking, } c.standbyStopCh.Store(make(chan struct{})) diff --git a/vault/request_handling.go b/vault/request_handling.go index 049ec79a8b..d16cf38041 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 464a348932..b8f964935e 100644 --- a/vault/testing.go +++ b/vault/testing.go @@ -225,6 +225,7 @@ func TestCoreWithSealAndUINoCleanup(t testing.T, opts *CoreConfig) *Core { conf.CensusAgent = opts.CensusAgent conf.AdministrativeNamespacePath = opts.AdministrativeNamespacePath conf.AllLoggers = logger.AllLoggers + conf.ImpreciseLeaseRoleTracking = opts.ImpreciseLeaseRoleTracking if opts.Logger != nil { conf.Logger = opts.Logger @@ -1553,6 +1554,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 337a3dc018..1ae7f8d89e 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].