Refactor license checking #52118

ywangd · 2020-02-10T02:25:54Z

Improve code reuse and readability.

Relates: #51864

elasticmachine · 2020-02-10T02:25:56Z

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

ywangd · 2020-02-10T02:29:58Z

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

@@ -343,7 +343,7 @@ public synchronized OperationMode getOperationMode() {
    }

    /** Return true if the license is currently within its time boundaries, false otherwise. */
-    public synchronized boolean isActive() {
+    public synchronized boolean allowForAllLicenses() {


Address feedback #51864 (comment)

I think the javadoc should be updated. Stricly it's accurate, but by intent this method now has a different purpose.

I am having some difficulty trying to rephrase this without the word "active" or something that explains it. How about "Return true if the feature is allowed by all non-expired licenses"?

ywangd · 2020-02-10T02:33:17Z

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

+    private synchronized boolean isAllowedBySecurityAndLicense(OperationMode minimumMode, boolean needActive, boolean allowTrial) {
+        if (false == isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled)) {
            return false;
        }
+        return isAllowedByLicense(minimumMode, needActive, allowTrial);
+    }
+
+    /**
+     * Test whether a feature is allowed by the status of license. Note difference to
+     * {@link #isAllowedBySecurityAndLicense} is this method does <b>Not</b> require security
+     * to be enabled.
+     *
+     * @param minimumMode  The minimum license to meet or exceed
+     * @param needActive   Whether current license needs to be active
+     * @param allowTrial   Whether the feature is allowed for trial license
+     *
+     * @return true if feature is allowed, otherwise false
+     */
+    public synchronized boolean isAllowedByLicense(OperationMode minimumMode, boolean needActive, boolean allowTrial) {
        if (needActive && false == status.active) {
            return false;
        }


Address feedback #51864 (comment)

The method isAllowedByLicense now does not check security at all, while isAllowedBySecurityAndLicense always checks security plus license.

I'm going to be picky and say I'd like an even simpler version of this method.

My absolute number 1 aim here is when an engineer needs to add a license check, they are not asked to make decisions that they shouldn't need to worry about.

I think that means we should have:

public boolean isAllowedByLicense(OperationMode minimumMode) { return AllowedByLicense(minimumMode, true, true); }

and use that method everywhere we can.

Put yourself in the shows of someone who is implementing a new license check.
They come here, they look for another method that has a similar license live (e.g. Platinum), and find the isCcrAllowed method. So, they model their isWorldDominationAllowed method on that.
But isCcrAllowed calls: isAllowedByLicense(PLATINUM, true, true)
Do I need to pass true for those parameters as well? Well, what do they mean? Oh, needActive ... well, what counts as an active license? let's look for what that paremter does... oh, it checks status.active ... what does that mean ? etc.
There should be an obvious method that does the right thing for 90% of cases, and we don't ask the engineer to worry about parameters that they almost certainly don't care about, and shouldn't be asked to make a decision on.

I am not a fan of many boolean parameters. So yes I agree the simplification is worthwhile. It is now added and replaced 10 calls of the more verbose method.

ywangd · 2020-02-10T02:34:17Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/TrainedModelConfig.java

@@ -243,13 +243,12 @@ public boolean isAvailableWithLicense(XPackLicenseState licenseState) {
        }

        // The model license does not matter, this is the highest licensed level
-        if (licenseState.isActive() && XPackLicenseState.isAllowedByOperationMode(
-            licenseState.getOperationMode(), License.OperationMode.PLATINUM, true)) {
+        if (licenseState.isAllowedByLicense(License.OperationMode.PLATINUM, true, true)) {


Address feedback #51864 (comment)
Single call to remove potential race condition.

ywangd · 2020-02-10T02:34:43Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/TrainedModelConfig.java

            return true;
        }

        // catch the rest, if the license is active and is at least the required model license
-        return licenseState.isActive() && License.OperationMode.compare(licenseState.getOperationMode(), licenseLevel) >= 0;
+        return licenseState.isAllowedByLicense(licenseLevel, true, false);


Same as above.

I doubt that we do not allow Trial license here. If the license is trial, it will be allowed by the code above this line. So allowTrial or not does not make a difference here. But it seems a bit funny to state it like this. Plus it could be simplified to isAllowedByLicense(licenseLevel) if allowTrial is true.

It feels like this whole method is trying too hard to duplicate logic that should be handled by XPackLicenseState.
I think we can replace the whole method with:

return licenseState.isAllowedByLicense(this.licenseLevel)

But perhaps we should make that a separate PR so that the ML team can review it.

I'll create a separate PR for it. thanks

ywangd · 2020-02-10T02:35:06Z

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

-        OperationMode mode = status.mode;
-        return mode == OperationMode.GOLD || mode == OperationMode.PLATINUM || mode == OperationMode.STANDARD ||
-                mode == OperationMode.TRIAL || mode == OperationMode.BASIC || mode == OperationMode.ENTERPRISE;
+        return status.mode != OperationMode.MISSING;


Address feedback #51864 (comment)

ywangd · 2020-02-10T02:44:39Z

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

-                    && newLicense.operationMode() != License.OperationMode.PLATINUM
-                    && newLicense.operationMode() != License.OperationMode.ENTERPRISE
-                    && newLicense.operationMode() != License.OperationMode.TRIAL) {
+                    && XPackLicenseState.isFipsAllowedForOperationMode(newLicense.operationMode())) {


~~Address feedback #51864 (comment)~~
See #52118 (comment)

ywangd · 2020-02-10T03:16:18Z

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

-                    && newLicense.operationMode() != License.OperationMode.PLATINUM
-                    && newLicense.operationMode() != License.OperationMode.ENTERPRISE
-                    && newLicense.operationMode() != License.OperationMode.TRIAL) {
+                    && false == XPackLicenseState.isFipsAllowedForOperationMode(newLicense.operationMode())) {


Address feedback #51864 (comment)

…-operation-mode-tailing

tvernum · 2020-02-14T02:22:58Z

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

+    private synchronized boolean isAllowedBySecurityAndLicense(OperationMode minimumMode, boolean needActive, boolean allowTrial) {
+        if (false == isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled)) {
            return false;
        }
+        return isAllowedByLicense(minimumMode, needActive, allowTrial);
+    }
+
+    /**
+     * Test whether a feature is allowed by the status of license. Note difference to
+     * {@link #isAllowedBySecurityAndLicense} is this method does <b>Not</b> require security
+     * to be enabled.
+     *
+     * @param minimumMode  The minimum license to meet or exceed
+     * @param needActive   Whether current license needs to be active
+     * @param allowTrial   Whether the feature is allowed for trial license
+     *
+     * @return true if feature is allowed, otherwise false
+     */
+    public synchronized boolean isAllowedByLicense(OperationMode minimumMode, boolean needActive, boolean allowTrial) {
        if (needActive && false == status.active) {
            return false;
        }


I'm going to be picky and say I'd like an even simpler version of this method.

My absolute number 1 aim here is when an engineer needs to add a license check, they are not asked to make decisions that they shouldn't need to worry about.

I think that means we should have:

public boolean isAllowedByLicense(OperationMode minimumMode) { return AllowedByLicense(minimumMode, true, true); }

and use that method everywhere we can.

Put yourself in the shows of someone who is implementing a new license check.
They come here, they look for another method that has a similar license live (e.g. Platinum), and find the isCcrAllowed method. So, they model their isWorldDominationAllowed method on that.
But isCcrAllowed calls: isAllowedByLicense(PLATINUM, true, true)
Do I need to pass true for those parameters as well? Well, what do they mean? Oh, needActive ... well, what counts as an active license? let's look for what that paremter does... oh, it checks status.active ... what does that mean ? etc.
There should be an obvious method that does the right thing for 90% of cases, and we don't ask the engineer to worry about parameters that they almost certainly don't care about, and shouldn't be asked to make a decision on.

tvernum · 2020-02-14T02:25:22Z

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

@@ -343,7 +343,7 @@ public synchronized OperationMode getOperationMode() {
    }

    /** Return true if the license is currently within its time boundaries, false otherwise. */
-    public synchronized boolean isActive() {
+    public synchronized boolean allowForAllLicenses() {


I think the javadoc should be updated. Stricly it's accurate, but by intent this method now has a different purpose.

tvernum · 2020-02-14T02:27:02Z

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

@@ -637,23 +637,23 @@ public boolean isIndexLifecycleAllowed() {
     *         {@code false}.
     */
    public boolean isEnrichAllowed() {
-        return isActive();
+        return allowForAllLicenses();
    }

    /**
     * Determine if EQL support should be enabled.
     * <p>
     *  EQL is available for all license types except {@link OperationMode#MISSING}


We can remove the comment above, since it's not accurate (we don't check missing).

Removed. I always went in and changed many of the inconsistent comments. However I am still not quite happy about current approach, e.g. "XXX is always allowed as long as there is an active license". This feels like documenting the internals instead of the overall purpose. It could be changed to "Determine whether XXX is allowed". But then it is basically duplicating the method name and might as well be removed. Please let me know how you think about this.

My default position is that the existence of a comment should imply that something non-obvious is going on.

If the code is obvious (both in terms of what it tries to do, and why it does it that way), then the comment is a distraction. It's one more thing to read and understand that eventually just tells you what was right in front of you.

In these cases, I would hope the new structure is sufficient to make the comments unnecessary.

Thanks Tim. This is my intention as well.

tvernum · 2020-02-14T02:29:22Z

...plugin/core/src/test/java/org/elasticsearch/license/AbstractLicensesIntegrationTestCase.java

@@ -88,7 +88,7 @@ public void onFailure(String source, @Nullable Exception e) {
    protected void assertLicenseActive(boolean active) throws Exception {
        assertBusy(() -> {
            for (XPackLicenseState licenseState : internalCluster().getDataNodeInstances(XPackLicenseState.class)) {
-                if (licenseState.isActive() == active) {
+                if (licenseState.allowForAllLicenses() == active) {


I don't think this call should change. I think we still need a (possibly package protected) isActive method here so that the test is asserting what it says it's asserting.

Added package private isActive. But having both of the two methods bothers me a bit. They are functionally identical. But it could be accidental. Given we should fallback to basic license which is always active, the call to allowForAllLicenses is really needed. It can either be removed or reduced to always return true.

tvernum · 2020-02-14T02:31:04Z

...in/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/TrainedModelConfigTests.java


-        when(licenseState.isActive()).thenReturn(true);
-        when(licenseState.getOperationMode()).thenReturn(License.OperationMode.PLATINUM);
+        // Active Platinum license is considered as highest and should always work


This isn't true any more, right?
I mean, technically right now there's no functional difference between Platinum and Enterprise modes, but Platinum isn't actually the highest now.

You are right. I was trying to hint the same thing in the comment, i.e. "considered as ...". But it is too subtle to understand. I'll reword it.

…-operation-mode-tailing

ywangd · 2020-02-14T04:59:55Z

Thanks Tim. I have updated accordingly. There are still rooms for improvement:

Potentially get rid of allowForAllLicenses() since the default basic license is always active.
If we do above, a simpler soluation is just return true from all those isXxxAllowed methods that calls it. A further improvement is to go into callers of these methods and remove the calls entirely.
Streamline the comments for those isXxxAllowed methods so they are consistent.

tvernum · 2020-02-18T23:39:16Z

GitHub mobile doesn't make it easy to reply to review comments, so I'll do it here.

For the javadoc, I'd say
Checks that the cluster has a valid licence of any level.

tvernum · 2020-02-21T00:55:15Z

Potentially get rid of allowForAllLicenses() since the default basic license is always active.

I'm OK with that, but I worry that it will end up with silly comments because people feel weird about just return true;

I think we'll see a bunch of:

public void isWorldPeaceAllowed() {
    return true; // this feature is allowed on all license levels
}

And if we do see that, it implies to me that we need something in code to say the same thing, like...

private static final boolean ALLOWED_ON_ALL_LICENSES = true;
public void isWorldPeaceAllowed() {
    return ALLOWED_ON_ALL_LICENSES;
}

But that feels pretty weird too.

I'm not too fussed though.

…-operation-mode-tailing

ywangd · 2020-02-21T01:55:22Z

I'm OK with that, but I worry that it will end up with silly comments because people feel weird about just return true;

Fair point. I think I'll leave it as is for now. The actual improvement should be removing these checks from the caller site when we can be sure they are available in any case. We could take care of this change separately.

Improve code resuse and readility. Add convenience checking method which covers most use cases without having to pass many boolean arguments.

ywangd · 2020-02-27T02:05:03Z

Backported:

v7.7.0

Continue refactor license checking

8eabbf1

ywangd added >enhancement :Security/License License functionality for commercial features v8.0.0 v7.7.0 labels Feb 10, 2020

ywangd requested a review from tvernum February 10, 2020 02:25

ywangd commented Feb 10, 2020

View reviewed changes

Fix checkstyle

f1c931b

ywangd commented Feb 10, 2020

View reviewed changes

Fix fips license test

9d1164c

ywangd commented Feb 10, 2020

View reviewed changes

ywangd added 2 commits February 10, 2020 21:14

More refactoring

4a20a97

Merge remote-tracking branch 'origin/master' into es-51081-enterprise…

9c9afb0

…-operation-mode-tailing

tvernum reviewed Feb 14, 2020

View reviewed changes

ywangd added 3 commits February 14, 2020 15:33

Merge remote-tracking branch 'origin/master' into es-51081-enterprise…

e1da109

…-operation-mode-tailing

Address feedback

95877fc

Revert accidental change

a34e3ca

ywangd requested a review from tvernum February 14, 2020 05:00

ywangd mentioned this pull request Feb 19, 2020

Add enterprise mode and refactor license check (#51864) #52115

Merged

tvernum approved these changes Feb 21, 2020

View reviewed changes

ywangd added 2 commits February 21, 2020 12:41

Merge remote-tracking branch 'origin/master' into es-51081-enterprise…

88a3e62

…-operation-mode-tailing

Remove low value comments

e1eca99

ywangd merged commit 72cf242 into elastic:master Feb 21, 2020

ywangd added the backport pending label Feb 21, 2020

ywangd mentioned this pull request Feb 24, 2020

Simplify ml license checking with XpackLicenseState internals #52684

Merged

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Feb 27, 2020

Refactor license checking (elastic#52118)

0880d20

Improve code resuse and readility. Add convenience checking method which covers most use cases without having to pass many boolean arguments.

ywangd added a commit that referenced this pull request Feb 27, 2020

Refactor license checking (#52118) (#52859)

f5c4e92

Improve code resuse and readility. Add convenience checking method which covers most use cases without having to pass many boolean arguments.

ywangd removed the backport pending label Feb 27, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

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

Refactor license checking #52118

Refactor license checking #52118

ywangd commented Feb 10, 2020

elasticmachine commented Feb 10, 2020

ywangd Feb 10, 2020

tvernum Feb 14, 2020

ywangd Feb 14, 2020 •

edited

Loading

ywangd Feb 10, 2020

tvernum Feb 14, 2020

ywangd Feb 14, 2020

ywangd Feb 10, 2020

ywangd Feb 10, 2020

ywangd Feb 14, 2020

tvernum Feb 21, 2020 •

edited

Loading

ywangd Feb 21, 2020 •

edited

Loading

ywangd Feb 10, 2020

ywangd Feb 10, 2020 •

edited

Loading

ywangd Feb 10, 2020

tvernum Feb 14, 2020

tvernum Feb 14, 2020

tvernum Feb 14, 2020

ywangd Feb 14, 2020

tvernum Feb 21, 2020

ywangd Feb 21, 2020

tvernum Feb 14, 2020

ywangd Feb 14, 2020

tvernum Feb 14, 2020

ywangd Feb 14, 2020

ywangd Feb 14, 2020

ywangd commented Feb 14, 2020

tvernum commented Feb 18, 2020 •

edited

Loading

tvernum commented Feb 21, 2020

ywangd commented Feb 21, 2020

ywangd commented Feb 27, 2020

Refactor license checking #52118

Refactor license checking #52118

Conversation

ywangd commented Feb 10, 2020

elasticmachine commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd Feb 14, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

ywangd Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd Feb 10, 2020 • 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

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 commented Feb 14, 2020

tvernum commented Feb 18, 2020 • edited Loading

tvernum commented Feb 21, 2020

ywangd commented Feb 21, 2020

ywangd commented Feb 27, 2020

ywangd Feb 14, 2020 •

edited

Loading

tvernum Feb 21, 2020 •

edited

Loading

ywangd Feb 21, 2020 •

edited

Loading

ywangd Feb 10, 2020 •

edited

Loading

tvernum commented Feb 18, 2020 •

edited

Loading