Move destructive operations check to TransportAction base class

[javanna]

Open index, close index and delete index can or cannot be executed against wildcard expressions depending on the value of the action.destructive_requires_name setting. This check used to be performed in each action separately. There were actually ways to bypass this check though, hence it should be performed earlier, in the TransportAction base class.

The main advantage of this change is that the indices are checked earlier, before any action filter gets executed.

This PR is huge, only because it adds the DestructiveOperations argument to the TransportAction base class, hence to all of its subclasses. The interesting part is in DestructiveOperations and TransportAction.

I also added some tests and rewrote the existing DestructiveOperationsIT (we can now remove cluster settings so that test deserved some love).

[rjernst]

I don't think this should be part of the TransportAction base class ctor. The arg list for the base class is already huge and a major part of why it is so difficult to deguice actions. Instead, could this be done as a flag on actions (indicating whether they are destructive, set by overriding a method returning boolean) and handled at a higher level where actions are called from the TransportService?

[rjernst]

I also don't think just leaving it where it is now (and fixing the bug) is an issue. With this change the fact that these operations like close index are destructive is now detached from the action itself (not actually in the close index action). Bugs are bugs, but we shouldn't complicate the entire action api for this edge case (we have very few destructive actions).

[javanna]

I don't think this should be part of the TransportAction base class ctor. The arg list for the base class is already huge and a major part of why it is so difficult to deguice actions. Instead, could this be done as a flag on actions (indicating whether they are destructive, set by overriding a method returning boolean) and handled at a higher level where actions are called from the TransportService?

I tend to agree that this change is too invasive. I was looking for alternatives too but I am not sure that handling it at the transport service level is the way to go, as that would imply checking only for things that come in through transport. How about actions that come in through REST? They use a client instance which calls the transport action locally, so the check would have to happen although the entry-point is not transport service. I thought about doing it via a special action filter which is always executed first and wasn't sure about it. That would be ok though maybe?

[rjernst]

I thought about doing it via a special action filter which is always executed first and wasn't sure about it.

I think this is also overkill. I would just keep the calls to DestructiveOperations in the actions themselves (and then calling from rest will still be covered). None of this will change the fact that we have to actively choose to make something a destructive operation (whether it be with the list in the this PR, or by adding an explicit call like is currently done).

[javanna]

With this change the fact that these operations like close index are destructive is now detached from the action itself

That is one of the goals of this PR though :) I would like to have a central point that tells whether an action is destructive or not based on its name. I would want to move that decision away from each specific action class, as that is something that can be easily bypassed.

[rjernst]

I would want to move that decision away from each specific action class, as that is something that can be easily bypassed.

What do you mean "bypassed"? I think you mean overlooked? Having a hardcoded list would mean no actions added eg by plugins could be considered destructive.

[javanna]

What do you mean "bypassed"? I think you mean overlooked? Having a hardcoded list would mean no actions added eg by plugins could be considered destructive.

No I mean bypassed, circumvented. I don't think we want to allow plugins to add their own destructive operations. We rather want to guarantee that the check for our own destructive actions works in every scenario, even when custom action filters are plugged in.

[s1monw]

I think we have many different options other than adding it to the ctor list. We can for instance:

• add the DestructiveOperations check as a ActionFilter
• add the DestructiveOperations check to the task manager

I kind of like the action filter way a lot since it means we would always exercise that codepath and can make it mandatory?

[javanna]

Agreed I will move this to an action filter then, will open a new PR soon.