Fix transport serialization of AsyncSearchUser #54761
Conversation
This change ensures that the AsyncSearchUser is correctly (de)serialized when an action executed by this user is sent to a remote node internally (via transport client).
jimczi
added
>non-issue
:Security/Authorization
|
Pinging @elastic/es-security (:Security/Authorization) |
|
@elasticmachine run elasticsearch-ci/2 |
| @@ -36,6 +38,9 @@ public static void writeTo(User user, StreamOutput output) throws IOException { | |||
| } else if (XPackSecurityUser.is(user)) { | |||
| output.writeBoolean(true); | |||
| output.writeString(XPackSecurityUser.NAME); | |||
| } else if (AsyncSearchUser.is(user)) { | |||
There was a problem hiding this comment.
Is this a concern for bwc? I think, in a mixed cluster environment, writing and reading this information from different versions would fail. But current failures don't seem to be releavant. I don't know the details about async search work. Is it 8.0 only?
There was a problem hiding this comment.
It's a 7.7 feature so nodes in this version will be able to run _async_search and to create the new .async-search index. For the few cases where an older node could be picked, the IllegalStateException thrown by readFrom should be enough to handle failures gracefully ?
There was a problem hiding this comment.
This is indeed a problem, but I don't see an easy way around it.
With the proposed changes, async search won't work until the master node is also upgraded, even if the coordinating node is. Does this sound reasonable to you @jimczi ? Are there other issues that would prevent the async search feature to work in a mixed cluster (assuming no, but maybe there is :) )?
There was a problem hiding this comment.
That's fine IMO. We expect users to upgrade clusters entirely before using new features . Also, Kibana will use _async_search internally so the ES cluster must be fully upgraded before upgrading Kibana.
|
We have a dedicated tests for security with async search but it is currently setup to run with a single node (default value for rest tests). This is the reason why this issue was not caught until now. We'll update the rest tests to force at least 2 nodes in a follow up. |
|
I should've thought about authz for the new async_search origin in a mixed cluster. I'm sorry I missed it. |
|
I pushed one commit to setup the async security rest test with 2 nodes: 82f9222 |
This change ensures that the AsyncSearchUser is correctly (de)serialized when an action executed by this user is sent to a remote node internally (via transport client).
This change ensures that the AsyncSearchUser is correctly (de)serialized when an action executed by this user is sent to a remote node internally (via transport client).
This change ensures that the AsyncSearchUser is correctly (de)serialized when
an action executed by this user is sent to a remote node internally (via transport client).