Add license feature usage api #59342

rjernst · 2020-07-09T20:55:17Z

This commit adds a new api to track when gold+ features are used within
x-pack. The tracking is done internally whenever a feature is checked
against the current license. The output of the api is a list of each
used feature, which includes the name, license level, and last time it
was used. In addition to a unit test for the tracking, a rest test is
added which ensures starting up a default configured node does not
result in any features registering as used.

There are a couple features which currently do not work well with the
tracking, as they are checked in a manner that makes them look always
used. Those features will be fixed in followups, and in this PR they are
omitted from the feature usage output.

This commit adds a new api to track when gold+ features are used within x-pack. The tracking is done internally whenever a feature is checked against the current license. The output of the api is a list of each used feature, which includes the name, license level, and last time it was used. In addition to a unit test for the tracking, a rest test is added which ensures starting up a default configured node does not result in any features registering as used. There are a couple features which currently do not work well with the tracking, as they are checked in a manner that makes them look always used. Those features will be fixed in followups, and in this PR they are omitted from the feature usage output.

elasticmachine · 2020-07-09T20:55:19Z

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

ywangd · 2020-07-10T05:14:47Z

...ain/java/org/elasticsearch/xpack/security/authz/interceptor/BulkShardRequestInterceptor.java

-        boolean shouldIntercept = licenseState.isSecurityEnabled() && licenseState.checkFeature(Feature.SECURITY_DLS_FLS);
+        boolean shouldIntercept = licenseState.isSecurityEnabled();
+        var licenseChecker = new MemoizedSupplier<>(() -> licenseState.checkFeature(Feature.SECURITY_DLS_FLS));


The existing expression of licenseState.isSecurityEnabled() && licenseState.checkFeature(Feature.SECURITY_DLS_FLS) has a potential racing issue between the two license checks. I noticed we have been somewhat inconsistent in handling this. In some places, e.g. ResizeRequestInterceptor, we use licenseState.copyCurrentLicenseState() to ensure checks are always done against the same license. In other places including this one, no special handling is done. The new changes also retain this behaviour. My question is: what is our recommend approach for this subtle issue? I am OK with either approach as long as it is consistently applied.

I am also curious about the benefit of the new MemoizedSupplier. Why do we need to check the license deeper in the loop? Isn't it sufficient and more efficient to check in the outermost if statement?

what is our recommend approach for this subtle issue?

It is true there is a race condition, but in practice I don't think we need to worry about it. This is an extreme edge case, where the license changes mid request. AFAIU, the copy method was added to handle the race condition in tests, not for any problem found by users. That said, the long term solution to this is to change all of our checks to use a stashed copy in the ThreadContext that would be inserted when the request is first received. That idea is described in #53909.

I am also curious about the benefit of the new MemoizedSupplier. Why do we need to check the license deeper in the loop?

We don't know if DLS/FLS is actually used until we resolve the each indices' access controls inside the loop. Thus we need to check when resolving each index. It is true this is similar to the problem described above, and could be partially solved by copying the license state. However, it also means needlessly updating the last used time in quick succession. I could go either way, but thought reducing those updates was worthwhile given the simpleness of implementing the MemoizedSupplier.

ywangd · 2020-07-10T05:21:21Z

...ain/java/org/elasticsearch/xpack/security/authz/interceptor/BulkShardRequestInterceptor.java

                    boolean dls = indexAccessControl.getDocumentPermissions().hasDocumentLevelPermissions();
                    if (fls || dls) {
-                        if (bulkItemRequest.request() instanceof UpdateRequest) {
+                        if (licenseChecker.get() && bulkItemRequest.request() instanceof UpdateRequest) {


Ah I see, the license checking is moved here because we need to accurately tracking the usage, i.e. only check it when it is absolutely necessary. In this case, I wonder whether we should check it after the instanceof, i.e. bulkItemRequest.request() instanceof UpdateRequest && licenseChecker.get().

ywangd · 2020-07-10T05:25:49Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/TransportGetFeatureUsageAction.java

+
+    @Override
+    protected void doExecute(Task task, GetFeatureUsageRequest request, ActionListener<GetFeatureUsageResponse> listener) {
+        Map<XPackLicenseState.Feature, Long> featureUsage = licenseState.getLastUsed();


IIUC, the feature usage is reported for the local node only? So in cloud with coordinating node setup, how can user get the feature usages for nodes other than the coordinating ones?

Cloud will need to hit every node in the cluster, not just coordinating nodes. AFAIU they can do this (this would be on their backend infrastructure side, and so would not be going through eg load balancers). It would be similar to eg health checks that hit each node.

Ok so this API is more geared towards Cloud internal management instead of end-users. Then it is fine.

rjernst · 2020-07-13T22:20:54Z

@elasticmachine run elasticsearch-ci/2

ywangd · 2020-07-14T02:01:05Z

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

    public XPackLicenseState copyCurrentLicenseState() {
        return executeAgainstStatus(status ->
-            new XPackLicenseState(listeners, isSecurityEnabled, isSecurityExplicitlyEnabled, status));
+            new XPackLicenseState(listeners, isSecurityEnabled, isSecurityExplicitlyEnabled, status, lastUsed, epochMillisProvider));


This worries me a bit since a copy can now mutate the original object. It is not an issue for the time being since no such usage exists. But might be a confusing behaviour in future?

That's exactly the intention. The copy is meant for read purposes, but mutation purposes must be unified, otherwise we would would not be able to check what checks have been done across copies.

ywangd · 2020-07-14T02:13:10Z

...ava/org/elasticsearch/xpack/security/authz/interceptor/IndicesAliasesRequestInterceptor.java

            final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();
            final AuditTrail auditTrail = auditTrailService.get();
            if (frozenLicenseState.isSecurityEnabled()) {


We are using both frozenLicenseState here and the original licenseState with MemoizedSupplier below. Given the current change, there is no benefit of having frozenLicenseState anymore. It can probably just be removed.

ywangd · 2020-07-14T02:13:43Z

...c/main/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptor.java

                          ActionListener<Void> listener) {
        if (requestInfo.getRequest() instanceof ResizeRequest) {
            final ResizeRequest request = (ResizeRequest) requestInfo.getRequest();
            final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();


Same here for using frozenLicenseState and licenseState together.

ywangd · 2020-07-14T02:27:53Z

...only-node/src/main/java/org/elasticsearch/cluster/coordination/VotingOnlyNodeFeatureSet.java

    @Override
    public boolean available() {
-        return licenseState != null && licenseState.isAllowed(Feature.VOTING_ONLY);
+        return licenseState != null && licenseState.checkFeature(Feature.VOTING_ONLY);


This method does not seem to be used anywhere? But the inner class VotingOnlyNodeFeatureSet#UsageInfoAction also has a availabe() method which internally uses licenseState.checkFeature. But all other XPackInfoFeatureTransportAction uses licenseState.isAllowed. Is this inconsistency intended?

I've fixed the checkFeature usage, it was incorrect. I'll address the unused class in a followup.

ywangd

LGTM.

I left a few comments which would be great if they could be addressed. But none of them is critical enough to delay the approval.

This commit adds a new api to track when gold+ features are used within x-pack. The tracking is done internally whenever a feature is checked against the current license. The output of the api is a list of each used feature, which includes the name, license level, and last time it was used. In addition to a unit test for the tracking, a rest test is added which ensures starting up a default configured node does not result in any features registering as used. There are a couple features which currently do not work well with the tracking, as they are checked in a manner that makes them look always used. Those features will be fixed in followups, and in this PR they are omitted from the feature usage output.

rjernst added >refactoring :Security/License License functionality for commercial features v8.0.0 v7.9.0 labels Jul 9, 2020

elasticmachine added the Team:Security Meta label for security team label Jul 9, 2020

rjernst added 6 commits July 9, 2020 14:04

checkstyle

9c389b1

fix compile

bc14423

fix local state insanity

2f36787

testing conventions...

d54ff1a

unit tests

057b83b

iter

5bcbfb2

ywangd reviewed Jul 10, 2020

View reviewed changes

rjernst added 4 commits July 10, 2020 12:09

tests iter

c23c2e7

Merge branch 'master' into refactor_license13

e2c04bf

another test

70042a7

fix a bunch of missed checkFeature -> isAllowed for telmetry

d95a4ea

ywangd reviewed Jul 14, 2020

View reviewed changes

ywangd approved these changes Jul 14, 2020

View reviewed changes

rjernst added 2 commits July 14, 2020 10:07

address more feedback

228d882

Merge branch 'master' into refactor_license13

fb6998d

rjernst merged commit 5d6a2dd into elastic:master Jul 14, 2020

rjernst deleted the refactor_license13 branch July 14, 2020 19:25

jasontedor mentioned this pull request Jul 31, 2020

Provide more context around the features lost during license expiration #60541

Open

This was referenced Sep 4, 2020

Feature Usage always reports Token Service #61956

Closed

Enable license feature usage for Security Realms #61963

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Add license feature usage api #59342

Add license feature usage api #59342

Uh oh!

Conversation

rjernst commented Jul 9, 2020

Uh oh!

elasticmachine commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jul 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants