Enforce minimum cache size for transit backend #12418

divyapola5 · 2021-08-24T16:04:03Z

https://hashicorp.atlassian.net/browse/VAULT-2914

This PR includes changes to enforce minimum cache size (10) for transit backend.

Testing done:

Default size:

divyapola@divyapola-C02CF744MD6R vault % vault secrets enable transit
Success! Enabled the transit secrets engine at: transit/
divyapola@divyapola-C02CF744MD6R vault % vault read transit/cache-config
Key Value
size 0

Set the cache size to a value less than 10:

divyapola@divyapola-C02CF744MD6R vault % vault write transit/cache-config size=6
WARNING! The following warnings were returned from Vault:

size 6 is less than default minimum 10. Cache size is set to 10
cache configurations will be applied when this backend is restarted

divyapola@divyapola-C02CF744MD6R vault % vault read transit/cache-config
Key Value
size 10

Set the cache size to 1:

divyapola@divyapola-C02CF744MD6R vault % vault write transit/cache-config size=1
WARNING! The following warnings were returned from Vault:

size 1 is less than default minimum 10. Cache size is set to 10
cache configurations will be applied when this backend is restarted

divyapola@divyapola-C02CF744MD6R vault % vault read transit/cache-config
Key Value
size 10

Successfully restarted vault:

divyapola@divyapola-C02CF744MD6R vault % vault server -config=config.hcl
You cannot specify a custom root token ID outside of "dev" mode. Your request
has been ignored.
==> Vault server configuration:

         Api Address: http://127.0.0.1:8200
                 Cgo: disabled
     Cluster Address: https://127.0.0.1:8201
          Go Version: go1.16.6
          Listener 1: tcp (addr: "0.0.0.0:8200", cluster address: "0.0.0.0:8201", max_request_duration: "1m30s", max_request_size: "33554432", tls: "disabled")
           Log Level: info
               Mlock: supported: false, enabled: false
       Recovery Mode: false
             Storage: file
             Version: Vault v1.8.0-dev
         Version Sha: 910da26a2b67dff7727fc01a56392bf4affaa2bb

==> Vault server started! Log data will stream in below:

2021-08-24T10:47:42.258-0500 [INFO] proxy environment: http_proxy="" https_proxy="" no_proxy=""
2021-08-24T10:47:47.681-0500 [INFO] core.cluster-listener.tcp: starting listener: listener_address=0.0.0.0:8201
2021-08-24T10:47:47.681-0500 [INFO] core.cluster-listener: serving cluster requests: cluster_listen_address=[::]:8201
2021-08-24T10:47:47.682-0500 [INFO] core: post-unseal setup starting
2021-08-24T10:47:47.685-0500 [INFO] core: loaded wrapping token key
2021-08-24T10:47:47.685-0500 [INFO] core: successfully setup plugin catalog: plugin-directory=""
2021-08-24T10:47:47.690-0500 [INFO] core: successfully mounted backend: type=system path=sys/
2021-08-24T10:47:47.692-0500 [INFO] core: successfully mounted backend: type=identity path=identity/
2021-08-24T10:47:47.693-0500 [INFO] core: successfully mounted backend: type=transit path=transit/
2021-08-24T10:47:47.693-0500 [INFO] core: successfully mounted backend: type=cubbyhole path=cubbyhole/
2021-08-24T10:47:47.700-0500 [INFO] core: successfully enabled credential backend: type=token path=token/
2021-08-24T10:47:47.701-0500 [INFO] rollback: starting rollback manager
2021-08-24T10:47:47.701-0500 [INFO] core: restoring leases
2021-08-24T10:47:47.703-0500 [INFO] identity: entities restored
2021-08-24T10:47:47.703-0500 [INFO] identity: groups restored
2021-08-24T10:47:47.704-0500 [INFO] expiration: lease restore complete
2021-08-24T10:47:47.706-0500 [INFO] core: post-unseal setup complete
2021-08-24T10:47:47.706-0500 [INFO] core: vault is unsealed

Verify that cache size is set to 10 after restarting vault:

divyapola@divyapola-C02CF744MD6R vault % vault operator unseal
Unseal Key (will be hidden):
Key Value
Seal Type shamir
Initialized true
Sealed false
Total Shares 1
Threshold 1
Version 1.8.0-dev
Storage Type file
Cluster Name vault-cluster-74d7615e
Cluster ID 9a05ee54-e2a3-f709-b33e-c18f27513c4a
HA Enabled false

divyapola@divyapola-C02CF744MD6R vault % vault read transit/cache-config
Key Value
size 10

Set cache size to 0:

divyapola@divyapola-C02CF744MD6R vault % vault write transit/cache-config size=0
WARNING! The following warnings were returned from Vault:

cache configurations will be applied when this backend is restarted

divyapola@divyapola-C02CF744MD6R vault % vault read transit/cache-config
Key Value
size 0

Set the cache size to value greater than 10:

divyapola@divyapola-C02CF744MD6R vault % vault write transit/cache-config size=24
WARNING! The following warnings were returned from Vault:

cache configurations will be applied when this backend is restarted

divyapola@divyapola-C02CF744MD6R vault % vault read transit/cache-config
Key Value
size 24

hashicorp-cla · 2021-08-24T16:04:08Z

All committers have signed the CLA.

sgmiller

Couple small points only.

builtin/logical/transit/backend.go

builtin/logical/transit/path_cache_config.go

sgmiller · 2021-08-24T16:48:38Z

And you should add a unit test for this behavior too. You can add it to the existing TestTransit_CacheConfig function.

website/content/api-docs/secret/transit.mdx

victorr

I added a small suggestion.

Good job!

mladlow · 2021-08-24T17:14:38Z

@divyapola5 we're aiming to include this in Vault 1.8.3, so once this PR has merged we'll need to backport it to the release/1.8.x branch.

ncabatoff · 2021-08-25T13:21:17Z

Fixes #12064.

ncabatoff · 2021-08-25T13:32:10Z

This change solves the immediate issue. From what I can see that was:

patchCacheConfigWrite stores the new cache size without making use of it
at startup we read the non-zero configured cache size from storage and pass it to NewTransitLRU
NewTransitLRU calls lru.New2Q(size), and uses int(size/2) for one of its internal LRU caches, which works out to 0 if size==1, which is invalid

There's still the bug that patchCacheConfigWrite isn't making use of the updated cache size, so it only takes effect after a restart. It feels like we should fix that too, else conceivably some future change to the lru library could re-introduce a variant of this bug. In other words, failing to apply the change prior to storing the config opens the door to another bug that prevents Vault from coming up. It's also a bad UX - we don't normally require a vault restart for changes to be applied.

divyapola5 · 2021-08-25T17:45:51Z

This change solves the immediate issue. From what I can see that was:

patchCacheConfigWrite stores the new cache size without making use of it

at startup we read the non-zero configured cache size from storage and pass it to NewTransitLRU

NewTransitLRU calls lru.New2Q(size), and uses int(size/2) for one of its internal LRU caches, which works out to 0 if size==1, which is invalid

There's still the bug that patchCacheConfigWrite isn't making use of the updated cache size, so it only takes effect after a restart. It feels like we should fix that too, else conceivably some future change to the lru library could re-introduce a variant of this bug. In other words, failing to apply the change prior to storing the config opens the door to another bug that prevents Vault from coming up. It's also a bad UX - we don't normally require a vault restart for changes to be applied.

pathCacheConfigWrite isn't making use of the updated cache size even before this change. Are you saying that we should change that behavior as part of this PR?

ncabatoff · 2021-08-25T18:17:05Z

pathCacheConfigWrite isn't making use of the updated cache size even before this change. Are you saying that we should change that behavior as part of this PR?

I'm saying we should try, yeah. It's possible that will turn out to be a bigger change than we want to tackle as part of this PR, but I'd like us to make the attempt.

…size

builtin/logical/transit/path_cache_config.go

sgmiller

A couple of requests plus this needs a Changelog entry, but otherwise looks good!

builtin/logical/transit/backend.go

sgmiller · 2021-09-13T16:46:16Z

sdk/helper/keysutil/lock_manager.go

@@ -101,6 +101,24 @@ func (lm *LockManager) InvalidatePolicy(name string) {
 	}
 }

+func (lm *LockManager) RefreshCache(cacheSize int) error {


Can we call this InitCache or something? Refresh kind of implies you're refreshing the cached data somehow.

Yes, that makes sense.

* Enforce Minimum cache size for transit backend * enfore minimum cache size and log a warning during backend construction * Update documentation for transit backend cache configuration * Added changelog * Addressed review feedback and added unit test * Modify code in pathCacheConfigWrite to make use of the updated cache size * Updated code to refresh cache size on transit backend without restart * Update code to acquire read and write locks appropriately

divyapola5 added 4 commits August 23, 2021 14:42

Enforce Minimum cache size for transit backend

9c2c354

enfore minimum cache size and log a warning during backend construction

ba78939

Update documentation for transit backend cache configuration

c8b349f

Update documentation for transit backend cache configuration

7d87642

divyapola5 requested review from victorr, sgmiller, briankassouf, ncabatoff and rculpepper August 24, 2021 16:04

divyapola5 self-assigned this Aug 24, 2021

Added changelog

1cc707e

vercel bot temporarily deployed to Preview – vault-storybook August 24, 2021 16:06 Inactive

vercel bot temporarily deployed to Preview – vault August 24, 2021 16:06 Inactive

divyapola5 changed the title ~~Cache size fix~~ Enforce minimum cache size for transit backend Aug 24, 2021

sgmiller requested changes Aug 24, 2021

View reviewed changes

builtin/logical/transit/backend.go Outdated Show resolved Hide resolved

builtin/logical/transit/backend.go Outdated Show resolved Hide resolved

builtin/logical/transit/path_cache_config.go Outdated Show resolved Hide resolved

victorr reviewed Aug 24, 2021

View reviewed changes

website/content/api-docs/secret/transit.mdx Show resolved Hide resolved

victorr approved these changes Aug 24, 2021

View reviewed changes

mladlow added this to the 1.8.3 milestone Aug 24, 2021

divyapola5 added 2 commits August 24, 2021 17:18

Merge remote-tracking branch 'origin/main' into cacheSizeFix

912af8c

Addressed review feedback and added unit test

95c7db0

vercel bot temporarily deployed to Preview – vault-storybook August 24, 2021 22:22 Inactive

vercel bot temporarily deployed to Preview – vault August 24, 2021 22:23 Inactive

divyapola5 closed this Aug 25, 2021

divyapola5 reopened this Aug 25, 2021

divyapola5 added 2 commits August 25, 2021 16:49

Merge remote-tracking branch 'origin/main' into cacheSizeFix

d2dbfed

Modify code in pathCacheConfigWrite to make use of the updated cache …

825d743

…size

vercel bot temporarily deployed to Preview – vault August 25, 2021 21:51 Inactive

vercel bot temporarily deployed to Preview – vault-storybook August 25, 2021 21:51 Inactive

heatherezell mentioned this pull request Sep 3, 2021

Setting transit/cache-config to > 0 causes terminal election/seal cycle #12064

Closed

ncabatoff reviewed Sep 7, 2021

View reviewed changes

builtin/logical/transit/path_cache_config.go Outdated Show resolved Hide resolved

divyapola5 added 2 commits September 10, 2021 13:44

Merge remote-tracking branch 'origin/main' into cacheSizeFix

90ba73d

Updated code to refresh cache size on transit backend without restart

3a4c78b

divyapola5 requested a review from taoism4504 as a code owner September 10, 2021 18:50

vercel bot temporarily deployed to Preview – vault-storybook September 10, 2021 18:50 Inactive

vercel bot temporarily deployed to Preview – vault September 10, 2021 18:50 Inactive

sgmiller requested changes Sep 13, 2021

View reviewed changes

divyapola5 added 2 commits September 13, 2021 14:18

Merge remote-tracking branch 'origin/main' into cacheSizeFix

3f73be3

Update code to acquire read and write locks appropriately

5f831bf

vercel bot temporarily deployed to Preview – vault September 13, 2021 19:22 Inactive

vercel bot temporarily deployed to Preview – vault-storybook September 13, 2021 19:22 Inactive

sgmiller approved these changes Sep 13, 2021

View reviewed changes

divyapola5 merged commit 94d4fdb into main Sep 13, 2021

divyapola5 deleted the cacheSizeFix branch September 13, 2021 21:45

divyapola5 added the backport label Sep 14, 2021

This was referenced Sep 27, 2021

Add missing read unlock calls in transit backend code #12651

Closed

Add missing read unlock calls in transit backend code #12652

Merged

Backport pr 12652 1.8.x : Add missing read unlock calls in transit backend code #12662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce minimum cache size for transit backend #12418

Enforce minimum cache size for transit backend #12418

divyapola5 commented Aug 24, 2021

hashicorp-cla commented Aug 24, 2021 •

edited

Loading

sgmiller left a comment

sgmiller commented Aug 24, 2021

victorr left a comment

mladlow commented Aug 24, 2021

ncabatoff commented Aug 25, 2021

ncabatoff commented Aug 25, 2021 •

edited

Loading

divyapola5 commented Aug 25, 2021

ncabatoff commented Aug 25, 2021

sgmiller left a comment

sgmiller Sep 13, 2021

divyapola5 Sep 13, 2021

Enforce minimum cache size for transit backend #12418

Enforce minimum cache size for transit backend #12418

Conversation

divyapola5 commented Aug 24, 2021

hashicorp-cla commented Aug 24, 2021 • edited Loading

sgmiller left a comment

Choose a reason for hiding this comment

sgmiller commented Aug 24, 2021

victorr left a comment

Choose a reason for hiding this comment

mladlow commented Aug 24, 2021

ncabatoff commented Aug 25, 2021

ncabatoff commented Aug 25, 2021 • edited Loading

divyapola5 commented Aug 25, 2021

ncabatoff commented Aug 25, 2021

sgmiller left a comment

Choose a reason for hiding this comment

sgmiller Sep 13, 2021

Choose a reason for hiding this comment

divyapola5 Sep 13, 2021

Choose a reason for hiding this comment

hashicorp-cla commented Aug 24, 2021 •

edited

Loading

ncabatoff commented Aug 25, 2021 •

edited

Loading