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

Add "enterprise" license OperationMode #51081

Closed
tvernum opened this issue Jan 16, 2020 · 9 comments · Fixed by #51864
Closed

Add "enterprise" license OperationMode #51081

tvernum opened this issue Jan 16, 2020 · 9 comments · Fixed by #51864
Assignees
Labels
:Security/License License functionality for commercial features

Comments

@tvernum
Copy link
Contributor

tvernum commented Jan 16, 2020

Elasticsearch supports the "enterprise" type stack license, but internally it maps that to the "platinum" operation mode.

We should create an explicit "enterprise" operating mode so that it is possible for new functionality to be tied to this license type.

Relates: #48510

@tvernum tvernum added the :Security/License License functionality for commercial features label Jan 16, 2020
@elasticmachine
Copy link
Collaborator

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

@tvernum tvernum changed the title Add "enterprise" license OperatingMode Add "enterprise" license OperationMode Jan 16, 2020
@tvernum
Copy link
Contributor Author

tvernum commented Jan 16, 2020

I think it might be time to do a refactor of XPackLicenseState when we do this. There are a lot of methods there, many of which are tied to explicit modes.

It might be better to replace the body of each of those isXyzAllowed() methods with a call to something common e.g.

    public boolean isSqlAllowed() {
        return checkMinimumLicense(OperationMode.BASIC);
    }

    public boolean isJdbcAllowed() {
        return checkMinimumLicense(OperationMode.PLATINUM);
    }

    private boolean checkMinimumLicense(OperationMode mode) {
        return checkMinimumLicense(mode, true);
    }
  
    private synchronized boolean checkMinimumLicense(OperationMode minimumMode, boolean allowTrial) {
        Status localStatus = status;
        if (localStatus.active == false ) {
            return false;
        }
        if (allowTrial && localStatus.mode == OperationMode.TRIAL) {
            return true;
        }
        return localStatus.mode >= minimumMode;
    }

@ywangd
Copy link
Member

ywangd commented Feb 4, 2020

Should every one of these methods check status.active == true? Some of them don't, e.g. isApiKeyServiceAllowed? If refactored, the active check will be applied. Or should we make exception for them?

@tvernum
Copy link
Contributor Author

tvernum commented Feb 4, 2020

Good point.
A few features, like security (and therefore API Keys) are intended to be permitted even if the license expires (active == false). We don't want to turn off security when a license expires, and if security is on, then you need to be able to authenticate (via any authentication method).

So, we need an exception.

@ywangd
Copy link
Member

ywangd commented Feb 4, 2020

There is also a MISSING operation mode which is not handled in every case. For an example, the XPackLicenseState#isSecurityEnabled handles it as default, i.e. same as gold or platinum. Is this an issue?

@tvernum
Copy link
Contributor Author

tvernum commented Feb 4, 2020

MISSING will mostly (fully?) go away with #45022
If it stays, I think we may need to look at it on a case-by-case basis but for non-security we could treat it like BASIC
For security we probably want to continue to treat it like PLATINUM so that a license problem doesn't break security.

@ywangd
Copy link
Member

ywangd commented Feb 5, 2020

Cool makes sense. Currently there is a bit inconsistency between code and javadocs. For an example, isEnrichAllowed only check whether a license is active. But its javadocs says all license types except MISSING. I lean towards removing the line in javadocs. Sounds reasonable to you?

@tvernum
Copy link
Contributor Author

tvernum commented Feb 5, 2020

Sounds reasonable to you?

Yes. If the implementation of isEnrichAllowed is clean (e.g. checkMinimumLicense(OperationMode.BASIC);) then I don't think it really needs Javadoc.

The purpose of the method is clear from the name. The behaviour is clear from the implementation. What more does the Javadoc add?

@ywangd
Copy link
Member

ywangd commented Feb 5, 2020

What more does the Javadoc add?

Nothing. Code should be treated as the ultimate reference. So I'll delete the doc line as it adds nothings but confusion. Thanks.

ywangd added a commit that referenced this issue Feb 9, 2020
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
ywangd added a commit to ywangd/elasticsearch that referenced this issue Feb 9, 2020
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
ywangd added a commit that referenced this issue Feb 21, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/License License functionality for commercial features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants