Remove basic feature checks from license state #59814

rjernst · 2020-07-17T20:49:14Z

The XPackLicenseState is a utility to handle checking the currently
configured license level against the required license level of each
licensed feature. Originally all licensed features were paid features
and licenses always had a limited time scope. However, since the basic
license was introduced, all users have an unlimited time use of basic
features; there is no actual "basic license", rather it is the base case
for the default distribution. Therefore, the checks on basic features in
license state are no-ops because the current license always at least
allows basic features, and the basic license cannot be expired.

This commit removes all of the features from the license state marked as
BASIC or MISSING (a level lower than BASIC that also predates the basic
license and is no longer relevant). It also adds an assertion that no
basic license features can be added to the license state in the future.

The XPackLicenseState is a utility to handle checking the currently configured license level against the required license level of each licensed feature. Originally all licensed features were paid features and licenses always had a limited time scope. However, since the basic license was introduced, all users have an unlimited time use of basic features; there is no actual "basic license", rather it is the base case for the default distribution. Therefore, the checks on basic features in license state are no-ops because the current license always at least allows basic features, and the basic license cannot be expired. This commit removes all of the features from the license state marked as BASIC or MISSING (a level lower than BASIC that also predates the basic license and is no longer relevant). It also adds an assertion that no basic license features can be added to the license state in the future.

elasticmachine · 2020-07-17T20:49:16Z

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

ywangd

I have one main concern:

Currently many of the basic features require the license to be active. My understanding is that we do not encourage users to run with expired commerical licenses forever. An expired PLATINUM license is still allowed to access some platinum features, e.g. DLS/FLS, for security reasons. If an expired license can access all BASIC featues, plus a fair bit more, this would put our licensing in an awkward situation. Therefore, we disable some of the BASIC features so that user can choose to either downgrade to a BASIC license or renew the subscription. With changes in this PR, an expired PLATINUM license would be all around better than a BASIC. Is this our intention?

I also have a few other less important suggestions:

With this change, there are many licenseState fields that can be removed from various TransportAction classes, e.g. RollupInfoTransportAction as well as ApiKeyService.
There seems to be opportunities for extracting common behaviours. For example, the XxxInfoTransportAction classes, since BASIC features can no longer be dsiabled and is always allowed, maybe we could have a superclass such as BasicFeatureInfoTransportAction. Security is a notable anomaly here. I wish it can also be always enabled, but that's a separate story.
For the Usage classes, e.g. RollupFeatureSetUsage, I wonder whether we could add another constructor:

public RollupFeatureSetUsage() {
    super(XPackField.ROLLUP, true, true);
}

and mark the current constructor public RollupFeatureSetUsage(boolean available) as deprecated or just remove it. So we are not relying on the callers to pass in the right value.

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/AnalyticsPlugin.java

...ics/src/main/java/org/elasticsearch/xpack/analytics/action/AnalyticsInfoTransportAction.java

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

ywangd · 2020-07-18T11:56:33Z

.../src/main/java/org/elasticsearch/xpack/monitoring/collector/enrich/EnrichStatsCollector.java

-        return isElectedMaster
-            && super.shouldCollect(isElectedMaster)
-            && licenseState.checkFeature(XPackLicenseState.Feature.ENRICH);
+        return isElectedMaster && super.shouldCollect(isElectedMaster);


The super.shouldCollect method is called here, while it is not called in StatsCollector. This super method always returns true after the change, so invoking it or not does not change the result. But I'd slightly prefer to always invoke it because:

In general, we may not wanna assume too much knowledge of the superclass.

If the superclass logic changes, we are protected.

We should be able to rely on compiler to optimise for us in this case.

...urity/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java

rjernst · 2020-11-21T01:39:16Z

@elasticmachine run elasticsearch-ci/2

rjernst · 2020-12-01T23:54:32Z

@ywangd I've brought this PR back up to date and addressed your comments. Can you take another look?

Re your broader comment:

With changes in this PR, an expired PLATINUM license would be all around better than a BASIC. Is this our intention?

Yes, this is the intention.

ywangd

LGTM. I left some comments, nothing really major.

With changes in this PR, an expired PLATINUM license would be all around better than a BASIC. Is this our intention?

Yes, this is the intention.

Well almost, the health and stats related actions are still blocked for expired licenses. So it still creates some prompt for users to choose either renew license or explicitly go back to basic.

I kinda like having a central list where every feature is explicitly listed along with its license requirement. It serves as a good reference. If we configure the license requirement correctly, something like ...(OperationMode.MISSING, false), we can even leave all the call sites of checkFature unchanged and just change the method itself to not track usages for any features of BASIC or lower. It also enforces license requirement be specified in one place so there is no place for inconsistency (among different classes usage/info/action etc). With that being said, I do like how much code we can delete because of the relevant Feature Enums are deleted. Given people who develop any new feature should also be responsible or consciously involved in license decision, I think it is reasonably to expect consistency will be taken care of. Overall I am fine with this approach.

ywangd · 2020-12-05T23:45:14Z

...onitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/ccr/StatsCollector.java

@@ -61,7 +61,6 @@ public StatsCollector(
    protected boolean shouldCollect(final boolean isElectedMaster) {
        // this can only run when monitoring is allowed and CCR is enabled and allowed, but also only on the elected master node
        return isElectedMaster
-                && super.shouldCollect(isElectedMaster)


I'd leave this untouched. The change assumes the superclass call always return true, which is the case with changes of this PR. But strictly speaking, the logic is more foolproof to keep this call so that any future changes will be covered.

ywangd · 2020-12-06T00:05:42Z

x-pack/plugin/core/src/test/java/org/elasticsearch/license/XPackLicenseStateTests.java

@@ -413,71 +362,55 @@ public void testLogstashInactive() {

    public void testSqlDefaults() {
        XPackLicenseState licenseState = TestUtils.newTestLicenseState();
-        assertThat(licenseState.checkFeature(XPackLicenseState.Feature.SQL), is(true));
        assertThat(licenseState.checkFeature(XPackLicenseState.Feature.JDBC), is(true));
    }

    public void testSqlBasic() {


Nit: these testSqlXxx now has nothing to do with SQL. Should we call them testJdbcXxx instead?

ywangd · 2020-12-06T00:18:31Z

...n/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/EnrichUsageTransportAction.java

@@ -51,7 +47,6 @@ protected void masterOperation(
        ClusterState state,
        ActionListener<XPackUsageFeatureResponse> listener
    ) {
-        boolean available = licenseState.isAllowed(XPackLicenseState.Feature.ENRICH);
-        listener.onResponse(new XPackUsageFeatureResponse(new EnrichFeatureSetUsage(available)));
+        listener.onResponse(new XPackUsageFeatureResponse(new EnrichFeatureSetUsage(true)));


For these basic features, I think it is better to drop the available parameter in its constructor similar to the enabled parameter, i.e. have something like:

public EnrichFeatureSetUsage() { super(XpackField.ENRICH, true, true); }

ywangd · 2020-12-06T00:23:54Z

...plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleUsageTransportAction.java

@@ -69,9 +63,9 @@ protected void masterOperation(Task task, XPackUsageRequest request, ClusterStat
                }).collect(Collectors.toMap(Tuple::v1, Tuple::v2));
                return new IndexLifecycleFeatureSetUsage.PolicyStats(phaseStats, policyUsage.getOrDefault(policy.getName(), 0));
            }).collect(Collectors.toList());
-            usage = new IndexLifecycleFeatureSetUsage(available, policyStats);
+            usage = new IndexLifecycleFeatureSetUsage(true, policyStats);


Same here, the available parameter can be dropped from the constructor

ywangd · 2020-12-06T00:25:04Z

x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SLMUsageTransportAction.java

        final SnapshotLifecycleMetadata slmMeta = state.metadata().custom(SnapshotLifecycleMetadata.TYPE);
-        final SLMFeatureSetUsage usage = new SLMFeatureSetUsage(available, slmMeta == null ? null : slmMeta.getStats());
+        final SLMFeatureSetUsage usage = new SLMFeatureSetUsage(true, slmMeta == null ? null : slmMeta.getStats());


Here as well for the available parameter.

ywangd · 2020-12-06T01:02:53Z

...est/java/org/elasticsearch/xpack/security/rest/action/user/RestHasPrivilegesActionTests.java

@@ -38,6 +40,7 @@
     */
    public void testBodyConsumed() throws Exception {
        final XPackLicenseState licenseState = mock(XPackLicenseState.class);
+        when(licenseState.isSecurityEnabled()).thenReturn(true);


To retain the existing behaviour of this test, should this mock return false instead of true so that action.handleRequest(...) does not throw exception but returns a response about security is not enabled (currently it returns security not available due to license)?

We discussed this in another channel. After another look, I think the change is in fact closer to what this test tries to assert:

Send a request without an username

It should not fail with IllegalArgumentException complaining about unconsumed payload

Both the existing version and the change do the job because both of them will run through BaseRestHandler#handleRequest, where the unconsumed body is checked.
The existing version does its job by ensure there is no exception. This relies on the fact that “security disabled” message takes precedence over the “no authenticated user” exception. The new change enables security so that the “no authenticated user” exception now bubbles up. Both “security disabled” message and “no authenticated user” exception have lower priority than the “unconsumed body” exception. So the test does its job if either of them can be asserted. But the new change has the advantage of an additional assertion on the exception to be “no authenticated user”, which is more to the point.

In summary, the new change does its job slightly differently than the existing version. But I think it's a small improvement. So we can discard my previous comment.

ywangd · 2020-12-06T01:03:48Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/SqlUsageTransportAction.java

@@ -58,7 +57,7 @@ protected void masterOperation(Task task, XPackUsageRequest request, ClusterStat
                .filter(Objects::nonNull)
                .collect(Collectors.toList());
            Counters mergedCounters = Counters.merge(countersPerNode);
-            SqlFeatureSetUsage usage = new SqlFeatureSetUsage(available, mergedCounters.toNestedMap());
+            SqlFeatureSetUsage usage = new SqlFeatureSetUsage(true, mergedCounters.toNestedMap());


Yet another available parameter.

ywangd · 2020-12-06T01:05:21Z

...transform/src/main/java/org/elasticsearch/xpack/transform/TransformUsageTransportAction.java

@@ -96,7 +90,7 @@ protected void masterOperation(
        }

        ActionListener<TransformIndexerStats> totalStatsListener = ActionListener.wrap(statSummations -> {
-            var usage = new TransformFeatureSetUsage(available, transformsCountByState, statSummations);
+            var usage = new TransformFeatureSetUsage(true, transformsCountByState, statSummations);


Here again, the available parameter.

ywangd · 2020-12-06T01:10:05Z

...lugin/vectors/src/main/java/org/elasticsearch/xpack/vectors/VectorsUsageTransportAction.java

@@ -76,7 +70,7 @@ protected void masterOperation(Task task, XPackUsageRequest request, ClusterStat
            }
        }
        VectorsFeatureSetUsage usage =
-            new VectorsFeatureSetUsage(vectorsAvailable, numDenseVectorFields, avgDenseVectorDims);
+            new VectorsFeatureSetUsage(true, numDenseVectorFields, avgDenseVectorDims);


The available parameter again.

ywangd · 2020-12-06T01:11:27Z

...only-node/src/main/java/org/elasticsearch/cluster/coordination/VotingOnlyNodeFeatureSet.java

-            final boolean available = licenseState.checkFeature(Feature.VOTING_ONLY);
-            final VotingOnlyNodeFeatureSetUsage usage =
-                new VotingOnlyNodeFeatureSetUsage(available);
+            final VotingOnlyNodeFeatureSetUsage usage = new VotingOnlyNodeFeatureSetUsage(true);


I believe this is the last occurrence of the available parameter.

The XPackLicenseState is a utility to handle checking the currently configured license level against the required license level of each licensed feature. Originally all licensed features were paid features and licenses always had a limited time scope. However, since the basic license was introduced, all users have an unlimited time use of basic features; there is no actual "basic license", rather it is the base case for the default distribution. Therefore, the checks on basic features in license state are no-ops because the current license always at least allows basic features, and the basic license cannot be expired. This commit removes all of the features from the license state marked as BASIC or MISSING (a level lower than BASIC that also predates the basic license and is no longer relevant). It also adds an assertion that no basic license features can be added to the license state in the future.

rjernst added >refactoring :Security/License License functionality for commercial features v8.0.0 v7.10.0 labels Jul 17, 2020

elasticmachine added the Team:Security Meta label for security team label Jul 17, 2020

rjernst added 3 commits July 17, 2020 14:22

remove forgotten analytics constant

cace4ea

Merge branch 'master' into refactor_license15

83dd57c

checkstyle

9a210ee

ywangd reviewed Jul 18, 2020

View reviewed changes

rjernst added 2 commits July 20, 2020 18:48

Add back disabling of health and stats on expired license

eaa1130

checkstyle

b9eef41

rjernst closed this Sep 18, 2020

rjernst deleted the refactor_license15 branch September 18, 2020 03:33

rjernst restored the refactor_license15 branch September 23, 2020 22:03

rjernst reopened this Sep 23, 2020

andreidan added v7.11.0 and removed v7.10.0 labels Oct 7, 2020

rjernst added 11 commits November 16, 2020 19:46

Merge branch 'master' into refactor_license15

7af8f1f

more merge cleanup

d665086

address some feedback

8def7f1

remove transform aux function

7536ec0

cleanup unused licenseState

d1559b7

spotless

2f15285

checkstyle

27a0e69

test tweak

3908943

Merge branch 'master' into refactor_license15

f797dc5

fix tests

767e9f1

remove mutations from spatial feature set usage

98ca4ec

rjernst added 2 commits November 30, 2020 15:07

Merge branch 'master' into refactor_license15

0a2d0d4

fix ML transforms with remote clusters

972807c

Merge branch 'master' into refactor_license15

e4a842e

ywangd approved these changes Dec 6, 2020

View reviewed changes

pugnascotia added v7.12.0 and removed v7.11.0 labels Dec 16, 2020

rjernst added 4 commits December 17, 2020 12:00

Merge branch 'master' into refactor_license15

baed4b9

more review comments

4951650

checkstyle

daf6b8b

fix compile

2c45154

rjernst merged commit d38e5e0 into elastic:master Dec 18, 2020

rjernst deleted the refactor_license15 branch December 18, 2020 04:48

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove basic feature checks from license state #59814

Remove basic feature checks from license state #59814

rjernst commented Jul 17, 2020

elasticmachine commented Jul 17, 2020

ywangd left a comment •

edited

Loading

ywangd Jul 18, 2020

rjernst commented Nov 21, 2020

rjernst commented Dec 1, 2020

ywangd left a comment

ywangd Dec 5, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

ywangd Dec 18, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

ywangd Dec 6, 2020

Remove basic feature checks from license state #59814

Remove basic feature checks from license state #59814

Conversation

rjernst commented Jul 17, 2020

elasticmachine commented Jul 17, 2020

ywangd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Nov 21, 2020

rjernst commented Dec 1, 2020

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

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

ywangd left a comment •

edited

Loading