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

feat: Storage overlay #1560

Merged
merged 30 commits into from May 9, 2023
Merged

Conversation

Sambigeara
Copy link
Contributor

@Sambigeara Sambigeara commented May 2, 2023

Closes #1106

Introduces the ability to define a second fallback storage driver via a configurable circuit breaker pattern:

storage:
  driver: "overlay"
  overlay:
    baseDriver: postgres
    fallbackDriver: disk
    fallbackErrorThreshold: 1 # number of errors allowed within the fallbackErrorWindow before failover occurs
    fallbackErrorWindow: 1s # the rolling window in which errors are aggregated
  disk:
    directory: policies
    watchForChanges: true
  postgres:
    url: "postgres://${PG_USER}:${PG_PASSWORD}@localhost:5432/postgres?sslmode=disable&search_path=cerbos"

I wasn't sure if this was a feat or an enhancement - can change.

@Sambigeara
Copy link
Contributor Author

Realised whilst raising PR that I need to add documentation - I'll get this done tomorrow morning.

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #1560 (56871e4) into main (5aca50e) will increase coverage by 0.16%.
The diff coverage is 49.06%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1560      +/-   ##
==========================================
+ Coverage   53.34%   53.50%   +0.16%     
==========================================
  Files         128      130       +2     
  Lines       14452    14605     +153     
==========================================
+ Hits         7709     7814     +105     
- Misses       6070     6105      +35     
- Partials      673      686      +13     
Impacted Files Coverage Δ
internal/server/server.go 50.90% <0.00%> (-0.83%) ⬇️
internal/storage/store.go 20.58% <0.00%> (-3.15%) ⬇️
internal/storage/overlay/conf.go 43.33% <43.33%> (ø)
internal/storage/overlay/store.go 59.81% <59.81%> (ø)
internal/storage/blob/tests.go 81.45% <100.00%> (ø)

... and 3 files with indirect coverage changes

internal/storage/overlay/conf.go Outdated Show resolved Hide resolved
internal/storage/overlay/conf.go Outdated Show resolved Hide resolved
internal/storage/overlay/conf.go Outdated Show resolved Hide resolved
internal/storage/overlay/store.go Outdated Show resolved Hide resolved
internal/storage/store.go Outdated Show resolved Hide resolved
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

I just realised that we can simplify this quite a bit. Sorry I didn't think about it earlier.

Essentially, the overlay driver only needs to implement the BinaryStore interface. If one or both of the wrapped drivers is a SourceStore, we can easily convert them to BinaryStore anyway. Therefore, you only need to have two struct fields of type BinaryStore instead of the four that it has now.

The SourceStore interface is normally used by the CompileManager when the engine is initialised but because we provide our special PolicyLoader, those methods will never be used. Similarly, because we don't support mutable operations, the admin service can work with just the Store interface which is already embedded in BinaryStore.

docs/modules/configuration/pages/storage.adoc Outdated Show resolved Hide resolved
internal/storage/overlay/conf.go Show resolved Hide resolved
internal/server/server.go Outdated Show resolved Hide resolved
internal/storage/overlay/overlay.go Outdated Show resolved Hide resolved
internal/storage/overlay/store.go Outdated Show resolved Hide resolved
internal/storage/overlay/store.go Outdated Show resolved Hide resolved
internal/storage/overlay/store.go Outdated Show resolved Hide resolved
internal/storage/overlay/store.go Outdated Show resolved Hide resolved
@Sambigeara
Copy link
Contributor Author

Aaah, I was getting a bit muddled about how updates to the underlying SourceStores would propagate to the PolicyLoaders (e.g. polling for updates to git/blob storage etc), but on further reading, it seems that this'll all be handled per store in the background via subscriptions, and therefore GetPolicySet will always reflect up to date store state? If so, that all makes sense!

On that, would it not still make sense to implement the Reloadable interface? We don't support mutable ops, but if we already support updates via polling, it doesn't seem unreasonable to expose this manual trigger?

Therefore, you only need to have two struct fields of type BinaryStore instead of the four that it has now.

I think we'll still need to maintain the four struct fields (two Stores and two PolicyLoaders), because there's a chicken-egg situation whereby the schemaMgr expects a Store implementation, but then is required by the compilation.Manager to build the PolicyLoader. I'm not sure how to solve that tidily without separately instantiating the stores and the policy loaders in two stages.

@Sambigeara
Copy link
Contributor Author

I've addressed most of the smaller review items but I've left the Reloadable interface in there for now until the need is clarified.

Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

I think it's reasonable to implement Reloadable because it allows users to reload the store through the Admin API. I left a comment down below about it.

You're right. The Store struct needs four fields. I wasn't thinking about the initialisation stage for SourceStores.

internal/storage/overlay/store.go Outdated Show resolved Hide resolved
internal/storage/overlay/store.go Show resolved Hide resolved
Copy link
Contributor

@charithe charithe left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
…rics

Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
…circuit breaker calc fn. Update tests

Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Sambigeara and others added 9 commits May 9, 2023 16:01
Co-authored-by: Charith Ellawala <charithe@users.noreply.github.com>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
Signed-off-by: Sam Lock <sam@swlock.co.uk>
…lel. Removed redundant SubscriptionManager

Signed-off-by: Samuel Lock <samuellock@Samuels-MacBook-Pro.local>
@Sambigeara Sambigeara merged commit d0b3f79 into cerbos:main May 9, 2023
18 of 19 checks passed
@Sambigeara Sambigeara deleted the feat/storage-overlay branch May 9, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overlay storage driver
2 participants