NoSQL: Limit inline policy mapping value size#4545
Conversation
Put a hard cap on the inline NoSQL policy-mapping value size. That value sits directly in the shared policy-mappings index, so leaving it unbounded lets one attachment bloat commit size and maintenance work for no real gain. Polaris still accepts client-supplied policy-mapping parameters on the attach-policy request and persists them inline in the NoSQL policy-mappings index. I could not find any in-tree first-party caller that intentionally sets up a non-empty parameter bag itself, but the field is still live API surface, so the cap belongs in the NoSQL write path as a defensive bound on that inline index value.
| if (serializedPolicyMappingSize > MAX_POLICY_MAPPING_INDEX_VALUE_SIZE) { | ||
| return state.noCommit( | ||
| new PolicyAttachmentResult( | ||
| UNEXPECTED_ERROR_SIGNALED, |
There was a problem hiding this comment.
Shouldn't we use a better error code for this client input validation failure?
There was a problem hiding this comment.
Ideally yes. I just could not find a legitimate reason why the limit would ever be exceeded. So I think that the accompanying error message should be okay.
| long targetId, | ||
| boolean doAttach, | ||
| @NonNull Map<String, String> parameters) { | ||
| private static final int MAX_POLICY_MAPPING_INDEX_VALUE_SIZE = 384; |
There was a problem hiding this comment.
Also should this be public? (and reused in tests)
There was a problem hiding this comment.
Added a comment and updated the tests.
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| * separate new {@link Obj}ect. | ||
| * </ul> | ||
| */ | ||
| public static final int MAX_POLICY_MAPPING_INDEX_VALUE_SIZE = 384; |
There was a problem hiding this comment.
How was 384 picked? The javadoc explains how to evolve it but not the rationale , a short note (typical param-bag size + headroom, or measured serialized size of expected real-world entries) would help a future reviewer judge whether a bump is safe.
There was a problem hiding this comment.
There isn't really a deeper policy-level reason for 384. The policy mapping parameters are an opaque property bag here, and AFAICT the policy design does not define what is supposed to go into it or what a reasonable size would be.
So this is mostly a NoSQL-storage guardrail. The mapping value is stored inline in the index, and index values are serialized as-is. 384 is intentionally conservative: large enough for the small parameter bags we currently support/test, but small enough that one mapping cannot take a noticeable chunk of the embedded index.
If we ever find a real use case that needs substantially more data in the mapping, I don't think we should keep bumping this forever. Then the payload should probably move into a separate object and the index should only keep the lookup data / reference.
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
4 participants
Put a hard cap on the inline NoSQL policy-mapping value size.
That value sits directly in the shared policy-mappings index, so leaving it unbounded lets one attachment bloat commit size and maintenance work for no real gain.
Polaris still accepts client-supplied policy-mapping parameters on the attach-policy request and persists them inline in the NoSQL policy-mappings index. I could not find any in-tree first-party caller that intentionally sets up a non-empty parameter bag itself, but the field is still live API surface, so the cap belongs in the NoSQL write path as a defensive bound on that inline index value.