Override DocRequest.type() on alerting request classes; tolerate ancillary top-level fields in ScheduledJob.parse#981
Conversation
The security plugin's ResourceAccessEvaluator gates a request only when DocRequest.type() matches one of the configured protected_types. The default implementation returns "indices", so without an override the evaluator skipped every alerting request and the ActionFilter granted access unconditionally. Override type() on every alerting DocRequest that targets the shared resource index (SCHEDULED_JOBS_INDEX). Adds a MONITOR_RESOURCE_TYPE constant in AlertingConstants so the value has a single source of truth. Comment requests do not need this since comments are not a registered resource type. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit cb17148)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to cb17148 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 138ac72
Suggestions up to commit 963e609
Suggestions up to commit e5b4e18
Suggestions up to commit 72d56dc
Suggestions up to commit 399e819
|
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit d47c2a5 |
|
Persistent review updated to latest commit 0461839 |
| } | ||
|
|
||
| override fun type(): String { | ||
| return AlertingConstants.MONITOR_RESOURCE_TYPE |
There was a problem hiding this comment.
Is workflow not a distinct type?
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit ba1899d |
| builder.startObject() | ||
| if (params.paramAsBoolean("with_type", false)) builder.startObject(type) | ||
| if (params.paramAsBoolean("with_type", false)) { | ||
| builder.field(ScheduledJob.RESOURCE_TYPE_FIELD, type) |
| builder.startObject() | ||
| if (params.paramAsBoolean("with_type", false)) builder.startObject(type) | ||
| if (params.paramAsBoolean("with_type", false)) { | ||
| builder.field(ScheduledJob.RESOURCE_TYPE_FIELD, type) |
…s in parse - Separate `with_resource_type` param from `with_type`; only storage writes emit the top-level `resource_type` discriminator. Keeps historical `with_type=true` XContent shape unchanged so cross-plugin/cross-version parsers aren't affected. - ScheduledJob.parse skips trailing top-level fields (e.g. the security plugin's `all_shared_principals` DLS field injected on resource-sharing indices) so it reads both new and post-share docs cleanly. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit 399e819 |
The security plugin's resource-sharing framework and the alerting migration endpoint both inject top-level fields alongside the wrapper — `resource_type`, `all_shared_principals`, `_migration_user_name`, `_migration_backend_roles`. Rewrite `parse` to iterate all top-level fields, dispatch to the registered subtype parser when the field is a known wrapper (`monitor` / `workflow`), and skip everything else via `skipChildren`. Fixes parse failures like: expecting token of type [START_OBJECT] but found [START_ARRAY] when a doc carries `_migration_backend_roles: [...]` at the top level. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit 72d56dc |
The 2-arg overload used by JobSweeper (when the type has already been identified) expected outer END_OBJECT immediately after the wrapper. Docs carrying `all_shared_principals` or the alerting migration scratch fields (`_migration_user_name`, `_migration_backend_roles`) trip that assumption. Mirror the no-arg overload's skip-until-END_OBJECT loop. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit e5b4e18 |
opensearch-project/security#6323 lets ResourceProvider.typeField be a type-specific nested path (`monitor.type` / `workflow.type`) instead of requiring a single shared top-level discriminator. So alerting no longer needs to emit `resource_type` on write, and common-utils no longer needs to expose that field or param. Kept: the tolerant top-level field iteration in ScheduledJob.parse. The security plugin's DLS injection (`all_shared_principals`) and the alerting migration endpoint's scratch owner fields still land alongside the wrapper, so the parser must still skip unknown top-level fields. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit 963e609 |
| } | ||
| next = xcp.nextToken() | ||
| } | ||
| XContentParserUtils.ensureExpectedToken(XContentParser.Token.END_OBJECT, next, xcp) |
There was a problem hiding this comment.
Why are these changes needed?
What the file is for
.opendistro-alerting-config is a shared filing cabinet. Monitors and workflows both live there. Each doc looks like a folder with one big envelope inside:
- Monitor doc:
{ "monitor": { …monitor fields… } } - Workflow doc:
{ "workflow": { …workflow fields… } }
ScheduledJob.parse is the mail-opener. Given one of these docs, it opens the outer folder, looks at the envelope's name (monitor or workflow), and hands the envelope's contents to the matching parser (Monitor.parse or Workflow.parse).
What the old parser did
The old code was hard-coded to a rigid procedure:
- Open the outer folder (
{) - Expect exactly ONE thing inside — the envelope
- Read the envelope's name
- Open the envelope, hand contents to the specialist
- Expect the outer folder to close (
}) with nothing else inside
If it found anything other than the single envelope — for example, a sticky note stuck to the inside of the folder — it errored out.
Why that broke
When resource-sharing is on, the security plugin stamps every doc with an all_shared_principals: [...] field for DLS. That's a sticky note next to the envelope. The old parser saw two things in the folder and threw.
What the new parser does
Both overloads (parse(xcp, id, version) and parse(xcp, type, id, version)) now delegate to a single private helper parseWrapper. It reads every item inside the outer folder:
- If the item's name is
monitororworkflow(defined inSCHEDULED_JOB_WRAPPER_FIELDS), that's the envelope — open it and hand off to the specialist. The 2-arg overload additionally requires the name to match the caller-supplied type. - Anything else (e.g.
all_shared_principals) — it's a sticky note, skip viaxcp.skipChildren().
At the end it verifies an envelope was found (requireNotNull(job)) and that the outer folder closed properly.
The shared helper is order-independent — a sticky note can appear before or after the envelope; the wrapper still gets picked up.
DarshitChanpura
changed the title
| * expected to be of the form: | ||
| * { "<job_type>" : { <job fields> } } | ||
| * Any additional top-level fields (e.g. `all_shared_principals` injected by the security | ||
| * plugin's resource-sharing framework, or migration scratch fields) are tolerated and |
There was a problem hiding this comment.
Can we update this comment if those "scratch fields" were coming from some previous WIP code?
Migration is handled upstream by the security plugin now (per-provider owner paths on ResourceProvider), so alerting no longer emits _migration_* scratch fields. Simplify the skip-loop comments to name only what the security plugin actually injects at index time (all_shared_principals for DLS). Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit 138ac72 |
| @@ -51,7 +75,18 @@ interface ScheduledJob : BaseModel { | |||
| fun parse(xcp: XContentParser, type: String, id: String = NO_ID, version: Long = NO_VERSION): ScheduledJob { | |||
There was a problem hiding this comment.
Is there an opportunity for code re-use here if the purpose is similar to the block above? I'm not sure if this is robust to ordering of keys.
Both parse overloads now delegate to a private parseWrapper helper. This fixes an order-dependence bug in the 2-arg overload: previously it called namedObject at the very first non-outer-START_OBJECT position, which failed if the security plugin's all_shared_principals field (or any other ancillary top-level field) happened to appear before the wrapper in the stored doc. parseWrapper iterates every top-level field and dispatches to namedObject the first time it hits a known wrapper (or the caller-supplied type), so ancillary fields can appear on either side of the wrapper. Removes ~30 lines of duplicated skip-loop logic. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit cb17148 |
2 participants
Summary
Two related changes required by alerting's onboarding to the security plugin's resource-sharing framework (RSC), driven by opensearch-project/alerting#2180 and opensearch-project/security#6323.
1.
DocRequest.type()overridesThe security plugin's ResourceAccessEvaluator gates a request only when
DocRequest.type()is contained in the configuredprotected_typeslist. The defaulttype()returns\"indices\", so without an override the evaluator skipped every alerting request and the ActionFilter granted access unconditionally.Follow-up to #980 which added
DocRequestbut did not overridetype().MONITOR_RESOURCE_TYPEandWORKFLOW_RESOURCE_TYPEconstants toAlertingConstants.IndexMonitorRequest,DeleteMonitorRequest,GetMonitorRequest,AcknowledgeAlertRequest,AcknowledgeChainedAlertRequest,GetAlertsRequest,GetFindingsRequest,DocLevelMonitorFanOutRequest) return\"monitor\".IndexWorkflowRequest,DeleteWorkflowRequest,GetWorkflowRequest,GetWorkflowAlertsRequest) return\"workflow\".2.
ScheduledJob.parsetolerates ancillary top-level fieldsWhen a doc is stored on a resource-sharing-protected index, the security plugin's
postIndexhook writesall_shared_principalsas a top-level field on the doc for DLS filtering. Legacy stored docs may also carry other top-level fields written by admin migration tools. The oldparseimplementation assumed the doc had a single wrapper ({\"monitor\": {...}}or{\"workflow\": {...}}) and threw on any additional top-level fields.Both
parseoverloads now iterate all top-level fields, dispatch to the registered subtype parser (Monitor.parse/Workflow.parse) when the field name matches a known wrapper, and skip any other top-level fields viaskipChildren().Backward compatibility
{\"monitor\": {...}}) parse identically — the wrapper is still the only recognized field.{\"monitor\": {...}, \"all_shared_principals\": [...]}) now parse instead of throwing.Related
protected_typesgating)typeFielditeration + per-provider owner paths; consumes the workflow type value)