fix(registry): cache existing DB object on subset re-registration to preserve owner field#5971
Open
skamenan7 wants to merge 4 commits into
Open
fix(registry): cache existing DB object on subset re-registration to preserve owner field#5971skamenan7 wants to merge 4 commits into
skamenan7 wants to merge 4 commits into
Conversation
…preserve owner field CachedDiskDistributionRegistry.register() was caching the incoming partial config object on subset re-registration instead of the full DB object. On every server restart, config-provided objects re-register without owner set, causing the cache to drop the owner field silently. Under the default ABAC policy (ResourceIsUnowned), a resource with owner=None in the cache appears unowned, which can grant broader access than intended. The DB was always correct; only the in-memory cache view was wrong. Fix: fetch the existing DB object before calling super().register(). On success, cache existing_obj when it exists (re-registration no-op path) or the incoming obj when it does not (first-time registration path). The super().get() DB read is intentional: in multi-worker deployments each process has its own in-memory cache, so reading from the DB directly is required to get the authoritative stored object regardless of whether this worker's cache was warmed by _ensure_initialized(). Also move inline User imports from two test bodies to module level, and add an explicit get_cached() assertion to test_cached_registry_updates to cover the else-obj branch of the new ternary (first-time registration). Closes: ogx-ai#5946 Relates-to: ogx-ai#5906 Signed-off-by: skamenan7 <skamenan@redhat.com>
skamenan7
force-pushed
the
feat/5906-clawpatch-code-audit-findings
branch
from
bd93752 to
9fc2a18
Compare
skamenan7
requested review from
bbrowning,
cdoern,
franciscojavierarceo,
leseb,
mattf and
raghotham
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CachedDiskDistributionRegistry.register()was unconditionally caching the incomingobjafter any successfulsuper().register()call. The parent class has two success paths: new registration and subset re-registration (a deliberate no-op). The cache layer wasn't distinguishing between them, so on a subset re-registration it'd write the incoming partial object into the cache — diverging silently from the DB.On every server restart, config-provided objects re-register without
owner. The no-op path fires, the cache ends up holding objects missingowner, and subsequent cache hits return incomplete data with no error or warning.Fix: fetch the existing DB object before calling
super().register(). If it's non-None (subset re-registration), cache it instead of the incoming partial object.Test plan
New test:
test_cached_registry_preserves_owner_on_subset_reregistration— registers a full object withowner, re-registers a config-provided subset withoutowner, asserts bothget_cached()andget()return the full object withownerintact.Breaking changes
None.
Closes #5946
Part of #5906 issues