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.

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

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

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

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