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.

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

Validate alias operations don't target data streams #58327

Merged
merged 2 commits into from Jun 18, 2020

Conversation

andreidan
Copy link
Contributor

This adds validation to make sure alias operations (add, remove, remove index)
don't target data streams or the backing indices.

Relates to #53100

This adds validation to make sure alias operations (add, remove, remove index)
don't target data streams or the backing indices.
@andreidan andreidan added v8.0.0 :Data Management/Data streams Data streams and their lifecycles v7.9.0 labels Jun 18, 2020
@andreidan andreidan requested a review from martijnvg June 18, 2020 13:51
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Data streams)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Jun 18, 2020
Copy link
Contributor

@danhermann danhermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left some minor comments about error messages.

if (indexAbstraction.getParentDataStream() != null) {
throw new IllegalArgumentException("The provided index [ " + action.getIndex()
+ "] is a backing index belonging to data stream [" + indexAbstraction.getParentDataStream().getName()
+ "]. data streams and their backing indices don't support aliases driven operations.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor error message suggestion -- I think "aliases driven operations" should be either "alias-driven operations" or just "alias operations." I prefer the latter as it's simple and clear.

if (indexAbstraction.getParentDataStream() != null) {
throw new IllegalArgumentException("The provided expressions [" + String.join(",", action.indices())
+ "] match a backing index belonging to data stream [" + indexAbstraction.getParentDataStream().getName()
+ "]. data streams and their backing indices don't support aliases.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: We're inconsistent across the codebase in whether we capitalize our error messages, but I think we should be consistent within a single error message. In other words, either make "The provided..." lowercase or capitalize the second sentence:

Suggested change
+ "]. data streams and their backing indices don't support aliases.");
+ "]. Data streams and their backing indices don't support aliases.");

if (indexAbstraction.getParentDataStream() != null) {
throw new IllegalArgumentException("The provided index [ " + action.getIndex()
+ "] is a backing index belonging to data stream [" + indexAbstraction.getParentDataStream().getName()
+ "]. data streams and their backing indices don't support aliases driven operations.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing about capitalization here.

@danhermann
Copy link
Contributor

danhermann commented Jun 18, 2020

Also, shouldn't this work prevent attempts to assign an alias to a data stream itself and not just its backing indices?

@andreidan
Copy link
Contributor Author

andreidan commented Jun 18, 2020

Also, shouldn't this work prevent attempts to assign an alias to a data stream itself and not just its backing indices?

@danhermann We do fail when we target data streams. I've updated the indexNameExpressionResolver method call to have an explicit false for includingDataStreams flag https://github.com/elastic/elasticsearch/pull/58327/files#diff-44411fbc7bbd2b4d012a6adbc0809228R117
There's also a test for this in DataStreamsIT https://github.com/elastic/elasticsearch/pull/58327/files#diff-2e53f9dfa6f6b9b7953c74807b403c30R437

The MetadataIndexAliasService works on indices already so that's why it validates just for backing indices.

@andreidan andreidan merged commit 8164489 into elastic:master Jun 18, 2020
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Jun 18, 2020
This adds validation to make sure alias operations (add, remove, remove index)
don't target data streams or the backing indices.

(cherry picked from commit 8164489)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit that referenced this pull request Jun 18, 2020
…58337)

This adds validation to make sure alias operations (add, remove, remove index)
don't target data streams or the backing indices.

(cherry picked from commit 8164489)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Data streams Data streams and their lifecycles >enhancement Team:Data Management Meta label for data/management team v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants