Add query param `include_source_on_error` for ingest requests by mosche · Pull Request #120725 · elastic/elasticsearch

mosche · 2025-01-23T14:49:27Z

By default ES will include the document source in the error message in case of parsing errors.
While this is useful for investigating the issue, it might not be acceptable in certain cases, e.g. to prevent sensitive data being captured in logs.

include_source_on_error allows to disable the default behavior of including the source in error messages.

This is supported for everything using the default RestRequest content parser and specifically also for:

bulk requests
index requests
update requests

Relates to ES-9186

…rning the source in case of parsing errors (JSON).

elasticsearchmachine · 2025-01-23T17:43:45Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2025-01-23T17:43:46Z

Hi @mosche, I've created a changelog YAML for you.

mosche · 2025-01-23T17:44:08Z

Where and how should this be documented?

prdoyle

I'm a little concerned that the default here is to leak potentially sensitive information, but perhaps that is unavoidable due to legacy compatibility?

mosche · 2025-01-24T15:03:54Z

I'm a little concerned that the default here is to leak potentially sensitive information, but perhaps that is unavoidable due to legacy compatibility?

Fair point 👍 Maintaining the current default, which is including the source, was an explicit requirement of the ticket based on discussions in our team sync.

mosche · 2025-01-24T15:37:41Z

Wondering, should include_source_on_error be added to any client specs? If so where? The ones in Yaml Specs?
Also, I didn't add include_source_on_error as capability to the endpoints as I wasn't able to test this in Yaml tests (is there a way to post an invalid json payload?). Let me know if you think this is useful anyways.

rjernst

LGTM, a few nits

rjernst · 2025-01-24T19:53:02Z

+        BytesReference source,
+        XContentType xContentType,
+        String routing,
+        Map<String, String> dynamicTemplates


why this new ctor?

Similar to the one above, mostly to enforce the correct default flag.
Slowly wondering, includeSourceOnError should probably better be passed along as nullable Boolean. That way the default can be handled in a single place only. Currently the default is all over the place.

DaveCTurner · 2025-01-27T09:13:20Z

Wondering, should include_source_on_error be added to any client specs? If so where? The ones in Yaml Specs?

As implemented in this PR, yes, we need to add this to the YAML spec for every endpoint whose request may have a body. We could change the implementation slightly and make this parameter truly common to all endpoints, and then we could call it a "common" parameter and document it alongside ?error_trace and so on and avoid the need for that.

Also, I didn't add include_source_on_error as capability to the endpoints as I wasn't able to test this in Yaml tests (is there a way to post an invalid json payload?). Let me know if you think this is useful anyways.

I don't think that's possible, the YAML tests should be translated to all sorts of input formats (CBOR, SMILE, ...) not just JSON. But that's not really the point of the YAML tests: these tests are more so that other client implementations can run without needing to run any Java code. So we should have some YAML tests that set this parameter if only to verify that it's accepted (and understood by other clients).

mosche · 2025-01-27T10:38:52Z

Thanks @DaveCTurner, I've updated the specs. I'll follow up with a separate PR to also use the param in YAML tests.
Setting up capabilities for those endpoints requires a bit of additional work to support existing query params which I'd rather keep separate in case it causes issues.

Regarding making this "common", I've decided against that for now. Considering that this is meant to be used in sensitive use cases, I'd rather be sure the parameter is also correctly consumed when provided. That's not always obvious and should better be checked for every endpoint when the behavior is required.

DaveCTurner · 2025-01-27T10:45:48Z

I'd rather be sure the parameter is also correctly consumed when provided.

I don't think this PR achieves that. There's loads of REST handlers which call contentParser() and therefore consume the new parameter but which don't necessarily propagate it correctly like we're doing here for index/bulk actions.

mosche · 2025-01-27T11:08:20Z

I don't think this PR achieves that. There's loads of REST handlers which call contentParser() and therefore consume the new parameter but which don't necessarily propagate it correctly like we're doing here for index/bulk actions.

Yes, exactly, that's what I meant to say. Goal of this PR isn't to support the param everywhere, this is why it should not be documented / handled as common query parameter. The usage of include_source_on_error (in combination with applyContentParser) is only tested for the usage in RestUpdateAction.

But granted, other actions might accept the parameter if using contentParser with unverified behavior.
I could explicitly check the parameter and enable using a specialized applyContentParser(Boolean includeSourceOnError, CheckedConsumer<XContentParser, IOException> applyParser)

DaveCTurner · 2025-01-27T11:12:22Z

Goal of this PR isn't to support the param everywhere, this is why it should not be documented / handled as common query parameter. The usage of include_source_on_error (in combination with applyContentParser) is only tested for the usage in RestUpdateAction.

Right yeah I think if we're not going to support it everywhere we should be much more selective about where we do support it. Currently this PR adds the extra parameter (but no testing or docs) to over 100 endpoints.

rjernst · 2025-01-27T16:00:39Z

    final FilterPath[] includes;
    final FilterPath[] excludes;
    final boolean filtersMatchFieldNamesWithDots;
+    final Boolean includeSourceOnError;


IMO a nullable Boolean is pretty confusing. We should always know whether we want to include source on errors, but null begs the question "what do we do in the null case". Since that case is only for bwc, it's more clear to set that default in the bwc case when constructing from StreamInput.

Since that case is only for bwc, it's more clear to set that default in the bwc case when constructing from StreamInput.

I don't think bwc is the primary concern. If it was just for reading from StreamInput I'd absolutely agree.
Both BulkRequest and IndexRequest have various ctors (and their usage is not always immediately obvious).
In any case, due to these additional ctors, even if moving the bwc fallback into the StreamInput ctor (using boolean), the field would still require inline initialisation so we don't ever default to false.

To me, the motivation for a nullable Boolean (true / false / undefined, use default) was mostly to have a single consistent definition of that default. But I agree, that can easily be more confusing that helpful. I'll remove that commit to revert back to boolean

…ngle place" This reverts commit 6a3e74a.

mosche · 2025-01-28T08:31:49Z

@rjernst I'll merge with previously discussed change reverted, let me know if there's anything to follow up on.

elasticsearchmachine · 2025-01-28T08:35:23Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 120725

…120725) (#121010) A new query parameter `?include_source_on_error` was added for create / index, update and bulk REST APIs to control if to include the document source in the error response in case of parsing errors. The default value is `true`. Relates to ES-9186.

mosche added 2 commits January 23, 2025 15:14

Adding query param include_source_on_error to optionally disable retu…

8e1f6ba

…rning the source in case of parsing errors (JSON).

cleanup

b347b12

elasticsearchmachine added the v9.0.0 label Jan 23, 2025

mosche added 3 commits January 23, 2025 15:51

Merge branch 'main' into xcontent/includeSourceOnError

d7aeccb

fix

b05a743

Merge branch 'main' into xcontent/includeSourceOnError

c3aee56

mosche marked this pull request as ready for review January 23, 2025 17:42

mosche requested a review from a team as a code owner January 23, 2025 17:42

mosche removed the request for review from a team January 23, 2025 17:42

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jan 23, 2025

mosche added :Core/Infra/REST API REST infrastructure and utilities >enhancement and removed needs:triage Requires assignment of a team area label labels Jan 23, 2025

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 23, 2025

Update docs/changelog/120725.yaml

1bcbac2

mosche requested a review from a team January 23, 2025 17:44

[CI] Auto commit changes from spotless

a01b45b

simitt mentioned this pull request Jan 23, 2025

Add parameter to ES _bulk request to exclude source from error responses elastic/go-docappender#226

Closed

Merge branch 'main' into xcontent/includeSourceOnError

6bafa83

mosche added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Jan 24, 2025

prdoyle reviewed Jan 24, 2025

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

Comment thread .../x-content/impl/src/main/java/org/elasticsearch/xcontent/provider/json/JsonXContentImpl.java

prdoyle approved these changes Jan 24, 2025

View reviewed changes

rjernst approved these changes Jan 24, 2025

View reviewed changes

review

77f4138

Merge branch 'main' into xcontent/includeSourceOnError

4d1ff9d

update api specs

3eae9e1

Make includeSourceOnError nullable to control default in a single place

6a3e74a

do not apply includeSourceOnError silently to all content parsers

c380dea

mosche changed the title ~~Add common query param include_source_on_error~~ Add query param include_source_on_error for ingest requests Jan 27, 2025

Merge branch 'main' into xcontent/includeSourceOnError

78d3878

rjernst reviewed Jan 27, 2025

View reviewed changes

mosche added 4 commits January 27, 2025 18:38

Revert "Make includeSourceOnError nullable to control default in a si…

e54173a

…ngle place" This reverts commit 6a3e74a.

fix after revert

a911e45

changelog

9f83956

Merge branch 'main' into xcontent/includeSourceOnError

f30bb94

mosche merged commit a608f06 into elastic:main Jan 28, 2025

mosche deleted the xcontent/includeSourceOnError branch January 28, 2025 08:33

elasticsearchmachine added the backport pending label Jan 28, 2025

mosche mentioned this pull request Jan 28, 2025

[8.x] Added query param ?include_source_on_error for ingest requests (#120725) #121010

Merged

mosche removed the backport pending label Jan 28, 2025

simitt mentioned this pull request Jan 31, 2025

Add support for include_source_on_error query parameter elastic/go-elasticsearch#949

Closed

pquentin mentioned this pull request Feb 6, 2025

Add include_source_on_error to bulk API elastic/elasticsearch-specification#3691

Merged

pquentin mentioned this pull request Mar 13, 2025

Add missing query parameter for CreateRequest elastic/elasticsearch-specification#3954

Merged

cmacknz mentioned this pull request Apr 3, 2025

Elasticsearch output: Add support for include_source_on_error query parameter elastic/beats#43675

Open

cmacknz mentioned this pull request Jun 26, 2025

[beat_receivers] Redact or drop logs that could contain event data in the monitoring filestream instance elastic/elastic-agent#8846

Closed

Conversation

mosche commented Jan 23, 2025

Uh oh!

elasticsearchmachine commented Jan 23, 2025

Uh oh!

elasticsearchmachine commented Jan 23, 2025

Uh oh!

mosche commented Jan 23, 2025

Uh oh!

prdoyle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mosche commented Jan 24, 2025

Uh oh!

mosche commented Jan 24, 2025

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjernst Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

mosche Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner commented Jan 27, 2025

Uh oh!

mosche commented Jan 27, 2025

Uh oh!

DaveCTurner commented Jan 27, 2025

Uh oh!

mosche commented Jan 27, 2025

Uh oh!

DaveCTurner commented Jan 27, 2025

Uh oh!

rjernst Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

mosche Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

mosche commented Jan 28, 2025

Uh oh!

elasticsearchmachine commented Jan 28, 2025

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants