Standardize snapshot indices parsing so that combinations of included and excluded indices are treated the same regardless of the order they are listed in

[stephen-crawford]

Description

Adds a custom comparator-based sort call before parsing indices using the SnapshotUtils class. Previously, the lack of sort meant that mixing negated and normal indices in the "indices" field of the REST calls would create different behavior based on the ordering of the indices. The added sorting logic corrects this by always parsing negated indices after all normal indices (during these situations, the indices parsing had been acting as expected).

In addition, the sorting logic includes a removal of empty strings from the index list since these strings would break the comparator's sort.

Three new tests were added to the SnapshotUtilsTests class in order to make sure that the ordering of the negative and normal indices did not change the behavior (and that it was the expected behavior).

The new behavior is as follows:

1. (Querying a single negated snapshot): If the body of the request reads indices: "-bar", the query will return all indices which are not "bar".
2. (Querying a single negated snapshot wildcard): If the body of the request reads indices: "-bar*", the query will return all indices which are not a subset of "bar*".
3. (Querying multiple negated snapshots): If the body of the request reads indices: "-bar, -baz", the query will return all indices which are not "bar" or `"baz".
4. (Querying multiple negated snapshot wildcards): If the body of the request reads indices: "-bar*, -baz*", the query will return all indices which are not a subset of "bar*" or a subset of `"baz*".
5. (Querying a single snapshot): If the body of the request reads indices: "bar", the query will return "bar".
6. (Querying multiple snapshot wildcards): If the body of the request reads indices: "bar*, baz*", the query will return all indices which are a subset of "bar*" or a subset of `"baz*".
7. (Querying multiple mixed snapshot wildcards): If the body of the request reads indices: "-bar*, ba*", the query will return all indices which are a subset of "ba*" but not a subset of "bar*". This was not the case previously.
8. (Querying multiple mixed snapshot wildcards): If the body of the request reads indices: "ba*, -bar*", the query will return all indices which are a subset of "ba*" but not a subset of "bar*".

Now behavior is persevered irregardless of the order of positive and negated wildcards queries.

Issues Resolved

Resolves issue
opensearch-project/security#1652

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

[stephen-crawford]

Hi @andrross, thank you for reviewing. To the best of my knowledge the only discussion of the query syntax is found here. It says "You can use , to create a list of indices, * to specify an index pattern, and - to exclude certain indices. Don’t put spaces between items. Default is all indices."

I have not read any other documentation that mentions the use of the special characters. I cannot speak for everyone using OpenSearch but given this issue, it seems like at least a fair number of users expect the behavior to be as this issue would make it. I also think that in general, unless we added documentation and an explanation for why this behavior exists, a change is necessary for user experience etc.

[andrross]

@scrawfor99 We're going to have to be super clear about the behavior change here, and then make a judgement call as to whether this is a bug fix versus a breaking change that must be deferred to the next major version. Can you clearly enumerate how this changes the behavior of the API in the commit message and PR description? Something similar to this comment is what I'm thinking.

[stephen-crawford]

Hi @andrross, thank you for your suggestion. I went ahead and added documentation into the body like you requested. Let me know if you need anything else.

Happy Holidays!

[andrross]

Thanks @scrawfor99! So the upshot here is that any call that started with one or more exclusion patterns and also contained explicit inclusion patterns would in effect ignore those inclusions and instead include everything. Is that right? Assuming that is the only behavior change then I do think that is a bug and something worth fixing in a minor version.

[andrross]

I really want to rewrite the logic in this method because it is very difficult to follow, but I'm going to resist that impulse for now. I am a bit concerned about possible implications of removing empty strings with the change here. I'm inclined to not sort the list, and instead just move the exclusions to the end. What do you think? That can be done with something like:

// Move the exclusions to end of list to ensure they are processed
// after explicitly selected indices are chosen.
final List<String> excludesAtEndSelectedIndices = Stream.concat(
    Arrays.stream(selectedIndices)
        .filter(s -> s.isEmpty() || s.charAt(0) != '-'),
    Arrays.stream(selectedIndices)
        .filter(s -> !s.isEmpty() && s.charAt(0) == '-'))
    .collect(Collectors.toUnmodifiableList());

Then just tweak the logic below to use this list instead of the selectedIndices array.

[stephen-crawford]

If you think that this is a better option that seems fine. It should save a bit of time on larger indices lists (though the indices list probably won't be large enough that this is not negligible). I can swap this over :)

As far as the code logic, I only just started looking at this class and its methods but if you feel there is a way it can be improved that you would like to be done just let me know and I will rewrite it. I have just been trying to make the targeted changes with as little modification as possible so as to be less likely to break anything but I agree that the code could be a bit easier to follow.

[andrross]

I have just been trying to make the targeted changes with as little modification as possible

This is definitely the right instinct and why we probably shouldn't completely rewrite this :)

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/8628/
• CommitID: 6beb70e
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/8629/
• CommitID: 39c7679
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/8630/
• CommitID: 4ea9c63
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[andrross]

You need to replace every usage of selectedIndices below with excludesAtEndSelectedIndices

[stephen-crawford]

Ooops--clearly I had vacation brain hah.

[andrross]

This should not be here

[stephen-crawford]

Sorry, must have grabbed it from an update pull.

[andrross]

The idea here is that a user can scan this list and quickly determine whether a change might impact them. It should also be as concise as possible, which can obviously be a challenge. At a minimum, it should list what changed from a user's perspective, which in this case I think is the snapshot restore and clone APIs. What do you think of:

"Fix index exclusion behavior in snapshot restore and clone APIs"

Users that don't use those APIs or don't use exclusions can quickly identify this change as not impacting them, otherwise they'll need to dig into the specifics of the linked issue to get more info.

[andrross]

Also, I believe we should backport this change to 2.x, so the changelog entry should go in the "Unreleased 2.x" section.

[stephen-crawford]

Gotcha. I had thought that you had requested that the CHANGELOG entry be swapped to the new description alongside the PR title. My misunderstanding. I agree that the new option is much easier to quickly read over.

I will move it to the Unreleased section as well.

[andrross]

I have just been trying to make the targeted changes with as little modification as possible

This is definitely the right instinct and why we probably shouldn't completely rewrite this :)

[andrross]

@reta @dblock

Just want to raise this issue as I believe we should backport this as a bug fix, though it does change the behavior of the snapshot restore (and clone) APIs.

The upshot here is that, given indexes "foo", "bar", "baz" in a snapshot, and the user provides the pattern "-bar*, ba*" in the restore API, the current behavior results in restoring "foo" and "baz". This is a bug. The correct behavior is to restore only "baz" (select indexes that match "ba*" and exclude those that match "bar*").

There is definitely a chance that some users are (intentionally or not) relying on this buggy behavior and the fix will break them. I think we should backport this as it is clearly wrong. What do you think?

[dblock]

Looks like a bug fix to me, so yes to backport. When users accidentally or purposely use behavior that is a side effect of a bug, it's still a bug, and not a breaking change. I also think we should release note it well.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/8732/
• CommitID: c7d07bd

[stephen-crawford]

@andrross should be all set!

[andrross]

Thanks @scrawfor99, nice work!

[cwperks]

Great work @scrawfor99 and thank you for following this through! 🎸