Fields caps does not honour ignore_unavailable by drempapis · Pull Request #116021 · elastic/elasticsearch

drempapis · 2024-10-31T14:49:41Z

Querying an existing and closed index fails with cluster_block_exception when the _field_caps with ignore_unavailable=true is called.

The close State is not considered when filtering "unavailable," missing, or unacceptable resources, propagating closed indices to the method TransportFieldCapabilitiesAction::checkIndexBlocks, which raises an exception.

In this PR, state-related code was added to filter out closed indices.

Closes #107767

elasticsearchmachine · 2024-10-31T14:50:05Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

…ogic to exclude closed indices to the TransportFieldsCapabilitiesAction class to avoid business-logic conflicts with implementation for other Actions.

drempapis · 2024-11-01T12:52:23Z

I updated the code after detecting test failures on ci. I moved the logic to exclude closed indices to the TransportFieldsCapabilitiesAction class to avoid business-logic conflicts with implementation for other Actions.

javanna · 2024-11-04T18:54:51Z

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java

+                if (metadata != null && metadata.getState() == IndexMetadata.State.CLOSE) {
+                    if (ignoreUnavailable) return false;
+                    else throw new IndexClosedException(metadata.getIndex());
+                } else return true;


I am a bit outdated on how indices resolution works these days, after recent refactoring. I was somehow not expecting that you need to explicitly handle closed indices, rather that provided indices and indices options, the method does what it needs to do, meaning not return closed indices. Can you double check that that option is no longer possible?

Thank you, @javanna, for reviewing the pr. You are right. Further debugging the code, I found that for the incoming requests, the allowClosedIndices is set to true. Changing the default value at the FieldCapabilitiesRequest level, mitigates that issue.

…he ignoreUnavailable set in the URL request. Changing that value in the corresponding Request class will make it false.

javanna · 2024-11-05T19:07:58Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesRequest.java

 public final class FieldCapabilitiesRequest extends ActionRequest implements IndicesRequest.Replaceable, ToXContentObject {
    public static final String NAME = "field_caps_request";
-    public static final IndicesOptions DEFAULT_INDICES_OPTIONS = IndicesOptions.strictExpandOpen();
+    public static final IndicesOptions DEFAULT_INDICES_OPTIONS = IndicesOptions.strictExpandOpenAndForbidClosed();


Nice! I believe this is ok. The doubt I have is just about the fact that we are changing index resolution for the field caps API, and that will affect existing usage. Yet if this is only to avoid errors when targeting closed indices, I think that's the right call, assuming there are no side effects. Are you able to get a green build?

Thank you, @javanna, I'll proceed with that

I am not 100% sure I am following this. Why are we changing the default behaviour? Don't we want the change only when we get ignore_unavailable=true?
My reasoning here is: if we target a closed index an we have ignore_unavailable=false we want an error back, while if the request has ignore_unavailable=true we can safely skip closed indices.

Thank you, @piergm, for the review. The allowClosedIndices is set to true in IndicesOptions.strictExpandOpen(), which affects the URL parameter ignore_unavailable behavior.

In the main branch, having a closed index for any value of ignore_unavailable, we get an exception of type.
cluster_block_exception. After changing the options in the field request, the API works as expected, returning an IndexClosedException for ignore_unavailable=false.

Oh I see, thank you for the clarification!

server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

drempapis · 2024-11-06T10:55:04Z

@elasticmachine update branch

drempapis · 2024-11-07T07:11:34Z

@elasticmachine update branch

piergm

LGTM, just one small nit 😄

server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java

cla-checker-service · 2024-11-07T10:48:38Z

💚 CLA has been signed

drempapis · 2024-11-07T12:19:22Z

@elasticmachine run elasticsearch-ci/part-4

drempapis · 2024-11-07T12:20:26Z

@elasticmachine test this please

drempapis · 2024-11-07T12:25:46Z

@elasticmachine update branch

…navailable

javanna

LGTM thanks @drempapis

The IndicesOption has been updated into the FieldCapabilitiesRequest to encapsulate the following logic. If we target a closed index and ignore_unavailable=false, we get an IndexClosedException, otherwise if the request contains ignore_unavailable=true, we safely skip the closed index.

The IndicesOption has been updated into the FieldCapabilitiesRequest to encapsulate the following logic. If we target a closed index and ignore_unavailable=false, we get an IndexClosedException, otherwise if the request contains ignore_unavailable=true, we safely skip the closed index. (cherry picked from commit 3ae7921)

drempapis · 2024-11-07T17:50:05Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

The IndicesOption has been updated into the FieldCapabilitiesRequest to encapsulate the following logic. If we target a closed index and ignore_unavailable=false, we get an IndexClosedException, otherwise if the request contains ignore_unavailable=true, we safely skip the closed index.

The IndicesOption has been updated into the FieldCapabilitiesRequest to encapsulate the following logic. If we target a closed index and ignore_unavailable=false, we get an IndexClosedException, otherwise if the request contains ignore_unavailable=true, we safely skip the closed index. (cherry picked from commit 3ae7921)

The IndicesOption has been updated into the FieldCapabilitiesRequest to encapsulate the following logic. If we target a closed index and ignore_unavailable=false, we get an IndexClosedException, otherwise if the request contains ignore_unavailable=true, we safely skip the closed index.

…#199717) - Closes: #199413 - Related: #199654 - Related ES PR: elastic/elasticsearch#116021 - Related ES PR: elastic/elasticsearch#116656 ## Summary This PR unskips tests and updates the Kibana API to the updated ES responses. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…elastic#199717) - Closes: elastic#199413 - Related: elastic#199654 - Related ES PR: elastic/elasticsearch#116021 - Related ES PR: elastic/elasticsearch#116656 ## Summary This PR unskips tests and updates the Kibana API to the updated ES responses. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit f61c043)

…elastic#199717) - Closes: elastic#199413 - Related: elastic#199654 - Related ES PR: elastic/elasticsearch#116021 - Related ES PR: elastic/elasticsearch#116656 ## Summary This PR unskips tests and updates the Kibana API to the updated ES responses. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

The IndicesOption has been updated into the FieldCapabilitiesRequest to encapsulate the following logic. If we target a closed index and ignore_unavailable=false, we get an IndexClosedException, otherwise if the request contains ignore_unavailable=true, we safely skip the closed index.

…elastic#199717) - Closes: elastic#199413 - Related: elastic#199654 - Related ES PR: elastic/elasticsearch#116021 - Related ES PR: elastic/elasticsearch#116656 ## Summary This PR unskips tests and updates the Kibana API to the updated ES responses. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

drempapis added 2 commits October 31, 2024 16:08

Update the code to filter out Closed Indices

43c476f

Fix typo

34d5826

drempapis added >bug Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations v9.0.0 v8.17.0 labels Oct 31, 2024

drempapis added 4 commits October 31, 2024 16:52

update changelog

7b5a833

update changelog

677b73a

Fix styling issue in the code

03ecd8c

I updated the code after detecting test failures on ci. I moved the l…

c7b4bc3

…ogic to exclude closed indices to the TransportFieldsCapabilitiesAction class to avoid business-logic conflicts with implementation for other Actions.

Rearranged the business logic around minimized in scope tests

fecac1b

javanna reviewed Nov 4, 2024

View reviewed changes

drempapis added 2 commits November 5, 2024 13:26

The allowClosedIndices is set to true by default, which contradicts t…

8d1a123

…he ignoreUnavailable set in the URL request. Changing that value in the corresponding Request class will make it false.

Merge branch 'main' into fields_caps_does_not_honour_ignore_unavailable

d17b2e7

drempapis requested a review from javanna November 5, 2024 13:20

javanna reviewed Nov 5, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java Outdated Show resolved Hide resolved

drempapis added 2 commits November 6, 2024 09:13

Merge branch 'main' into fields_caps_does_not_honour_ignore_unavailable

57fbfcd

Remote accidentally introduces empty line

8adaa83

Merge branch 'main' into fields_caps_does_not_honour_ignore_unavailable

646fed9

Merge branch 'main' into fields_caps_does_not_honour_ignore_unavailable

c3affc9

piergm approved these changes Nov 7, 2024

View reviewed changes

server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java Outdated Show resolved Hide resolved

Remove code not in use following the pr directives

5140e0c

elasticmachine and others added 2 commits November 7, 2024 13:25

Merge branch 'main' into fields_caps_does_not_honour_ignore_unavailable

f19bfd8

Merge branch 'elastic:main' into fields_caps_does_not_honour_ignore_u…

775e258

…navailable

javanna approved these changes Nov 7, 2024

View reviewed changes

drempapis merged commit 3ae7921 into elastic:main Nov 7, 2024

drempapis mentioned this pull request Nov 7, 2024

Fields caps does not honour ignore_unavailable - backport 8.x (#116021) #116426

Closed

drempapis mentioned this pull request Nov 7, 2024

[8.x] Fields caps does not honour ignore_unavailable (#116021) #116430

Merged

This was referenced Nov 11, 2024

[Discover] Add logging and retry to the closed index test elastic/kibana#199654

Closed

[Discover][Field caps] Align with the ES responses for closed indices elastic/kibana#199717

Merged

drempapis mentioned this pull request Nov 12, 2024

_validate request does not honour ignore_unavailable #116656

Merged

drempapis self-assigned this Dec 17, 2024

drempapis deleted the fields_caps_does_not_honour_ignore_unavailable branch January 22, 2025 10:31

Conversation

drempapis commented Oct 31, 2024

Uh oh!

elasticsearchmachine commented Oct 31, 2024

Uh oh!

drempapis commented Nov 1, 2024

Uh oh!

javanna Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

drempapis Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

javanna Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

drempapis Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

piergm Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

drempapis Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

piergm Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drempapis commented Nov 6, 2024

Uh oh!

drempapis commented Nov 7, 2024

Uh oh!

piergm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cla-checker-service bot commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drempapis commented Nov 7, 2024

Uh oh!

drempapis commented Nov 7, 2024

Uh oh!

drempapis commented Nov 7, 2024

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

drempapis commented Nov 7, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cla-checker-service bot commented Nov 7, 2024 •

edited

Loading