Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[HLRC] Ensure that response parsers support unknown fields #36938

Closed
20 of 25 tasks
martijnvg opened this issue Dec 21, 2018 · 7 comments
Closed
20 of 25 tasks

[HLRC] Ensure that response parsers support unknown fields #36938

martijnvg opened this issue Dec 21, 2018 · 7 comments
Labels
Team:Data Management Meta label for data/management team

Comments

@martijnvg
Copy link
Member

martijnvg commented Dec 21, 2018 •
edited
Loading

In the HLRC code response parsers using object parsers need to support handeling of unknown field. Otherwise a released HLRC may fail parsing responses from ES nodes released after it. All HLRC areas need to be checked.

After a quick check the follow hlrc areas use ConstructingObjectParser with ignoreUnknownFields flag set to false:

The following use old school parsing and will (likely) fail with unknown fields. FWIW, some of these are just a guess as to how bad they will behave.

  • bulk
  • getMapping (has some asserts, could fail)
  • snapshot.getRepository
  • license.get (just returns whatever json it gets, so its questionable)
  • security.putPrivileges

Disclaimer: It is possible that other old school parsers will quite easily bomb out given unknown fields. This list is likely not exhaustive depending on where the json fields are added.

Maybe we need make an automated check for this? (maybe forbidden apis that checks response classes in hlrc module only)
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@hub-cap
Copy link
Contributor

hub-cap commented Jan 14, 2019

Ive gone thru all of our APIs and updated the list above with the full list of parsers (or nested parsers) that sets ignoreUnknownFields to false. These all need to be addressed. The second list is the old parsers that are not ObjectParsers that dont allow (or did not appear to allow unknown fields).

Please note I have added a bit of Disclaimer to the issue as well. I think that it would be easy to fail some of the parsers depending on where the extra fields are inserted. This would require a user to upgrade. I think as a fallback we should definitely record every time someone messes with the actions to add fields and signify they are breaking to the older versions of the HLRC.

I dont know how we could check that we are using the ConstructingObjectParser's with false at runtime... The only way we could do it is saying "you must use this constructor", but the 2 arg constructor already sets ignoreUnknownFields to false. We can at least put that one in forbiddenAPIs.

hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 14, 2019
@hub-cap
Update Delete Watch to allow unknown fields
8934989 
DeleteWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields.

Relates elastic#36938
@hub-cap hub-cap mentioned this issue Jan 14, 2019
Merged
hub-cap added a commit that referenced this issue Jan 15, 2019
@hub-cap
Update Delete Watch to allow unknown fields (#37435)
cd78565 
DeleteWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields.

Relates #36938
hub-cap added a commit that referenced this issue Jan 15, 2019
@hub-cap
Update Delete Watch to allow unknown fields (#37435)
de08572 
DeleteWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields.

Relates #36938
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 15, 2019
@hub-cap
Update Put Watch to allow unknown fields
94edb79 
PutWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields.

Relates elastic#36938
@hub-cap hub-cap mentioned this issue Jan 15, 2019
Merged
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 15, 2019
@hub-cap
Update Execute Watch to allow unknown fields
10837a5 
ExecuteWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields. It also creates a new client side test for the response.

Relates elastic#36938
@hub-cap hub-cap mentioned this issue Jan 15, 2019
Merged
hub-cap added a commit that referenced this issue Jan 16, 2019
@hub-cap
Update Put Watch to allow unknown fields (#37494)
86697f2 
PutWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields.

Relates #36938
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 16, 2019
@hub-cap
Deprecate HLRC EmptyResponse used by security
b44e5ab 
The EmptyResponse is essentially the same as returning a boolean, which
is done in other places. This commit deprecates all the existing
EmptyResponse methods and creates new boolean methods that have method
params reordered so they can exist with the deprecated methods. A
followup PR in master will remove the existing deprecated methods, fix
the parameter ordering and deprecate the incorrectly ordered parameter
methods.

Relates elastic#36938
@hub-cap hub-cap mentioned this issue Jan 16, 2019
Merged
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 17, 2019
@hub-cap
Deprecate HLRC security methods
e3a6fa1 
This commit deprecates the few methods that had their parameters
reordered to facilitate the move from EmptyResponse to boolean. This
commit also readds the boolean based methods with the proper
signatures.

Relates elastic#37540
Relates elastic#36938
hub-cap added a commit that referenced this issue Jan 17, 2019
@hub-cap
Update Put Watch to allow unknown fields (#37494)
15eeacf 
PutWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields.

Relates #36938
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 17, 2019
@hub-cap
Update get users to allow unknown fields
4a58a14 
The subparser in get users allows for unknown fields. This commit sets
the value to true for the parser and modifies the test such that it
accurately tests it.

Relates elastic#36938
@hub-cap hub-cap mentioned this issue Jan 17, 2019
Merged
hub-cap added a commit that referenced this issue Jan 18, 2019
@hub-cap
Update Execute Watch to allow unknown fields (#37498)
604422c 
ExecuteWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields. It also creates a new client side test for the response.

Relates #36938
hub-cap added a commit that referenced this issue Jan 18, 2019
@hub-cap
Update Execute Watch to allow unknown fields (#37498)
1532682 
ExecuteWatchResponse did not allow unknown fields. This commit fixes the
test and ConstructingObjectParser such that it does now allow unknown
fields. It also creates a new client side test for the response.

Relates #36938
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Jan 18, 2019
@hub-cap
Update verify repository to allow unknown fields
ae7185a 
The subparser in verify repository allows for unknown fields. This
commit sets the value to true for the parser and modifies the test such
that it accurately tests it.

Relates elastic#36938
@hub-cap hub-cap mentioned this issue Jan 18, 2019
Merged
dakrone added a commit to dakrone/elasticsearch that referenced this issue Feb 4, 2019
@dakrone
Support unknown fields in ingest pipeline map
49a5ef5 
We already support unknown objects in the list of pipelines, this changes the
`PipelineConfiguration` to support fields other than just `id` and `config`.

Relates to elastic#36938
@dakrone dakrone mentioned this issue Feb 4, 2019
Merged
hub-cap added a commit that referenced this issue Feb 4, 2019
@hub-cap
Deprecate HLRC security methods (#37883)
66530db 
This commit deprecates the few methods that had their parameters
reordered to facilitate the move from EmptyResponse to boolean. This
commit also readds the boolean based methods with the proper
signatures.

Relates #37540
Relates #36938
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Feb 4, 2019
@hub-cap
Fix ILM explain response to allow unknown fields
11a8f65 
IndexLifecycleExplainResponse did not allow unknown fields. This commit
fixes the test and ConstructingObjectParser such that it allows unknown
fields.

Relates elastic#36938
Backport of elastic#38054
@hub-cap hub-cap mentioned this issue Feb 4, 2019
Merged
hub-cap added a commit that referenced this issue Feb 4, 2019
@hub-cap
Fix ILM status to allow unknown fields (#38342)
0adef95 
The ILM status parser did not allow for unknown fields. This commit
fixes that and adds an xContentTester to the response test.

Relates #36938
Backport of #38043
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Feb 4, 2019
@hub-cap
Update IndexTemplateMetaData to use ObjectParser
086c244 
The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates elastic#36938
dakrone added a commit that referenced this issue Feb 5, 2019
@dakrone
Support unknown fields in ingest pipeline map configuration (#38352)
d862453 
We already support unknown objects in the list of pipelines, this changes the
`PipelineConfiguration` to support fields other than just `id` and `config`.

Relates to #36938
dakrone added a commit to dakrone/elasticsearch that referenced this issue Feb 5, 2019
@dakrone
Support unknown fields in ingest pipeline map configuration (elastic#…
bae3d92 
…38352)

We already support unknown objects in the list of pipelines, this changes the
`PipelineConfiguration` to support fields other than just `id` and `config`.

Relates to elastic#36938
@dakrone dakrone mentioned this issue Feb 5, 2019
Merged
hub-cap added a commit that referenced this issue Feb 5, 2019
@hub-cap
Fix ILM explain response to allow unknown fields (#38363)
bfa1a8c 
IndexLifecycleExplainResponse did not allow unknown fields. This commit
fixes the test and ConstructingObjectParser such that it allows unknown
fields.

Relates #36938
Backport of #38054
hub-cap added a commit that referenced this issue Feb 5, 2019
@hub-cap
Update Rollup Caps to allow unknown fields (#38339)
8742db3 
This commit ensures that the parts of rollup caps that can allow unknown
fields will allow them. It also modifies the test such that we can use
the features we need for disallowing fields in spots where they would
not be allowed.

Relates #36938
hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Feb 5, 2019
@hub-cap
Update Rollup Caps to allow unknown fields
1f44591 
This commit ensures that the parts of rollup caps that can allow unknown
fields will allow them. It also modifies the test such that we can use
the features we need for disallowing fields in spots where they would
not be allowed.

Relates elastic#36938
Backport of elastic#38339
@hub-cap hub-cap mentioned this issue Feb 5, 2019
Merged
@martijnvg martijnvg mentioned this issue Feb 5, 2019
Merged
dakrone added a commit that referenced this issue Feb 5, 2019
@dakrone
Support unknown fields in ingest pipeline map configuration (#38429)
9dde53b 
Backport of #38352

We already support unknown objects in the list of pipelines, this changes the
`PipelineConfiguration` to support fields other than just `id` and `config`.

Relates to #36938
hub-cap added a commit that referenced this issue Feb 5, 2019
@hub-cap
Update Rollup Caps to allow unknown fields (#38446)
6d16374 
This commit ensures that the parts of rollup caps that can allow unknown
fields will allow them. It also modifies the test such that we can use
the features we need for disallowing fields in spots where they would
not be allowed.

Relates #36938
Backport of #38339
martijnvg added a commit that referenced this issue Feb 6, 2019
@martijnvg @hub-cap
Update IndexTemplateMetaData to allow unknown fields (#38448)
9492dce 
Updated IndexTemplateMetaData to use ObjectParser.

The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates #36938

Co-authored-by: Michael Basnight <[email protected]>
Co-authored-by: Martijn van Groningen <[email protected]>
martijnvg added a commit to martijnvg/elasticsearch that referenced this issue Feb 6, 2019
@martijnvg @hub-cap
Update IndexTemplateMetaData to allow unknown fields (elastic#38448)
fbe6370 
Updated IndexTemplateMetaData to use ObjectParser.

The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates elastic#36938

Co-authored-by: Michael Basnight <[email protected]>
Co-authored-by: Martijn van Groningen <[email protected]>
@martijnvg martijnvg mentioned this issue Feb 6, 2019
Merged
martijnvg added a commit that referenced this issue Feb 6, 2019
@martijnvg @hub-cap
Update IndexTemplateMetaData to allow unknown fields (#38448) (#38493)
65f807c 
Updated IndexTemplateMetaData to use ObjectParser.

The IndexTemplateMetaData class used old parsing logic and was not
resiliant to new fields. This commit updates it to use the
ConstructingObjectParser and allow unknown fields.

Relates #36938

Co-authored-by: Michael Basnight <[email protected]>
Co-authored-by: Martijn van Groningen <[email protected]>
@hub-cap
Copy link
Contributor

hub-cap commented Oct 7, 2019

I wonder if we can close this. Im leaning toward yes. @martijnvg what do you think?

@rjernst rjernst added the Team:Data Management Meta label for data/management team label May 4, 2020
@swallez
Copy link
Member

swallez commented Dec 2, 2021 •
edited by gbanasiak
Loading

7.10 HLRC fails to parse an ApiKey returned by a 7.14 cluster because of the new metadata field that was introduced in 7.13.

@dakrone
Copy link
Member

dakrone commented Mar 8, 2024

Closing this as we've removed the high level rest client in favor of the Java client.

@dakrone dakrone closed this as completed Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Team:Data Management Meta label for data/management team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@dakrone @swallez @rjernst @martijnvg @hub-cap @elasticmachine