Convert remote license checker to use LicensedFeature #79876

rjernst · 2021-10-26T21:51:08Z

The RemoteClusterLicenseChecker pulls the license level of a remote
cluster and checks it allows a local feature to communicate with that
cluster. It does the check with a lambda, but these methods could be out
of sync with the actual licensed feature. This commit converts the
remote license checker to take in the feature object that should be
checked.

The RemoteClusterLicenseChecker pulls the license level of a remote cluster and checks it allows a local feature to communicate with that cluster. It does the check with a lambda, but these methods could be out of sync with the actual licensed feature. This commit converts the remote license checker to take in the feature object that should be checked.

elasticmachine · 2021-10-26T21:51:12Z

Pinging @elastic/es-security (Team:Security)

ywangd

I have a question for whether null is equivalent to "alway-true" license checker.

ywangd · 2021-10-26T23:50:18Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

     * 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.


Suggested change

* 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.

ywangd · 2021-10-26T23:59:31Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

+                if (licenseInfo.getStatus() == LicenseStatus.ACTIVE == false
+                    || isAllowedByOperationMode(License.OperationMode.parse(licenseInfo.getMode()),
+                                                feature.getMinimumOperationMode()) == false) {


It's not a real issue but I feel a bit itchy that we don't check whether the feature actual needs an Active license here. It does and requiring active license is a norm going forward. But for the sake of clarity in this particular scope, I'd have something like

Suggested change

if (licenseInfo.getStatus() == LicenseStatus.ACTIVE == false

|| isAllowedByOperationMode(License.OperationMode.parse(licenseInfo.getMode()),

feature.getMinimumOperationMode()) == false) {

if ((feature.isNeedsActive() && licenseInfo.getStatus() == LicenseStatus.ACTIVE == false)

|| isAllowedByOperationMode(License.OperationMode.parse(licenseInfo.getMode()),

feature.getMinimumOperationMode()) == false) {

ywangd · 2021-10-27T00:05:14Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

        if (remoteClusterLicenseInfo.licenseInfo().getStatus() != LicenseStatus.ACTIVE) {
            error.append(String.format(Locale.ROOT, "the license on cluster [%s] is not active", remoteClusterLicenseInfo.clusterAlias()));


Similarly, this assumes the feature requires an active license, which is true. But the assumption is somewhat disconnected.

I've adjusted this so that if the feature is present, it checks the isNeedsActive(). It also checks if the feature is not present, to match the current behavior where an active license is still needed for the BASIC functionality.

ywangd · 2021-10-27T00:15:06Z

.../src/main/java/org/elasticsearch/xpack/transform/action/TransportPreviewTransformAction.java

-            DiscoveryNode.isRemoteClusterClient(settings)
-                /* transforms are BASIC so always allowed, no need to check license */
-                ? new RemoteClusterLicenseChecker(client, mode -> true) : null,
+            /* transforms are BASIC so always allowed, no need to check license */
+            null,


I am familiar with code for the transform feature. But it seems a null license checker would disable CCS according to the following comment:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/validation/SourceDestValidator.java

Line 267 in e82a70c

* @param remoteClusterLicenseChecker A RemoteClusterLicenseChecker or null if CCS is disabled

So there is a difference between null and a license checker that always return true?

Good catch, I misunderstood what null meant here. I've changed the remote checker so that null can be passed into the ctor to mean the checker is a noop.

ywangd · 2021-10-27T00:15:24Z

...src/main/java/org/elasticsearch/xpack/transform/action/TransportValidateTransformAction.java

-            DiscoveryNode.isRemoteClusterClient(settings)
-                /* transforms are BASIC so always allowed, no need to check license */
-                ? new RemoteClusterLicenseChecker(client, mode -> true) : null,
+            /* transforms are BASIC so always allowed, no need to check license */
+            null,


Same as above

rjernst · 2021-10-27T21:15:35Z

@ywangd This is read for another review.

ywangd

I left a few more suggestions. Thanks!

ywangd · 2021-10-27T23:21:17Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

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


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.

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.

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.

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

ywangd · 2021-10-27T23:27:34Z

.../src/main/java/org/elasticsearch/xpack/transform/action/TransportPreviewTransformAction.java

-                ? new RemoteClusterLicenseChecker(client, mode -> true)
+                ? new RemoteClusterLicenseChecker(client, null)


Exposing a null feature to the caller does not seem to be very intuitive. I think a more descriptive approach is to have a static method that returns an instance of license checker, e.g. something like

RemoteClusterLicenseChecker.activeLicenseChecker(client)

Given there are only 2 callers with null, I see this as a special case not warranting another method. I'm inclined to leave this for now.

rjernst · 2021-10-28T00:06:33Z

@ywangd I pushed an update which makes the logic much simpler to understand.

ywangd

LGTM

Thanks for the iterations!

ywangd · 2021-10-28T00:11:00Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

+        } else if (feature != null) {
+            assert isAllowedByOperationMode(
+                License.OperationMode.parse(remoteClusterLicenseInfo.licenseInfo().getMode()),
+                feature.getMinimumOperationMode()
+            ) == false : "license must be incompatible to build error message";


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?

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

The RemoteClusterLicenseChecker pulls the license level of a remote cluster and checks it allows a local feature to communicate with that cluster. It does the check with a lambda, but these methods could be out of sync with the actual licensed feature. This commit converts the remote license checker to take in the feature object that should be checked.

elasticsearchmachine · 2021-10-28T01:33:50Z

💔 Backport failed

Status	Branch	Result
✅	8.0
❌	7.16	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 79876

The RemoteClusterLicenseChecker pulls the license level of a remote cluster and checks it allows a local feature to communicate with that cluster. It does the check with a lambda, but these methods could be out of sync with the actual licensed feature. This commit converts the remote license checker to take in the feature object that should be checked.

…formance * upstream/master: (153 commits) [ML] update truncation default & adding field output when input is truncated (elastic#79942) [ML] stop using isAllowedByLicense for model license checks (elastic#79908) [ML] Retain built-in ML roles granting Kibana privileges (elastic#80014) [Transform] remove old mixed cluster BWC layers, not required for 8x (elastic#79927) Increase test timeout for CoordinatorTests testAllSearchesExecuted [Transform] add rolling upgrade tests for upgrade endpoint (elastic#79721) [ML] Update trained model docs for truncate parameter for bert tokenization (elastic#79652) `CoordinatorTests` sometimes needs three term bumps (elastic#79574) [ML] Account for service being triggered twice in tests (elastic#80000) SearchContext: remove unused variable (elastic#79917) Revert "Deprecate resolution loss on date field (elastic#78921)" (elastic#79914) Re-enable GeoIpDownloaderIT#testStartWithNoDatabases() (elastic#79907) Fix SnapshotBasedIndexRecoveryIT#testSeqNoBasedRecoveryIsUsedAfterPrimaryFailOver (elastic#79469) Fix RecoverySourceHandlerTests (elastic#79546) SQL: stabilize SqlSearchPageTimeoutIT (elastic#79928) Wait 3 seconds for the server to reload trust (elastic#79778) Skip automatically preserved request headers when rewriting (elastic#79973) Check whether stdout is a real console (elastic#79882) Convert remote license checker to use LicensedFeature (elastic#79876) Miscellaneous fixes for LDAP SDK v6 upgrade (elastic#79891) ... # Conflicts: # libs/x-content/src/main/java/org/elasticsearch/xcontent/support/filtering/FilterPath.java # libs/x-content/src/test/java/org/elasticsearch/xcontent/support/filtering/FilterPathGeneratorFilteringTests.java # libs/x-content/src/test/java/org/elasticsearch/xcontent/support/filtering/FilterPathTests.java

rjernst added >refactoring :Security/License License functionality for commercial features v8.0.0 labels Oct 26, 2021

rjernst requested a review from ywangd October 26, 2021 21:51

elasticmachine added the Team:Security Meta label for security team label Oct 26, 2021

rjernst added 4 commits October 26, 2021 15:35

iter

6a50271

cleanup formatting

515ba66

checkstyle

da5da16

fix tests

7545161

ywangd reviewed Oct 27, 2021

View reviewed changes

rjernst added 2 commits October 27, 2021 05:52

address feedback

2acd57c

Merge branch 'master' into license/remote

8912627

rjernst force-pushed the license/remote branch from 73fdae6 to 8912627 Compare October 27, 2021 16:09

rjernst added 6 commits October 27, 2021 09:25

fix compile

65c1db5

reapply spotless

52f4659

Merge branch 'master' into license/remote

f1264b4

Merge branch 'master' into license/remote

6095dcc

Merge branch 'master' into license/remote

c6b716f

Merge branch 'master' into license/remote

47798b4

ywangd reviewed Oct 27, 2021

View reviewed changes

rjernst added 3 commits October 27, 2021 16:52

address feedback

5bbf127

tweak

c72827d

Merge branch 'master' into license/remote

076cec9

more iter

a7d25f4

ywangd approved these changes Oct 28, 2021

View reviewed changes

tweak error message building

9240b9d

rjernst added 2 commits October 27, 2021 17:17

Merge branch 'master' into license/remote

4f5ff16

spotless

3c18ee2

rjernst added v8.1.0 v7.16.1 v8.0.0 auto-backport-and-merge and removed v8.0.0 labels Oct 28, 2021

rjernst merged commit af226be into elastic:master Oct 28, 2021

rjernst deleted the license/remote branch October 28, 2021 01:32

rjernst mentioned this pull request Oct 28, 2021

[8.0] Convert remote license checker to use LicensedFeature (#79876) #79975

Merged

danhermann added v7.16.0 and removed v7.16.1 labels Nov 3, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert remote license checker to use LicensedFeature #79876

Convert remote license checker to use LicensedFeature #79876

rjernst commented Oct 26, 2021

elasticmachine commented Oct 26, 2021

ywangd left a comment

ywangd Oct 26, 2021

ywangd Oct 26, 2021

ywangd Oct 27, 2021

rjernst Oct 27, 2021

ywangd Oct 27, 2021

rjernst Oct 27, 2021

ywangd Oct 27, 2021

rjernst commented Oct 27, 2021

ywangd left a comment

ywangd Oct 27, 2021

rjernst Oct 27, 2021 •

edited

Loading

ywangd Oct 27, 2021

ywangd Oct 27, 2021

rjernst Oct 27, 2021

rjernst commented Oct 28, 2021

ywangd left a comment

ywangd Oct 28, 2021

rjernst Oct 28, 2021

elasticsearchmachine commented Oct 28, 2021

		* 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.

		if (remoteClusterLicenseInfo.licenseInfo().getStatus() != LicenseStatus.ACTIVE) {
		error.append(String.format(Locale.ROOT, "the license on cluster [%s] is not active", remoteClusterLicenseInfo.clusterAlias()));

		? new RemoteClusterLicenseChecker(client, mode -> true)
		? new RemoteClusterLicenseChecker(client, null)

Convert remote license checker to use LicensedFeature #79876

Convert remote license checker to use LicensedFeature #79876

Conversation

rjernst commented Oct 26, 2021

elasticmachine commented Oct 26, 2021

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Oct 27, 2021

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Oct 28, 2021

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 28, 2021

💔 Backport failed

rjernst Oct 27, 2021 •

edited

Loading