Skip to content

Prevent blank revisions from being read from the backend#32871

Merged
rosstimothy merged 2 commits intomasterfrom
tross/prevent_empty_revision
Oct 10, 2023
Merged

Prevent blank revisions from being read from the backend#32871
rosstimothy merged 2 commits intomasterfrom
tross/prevent_empty_revision

Conversation

@rosstimothy
Copy link
Copy Markdown
Contributor

Overwrites any empty resource revisions with a placeholder value to prevent any blank revisions from being provided to users.

Comment thread lib/backend/backend.go Outdated
Copy link
Copy Markdown
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

Why do we need the blank revision handling for the memory backend?

Comment thread lib/backend/backend.go Outdated
@rosstimothy
Copy link
Copy Markdown
Contributor Author

Why do we need the blank revision handling for the memory backend?

We either have to insert the place holder into the event stream so that resources in the cache can never have an empty string or add blank revision handling to the memory backend.

@rosstimothy rosstimothy marked this pull request as ready for review October 3, 2023 12:55
@github-actions github-actions Bot requested review from bl-nero and kimlisa October 3, 2023 12:56
Comment thread lib/backend/dynamo/dynamodbbk.go
@rosstimothy rosstimothy requested a review from espadolini October 3, 2023 21:19
@espadolini
Copy link
Copy Markdown
Contributor

We either have to insert the place holder into the event stream so that resources in the cache can never have an empty string or add blank revision handling to the memory backend.

It's the job of the backend driver to never produce an empty revision string, if that means changing the event stream code to make it so that the empty revision string never appears even when "it should" then that's what we should do, IMO.

@rosstimothy
Copy link
Copy Markdown
Contributor Author

We either have to insert the place holder into the event stream so that resources in the cache can never have an empty string or add blank revision handling to the memory backend.

It's the job of the backend driver to never produce an empty revision string, if that means changing the event stream code to make it so that the empty revision string never appears even when "it should" then that's what we should do, IMO.

What's the downside to having the cache just work in the event the upstream backend has a bug?

Copy link
Copy Markdown
Contributor

@fspmarshall fspmarshall left a comment

Choose a reason for hiding this comment

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

R.E. the memory backend handling blank revisions; I think it should handle them correctly, but if it observes them while in mirror mode it should emit a warning.

Copy link
Copy Markdown
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

LGTM other than the memory backend

@rosstimothy rosstimothy force-pushed the tross/prevent_empty_revision branch 3 times, most recently from a7f1434 to 1d4f698 Compare October 7, 2023 18:51
@espadolini
Copy link
Copy Markdown
Contributor

espadolini commented Oct 9, 2023

Considering the situation with the cache applyFns I think we should reconsider the special handling of the blank revision string in the memory backend in mirror mode: an item of a cached type that's stored in dynamodbbk or litebk that has a blank revision because it was last modified before revisions were introduced will be yielded from the cache with its correct revision until it's overwritten, resulting in very confusing behavior if it's conditionally updated. If we instead return it from the cache with a blank string revision we will never accidentally condupdate it.

@rosstimothy rosstimothy force-pushed the tross/prevent_empty_revision branch 4 times, most recently from 3b0f50f to dc0c58e Compare October 10, 2023 14:00
Comment thread lib/backend/memory/memory.go Outdated
Overwrites any empty resource revisions with a placeholder value to
prevent any blank revisions from being provided to users.
All resource revisions are now being set on a backend.Item before
persisting the item to the backend.
@rosstimothy rosstimothy force-pushed the tross/prevent_empty_revision branch from dc0c58e to db9d68e Compare October 10, 2023 15:18
@rosstimothy rosstimothy enabled auto-merge October 10, 2023 15:18
@rosstimothy rosstimothy added this pull request to the merge queue Oct 10, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023
@rosstimothy rosstimothy added this pull request to the merge queue Oct 10, 2023
Merged via the queue into master with commit 4f569a4 Oct 10, 2023
@rosstimothy rosstimothy deleted the tross/prevent_empty_revision branch October 10, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants