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

Refactor license checking #52118

Merged
merged 10 commits into from
Feb 21, 2020
Merged

Refactor license checking #52118

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8eabbf1
Continue refactor license checking
ywangd Feb 10, 2020
f1c931b
Fix checkstyle
ywangd Feb 10, 2020
9d1164c
Fix fips license test
ywangd Feb 10, 2020
4a20a97
More refactoring
ywangd Feb 10, 2020
9c9afb0
Merge remote-tracking branch 'origin/master' into es-51081-enterprise…
ywangd Feb 10, 2020
e1da109
Merge remote-tracking branch 'origin/master' into es-51081-enterprise…
ywangd Feb 14, 2020
95877fc
Address feedback
ywangd Feb 14, 2020
a34e3ca
Revert accidental change
ywangd Feb 14, 2020
88a3e62
Merge remote-tracking branch 'origin/master' into es-51081-enterprise…
ywangd Feb 21, 2020
e1eca99
Remove low value comments
ywangd Feb 21, 2020
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
4 changes: 1 addition & 3 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/LicenseService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,7 @@ && isProductionMode(settings, clusterService.localNode())) {
throw new IllegalStateException("Cannot install a [" + newLicense.operationMode() +
"] license unless TLS is configured or security is disabled");
} else if (XPackSettings.FIPS_MODE_ENABLED.get(settings)
&& newLicense.operationMode() != License.OperationMode.PLATINUM
&& newLicense.operationMode() != License.OperationMode.ENTERPRISE
&& newLicense.operationMode() != License.OperationMode.TRIAL) {
&& false == XPackLicenseState.isFipsAllowedForOperationMode(newLicense.operationMode())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Address feedback #51864 (comment)

throw new IllegalStateException("Cannot install a [" + newLicense.operationMode() +
"] license unless FIPS mode is disabled");
}
Expand Down
108 changes: 56 additions & 52 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 @@ -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() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Address feedback #51864 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ywangd ywangd Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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"?

return status.active;
}

Expand All @@ -352,21 +352,21 @@ public synchronized boolean isActive() {
* @see #allowedRealmType() for the enabled realms
*/
public boolean isAuthAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.BASIC, true, false, true);
return isAllowedBySecurityAndLicense(OperationMode.BASIC, false, true);
}

/**
* @return true if IP filtering should be enabled
*/
public boolean isIpFilteringAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.GOLD, true, false, true);
return isAllowedBySecurityAndLicense(OperationMode.GOLD, false, true);
}

/**
* @return true if auditing should be enabled
*/
public boolean isAuditingAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.GOLD, true, false, true);
return isAllowedBySecurityAndLicense(OperationMode.GOLD, false, true);
}

/**
Expand All @@ -376,7 +376,7 @@ public boolean isAuditingAllowed() {
* @return true if the license allows for the stats and health APIs to be used.
*/
public boolean isStatsAndHealthAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -392,7 +392,7 @@ public boolean isStatsAndHealthAllowed() {
* @return {@code true} to enable DLS and FLS. Otherwise {@code false}.
*/
public boolean isDocumentAndFieldLevelSecurityAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, true, false, true);
return isAllowedBySecurityAndLicense(OperationMode.PLATINUM, false, true);
}

/** Classes of realms that may be available based on the license type. */
Expand Down Expand Up @@ -432,37 +432,37 @@ public synchronized AllowedRealmType allowedRealmType() {
* @return whether custom role providers are allowed based on the license {@link OperationMode}
*/
public boolean isCustomRoleProvidersAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, true, true, true);
return isAllowedBySecurityAndLicense(OperationMode.PLATINUM, true, true);
}

/**
* @return whether the Elasticsearch {@code TokenService} is allowed based on the license {@link OperationMode}
*/
public boolean isTokenServiceAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.GOLD, true, false, true);
return isAllowedBySecurityAndLicense(OperationMode.GOLD, false, true);
}

/**
* @return whether the Elasticsearch {@code ApiKeyService} is allowed based on the current node/cluster state
*/
public boolean isApiKeyServiceAllowed() {
return isAllowedBySecurity();
return isAllowedBySecurityAndLicense(OperationMode.MISSING, false, true);
}

/**
* @return whether "authorization_realms" are allowed based on the license {@link OperationMode}
* @see org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings
*/
public boolean isAuthorizationRealmAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, true, true, true);
return isAllowedBySecurityAndLicense(OperationMode.PLATINUM, true, true);
}

/**
* @return whether a custom authorization engine is allowed based on the license {@link OperationMode}
* @see org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings
*/
public boolean isAuthorizationEngineAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, true, true, true);
return isAllowedBySecurityAndLicense(OperationMode.PLATINUM, true, true);
}

/**
Expand All @@ -479,7 +479,7 @@ public boolean isAuthorizationEngineAllowed() {
* @return {@code true} as long as the license is valid. Otherwise {@code false}.
*/
public boolean isWatcherAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.STANDARD, false, true, true);
return isAllowedByLicense(OperationMode.STANDARD, true, true);
}

/**
Expand All @@ -488,7 +488,7 @@ public boolean isWatcherAllowed() {
* @return true if the license is active
*/
public boolean isMonitoringAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -511,7 +511,7 @@ public boolean isMonitoringClusterAlertsAllowed() {
* @return {@code true} if the user is allowed to modify the retention. Otherwise {@code false}.
*/
public boolean isUpdateRetentionAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.STANDARD, false, false, true);
return isAllowedByLicense(OperationMode.STANDARD, false, true);
}

/**
Expand All @@ -526,7 +526,7 @@ public boolean isUpdateRetentionAllowed() {
* @return {@code true} as long as the license is valid. Otherwise {@code false}.
*/
public boolean isGraphAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, false, true, true);
return isAllowedByLicense(OperationMode.PLATINUM, true, true);
}

/**
Expand All @@ -543,7 +543,7 @@ public boolean isGraphAllowed() {
* {@code false}.
*/
public boolean isMachineLearningAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, false, true, true);
return isAllowedByLicense(OperationMode.PLATINUM, true, true);
}

public static boolean isMachineLearningAllowedForOperationMode(final OperationMode operationMode) {
Expand All @@ -556,7 +556,7 @@ public static boolean isMachineLearningAllowedForOperationMode(final OperationMo
* @return true if the license is active
*/
public boolean isTransformAllowed() {
return isActive();
return allowForAllLicenses();
}

public static boolean isTransformAllowedForOperationMode(final OperationMode operationMode) {
Expand All @@ -574,7 +574,7 @@ public static boolean isFipsAllowedForOperationMode(final OperationMode operatio
* @return true if the license is active
*/
public boolean isRollupAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -583,31 +583,31 @@ public boolean isRollupAllowed() {
* @return true if the license is active
*/
public boolean isVotingOnlyAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
* Logstash is allowed as long as there is an active license of type TRIAL, STANDARD, GOLD or PLATINUM
* @return {@code true} as long as there is a valid license
*/
public boolean isLogstashAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.STANDARD, false, true, true);
return isAllowedByLicense(OperationMode.STANDARD, true, true);
}

/**
* Beats is allowed as long as there is an active license of type TRIAL, STANDARD, GOLD or PLATINUM
* @return {@code true} as long as there is a valid license
*/
public boolean isBeatsAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.STANDARD, false, true, true);
return isAllowedByLicense(OperationMode.STANDARD, true, true);
}

/**
* Deprecation APIs are always allowed as long as there is an active license
* @return {@code true} as long as there is a valid license
*/
public boolean isDeprecationAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -617,7 +617,7 @@ public boolean isDeprecationAllowed() {
* {@code false}.
*/
public boolean isUpgradeAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -627,7 +627,7 @@ public boolean isUpgradeAllowed() {
* {@code false}.
*/
public boolean isIndexLifecycleAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -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}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Tim. This is my intention as well.

*/
public synchronized boolean isEqlAllowed() {
return status.active;
public boolean isEqlAllowed() {
return allowForAllLicenses();
}

/**
* Determine if SQL support should be enabled.
*/
public boolean isSqlAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -662,21 +662,21 @@ public boolean isSqlAllowed() {
* JDBC is available only in for {@link OperationMode#PLATINUM} and {@link OperationMode#TRIAL} licences
*/
public boolean isJdbcAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, false, true, true);
return isAllowedByLicense(OperationMode.PLATINUM, true, true);
}

/**
* Determine if support for flattened object fields should be enabled.
*/
public boolean isFlattenedAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
* Determine if Vectors support should be enabled.
*/
public boolean isVectorsAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -685,7 +685,7 @@ public boolean isVectorsAllowed() {
* ODBC is available only in for {@link OperationMode#PLATINUM} and {@link OperationMode#TRIAL} licences
*/
public boolean isOdbcAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, false, true, true);
return isAllowedByLicense(OperationMode.PLATINUM, true, true);
}

/**
Expand All @@ -695,7 +695,7 @@ public boolean isOdbcAllowed() {
* {@code false}.
*/
public boolean isSpatialAllowed() {
return isActive();
return allowForAllLicenses();
}

/**
Expand All @@ -704,20 +704,14 @@ public boolean isSpatialAllowed() {
* @return true if the license is active
*/
public boolean isDataScienceAllowed() {
return isActive();
}

public synchronized boolean isTrialLicense() {
return status.mode == OperationMode.TRIAL;
return allowForAllLicenses();
}

/**
* @return true if security is available to be used with the current license type
*/
public synchronized boolean isSecurityAvailable() {
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;
Copy link
Member Author

Choose a reason for hiding this comment

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

Address feedback #51864 (comment)

}

/**
Expand Down Expand Up @@ -780,7 +774,7 @@ private static boolean isSecurityEnabled(final OperationMode mode, final boolean
* @return true is the license is compatible, otherwise false
*/
public boolean isCcrAllowed() {
return isAllowedByLicenseAndSecurity(OperationMode.PLATINUM, false, true, true);
return isAllowedByLicense(OperationMode.PLATINUM, true, true);
}

public static boolean isCcrAllowedForOperationMode(final OperationMode operationMode) {
Expand All @@ -806,26 +800,36 @@ public synchronized XPackLicenseState copyCurrentLicenseState() {
return new XPackLicenseState(this);
}

private synchronized boolean isAllowedBySecurity() {
return isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled);
}

/**
* Test whether a feature is allowed by the status of current license and security configuration.
* Test whether a feature is allowed by the status of license and security configuration.
* Note the difference to {@link #isAllowedByLicense} is this method requires security
* to be enabled.
*
* @param minimumMode The minimum license to meet or exceed
* @param needSecurity Whether security is required for feature to be allowed
* @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
*/
private synchronized boolean isAllowedByLicenseAndSecurity(
OperationMode minimumMode, boolean needSecurity, boolean needActive, boolean allowTrial) {

if (needSecurity && false == isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled)) {
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;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Address feedback #51864 (comment)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Expand Down
5 changes: 2 additions & 3 deletions ...ugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/TrainedModelConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

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);
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

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

Copy link
Contributor

@tvernum tvernum Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ywangd ywangd Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'll create a separate PR for it. thanks

}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...gin/core/src/test/java/org/elasticsearch/license/AbstractLicensesIntegrationTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return;
}
}
Expand Down
Loading