Skip to content
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

Convert remote license checker to use LicensedFeature #79876

Merged
merged 20 commits into from
Oct 28, 2021
Merged

Convert remote license checker to use LicensedFeature #79876

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
24dca59
Convert remote license checker to use LicensedFeature
rjernst Oct 26, 2021
6a50271
iter
rjernst Oct 26, 2021
515ba66
cleanup formatting
rjernst Oct 26, 2021
da5da16
checkstyle
rjernst Oct 26, 2021
7545161
fix tests
rjernst Oct 26, 2021
2acd57c
address feedback
rjernst Oct 27, 2021
8912627
Merge branch 'master' into license/remote
rjernst Oct 27, 2021
65c1db5
fix compile
rjernst Oct 27, 2021
52f4659
reapply spotless
rjernst Oct 27, 2021
f1264b4
Merge branch 'master' into license/remote
rjernst Oct 27, 2021
6095dcc
Merge branch 'master' into license/remote
rjernst Oct 27, 2021
c6b716f
Merge branch 'master' into license/remote
rjernst Oct 27, 2021
47798b4
Merge branch 'master' into license/remote
rjernst Oct 27, 2021
5bbf127
address feedback
rjernst Oct 27, 2021
c72827d
tweak
rjernst Oct 27, 2021
076cec9
Merge branch 'master' into license/remote
rjernst Oct 27, 2021
a7d25f4
more iter
rjernst Oct 28, 2021
9240b9d
tweak error message building
rjernst Oct 28, 2021
4f5ff16
Merge branch 'master' into license/remote
rjernst Oct 28, 2021
3c18ee2
spotless
rjernst Oct 28, 2021
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
15 changes: 3 additions & 12 deletions x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.indices.IndexClosedException;
import org.elasticsearch.license.RemoteClusterLicenseChecker;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xpack.ccr.action.ShardChangesAction;
import org.elasticsearch.xpack.core.ClientHelper;
Expand Down Expand Up @@ -242,7 +241,7 @@ private void checkRemoteClusterLicenseAndFetchClusterState(
final Function<Exception, ElasticsearchStatusException> unknownLicense
) {
// we have to check the license on the remote cluster
new RemoteClusterLicenseChecker(client, XPackLicenseState::isCcrAllowedForOperationMode).checkRemoteClusterLicenses(
new RemoteClusterLicenseChecker(client, CcrConstants.CCR_FEATURE).checkRemoteClusterLicenses(
Collections.singletonList(clusterAlias),
new ActionListener<RemoteClusterLicenseChecker.LicenseCheck>() {

Expand Down Expand Up @@ -450,11 +449,7 @@ private static ElasticsearchStatusException indexMetadataNonCompliantRemoteLicen
clusterAlias,
leaderIndex,
clusterAlias,
RemoteClusterLicenseChecker.buildErrorMessage(
"ccr",
licenseCheck.remoteClusterLicenseInfo(),
RemoteClusterLicenseChecker::isAllowedByLicense
)
RemoteClusterLicenseChecker.buildErrorMessage(CcrConstants.CCR_FEATURE, licenseCheck.remoteClusterLicenseInfo())
);
return new ElasticsearchStatusException(message, RestStatus.BAD_REQUEST);
}
Expand All @@ -467,11 +462,7 @@ private static ElasticsearchStatusException clusterStateNonCompliantRemoteLicens
Locale.ROOT,
"can not fetch remote cluster state as the remote cluster [%s] is not licensed for [ccr]; %s",
clusterAlias,
RemoteClusterLicenseChecker.buildErrorMessage(
"ccr",
licenseCheck.remoteClusterLicenseInfo(),
RemoteClusterLicenseChecker::isAllowedByLicense
)
RemoteClusterLicenseChecker.buildErrorMessage(CcrConstants.CCR_FEATURE, licenseCheck.remoteClusterLicenseInfo())
);
return new ElasticsearchStatusException(message, RestStatus.BAD_REQUEST);
}
Expand Down
49 changes: 25 additions & 24 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import static org.elasticsearch.license.XPackLicenseState.isAllowedByOperationMode;

/**
* Checks remote clusters for license compatibility with a specified license predicate.
* Checks remote clusters for license compatibility with a specified licensed feature.
*/
public final class RemoteClusterLicenseChecker {

Expand Down Expand Up @@ -125,23 +126,19 @@ private LicenseCheck(final RemoteClusterLicenseInfo remoteClusterLicenseInfo) {

private static final ClusterNameExpressionResolver clusterNameExpressionResolver = new ClusterNameExpressionResolver();
private final Client client;
private final Predicate<License.OperationMode> predicate;
private final LicensedFeature feature;

/**
* Constructs a remote cluster license checker with the specified license predicate for checking license compatibility. The predicate
* does not need to check for the active license state as this is handled by the remote cluster license checker.
* Constructs a remote cluster license checker with the specified licensed feature for checking license compatibility. The feature
* does not need to check for the active license state as this is handled by the remote cluster license checker. If the feature
* is {@code null} the check always succeeds.
ywangd marked this conversation as resolved.
Show resolved Hide resolved
*
* @param client the client
* @param predicate the license predicate
* @param feature the licensed feature
*/
public RemoteClusterLicenseChecker(final Client client, final Predicate<License.OperationMode> predicate) {
public RemoteClusterLicenseChecker(final Client client, final LicensedFeature feature) {
ywangd marked this conversation as resolved.
Show resolved Hide resolved
this.client = client;
this.predicate = predicate;
}

public static boolean isAllowedByLicense(final XPackInfoResponse.LicenseInfo licenseInfo) {
final License.OperationMode mode = License.OperationMode.parse(licenseInfo.getMode());
return XPackLicenseState.isAllowedByOperationMode(mode, License.OperationMode.PLATINUM);
this.feature = feature;
}

/**
Expand Down Expand Up @@ -169,8 +166,13 @@ public void onResponse(final XPackInfoResponse xPackInfoResponse) {
listener.onFailure(new ResourceNotFoundException("license info is missing for cluster [" + clusterAlias.get() + "]"));
return;
}
if ((licenseInfo.getStatus() == LicenseStatus.ACTIVE) == false
|| predicate.test(License.OperationMode.parse(licenseInfo.getMode())) == false) {

if (((feature == null || feature.isNeedsActive()) && licenseInfo.getStatus() != LicenseStatus.ACTIVE)
|| feature != null
&& isAllowedByOperationMode(
License.OperationMode.parse(licenseInfo.getMode()),
feature.getMinimumOperationMode()
) == false) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we break into smaller and more readable pieces? I had to copy/paste it into an IDE to parse it.

I also think the logic is incorrect when feature !=null. When feature != null, the second half of the condition is triggered and it bypasses the active license check because it is or'd.

Copy link
Member Author

@rjernst rjernst Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The formatting got destroyed by spotless. I will try to restructure, but I think the logic is correct. This entire conditional is a short-circuit, so the first part of the OR is whether the license is active. If it is active (first part evaluates to false), then the second part is checked, which is only necessary when feature != null.

Copy link
Member

Choose a reason for hiding this comment

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

OK I think you are right. The whole thing is a negative check, i.e. it checks for violation instead of conformation. That's what got me confused initially. I think this is also a sign that the logic here is not easy to parse. I'd appreciate any improvement for readability.

listener.onResponse(LicenseCheck.failure(new RemoteClusterLicenseInfo(clusterAlias.get(), licenseInfo)));
return;
}
Expand Down Expand Up @@ -274,22 +276,21 @@ public static List<String> remoteClusterAliases(final Set<String> remoteClusters
* @param remoteClusterLicenseInfo the remote cluster license info of the cluster that failed the license check
* @return an error message representing license incompatibility
*/
public static String buildErrorMessage(
final String feature,
final RemoteClusterLicenseInfo remoteClusterLicenseInfo,
final Predicate<XPackInfoResponse.LicenseInfo> predicate
) {
public static String buildErrorMessage(final LicensedFeature feature, final RemoteClusterLicenseInfo remoteClusterLicenseInfo) {
final StringBuilder error = new StringBuilder();
if (remoteClusterLicenseInfo.licenseInfo().getStatus() != LicenseStatus.ACTIVE) {
if ((feature == null || feature.isNeedsActive()) && remoteClusterLicenseInfo.licenseInfo().getStatus() != LicenseStatus.ACTIVE) {
error.append(String.format(Locale.ROOT, "the license on cluster [%s] is not active", remoteClusterLicenseInfo.clusterAlias()));
} else {
assert predicate.test(remoteClusterLicenseInfo.licenseInfo()) == false : "license must be incompatible to build error message";
} else if (feature != null) {
assert isAllowedByOperationMode(
License.OperationMode.parse(remoteClusterLicenseInfo.licenseInfo().getMode()),
feature.getMinimumOperationMode()
) == false : "license must be incompatible to build error message";
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We no longer have a else block which is for the impossible case of feature == null && licenseStatus == ACTIVE. But should we add an assertion for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove feature != null here. The isAllowed assertion now takes into account the null feature.

final String message = String.format(
Locale.ROOT,
"the license mode [%s] on cluster [%s] does not enable [%s]",
License.OperationMode.parse(remoteClusterLicenseInfo.licenseInfo().getMode()),
remoteClusterLicenseInfo.clusterAlias(),
feature
feature.getName()
);
error.append(message);
}
Expand Down
10 changes: 1 addition & 9 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,19 +440,11 @@ public Map<FeatureUsage, Long> getLastUsed() {
return usage.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> timeConverter.apply(e.getValue())));
}

public static boolean isMachineLearningAllowedForOperationMode(final OperationMode operationMode) {
return isAllowedByOperationMode(operationMode, OperationMode.PLATINUM);
}

public static boolean isFipsAllowedForOperationMode(final OperationMode operationMode) {
return isAllowedByOperationMode(operationMode, OperationMode.PLATINUM);
}

public static boolean isCcrAllowedForOperationMode(final OperationMode operationMode) {
return isAllowedByOperationMode(operationMode, OperationMode.PLATINUM);
}

public static boolean isAllowedByOperationMode(final OperationMode operationMode, final OperationMode minimumMode) {
static boolean isAllowedByOperationMode(final OperationMode operationMode, final OperationMode minimumMode) {
if (OperationMode.TRIAL == operationMode) {
return true;
}
Expand Down
50 changes: 19 additions & 31 deletions ...plugin/core/src/test/java/org/elasticsearch/license/RemoteClusterLicenseCheckerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,8 @@ public void testCheckRemoteClusterLicensesGivenCompatibleLicenses() {
responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null));
responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null));

final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(
client,
operationMode -> XPackLicenseState.isAllowedByOperationMode(operationMode, License.OperationMode.PLATINUM)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(client, feature);
final AtomicReference<RemoteClusterLicenseChecker.LicenseCheck> licenseCheck = new AtomicReference<>();

licenseChecker.checkRemoteClusterLicenses(
Expand Down Expand Up @@ -202,10 +200,8 @@ public void testCheckRemoteClusterLicensesGivenIncompatibleLicense() {
return null;
}).when(client).execute(same(XPackInfoAction.INSTANCE), any(), any());

final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(
client,
operationMode -> XPackLicenseState.isAllowedByOperationMode(operationMode, License.OperationMode.PLATINUM)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(client, feature);
final AtomicReference<RemoteClusterLicenseChecker.LicenseCheck> licenseCheck = new AtomicReference<>();

licenseChecker.checkRemoteClusterLicenses(
Expand Down Expand Up @@ -251,10 +247,8 @@ public void testCheckRemoteClusterLicencesGivenNonExistentCluster() {
responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null));
responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null));

final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(
client,
operationMode -> XPackLicenseState.isAllowedByOperationMode(operationMode, License.OperationMode.PLATINUM)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(client, feature);
final AtomicReference<Exception> exception = new AtomicReference<>();

licenseChecker.checkRemoteClusterLicenses(
Expand Down Expand Up @@ -294,10 +288,8 @@ public void testRemoteClusterLicenseCallUsesSystemContext() throws InterruptedEx
return null;
}).when(client).execute(same(XPackInfoAction.INSTANCE), any(), any());

final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(
client,
operationMode -> XPackLicenseState.isAllowedByOperationMode(operationMode, License.OperationMode.PLATINUM)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(client, feature);

final List<String> remoteClusterAliases = Collections.singletonList("valid");
licenseChecker.checkRemoteClusterLicenses(
Expand Down Expand Up @@ -337,10 +329,8 @@ public void testListenerIsExecutedWithCallingContext() throws InterruptedExcepti
responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null));
responses.add(new XPackInfoResponse(null, createPlatinumLicenseResponse(), null));

final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(
client,
operationMode -> XPackLicenseState.isAllowedByOperationMode(operationMode, License.OperationMode.PLATINUM)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(client, feature);

final AtomicBoolean listenerInvoked = new AtomicBoolean();
threadPool.getThreadContext().putHeader("key", "value");
Expand Down Expand Up @@ -383,10 +373,8 @@ public void testBuildErrorMessageForActiveCompatibleLicense() {
"platinum-cluster",
platinumLicence
);
final AssertionError e = expectThrows(
AssertionError.class,
() -> RemoteClusterLicenseChecker.buildErrorMessage("", info, RemoteClusterLicenseChecker::isAllowedByLicense)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "foo", License.OperationMode.PLATINUM);
final AssertionError e = expectThrows(AssertionError.class, () -> RemoteClusterLicenseChecker.buildErrorMessage(feature, info));
assertThat(e, hasToString(containsString("license must be incompatible to build error message")));
}

Expand All @@ -396,9 +384,10 @@ public void testBuildErrorMessageForIncompatibleLicense() {
"basic-cluster",
basicLicense
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
assertThat(
RemoteClusterLicenseChecker.buildErrorMessage("Feature", info, RemoteClusterLicenseChecker::isAllowedByLicense),
equalTo("the license mode [BASIC] on cluster [basic-cluster] does not enable [Feature]")
RemoteClusterLicenseChecker.buildErrorMessage(feature, info),
equalTo("the license mode [BASIC] on cluster [basic-cluster] does not enable [feature]")
);
}

Expand All @@ -408,8 +397,9 @@ public void testBuildErrorMessageForInactiveLicense() {
"expired-cluster",
expiredLicense
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "foo", License.OperationMode.PLATINUM);
assertThat(
RemoteClusterLicenseChecker.buildErrorMessage("Feature", info, RemoteClusterLicenseChecker::isAllowedByLicense),
RemoteClusterLicenseChecker.buildErrorMessage(feature, info),
equalTo("the license on cluster [expired-cluster] is not active")
);
}
Expand All @@ -424,10 +414,8 @@ public void testCheckRemoteClusterLicencesNoLicenseMetadata() {
return null;
}).when(client).execute(same(XPackInfoAction.INSTANCE), any(), any());

final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(
client,
operationMode -> XPackLicenseState.isAllowedByOperationMode(operationMode, License.OperationMode.PLATINUM)
);
LicensedFeature.Momentary feature = LicensedFeature.momentary(null, "feature", License.OperationMode.PLATINUM);
final RemoteClusterLicenseChecker licenseChecker = new RemoteClusterLicenseChecker(client, feature);
final AtomicReference<Exception> exception = new AtomicReference<>();

licenseChecker.checkRemoteClusterLicenses(
Expand Down
Loading