-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Add enterprise mode and refactor #51864
Add enterprise mode and refactor #51864
Conversation
Pinging @elastic/es-security (:Security/License) |
} | ||
|
||
private synchronized boolean checkMinimumLicense( | ||
OperationMode minimumMode, boolean needSecurity, boolean needActive, boolean allowTrial) { |
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.
The allowTrial
argument is always true for all usages. It can be dropped unless anyone can foresee some future usage.
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 we should leave this be. While this is currently the case, there is no guarantee that all features will be available in trial*, and having this as a parameter would help future implementations take this into account in license checks.
* We do have differences in handling trial licenses already w.r.t. to functionality already, see #isTransportTlsRequired
for instance
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.
The semantic of isTransportRequired method is a bit different from the isXxxAllowed methods. So I can see trial could be handled differently. With that being said, I agree to keep the argument for 1) future proof; 2) consistent with pre-refactor logic
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.
The semantic of isTransportRequired method is a bit different from the isXxxAllowed methods. So I can see trial could be handled differently.
agreed, I was just trying to make a point that it is not unheard of to have the need to treat trial differently.
@@ -880,4 +809,24 @@ public static boolean isPlatinumOrTrialOperationMode(final OperationMode operati | |||
public synchronized XPackLicenseState copyCurrentLicenseState() { | |||
return new XPackLicenseState(this); | |||
} | |||
|
|||
private synchronized boolean checkSecurityEnabled() { |
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.
It is possible to replace this method with just a call of checkMinimumLicense(MISSING, true, false, true)
. But I feel a separate method clears the intention and is more readable.
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.
It is possible to replace this method with just a call of checkMinimumLicense(MISSING, true, false, true)
is it ? That wouldn't handle the intricacies of isSecurityExplicitlyEnabled
vs isSecurityEnabled
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.
isSecurityEnabled
is called inside checkMinimumLicense
as well. So it can handle the subtlety. I do prefer to keep it separated for clarity.
public synchronized boolean isStatsAndHealthAllowed() { | ||
return status.active; | ||
public boolean isStatsAndHealthAllowed() { | ||
return isActive(); |
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.
isActive
is an existing synchronized method.
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'd prefer a method name like allowForAllLicenses()
Most of these isXYZAllowed()
methods are added by people who deal with licensing questions very rarely. They just want to come in and implement a method in the most obvious way.
They don't know what "active" means (I forget what it means half the time), and their likely response here is "I'll just copy one of these other methods and hope that it's right". And it will be, but I'd rather we had something that was clearly correct to them, so that they don't need to hope. And that is a method that is named to be exactly in line with their requirement.
There's a good chance the PR reviewer isn't an expert on licensing either, so a method that copies this code isn't obviously correct to the reviewer - they need to look at how other methods are implemented to check. A method that has the equivalent of isMyFeatureAllowed() { return thisIsAllowedForAllLicenseTypes(); }
is correct just from reading the diff.
I'm very open to discussion on a preferred name, but I'm pretty keen for these to be readable in a literal sense.
I noticed some inconsistency in how the However a few of the |
Platinum operation mode is used as default model in following places:
While I don't think they need to be changed (to the new Enterprise mode), some confirmation would be helpful. The platinum mode is also used as default in some tests, but I think they are less important. |
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.
Looks good Yang, I added some comments
return isSecurityEnabled(status.mode, isSecurityExplicitlyEnabled, isSecurityEnabled); | ||
} | ||
|
||
private synchronized boolean checkMinimumLicense( |
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.
The name of the method threw me off while initially reading the code. I don't have a good suggestion that isn't too long for a method name, but can we add a line of javadoc explaining that this checks if the current license is at least minimumMode
and the rest of the requirements are satisfied ?
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 agree that the name is not good. I am not happy with the usage of check
since this name usually feels like a void method. How about isAllowedByLicenseAndSecurity
? I'll also add javadoc. Thanks
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.
} | ||
|
||
private synchronized boolean checkMinimumLicense( | ||
OperationMode minimumMode, boolean needSecurity, boolean needActive, boolean allowTrial) { |
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 we should leave this be. While this is currently the case, there is no guarantee that all features will be available in trial*, and having this as a parameter would help future implementations take this into account in license checks.
* We do have differences in handling trial licenses already w.r.t. to functionality already, see #isTransportTlsRequired
for instance
@@ -31,7 +31,7 @@ | |||
public class XPackLicenseState { | |||
|
|||
public static final Set<OperationMode> FIPS_ALLOWED_LICENSE_OPERATION_MODES = |
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.
We can probably remove this and expose an isFipsModeAllowed()
method here. Happy with a followup but I think it makes sense for the current refactoring
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.
Sounds like a reasonable change to me. There are a few other public static boolean isXxxAllowedForOperationMode
methods. This can be another addition.
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.
@@ -880,4 +809,24 @@ public static boolean isPlatinumOrTrialOperationMode(final OperationMode operati | |||
public synchronized XPackLicenseState copyCurrentLicenseState() { | |||
return new XPackLicenseState(this); | |||
} | |||
|
|||
private synchronized boolean checkSecurityEnabled() { |
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.
It is possible to replace this method with just a call of checkMinimumLicense(MISSING, true, false, true)
is it ? That wouldn't handle the intricacies of isSecurityExplicitlyEnabled
vs isSecurityEnabled
} | ||
|
||
public static boolean isPlatinumOrTrialOperationMode(final OperationMode operationMode) { | ||
return operationMode == OperationMode.PLATINUM || operationMode == OperationMode.TRIAL; | ||
public static boolean isPlatinumPlusOrTrialOperationMode(final OperationMode operationMode) { |
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.
Could we make this generic too ? Something like isMinimumLicense(final OperationMode minimumMode, final OperationMode currentMode)
? The callers of this should know what the minimum allowed op mode is
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 thought about consolidate this with checkMinimumLicense
method as well. I didn't do it because this method is static and not synchronised. Also it is used as a predicate for RemoteClusterLicenseChecker. So it has some ripple effect.
It will be more feasible if it does not need to be consolidated with checkMinimumLicense
. I'll give it a go.
PS: I am confused by the inconsistent synchronisation involved for license check. These static methods can possibly run into racing issues. Is this not a concern? If so why other methods require synchronize?
There are pair of methods for some checks, e.g. isMachineLearningAllowedForOperationMode
and isMachineLearningAllowed
, where the former is static and the latter is non-static and synchronized. How so?
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.
This methoid is now refactored to be isAllowedByOperationMode. It is also used by the isAllowedByLicenseAndSecurity (previously checkMinimumLicense) method. So that's an extra generification.
.issuedTo("customer") | ||
.issuer("elasticsearch") | ||
.maxNodes(5); | ||
if (version == License.VERSION_START) { | ||
builder.subscriptionType((type != null) ? type : randomFrom("dev", "gold", "platinum", "silver")); | ||
builder.feature(randomAlphaOfLength(10)); | ||
} | ||
if ("enterprise".equals(licenseType)) { | ||
builder.version(License.VERSION_ENTERPRISE).maxResourceUnits(5).maxNodes(-1); |
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.
nit: I think we generally prefer separate calls for builder as it is clearer to read, or at least split them in different lines. Also maxResourceUnits can be called with a randomIntegerInRange
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.
Splitted them in different lines. Also updated for random integer.
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.
Just minor nits ( more suggestions, than requests for changes ) and a +1 to continue the refactoring as your analysis has indicated that we can (removing local status variable, make isXXXAllowed call isActive()
.
Platinum operation mode is used as default model in following places:
* [OperationModeFileWatcher](https://github.com/elastic/elasticsearch/blob/7.6/x-pack/plugin/core/src/main/java/org/elasticsearch/license/OperationModeFileWatcher.java#L35). * [TrainedModelConfig#build](https://github.com/elastic/elasticsearch/blob/7.6/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/TrainedModelConfig.java#L583) * [TransportPutDataFrameAnalyticsAction](https://github.com/elastic/elasticsearch/blob/7.6/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java#L96) * [TransportPutTrainedModelAction#masterOperation](https://github.com/elastic/elasticsearch/blob/7.6/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutTrainedModelAction.java#L101) * [TransportStartDataFrameAnalyticsAction](https://github.com/elastic/elasticsearch/blob/7.6/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDataFrameAnalyticsAction.java#L128) * [AnalyticsResultProcessor#createTrainedModelConfig](https://github.com/elastic/elasticsearch/blob/7.6/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/dataframe/process/AnalyticsResultProcessor.java#L188)
While I don't think they need to be changed (to the new Enterprise mode), some confirmation would be helpful. The platinum mode is also used as default in some tests, but I think they are less important.
All of these usages are explicit, reflect the desired level of features and are not affected by the introduction of a new tier so I agree with your analysis that these shouldn't be changed.
Also +1 to changing all isIndexLifeCycleAllowed
, isEnrichAllowed
and isSpatialAllowed
implementations to return isActive()
@@ -138,9 +138,9 @@ public RemoteClusterLicenseChecker(final Client client, final Predicate<License. | |||
this.predicate = predicate; | |||
} | |||
|
|||
public static boolean isLicensePlatinumOrTrial(final XPackInfoResponse.LicenseInfo licenseInfo) { | |||
public static boolean isAllowedByLicenseInfo(final XPackInfoResponse.LicenseInfo licenseInfo) { |
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.
nit:
public static boolean isAllowedByLicenseInfo(final XPackInfoResponse.LicenseInfo licenseInfo) { | |
public static boolean isAllowedByLicense(final XPackInfoResponse.LicenseInfo licenseInfo) { |
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.
Updated
@@ -274,14 +274,20 @@ public static License generateSignedLicense(String type, int version, long issue | |||
.version(version) | |||
.expiryDate(System.currentTimeMillis() + expiryDuration.getMillis()) | |||
.issueDate(issue) | |||
.type(licenseType) | |||
.type(licenseType ) |
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.
nit: extra space
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.
Fixed
.issuedTo("customer") | ||
.issuer("elasticsearch") | ||
.maxNodes(5); | ||
if (version == License.VERSION_START) { | ||
builder.subscriptionType((type != null) ? type : randomFrom("dev", "gold", "platinum", "silver")); | ||
builder.feature(randomAlphaOfLength(10)); | ||
} | ||
if ("enterprise".equals(licenseType)) { | ||
builder |
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.
nit:
builder | |
builder.version(License.VERSION_ENTERPRISE) |
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.
Updated
.issuedTo("customer") | ||
.issuer("elasticsearch") | ||
.maxNodes(5); | ||
if (version == License.VERSION_START) { | ||
builder.subscriptionType((type != null) ? type : randomFrom("dev", "gold", "platinum", "silver")); | ||
builder.feature(randomAlphaOfLength(10)); | ||
} | ||
if ("enterprise".equals(licenseType)) { | ||
builder | ||
.version(License.VERSION_ENTERPRISE) |
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.
nit:
.version(License.VERSION_ENTERPRISE) |
} | ||
|
||
private synchronized boolean checkMinimumLicense( | ||
OperationMode minimumMode, boolean needSecurity, boolean needActive, boolean allowTrial) { |
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.
The semantic of isTransportRequired method is a bit different from the isXxxAllowed methods. So I can see trial could be handled differently.
agreed, I was just trying to make a point that it is not unheard of to have the need to treat trial differently.
} | ||
|
||
/** | ||
* Test whether a feature is allowed by the status of current license and security config. |
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.
nit:
* Test whether a feature is allowed by the status of current license and security config. | |
* Test whether a feature is allowed by the status of current license and security configuration. |
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.
Updated
private synchronized boolean isAllowedByLicenseAndSecurity( | ||
OperationMode minimumMode, boolean needSecurity, boolean needActive, boolean allowTrial) { | ||
|
||
final Status localStatus = status; |
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 agree with your reasoning that since status
is not volatile, we can remove the local variables from here and other places ( I don't think there was an explicit reason this was not done in #33396 )
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.
Removed. Also see my comments below
Repost slack message here for reference:
Both of above suggestions (add synchronized, remove local copy) are now implemented. |
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.
LGTM Yang, thanks for the iterations
Add enterprise operation mode to properly map enterprise license. Aslo refactor XPackLicenstate class to consolidate license status and mode checks. This class has many sychronised methods to check basically three things: * Minimum operation mode required * Whether security is enabled * Whether current license needs to be active Depends on the actual feature, either 1, 2 or all of above checks are performed. These are now consolidated in to 3 helper methods (2 of them are new). The synchronization is pushed down to the helper methods so actual checking methods no longer need to worry about it. resolves: elastic#51081
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.
If I'm listed as a reviewer, & haven't reviewed yet, can you give me a ping before you merge?
I often choose to wait for the other reviewer to finish so that I'm not duplicating their work, and I need a window after they approve to run through and look at it.
@@ -256,6 +256,7 @@ public void registerLicense(final PutLicenseRequest request, final ActionListene | |||
"] 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 |
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'd prefer that we change this to either be < PLATINUM
, or use methods on XPackLicenseState so that we don't have a bunch of code that assumes the set of licenses and their ordering.
public synchronized boolean isStatsAndHealthAllowed() { | ||
return status.active; | ||
public boolean isStatsAndHealthAllowed() { | ||
return isActive(); |
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'd prefer a method name like allowForAllLicenses()
Most of these isXYZAllowed()
methods are added by people who deal with licensing questions very rarely. They just want to come in and implement a method in the most obvious way.
They don't know what "active" means (I forget what it means half the time), and their likely response here is "I'll just copy one of these other methods and hope that it's right". And it will be, but I'd rather we had something that was clearly correct to them, so that they don't need to hope. And that is a method that is named to be exactly in line with their requirement.
There's a good chance the PR reviewer isn't an expert on licensing either, so a method that copies this code isn't obviously correct to the reviewer - they need to look at how other methods are implemented to check. A method that has the equivalent of isMyFeatureAllowed() { return thisIsAllowedForAllLicenseTypes(); }
is correct just from reading the diff.
I'm very open to discussion on a preferred name, but I'm pretty keen for these to be readable in a literal sense.
final boolean isSecurityCurrentlyEnabled = isSecurityEnabled(mode, isSecurityExplicitlyEnabled, isSecurityEnabled); | ||
return isSecurityCurrentlyEnabled; | ||
public boolean isApiKeyServiceAllowed() { | ||
return isAllowedBySecurity(); |
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.
This reads strangely to me.
I realise that, in terms of readability, it's logically equivalent to the isAllowedByLicenseAndSecurity
method, but here with no parameters, it just looks like a mistake.
Given this is the only place it's called, I would just replace it with isAllowedByLicenseAndSecurity(OperationMode.BASIC, true, false, true);
return false; | ||
} | ||
public boolean isWatcherAllowed() { | ||
return isAllowedByLicenseAndSecurity(OperationMode.STANDARD, false, true, true); |
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 don't like the version of this method that passes false
for security.
It's calling a method named isAllowedBy___Security` and then says "oh, but not really security".
Reading this method, on its own, if you aren't intimately familiar with what isAllowedByLicenseAndSecurity
does, and what those parameters mean, a reader is left:
- Wondering why watcher cares about security (because it's in that method name).
- What all those parameters mean, and whether they're correct.
People adding a new license check for paid functionality shouldn't be expected to make decisions about so many parameters. There should be one obvious method with the smallest set of parameters that they can use.
} | ||
|
||
public static boolean isTransformAllowedForOperationMode(final OperationMode operationMode) { | ||
// any license (basic and upwards) | ||
return operationMode != License.OperationMode.MISSING; | ||
} | ||
|
||
public static boolean isFipsAllowedForOperationMode(final OperationMode operationMode) { | ||
return isAllowedByOperationMode(operationMode, OperationMode.PLATINUM, true); | ||
} |
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.
It bothers me that we have this method, and them re-implement the same OperationMode
check for FIPS in LicenseService
@@ -796,7 +708,7 @@ public synchronized boolean isTrialLicense() { | |||
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.TRIAL || mode == OperationMode.BASIC || mode == OperationMode.ENTERPRISE; |
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 we can just replace with with mode != MISSING
.
@@ -243,7 +243,8 @@ public boolean isAvailableWithLicense(XPackLicenseState licenseState) { | |||
} | |||
|
|||
// The model license does not matter, this is the highest licensed level | |||
if (licenseState.isActive() && XPackLicenseState.isPlatinumOrTrialOperationMode(licenseState.getOperationMode())) { | |||
if (licenseState.isActive() && XPackLicenseState.isAllowedByOperationMode( | |||
licenseState.getOperationMode(), License.OperationMode.PLATINUM, true)) { |
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.
Strictly speaking, this check is incorrect because there is a race condition between the 2 calls. I don't think we should be perpetuating this style of call, and should actually push it to a single call that passes in the licenseState
object and checks both active
and operation mode.
I will make sure you get notified in the future. |
Add enterprise operation mode to properly map enterprise license. Aslo refactor XPackLicenstate class to consolidate license status and mode checks. This class has many sychronised methods to check basically three things: * Minimum operation mode required * Whether security is enabled * Whether current license needs to be active Depends on the actual feature, either 1, 2 or all of above checks are performed. These are now consolidated in to 3 helper methods (2 of them are new). The synchronization is pushed down to the helper methods so actual checking methods no longer need to worry about it. resolves: #51081
Add enterprise operation mode to properly map enterprise license.
Aslo refactor XPackLicenstate class to consolidate license status and mode checks,
which turns out to be the majority changes of this PR.
This class has many sychronised methods to check basically three things:
Depends on the actual feature, either 1, 2 or all of above checks are performed. These are now consolidated in to 3 helper methods (2 of them are new).
There are also some other methods that also checks the same things but they are static method without synchronisation. Therefore they are left unchanged, e.g. isMachineLearningAllowedForOperationMode, isTransformAllowedForOperationMode.
resolves: #51081