Support wait_for_completion on Security configuration write APIs (#6337)

[nishthm]

Description

Category: Enhancement

Adds support for the standard wait_for_completion query parameter to Security configuration write APIs. Callers can now submit a configuration change with ?wait_for_completion=false and receive an OpenSearch task ID immediately ({"task":"<nodeId>:<taskId>"}), then poll the outcome via the standard Tasks API (GET /_tasks/{task_id}). Omitting the parameter — or setting it to true — preserves the existing synchronous behavior byte-for-byte.

Why: Security configuration writes today fan out a cluster-wide reload after the index write, and the caller has to hold the HTTP connection open for the entire lifecycle. If the connection drops there's no way to tell whether the change committed. Every other long-running OpenSearch API (reindex, update-by-query, force-merge, open-index…) solves this via the Tasks framework; Security did not. This change brings Security in line with that convention.

Old vs new behavior:

wait_for_completion	Behavior
omitted / true (default)	Unchanged. Same response body and status code the endpoint returned before this PR.
false	Update is submitted through the task framework. HTTP 200 with body {"task":"nodeId:taskId"} is returned as soon as the task is registered — before the index write or fan-out reload has completed.

Task completion boundary. The task completes only after ConfigUpdatingActionListener receives all-node acknowledgements of the reload — exactly the point at which the sync path returns today. Any per-node reload failure surfaces as a task-level error.

Task result payload. The stored task result in .tasks contains only {"status":"...","message":"'entity' created."} — never configuration contents. Standard _tasks authorization is therefore sufficient to satisfy the "does not expose Security configuration contents" acceptance criterion; no security-plugin-specific gating on task lookup was added.

Cancellation. Deliberately not supported. SecurityConfigWriteRequest#createTask returns a plain Task rather than a CancellableTask, so POST /_tasks/{id}/_cancel is rejected by TransportCancelTasksAction with "task [...] doesn't support cancellation". Rationale: a mid-fan-out cancel could leave the index write committed but only a subset of nodes reloaded — worse than letting the request run to completion.

Scope

In this PR — the shared plumbing plus opt-in for the 5 endpoints that a Terraform provider calls when provisioning a cluster:

• Roles (RolesApiAction)
• Role mappings (RolesMappingApiAction)
• Internal users (InternalUsersApiAction)
• Tenants (TenantsApiAction)
• Audit (AuditApiAction)

Deferred:

• ActionGroupsApiAction, SecurityConfigApiAction — same shape (config write + fan-out reload), share the same plumbing, but not called by the Terraform provider. Opt-in is a one-line change per subclass; follow-up PR.
• AccountApiAction (self-service password change) — explicitly excluded per feedback in the issue thread; async has near-zero value for an interactive endpoint.
• AllowlistApiAction, NodesDnApiAction, RateLimitersApiAction, MultiTenancyConfigApiAction — same pattern, follow-up as demand appears.

Companion PRs to follow (separate repositories):

• opensearch-project/documentation-website — document the new query parameter, response shape, task lookup.
• opensearch-project/opensearch-api-specification — add wait_for_completion to the affected endpoint schemas so generated client SDKs pick it up.

Implementation

New files (src/main/java/org/opensearch/security/action/configupdate/):

• SecurityConfigWriteAction — ActionType with name cluster:admin/opendistro_security/api/write_config.
• SecurityConfigWriteRequest — carries cType, serialized config bytes, seqNo/primaryTerm for optimistic concurrency, security index name, task description, success message, and success status. createTask returns a plain (non-cancellable) Task. getShouldStoreResult() returns true so TransportAction.execute wraps the listener with TaskResultStoringActionListener automatically.
• SecurityConfigWriteResponse — ActionResponse + ToXContentObject; body is {"status":"OK","message":"..."} matching the sync path exactly. Deliberately does not include configuration contents.
• TransportSecurityConfigWriteAction — HandledTransportAction that performs the IndexRequest and then broadcasts ConfigUpdateAction; only completes the listener after all-node ack, mirroring the sync path's ConfigUpdatingActionListener chain.

Modified files:

• OpenSearchSecurityPlugin.java — registers the new transport action.
• AbstractApiAction.java — adds supportsAsync() hook (default false) and a private maybeSubmitAsTask helper wired via withAsyncTaskSubmitter(...). Consumes wait_for_completion in prepareRequest directly (not inside consumeParameters) so subclass overrides that don't call super still don't reject the parameter as unrecognized.
• RequestHandler.java — introduces the AsyncTaskSubmitter functional interface and pre-branches PUT/PATCH/DELETE on it. If the endpoint doesn't opt in, AsyncTaskSubmitter.NEVER returns false and the sync path is unchanged. Response message and status are precomputed once from the loaded configuration state so the sync body and the stored task result are byte-identical.
• 5 endpoint classes — override supportsAsync() to return true.

Backwards compatibility. The pre-branch runs only when the endpoint has opted in and wait_for_completion=false is explicitly present. The sync response body was refactored to compute (status, message) once instead of twice, but produces the same JSON: {"status":"CREATED","message":"'my_role' created."} for a new PUT, {"status":"OK","message":"'my_role' updated."} for an update, {"status":"OK","message":"'my_role' deleted."} for a delete. The public static helper AbstractApiAction.saveAndUpdateConfigsAsync is left untouched because RollbackVersionApiAction and ConfigUpgradeApiAction still depend on its signature.

Issues Resolved

Closes #6337

Is this a backport? No. New feature, targets main.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? No — the transport action's authorization runs through the same REST-admin check the existing sync path uses, at the REST layer, before task submission. No new plugin-specific action name needs a UI-side entry.

Testing

New integration test file: src/integrationTest/java/org/opensearch/security/api/WaitForCompletionRestApiIntegrationTest.java

Covers each acceptance criterion in the issue:

• Sync success (parameter omitted, and parameter explicitly set to true) — response status and body match the pre-PR behavior.
• Sync failure — invalid body still returns 400 even when wait_for_completion=false is requested, because validation runs before task submission (no phantom task IDs for requests that never execute).
• Async success and task lookup — one test per opted-in endpoint (Roles, RolesMapping, InternalUsers, Tenants, Audit): submit with wait_for_completion=false, verify the returned task ID follows <nodeId>:<taskId> shape, poll _tasks/{id}?wait_for_completion=true&timeout=30s, assert completed=true and — for Roles and InternalUsers — that the stored task result contains the exact success message the sync path would have returned.
• Task cancellation refused — POST /_tasks/{id}/_cancel returns a response whose body contains "doesn't support cancellation".

Regression coverage: the existing RolesRestApiIntegrationTest and InternalUsersRestApiIntegrationTest suites pass unmodified against these changes (17 tests, 0 failures).

Manual verification — run against a real multi-node local cluster built from this branch:

#	Scenario	Expected	Actual
1	PUT /roles/sync_role (no param)	201 + "created"	HTTP 201 {"status":"CREATED","message":"'sync_role' created."}
2	PUT /roles/async_role?wait_for_completion=false	200 + {"task":"nodeId:taskId"}	HTTP 200 {"task":"WkDLHW0bRzG7OmOx3bpeRg:133"}
3	GET /_tasks/{id}?wait_for_completion=true	Stored result mirrors sync response	completed:true, cancellable:false, action:"cluster:admin/opendistro_security/api/write_config", description:"roles/async_role", response:{"status":"CREATED","message":"'async_role' created."}
4	POST /_tasks/{id}/_cancel	Refused	caused_by: illegal_argument_exception "task [...] doesn't support cancellation"
5	Invalid body + wait_for_completion=false	Sync 400, no phantom task	HTTP 400 {"status":"error","reason":"Invalid configuration","invalid_keys":{"keys":"unknown_field"}}
6	Async PUT on internal users	Task ID returned	HTTP 200 {"task":"...:218"}
7	Async PUT on non-opted-in endpoint (actiongroups)	Parameter silently ignored, sync response	HTTP 201 {"status":"CREATED","message":"'manual_ag' created."}

The task-manager output in row 3 also confirms "cancellable":false at the framework level, so _tasks/{id}/_cancel rejection in row 4 comes from
TransportCancelTasksAction rather than any custom check — non-cancellability is a property of the Task type, not a plugin-level filter.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 6e8cd12)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review PATCH async submit uses stale configuration In the PATCH branch, asyncTaskSubmitter.trySubmit(...) is called with securityConfiguration.configuration() (the loaded config) and the pre-computed successMessage ('entity' updated. or Resource updated.). However, unlike the sync path which passes the config through saveOrUpdateConfigurationHandler (which applies the JSON patch mutation before persisting), the async submitter's maybeSubmitAsTask builds an IndexRequest directly from the unmodified configuration and never applies the patch. This means PATCH via wait_for_completion=false would persist the unpatched configuration, silently succeeding without actually applying the client's changes. The integration test asyncPatchRole_returnsTaskIdAndUpdates asserts the patch is applied — verify this scenario behaves correctly, because from the diff the patch mutation appears to happen inside the saveOrUpdateConfigurationHandler chain which is bypassed on the async path. add(method, (channel, request, client) -> mapper.apply(request).valid(securityConfiguration -> { // Message + status for both the sync response body AND the async // stored task result must be identical, so we resolve them once // here based on the loaded configuration state. final boolean hasEntityName = securityConfiguration.maybeEntityName().isPresent(); final String entityName = hasEntityName ? securityConfiguration.entityName() : null; final String successMessage = hasEntityName ? "'" + entityName + "' updated." : "Resource updated."; final RestStatus successStatus = RestStatus.OK; if (asyncTaskSubmitter.trySubmit( channel, request, client, securityConfiguration.configuration(), entityName, successMessage, successStatus )) { return; } saveOrUpdateConfigurationHandler.apply( client, securityConfiguration.configuration(), new AbstractApiAction.OnSucessActionListener<>(channel) { @Override public void onResponse(IndexResponse indexResponse) { response(channel, successStatus, Responses.payload(successStatus, successMessage)); } } ); }).error((status, toXContent) -> response(channel, status, toXContent))); Wire-format compatibility (rolling upgrade) SecurityConfigWriteRequest/SecurityConfigWriteResponse are new types with unversioned writeTo/readFrom. Since this action name (cluster:admin/opendistro_security/api/write_config) is only executed locally via nodeClient.executeLocally(...) from the REST layer (never fanned out to other nodes), older nodes cannot receive this request, so the missing version guards are acceptable. However, this should be confirmed — if any future code path routes this request over transport to a peer, older nodes will fail to deserialize it. Consider adding a comment documenting the local-only invariant, or add a Version guard as insurance. public SecurityConfigWriteRequest(final StreamInput in) throws IOException { super(in); this.indexRequest = new IndexRequest(in); this.cType = in.readString(); this.description = in.readString(); this.successMessage = in.readString(); this.successStatus = in.readEnum(RestStatus.class); } @Override public void writeTo(final StreamOutput out) throws IOException { // TransportRequest.writeTo writes the parent task id — must be called for symmetry with // the StreamInput ctor's super(in) (which reads it back). super.writeTo(out); indexRequest.writeTo(out); out.writeString(cType); out.writeString(description); out.writeString(successMessage); out.writeEnum(successStatus); } Response sent twice on IOException In maybeSubmitAsTask, if channel.newBuilder()/sendResponse throws IOException after the task has already been submitted via nodeClient.executeLocally(...), the code converts to an OpenSearch exception which will bubble up and typically cause the REST layer to send an error response — but the task has already started and its result will be stored to .tasks. The caller receives an error response but the task ID is lost, so the write silently completes with no way to retrieve the result. Consider capturing the task ID before the try block and logging it on failure, or handling the IOException without failing after task submission. final org.opensearch.tasks.Task task = nodeClient.executeLocally( SecurityConfigWriteAction.INSTANCE, updateRequest, org.opensearch.tasks.LoggingTaskListener.instance() ); try (final XContentBuilder builder = channel.newBuilder()) { builder.startObject(); builder.field("task", nodeClient.getLocalNodeId() + ":" + task.getId()); builder.endObject(); channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); } catch (final IOException e) { throw ExceptionsHelper.convertToOpenSearchException(e); } return true;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 6e8cd12

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle task submission failures gracefully executeLocally submits the task synchronously on the calling thread up to the point where the async work begins; if that submission itself throws (e.g., the client is closed, thread pool rejected), the exception will propagate out of maybeSubmitAsTask, the outer handler will not know that a response was intended, and the caller may receive a generic 500 with a stack trace instead of the standard REST error envelope. Wrap the submission in try/catch and route failures through the channel with a proper error response, so async submission failures are surfaced consistently. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [199-213] -final org.opensearch.tasks.Task task = nodeClient.executeLocally( - SecurityConfigWriteAction.INSTANCE, - updateRequest, - org.opensearch.tasks.LoggingTaskListener.instance() -); +final org.opensearch.tasks.Task task; +try { + task = nodeClient.executeLocally( + SecurityConfigWriteAction.INSTANCE, + updateRequest, + org.opensearch.tasks.LoggingTaskListener.instance() + ); +} catch (final Exception e) { + LOGGER.error("Failed to submit async security config write task", e); + internalServerError(channel, "Failed to submit async task: " + e.getMessage()); + return true; +} try (final XContentBuilder builder = channel.newBuilder()) { builder.startObject(); builder.field("task", nodeClient.getLocalNodeId() + ":" + task.getId()); builder.endObject(); channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); } catch (final IOException e) { throw ExceptionsHelper.convertToOpenSearchException(e); } return true; Suggestion importance[1-10]: 5 __ Why: Wrapping executeLocally in a try/catch improves error handling by ensuring submission failures produce a clean REST error rather than a raw 500. Minor but reasonable robustness improvement.	Low
	Document or guard transport wire compatibility This is a new transport action registered cluster-wide, but the request/response wire format has no version guards. In a mixed-version rolling upgrade, an older node that has this plugin at an earlier revision could receive/send a different byte layout with no compatibility escape hatch. Consider gating additive fields with out.getVersion().onOrAfter(V) / in.getVersion().onOrAfter(V) conventions from the start, or explicitly documenting that this action is only ever dispatched locally (never over the transport) so cross-node compatibility is not a concern. src/main/java/org/opensearch/security/action/configupdate/SecurityConfigWriteRequest.java [59-66] public SecurityConfigWriteRequest(final StreamInput in) throws IOException { super(in); this.indexRequest = new IndexRequest(in); this.cType = in.readString(); this.description = in.readString(); this.successMessage = in.readString(); this.successStatus = in.readEnum(RestStatus.class); + // Any future additive field must be guarded on both sides: + // if (in.getVersion().onOrAfter(Version.V_x_y_z)) { this.newField = in.readX(); } } Suggestion importance[1-10]: 4 __ Why: Wire-format versioning is a legitimate concern for transport actions, though in practice this action is submitted via executeLocally and may not cross nodes. The suggestion is more informational and lacks concrete added guards.	Low
	Strengthen request validation checks Because cType is Objects.requireNonNulled in the constructor, the only way this branch triggers is if callers pass an empty string — but the deserialization constructor accepts any string. If an older/malicious peer ever sends an empty cType, the current code returns a validation exception with no field-level attachment. More importantly, indexRequest field itself is never null-checked because the constructor requires it — but on deserialization a corrupt stream is impossible to produce here. Consider also validating that indexRequest.id() matches cType to catch mismatched payloads early. src/main/java/org/opensearch/security/action/configupdate/SecurityConfigWriteRequest.java [81-89] @Override public ActionRequestValidationException validate() { if (Strings.isNullOrEmpty(cType)) { final var e = new ActionRequestValidationException(); e.addValidationError("cType is required"); return e; } - // Defer index-side preconditions to indexRequest.validate() on the receiving node. + if (!cType.equals(indexRequest.id())) { + final var e = new ActionRequestValidationException(); + e.addValidationError("cType [" + cType + "] must match indexRequest id [" + indexRequest.id() + "]"); + return e; + } return indexRequest.validate(); } Suggestion importance[1-10]: 3 __ Why: Adding a cross-check between cType and indexRequest.id() is a defensive validation, but the values are always set together on the coordinator, making this a low-impact hardening.	Low

---

Previous suggestions

Suggestions up to commit 086f134

Category	Suggestion	Impact
Possible issue	Avoid enum ordinal serialization for RestStatus Serializing RestStatus as an enum ordinal is fragile: any reorder/insertion in RestStatus breaks wire compat across mixed-version clusters. Prefer serializing the numeric HTTP status code (out.writeVInt(status.getStatus()) / RestStatus.fromCode(in.readVInt())), which is stable and version-independent. The same concern applies to SecurityConfigWriteRequest.successStatus. src/main/java/org/opensearch/security/action/configupdate/SecurityConfigWriteResponse.java [49-53] public SecurityConfigWriteResponse(final StreamInput in) throws IOException { super(in); - this.status = in.readEnum(RestStatus.class); + this.status = RestStatus.fromCode(in.readVInt()); this.message = in.readString(); } Suggestion importance[1-10]: 6 __ Why: Valid concern about wire-format stability across versions using enum ordinal vs. HTTP status code, though writeEnum/readEnum in OpenSearch typically serializes by name/ordinal in a stable manner. Still, using status code is more robust.	Low
	Verify IndexRequest stream constructor symmetry IndexRequest has a (StreamInput, ShardId) and a no-arg + readFrom pattern in some OpenSearch versions — using new IndexRequest(in) directly may throw if the stream position was already advanced by super(in), or if IndexRequest expects readFrom(in) instead. Confirm the API and that IndexRequest's stream constructor writes its full writeTo payload symmetrically; otherwise the transport call will fail on the receiving node. Also note that transporting a full IndexRequest between nodes for a REST-triggered async task is unusual — coordinator-only execution would sidestep this concern entirely. src/main/java/org/opensearch/security/action/configupdate/SecurityConfigWriteRequest.java [59-66] +public SecurityConfigWriteRequest(final StreamInput in) throws IOException { + super(in); + this.indexRequest = new IndexRequest(in); + this.cType = in.readString(); + this.description = in.readString(); + this.successMessage = in.readString(); + this.successStatus = in.readEnum(RestStatus.class); +} - Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify the API and doesn't propose a concrete change (improved_code is identical to existing_code). Low actionable value.	Low
General	Log task id if response fails The task is submitted via executeLocally before the response is built. If channel.newBuilder()/sendResponse throws, the task is already running and will complete/write to the security index — but the caller will get an exception instead of the task id, losing the ability to track it. Consider building the response body before submitting the task, or at minimum log the task id in the failure path so operators can still recover it. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [205-213] -try (final XContentBuilder builder = channel.newBuilder()) { +try { + final XContentBuilder builder = channel.newBuilder(); builder.startObject(); builder.field("task", nodeClient.getLocalNodeId() + ":" + task.getId()); builder.endObject(); channel.sendResponse(new BytesRestResponse(RestStatus.OK, builder)); } catch (final IOException e) { + LOGGER.error("Failed to send task-id response for submitted task {}:{}", nodeClient.getLocalNodeId(), task.getId(), e); throw ExceptionsHelper.convertToOpenSearchException(e); } return true; Suggestion importance[1-10]: 4 __ Why: Reasonable observability improvement — if the response fails, the caller loses the task id. Adding a log line helps operators recover, but the impact is minor.	Low
	Avoid double-consuming query parameter paramAsBoolean only marks the parameter as consumed when it is actually read here. However, consumeParameters runs earlier in prepareRequest and already consumes wait_for_completion unconditionally. Ensure paramAsBoolean here doesn't throw if the param name is already consumed — verify the parameter is still readable after being consumed, otherwise this branch will never be taken for async requests. Consider reading via request.param("wait_for_completion") and parsing manually, or reordering so this check happens before the centralized consume. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [169-171] -if (request.paramAsBoolean("wait_for_completion", true)) { +final String waitForCompletion = request.param("wait_for_completion"); +if (waitForCompletion == null || Boolean.parseBoolean(waitForCompletion)) { return false; } Suggestion importance[1-10]: 2 __ Why: paramAsBoolean in OpenSearch can be called multiple times safely; consuming a parameter just marks it read and doesn't prevent re-reading. The concern is largely unfounded.	Low

Suggestions up to commit a7d06dc

Category	Suggestion	Impact
Possible issue	Stash thread context for index write The client.index() call is not wrapped in the caller's thread context, unlike the sync path in AbstractApiAction.saveAndUpdateConfigsAsync. Direct index writes to the security index typically require system-context / stashed thread context to bypass security filters. Without this, the write may fail authorization or leak the caller's user context into the index operation. Wrap the call in threadPool.getThreadContext().stashContext() as done elsewhere. src/main/java/org/opensearch/security/action/configupdate/TransportSecurityConfigWriteAction.java [69] -client.index(indexRequest, ActionListener.wrap(indexResponse -> { +try (var ignored = client.threadPool().getThreadContext().stashContext()) { + client.index(indexRequest, ActionListener.wrap(indexResponse -> { Suggestion importance[1-10]: 7 __ Why: Direct writes to the security index typically require a stashed system context to bypass security filters, and missing this could cause authorization failures. This is a potentially important correctness issue worth verifying.	Medium
General	Avoid mutating shared configuration object configuration.removeStatic() mutates the shared configuration instance passed into the pre-branch. If the async submission ultimately fails or falls back, or if the same configuration object is referenced elsewhere in the request handling flow, the static entries are permanently lost from that object. Either operate on a defensive copy or ensure this mutation is safe/necessary at this point in the flow. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [182-188] -configuration.removeStatic(); +final SecurityDynamicConfiguration<?> configCopy = configuration.deepClone(); +configCopy.removeStatic(); final BytesReference content; try { - content = XContentHelper.toXContent(configuration, XContentType.JSON, ToXContent.EMPTY_PARAMS, false); + content = XContentHelper.toXContent(configCopy, XContentType.JSON, ToXContent.EMPTY_PARAMS, false); } catch (final IOException e) { throw ExceptionsHelper.convertToOpenSearchException(e); } Suggestion importance[1-10]: 6 __ Why: Mutating the shared configuration via removeStatic() could cause subtle side effects if the object is referenced downstream. A defensive copy would be safer, though impact depends on the object's lifecycle.	Low
	Reject async param on non-async endpoints When supportsAsync() is false but the caller passed wait_for_completion=false, the request silently falls through to the sync path, which is misleading. Consider either rejecting with a 400 for endpoints that don't support async, or at minimum documenting this behavior. Also, wait_for_completion is consumed in prepareRequest regardless of supportsAsync(), so on non-async endpoints the parameter is accepted but ignored — this may confuse API clients expecting async behavior. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [166-171] if (!supportsAsync()) { + if (!request.paramAsBoolean("wait_for_completion", true)) { + throw new IllegalArgumentException("wait_for_completion=false is not supported for this endpoint"); + } return false; } if (request.paramAsBoolean("wait_for_completion", true)) { return false; } Suggestion importance[1-10]: 3 __ Why: Silently ignoring wait_for_completion=false on non-async endpoints is arguably intentional for backward compatibility, and rejecting could break clients. The suggestion is a debatable API design choice rather than a clear bug.	Low
	Remove misleading matcher wrapper The helper method not_ is defined after its first usage in invalidBody_returnsSyncErrorEvenWhenAsyncRequested — while Java allows this via forward reference for methods, the naming (not_ with trailing underscore) and the misleading comment claiming a static-import collision is confusing since no not is statically imported. Just static-import org.hamcrest.CoreMatchers.not and remove the wrapper. src/integrationTest/java/org/opensearch/security/api/WaitForCompletionRestApiIntegrationTest.java [138-140] -private static org.hamcrest.Matcher<String> not_(org.hamcrest.Matcher<String> inner) { - return org.hamcrest.CoreMatchers.not(inner); -} +// (remove not_ wrapper; add: import static org.hamcrest.CoreMatchers.not;) Suggestion importance[1-10]: 3 __ Why: Minor code style improvement in test code; the wrapper works but is confusingly named and documented. Low impact.	Low

[codecov]

Codecov Report

❌ Patch coverage is 68.20809% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.35%. Comparing base (af1d7ee) to head (a7d06dc).

Files with missing lines	Patch %	Lines
...ction/configupdate/SecurityConfigWriteRequest.java	44.44%	23 Missing and 2 partials ⚠️
...tion/configupdate/SecurityConfigWriteResponse.java	50.00%	9 Missing ⚠️
...nfigupdate/TransportSecurityConfigWriteAction.java	65.38%	8 Missing and 1 partial ⚠️
...arch/security/dlic/rest/api/AbstractApiAction.java	76.47%	6 Missing and 2 partials ⚠️
...nsearch/security/dlic/rest/api/RequestHandler.java	90.24%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6409      +/-   ##
==========================================
- Coverage   75.38%   75.35%   -0.03%     
==========================================
  Files         456      460       +4     
  Lines       30254    30402     +148     
  Branches     4574     4584      +10     
==========================================
+ Hits        22806    22909     +103     
- Misses       5310     5351      +41     
- Partials     2138     2142       +4     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	84.01% <100.00%> (+0.01%)	⬆️
...action/configupdate/SecurityConfigWriteAction.java	100.00% <100.00%> (ø)
...nsearch/security/dlic/rest/api/AuditApiAction.java	91.54% <100.00%> (+0.12%)	⬆️
...security/dlic/rest/api/InternalUsersApiAction.java	93.60% <100.00%> (+0.05%)	⬆️
...nsearch/security/dlic/rest/api/RolesApiAction.java	96.15% <100.00%> (+0.07%)	⬆️
.../security/dlic/rest/api/RolesMappingApiAction.java	97.36% <100.00%> (+0.07%)	⬆️
...earch/security/dlic/rest/api/TenantsApiAction.java	95.45% <100.00%> (+0.21%)	⬆️
...nsearch/security/dlic/rest/api/RequestHandler.java	95.14% <90.24%> (-3.71%)	⬇️
...arch/security/dlic/rest/api/AbstractApiAction.java	87.29% <76.47%> (-1.39%)	⬇️
...tion/configupdate/SecurityConfigWriteResponse.java	50.00% <50.00%> (ø)
... and 2 more

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[DarshitChanpura]

Design's fine. Sync parity and the fails-closed transport path check out (header is stripped cross-node, so the privileged write can't be reached off the REST path). One thing to fix before merge, rest is minor.

Fix: TransportSecurityConfigWriteAction#doExecute re-implements the IndexRequest build and ConfigUpdate fan-out that already exist as createIndexRequestForConfig and ConfigUpdatingActionListener. Identical today, so a change to the write semantics hits sync only. Extract the shared builder/listener so they can't drift.

Minor:

• SecurityConfigWriteResponse#writeTo skips super.writeTo(out) but the read ctor calls super(in) — align them.
• not_ helper cites a static-import collision, but not isn't imported here — use CoreMatchers.not(...).
• Async DELETE/PATCH go through the submitter but only PUT is tested; add them.
• Task description is cType/entityName, readable via _tasks — names not contents, noting only.

Docs and api-specification companions are marked "to follow" — nothing blocks them, link the PRs.

[github-actions]

Persistent review updated to latest commit 086f134

[nishthm]

@DarshitChanpura

Pushed 086f134 on top of a rebase on latest main.

Required — reuse createIndexRequestForConfig and ConfigUpdatingActionListener. Done. Widened both to public static on
AbstractApiAction. SecurityConfigWriteRequest now carries a pre-built IndexRequest (constructed via the shared helper on the coordinator); TransportSecurityConfigWriteAction.doExecute collapses to a one-liner that wires it through ConfigUpdatingActionListener — the same call the sync save makes. Sync and async now share one path.

SecurityConfigWriteResponse#writeTo / read-ctor asymmetry. Kept super(in) and added a comment explaining why the asymmetry is inherent: Writeable.writeTo is abstract up through ActionResponse so there's no concrete super.writeTo to call, and TransportMessage(StreamInput) is documented as a no-op — nothing is actually asymmetric on the wire. Matches how every other ActionResponse in the repo is written (ConfigUpdateResponse in this same package included). On the Request side TransportRequest.writeTo IS concrete (writes the parent task id), so super.writeTo is called explicitly there.

not_ wrapper. Removed; static-imported CoreMatchers.not directly.

Async DELETE/PATCH not tested. Added asyncDeleteRole_returnsTaskIdAndRemoves and asyncPatchRole_returnsTaskIdAndUpdates — both verify task shape, completion, the stored task-result message, and the post-condition via sync GET.

Docs / api-spec companions. Will open both and link here.

[github-actions]

Persistent review updated to latest commit 6e8cd12

[cwperks]

ActionGroupsApiAction, SecurityConfigApiAction — same shape (config write + fan-out reload), share the same plumbing, but not called by the Terraform provider. Opt-in is a one-line change per subclass; follow-up PR.

This would also be good to do following up to this PR for consistency.

[cwperks]

Thank you @nishthm ! The changes in this PR look good to me. I like that this creates an opt-in model for the security endpoints that can be extended to other security endpoints in the future.

[cwperks]

Are there subclasses that don't call super? Should we consider adding to the subclasses?