Skip to content

Remove @UpdateForV9 annotations from Security code #123176

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Mar 12, 2025
Merged

Remove @UpdateForV9 annotations from Security code #123176

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c18b3ab
Remove `@UpdateForV9` annotation on code that has been deprecated and…
mattc58 Feb 21, 2025
94d6062
[CI] Auto commit changes from spotless
Feb 21, 2025
1f236f1
Merge remote-tracking branch 'upstream/main' into remove-security-upd…
mattc58 Feb 24, 2025
a402f2b
Change UpdateForV9 to UpdateForV10 for License `hide_enterprise`
mattc58 Feb 24, 2025
7edaedc
Merge remote-tracking branch 'origin/remove-security-updateforv9' int…
mattc58 Feb 24, 2025
e206c2a
Merge branch 'main' into remove-security-updateforv9
jakelandis Mar 11, 2025
32682ba
kick can down the road for accept_enterprise
jakelandis Mar 11, 2025
f3b7c6d
adjust rest api compat for accept_enterprise
jakelandis Mar 11, 2025
a3336f2
adjust to no longer rely on rest api compatiblity
jakelandis Mar 11, 2025
2012ab2
full circle back to where I started
jakelandis Mar 11, 2025
dc697d4
remove unecessary code
jakelandis Mar 12, 2025
e756b91
Merge branch 'main' into remove-security-updateforv9
mattc58 Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/License.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.core.UpdateForV10;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
Expand Down Expand Up @@ -142,7 +142,7 @@ static boolean isEnterprise(String typeName) {
* XContent param name to map the "enterprise" license type to "platinum"
* for backwards compatibility with older clients
*/
@UpdateForV9(owner = UpdateForV9.Owner.SECURITY) // v7 REST API no longer exists: eliminate ref to RestApiVersion.V_7
@UpdateForV10(owner = UpdateForV10.Owner.SECURITY) // v7 REST API no longer exists: eliminate ref to RestApiVersion.V_7
Copy link
Contributor

@jakelandis jakelandis Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that is as simple as it appears since this a marker for some partially removed functionality:

This is related to use and deprecation of "accept_enterprise" for license. This specific string isn't the primary reason for the annotation, rather it a marker for the related code which has been partially removed.

Specifically it is part of the change code via 8.x branch: https://github.com/elastic/elasticsearch/blob/8.x/x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestGetLicenseAction.java#L82

It also says that if you send "accept_enterprise=false" on or after REST API v8, then raise an exception. To put another way it says it if you send "accept_enterprise=false" before REST API v8 by sending the v7 compatibility header, then don't error.

Since we removed support for v7 compatibility that logic was changed via https://github.com/elastic/elasticsearch/pull/114881/files#diff-f0af8e6057207cbe3628cfbc9bcab24d2d38ad1c39bdb34efb92262dbd3eb8e4L75.

It also appears there was additional related stuff to the v7 REST API compatibility that was removed s
https://github.com/elastic/elasticsearch/blob/8.x/x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestGetLicenseAction.java#L66-L72 (this part is relevant to this exact string)

So it appears that some of this functionality is already partially removed with the expectation it will be fully removed. Any gates that changed behavior based on the v7 REST API have been removed. I am not familiar enough with the functionality or original change to comment on exactly what functionality is gated by v7 REST API compatibility vs. breaking change vs. the expectations in the current state.

It now says if you send "accept_enterprise=false" for ANY REST API version, then raise an exception. I think this is OK and desired since it still consumes the parameter (avoiding an exception) and will continue emit a warning as it did in v8. However, since there are other parts that have been partially removed I am not sure what happens if you send "accept_enterprise=true". If that doesn't work or it errors, then maybe we are better off just removing fully removing support for accept_enterprise (which is a bigger change). If we leave it as-is (this PR), then I think should at least ensure no error and correct results for "accept_enterprise=true". Else, we should restore the relevant code from 8.x and bump REST API versions V_7 to V_8 ; and V_8 to V_9 so it behave identically to 8.x.

Copy link
Contributor

@jakelandis jakelandis Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added back all the original code that we removed via different PRs here: 32682ba , then adjusted the functionality per REST API version needs : a3336f2, simplified the code 2012ab2 ... and ended up right back where @mattc58 left off and I started 🪃 . The only real change since @mattc58's version was some minor clean up of dead code paths: dc697d4

public static final String XCONTENT_HIDE_ENTERPRISE = "hide_enterprise";

public static final Comparator<License> LATEST_ISSUE_DATE_FIRST = Comparator.comparing(License::issueDate).reversed();
Expand Down
8 changes: 1 addition & 7 deletions ...java/org/elasticsearch/xpack/core/security/action/settings/GetSecuritySettingsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;

Expand All @@ -40,13 +39,8 @@ public Request(TimeValue masterNodeTimeout) {
super(masterNodeTimeout);
}

@UpdateForV9(owner = UpdateForV9.Owner.SECURITY) // no need for bwc any more, this can be inlined
public static Request readFrom(StreamInput in) throws IOException {
if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_15_0)) {
return new Request(in);
} else {
return new Request(TimeValue.THIRTY_SECONDS);
}
return new Request(in);
}

private Request(StreamInput in) throws IOException {
Expand Down
8 changes: 1 addition & 7 deletions ...a/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.xcontent.ConstructingObjectParser;
import org.elasticsearch.xcontent.ParseField;
import org.elasticsearch.xcontent.XContentParser;
Expand Down Expand Up @@ -116,13 +115,8 @@ public Request(
this.profilesIndexSettings = Objects.requireNonNullElse(profilesIndexSettings, Collections.emptyMap());
}

@UpdateForV9(owner = UpdateForV9.Owner.SECURITY) // no need for bwc any more, this can be inlined
public static Request readFrom(StreamInput in) throws IOException {
if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_15_0)) {
return new Request(in);
} else {
return new Request(TimeValue.THIRTY_SECONDS, TimeValue.THIRTY_SECONDS, in);
}
return new Request(in);
}

private Request(StreamInput in) throws IOException {
Expand Down