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

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Feb 10, 2020

Improve code reuse and readability.

Relates: #51864

@ywangd ywangd added >enhancement :Security/License License functionality for commercial features v8.0.0 v7.7.0 labels Feb 10, 2020
@ywangd ywangd requested a review from tvernum February 10, 2020 02:25
@elasticmachine
Copy link
Collaborator

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

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

Comment on lines 818 to 839
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.

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

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)

&& newLicense.operationMode() != License.OperationMode.PLATINUM
&& newLicense.operationMode() != License.OperationMode.ENTERPRISE
&& newLicense.operationMode() != License.OperationMode.TRIAL) {
&& XPackLicenseState.isFipsAllowedForOperationMode(newLicense.operationMode())) {
Copy link
Member Author

@ywangd ywangd Feb 10, 2020

Choose a reason for hiding this comment

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

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

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

Comment on lines 818 to 839
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
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.

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

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@ywangd
Copy link
Member Author

ywangd commented Feb 14, 2020

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

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

@ywangd ywangd requested a review from tvernum February 14, 2020 05:00
@tvernum
Copy link
Contributor

tvernum commented Feb 18, 2020

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

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

@tvernum
Copy link
Contributor

tvernum commented Feb 21, 2020

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

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

I think we'll see a bunch of:

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

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

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

But that feels pretty weird too.

I'm not too fussed though.

@ywangd
Copy link
Member Author

ywangd commented Feb 21, 2020

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

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

@ywangd ywangd merged commit 72cf242 into elastic:master Feb 21, 2020
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Feb 27, 2020
Improve code resuse and readility. Add convenience checking method which
covers most use cases without having to pass many boolean arguments.
ywangd added a commit that referenced this pull request Feb 27, 2020
Improve code resuse and readility. Add convenience checking method which
covers most use cases without having to pass many boolean arguments.
@ywangd
Copy link
Member Author

ywangd commented Feb 27, 2020

Backported:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/License License functionality for commercial features v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants