-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Refactor license checking #52118
Changes from 1 commit
8eabbf1
f1c931b
9d1164c
4a20a97
9c9afb0
e1da109
95877fc
a34e3ca
88a3e62
e1eca99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
return status.active; | ||
} | ||
|
||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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(); | ||
} | ||
|
||
/** | ||
|
@@ -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. */ | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -488,7 +488,7 @@ public boolean isWatcherAllowed() { | |
* @return true if the license is active | ||
*/ | ||
public boolean isMonitoringAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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(); | ||
} | ||
|
||
/** | ||
|
@@ -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(); | ||
} | ||
|
||
/** | ||
|
@@ -617,7 +617,7 @@ public boolean isDeprecationAllowed() { | |
* {@code false}. | ||
*/ | ||
public boolean isUpgradeAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
/** | ||
|
@@ -627,7 +627,7 @@ public boolean isUpgradeAllowed() { | |
* {@code false}. | ||
*/ | ||
public boolean isIndexLifecycleAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
/** | ||
|
@@ -637,7 +637,7 @@ public boolean isIndexLifecycleAllowed() { | |
* {@code false}. | ||
*/ | ||
public boolean isEnrichAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
/** | ||
|
@@ -653,7 +653,7 @@ public synchronized boolean isEqlAllowed() { | |
* Determine if SQL support should be enabled. | ||
*/ | ||
public boolean isSqlAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
/** | ||
|
@@ -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(); | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -695,7 +695,7 @@ public boolean isOdbcAllowed() { | |
* {@code false}. | ||
*/ | ||
public boolean isSpatialAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
/** | ||
|
@@ -704,7 +704,7 @@ public boolean isSpatialAllowed() { | |
* @return true if the license is active | ||
*/ | ||
public boolean isDataScienceAllowed() { | ||
return isActive(); | ||
return allowForAllLicenses(); | ||
} | ||
|
||
public synchronized boolean isTrialLicense() { | ||
|
@@ -715,9 +715,7 @@ public synchronized boolean isTrialLicense() { | |
* @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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Address feedback #51864 (comment) |
||
} | ||
|
||
/** | ||
|
@@ -780,7 +778,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) { | ||
|
@@ -806,26 +804,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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Address feedback #51864 (comment) The method There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
and use that method everywhere we can. Put yourself in the shows of someone who is implementing a new license check. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Address feedback #51864 (comment) |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
But perhaps we should make that a separate PR so that the ML team can review it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll create a separate PR for it. thanks |
||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address feedback #51864 (comment)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?