-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Reapply "Remove transport version V_8_17_0 (#136311)" (#136500) #139144
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
base: main
Are you sure you want to change the base?
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| + "and generateTransportVersion gradle task", | ||
| TransportVersions.DEFINED_VERSIONS.getLast().id(), | ||
| equalTo(8_797_0_05) | ||
| equalTo(8_772_0_06) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this test goes away entirely when we ditch TransportVersions.java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I considered ditching this already, but didn't want any additional accidents in the interim.
|
|
||
| public static final TransportVersion ROLE_REMOTE_CLUSTER_PRIVS = TransportVersions.V_8_15_0; | ||
| public static final TransportVersion ROLE_MONITOR_STATS = TransportVersions.V_8_17_0; | ||
| public static final TransportVersion ROLE_MONITOR_STATS = TransportVersion.fromId(8797002); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to keep this. So far as I can tell, we don't use binary serialization to persist this as part of Authentication. The only usage I see there actually serializes this as xcontent, so I think we can ditch the transport backcompat here. Perhaps someone in @elastic/es-security can confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I'm definitely exercising more caution removing anything security related at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct thing to do here is to update these to minimumCompatible transport version and remove tests that no longer make sense, but I would like security to confirm that as well.
This reverts commit ef42c70.