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 all 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
elasticsearchmachine 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
3 changes: 1 addition & 2 deletions libs/core/src/main/java/org/elasticsearch/core/UpdateForV9.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ enum Owner {
ENTERPRISE_SEARCH,
MACHINE_LEARNING,
PROFILING,
SEARCH_ANALYTICS,
SECURITY,
SEARCH_ANALYTICS
}

/**
Expand Down
15 changes: 1 addition & 14 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,6 @@
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.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
Expand Down Expand Up @@ -137,13 +136,6 @@ static boolean isEnterprise(String typeName) {
* to a specific license version
*/
public static final String LICENSE_VERSION_MODE = "license_version";
/**
* Set for RestApiVersion#V_7 requests only
* 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
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 Expand Up @@ -497,8 +489,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) throws IOException {
boolean licenseSpecMode = params.paramAsBoolean(LICENSE_SPEC_VIEW_MODE, false);
boolean restViewMode = params.paramAsBoolean(REST_VIEW_MODE, false);
boolean hideEnterprise = params.paramAsBoolean(XCONTENT_HIDE_ENTERPRISE, false);

boolean previouslyHumanReadable = builder.humanReadable();
Copy link
Contributor

Choose a reason for hiding this comment

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

The only way this could have ever been true is via REST API compatibility with v7: https://github.com/elastic/elasticsearch/blob/8.x/x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestGetLicenseAction.java#L66-L72 . 9.x does not support REST API compatibility with v7, so this value will always be false. (If you used v7 compatibility, you could set the accept_enterprise parameter to false which resulted in XCONTENT_HIDE_ENTERPRISE being true, now that v7 compatibility is gone there is no way to set accept_enterprise to false (via other validation) , so XCONTENT_HIDE_ENTERPRISE is always false) This change is adjusting the code for hideEnterprise is always false.

if (licenseSpecMode && restViewMode) {
throw new IllegalArgumentException("can have either " + REST_VIEW_MODE + " or " + LICENSE_SPEC_VIEW_MODE);
Expand All @@ -517,8 +507,7 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t
builder.field(Fields.STATUS, LicenseUtils.status(this).label());
}
builder.field(Fields.UID, uid);
final String bwcType = hideEnterprise && LicenseType.isEnterprise(type) ? LicenseType.PLATINUM.getTypeName() : type;
builder.field(Fields.TYPE, bwcType);
builder.field(Fields.TYPE, type);
if (licenseVersion == VERSION_START) {
builder.field(Fields.SUBSCRIPTION_TYPE, subscriptionType);
}
Expand All @@ -534,8 +523,6 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t
if (licenseVersion >= VERSION_ENTERPRISE) {
builder.field(Fields.MAX_NODES, maxNodes == -1 ? null : maxNodes);
builder.field(Fields.MAX_RESOURCE_UNITS, maxResourceUnits == -1 ? null : maxResourceUnits);
} else if (hideEnterprise && maxNodes == -1) {
builder.field(Fields.MAX_NODES, maxResourceUnits);
} else {
builder.field(Fields.MAX_NODES, maxNodes);
}
Expand Down
9 changes: 4 additions & 5 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestGetLicenseAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.core.UpdateForV10;
import org.elasticsearch.protocol.xpack.license.GetLicenseRequest;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.RestRequest;
Expand Down Expand Up @@ -55,22 +55,21 @@ public String getName() {
* The licenses are sorted by latest issue_date
*/
@Override
@UpdateForV9(owner = UpdateForV9.Owner.SECURITY) // remove support for accept_enterprise param
@UpdateForV10(owner = UpdateForV10.Owner.SECURITY) // remove support for accept_enterprise param
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
final Map<String, String> overrideParams = Maps.newMapWithExpectedSize(2);
overrideParams.put(License.REST_VIEW_MODE, "true");
overrideParams.put(License.LICENSE_VERSION_MODE, String.valueOf(License.VERSION_CURRENT));

// In 7.x, there was an opt-in flag to show "enterprise" licenses. In 8.0 the flag is deprecated and can only be true
// TODO Remove this from 9.0
// In 7.x, there was an opt-in flag to show "enterprise" licenses. In 8.0+ the flag is deprecated and can only be true
if (request.hasParam("accept_enterprise")) {
deprecationLogger.warn(
DeprecationCategory.API,
"get_license_accept_enterprise",
"Including [accept_enterprise] in get license requests is deprecated."
+ " The parameter will be removed in the next major version"
);
if (request.paramAsBoolean("accept_enterprise", true) == false) {
if (request.paramAsBoolean("accept_enterprise", true) == false) { // consumes the parameter to avoid error
Copy link
Contributor

Choose a reason for hiding this comment

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

no actual change here (just updated comment). However, it is worth noting that this is BWC to leave the code as-is.

8.x has this same behavior, and the gating by REST API version (removed in a different PR) was not necessary:
https://github.com/elastic/elasticsearch/blob/8.x/x-pack/plugin/core/src/main/java/org/elasticsearch/license/RestGetLicenseAction.java#L82-L84

Removing support for accept_enterprise = true is technically a breaking change, so it is not fully removed here. (accept_enterprise = false was only supported by v7 compatibility) )The behavior behind this only worked for v7 compatibility, so this flag does nothing. We read it here to keep from throwning exception for unkonwn parameter. Arguably we should have removed this v9, but given timing, other priorities, and low impact, this didn't happen. We should finally remove this for v10 and doesn't actually do anything in v9 (nor did it v8 unless you sent v7 compatibility).

throw new IllegalArgumentException("The [accept_enterprise] parameters may not be false");
}
}
Expand Down
7 changes: 3 additions & 4 deletions ...ugin/core/src/main/java/org/elasticsearch/xpack/core/rest/action/RestXPackInfoAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import org.elasticsearch.client.internal.node.NodeClient;
import org.elasticsearch.common.logging.DeprecationCategory;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.core.UpdateForV10;
import org.elasticsearch.protocol.xpack.XPackInfoRequest;
import org.elasticsearch.rest.BaseRestHandler;
import org.elasticsearch.rest.RestRequest;
Expand Down Expand Up @@ -41,22 +41,21 @@ public String getName() {
}

@Override
@UpdateForV9(owner = UpdateForV9.Owner.SECURITY) // accept_enterprise parameter no longer supported?
@UpdateForV10(owner = UpdateForV10.Owner.SECURITY) // remove once accept_enterprise parameter no longer supported
public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {

// we piggyback verbosity on "human" output
boolean verbose = request.paramAsBoolean("human", true);

// In 7.x, there was an opt-in flag to show "enterprise" licenses. In 8.0 the flag is deprecated and can only be true
// TODO Remove this from 9.0
if (request.hasParam("accept_enterprise")) {
deprecationLogger.warn(
DeprecationCategory.API,
"get_license_accept_enterprise",
"Including [accept_enterprise] in get license requests is deprecated."
+ " The parameter will be removed in the next major version"
);
if (request.paramAsBoolean("accept_enterprise", true) == false) {
if (request.paramAsBoolean("accept_enterprise", true) == false) { // consumes the parameter to avoid error
throw new IllegalArgumentException("The [accept_enterprise] parameters may not be false");
}
}
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