Refactor Security code to use TransportVersion #93089
Conversation
Add initial set of javadocs
Add initial set of javadocs
TransportVersion tests
| @@ -87,7 +87,7 @@ public Map<String, Object> getMetadata() { | |||
| return metadata; | |||
| } | |||
|
|
|||
| public Version getVersion() { | |||
| public TransportVersion getTransportVersion() { | |||
There was a problem hiding this comment.
In other places, when replacing existing version methods, I've just left it as getVersion
There was a problem hiding this comment.
I tended to replace the method name too. I feel it might read better
| } | ||
|
|
||
| public Subject(User user, Authentication.RealmRef realm, Version version, Map<String, Object> metadata) { | ||
| public Subject(User user, Authentication.RealmRef realm, TransportVersion version, Map<String, Object> metadata) { |
There was a problem hiding this comment.
What does this version number actually mean in the security code?
There was a problem hiding this comment.
judging by when this is called I would suspect that this has the same meaning as a transport version in StreamInput
https://github.com/pgomulka/elasticsearch/blob/1db5baa41b5c933ead5cba9a848b9aa95e749546/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java#L155
once this PR is ready for review I will follow up with security team
There was a problem hiding this comment.
as discussed ofline: Version in Subject and UserToken is used for serialisation into a Base64 strings. It does not use transport protocol (serialisation is into ThreadContext or XContent) but is using StreamInput/StreamOutput api
| @@ -110,7 +110,7 @@ public String getId() { | |||
| /** | |||
| * The version of the node this token was created on | |||
| */ | |||
| Version getVersion() { | |||
| TransportVersion getTransportVersion() { | |||
There was a problem hiding this comment.
From the comment, this is the actual node version. Do we want to change this to TransportVersion?
There was a problem hiding this comment.
I updated the comment. I think this should be a transportVersion as per https://github.com/elastic/elasticsearch/pull/93089/files#r1087901056
pgomulka
changed the title
pgomulka
added
:Core/Infra/Core
|
@elastic/es-security would you be able to take a look at the PR? Similar question to |
elasticsearchmachine
added
Team:Core/Infra
|
Pinging @elastic/es-security (Team:Security) |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
With the introduction of TransportVersion we want to use this object in transport protocol related code (usages of StreamInput, StreamOutput).
Also with serverless release being more frequent then on-prem the code changes that relied on Node's Version to perform a conditional logic are also refactored to use TransportVersion which has higher granularity