Relax data stream alias validation logic when removing an alias #74403
Conversation
…dices. When removing aliases allow that an alias action's index expression can match both data streams and regular indices. This allows api calls like `DELETE /_all/_alias/my-alias`, which can common in tear down / cleanup logic. In this case `my-alias` just points to regular indices, but `_all` can be expanded to data streams too if exist. This can then trigger validation logic that prevents adding aliases that refer to both indices and data streams. However this api call never adds any alias, only removes it. So failing with this validation error doesn't make much sense. This change adjusts the validation logic so that: 'match with both data streams and regular indices are disallowed' validation is only executed for alias actions that add aliases. Relates to elastic#66163
martijnvg
added
>non-issue
v8.0.0
:Data Management/Data streams
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-core-features (Team:Core/Features) |
danhermann
left a comment
danhermann
left a comment
There was a problem hiding this comment.
The main change here to allow deletion of both index and DS alias in the same call LGTM. I had two questions about the implementation that are not blocking.
| } | ||
| } | ||
| break; | ||
| continue; |
There was a problem hiding this comment.
Does the continue here do something different than the break would have done?
There was a problem hiding this comment.
In this case data stream aliases are added, if break were to be used here, then this transport action could attempt adding indices aliases with the same name (which would then fail). A few lines below, at the end of the if statement a continue statement used to do what this continue statement is doing.
| if (nonBackingIndices.isEmpty() == false) { | ||
| // Regular aliases/indices match as well with the provided expression. | ||
| // (Only when adding new aliases, matching both data streams and indices is disallowed) | ||
| break; | ||
| } else { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I didn't follow the logic here. Do the break and continue result in different outcomes depending on the value of nonBackingIndices.isEmpty()?
There was a problem hiding this comment.
Yes. The noneBackingIndices set contains the names of regular indices that match with this alias action. In that case, this transport action should attempt to remove indices aliases.
If the set is empty then the logic should stop here, because otherwise resolving to concrete indices that happens a bit later would fail. (since no regular indices would be resolved)
|
Thanks for the explanation. I see how it works now. I think it's a somewhat complicated interaction between the outer for loop, an inner switch statement for data stream aliases, and the code block after that for index aliases though it's not immediately clear to me that it could easily be simplified. |
Yes, it is more complicated now. I think in a follow-up, I will start making this code unit testable and try to break this big method down in simpler units. |
…dices. (elastic#74403) When removing aliases allow that an alias action's index expression can match both data streams and regular indices. This allows api calls like `DELETE /_all/_alias/my-alias`, which can common in tear down / cleanup logic. In this case `my-alias` just points to regular indices, but `_all` can be expanded to data streams too if exist. This can then trigger validation logic that prevents adding aliases that refer to both indices and data streams. However this api call never adds any alias, only removes it. So failing with this validation error doesn't make much sense. This change adjusts the validation logic so that: 'match with both data streams and regular indices are disallowed' validation is only executed for alias actions that add aliases. Relates to elastic#66163
…dices. (#74543) Backporting #74403 to 7.x branch. When removing aliases allow that an alias action's index expression can match both data streams and regular indices. This allows api calls like `DELETE /_all/_alias/my-alias`, which can common in tear down / cleanup logic. In this case `my-alias` just points to regular indices, but `_all` can be expanded to data streams too if exist. This can then trigger validation logic that prevents adding aliases that refer to both indices and data streams. However this api call never adds any alias, only removes it. So failing with this validation error doesn't make much sense. This change adjusts the validation logic so that: 'match with both data streams and regular indices are disallowed' validation is only executed for alias actions that add aliases. Relates to #66163
4 participants
When removing aliases allow that an alias action's index expression can match
both data streams and regular indices.
This allows api calls like
DELETE /_all/_alias/my-alias, which can be commonin tear down / cleanup logic. In this case
my-aliasjust points to regularindices, but
_allcan be expanded to data streams too if exist. This canthen trigger validation logic that prevents adding aliases that refer to both
indices and data streams. However this api call never adds any alias, only
removes it. So failing with this validation error doesn't make much sense.
This change adjusts the validation logic so that: 'match with both data streams and
regular indices are disallowed' validation is only executed for alias actions
that add aliases.
Additionally with the change, a single alias remove action that spans across
indices and data stream aliases is allowed. For example
DELETE /_all/_alias/*will be allowed with this PR, which removes all aliases.
Relates to #66163