Skip to content

Conversation

@tvernum
Copy link
Contributor

@tvernum tvernum commented Nov 23, 2020

The existing FeatureUsage API tracks the last time that a feature was
used. This is useful for most features, but some features like
security realms are on by virtue of being configured on the node, and
their use is not tracked by events but simply by being turned on.

This change modifies the feature tracking to support turning a feature
on permanently, so that the last used time is always returned as
"now".

It also supports having identifiers for such "always on" features, so
that callers can understand what entity in elasticsearch caused the
feature to be tracked as on (e.g. the name of the security realm).

The existing FeatureUsage API tracks the last time that a feature was
used. This is useful for most features, but some features like
security realms are on by virtue of being configured on the node, and
their use is not tracked by events but simply by being turned on.

This change modifies the feature tracking to support turning a feature
on permanently, so that the last used time is always returned as
"now".

It also supports having identifiers for such "always on" features, so
that callers can understand what entity in elasticsearch cause the
feature to be tracked as on (e.g. the name of the security realm).
@tvernum tvernum added >non-issue :Security/License License functionality for commercial features v8.0.0 v7.11.0 labels Nov 23, 2020
@tvernum tvernum requested review from rjernst and ywangd November 23, 2020 05:38
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Nov 23, 2020
@elasticmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor Author

tvernum commented Nov 23, 2020

This is just the tracking side of the change - it doesn't actually enable it for any licensed features.
After I merge, I plan to rebase #61963 on top so that configured realms can be tracked as "on-by-config".

@rjernst I opted to go with an explicit method call, rather than a defined list of features for a couple of reasons:

  1. (the main reason) it made it easier to add the identifiers part if they could only be applied to on-by-config features.
  2. Given that this will be used infrequently, it seems like having it called explicitly in the relevant places would be a small, manageable change (which was not true for the original tracking working).

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

I left a few comments/discussion points.

public FeatureUsage(Feature feature, long lastUsed, Set<String> identifiers) {
this.feature = feature;
this.lastUsed = lastUsed;
this.identifiers = Objects.requireNonNull(identifiers);
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, most feature usages have no identifier. Would it be better to allow null for identifiers, which in turn can be skipped in the final response returned to users instead of cluttering the response with "identifiers": []?

return isAllowedByLicense(minimumMode, true);
}

public static class FeatureUsage {
Copy link
Member

Choose a reason for hiding this comment

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

This class has no explicit info about whether the feature is "on-by-default". Does it matter? Or it's more of a documentation issue?

Comment on lines +534 to +536
public synchronized boolean setFeatureActive(Feature feature, String... identifiers) {
boolean allowed = checkFeature(feature);
if (allowed && isTrackableFeature(feature)) {
Copy link
Member

Choose a reason for hiding this comment

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

A few interesting points here:

  • Do we need a pairing unset method? This could be useful if we have any "on-by-configuration" features that can turned on and off on the fly? Or if the license expires or gets deleted or is somehow downgraded, I assume it's the caller's responsibility to call the unset method so it's removed from the tracking? Or should unlicensed features be filtered out at getFeatureUsage time?
  • If the feature is not trackable, should we throw an assert error to notify the developer that license is not configured correctly?
  • How can we ensure that every "on-by-default" feature (past and future) is indeed registered with this method?

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I wonder if we could simplify this a bit by not needing to distinguish between "always on" and tracked features within code, but just in the storage of the tracked values and in the output of the feature usage response.

The general idea would be to continue to use the feature -> long map, but set the value to -1 if the feature is "always on" (defined in the Feature enum). Then in serialization, the lastUsedTime becomes an optional, and if it is null, we do not output it. One advantage to this approach is the license checking methods do not need to change, and how it is tracked continues to be an implementation detail relevant to the particular feature.

@tvernum
Copy link
Contributor Author

tvernum commented Dec 2, 2020

I think we could do something to remove the need to store 2 feature maps if we wanted.

However, attempting to do this transparently creates an issue of the identifiers part of the change.
The caller (e.g. Realms) is the only thing that know the ids we want to associate with the feature. So, it will need to call some method to register those identifiers with XPackLicenseState.

So, at the very least we'd need something like:

public void setFeatureIdentifiers(Feature f, Collection<String> ids) {
}

That doesn't have to turn the feature "on", but the calling code isn't made any simpler, shorter or less-entangled in licensing by taking that approach.
So, it just seemed simpler to make the whole thing explicit.

@rjernst
Copy link
Member

rjernst commented Dec 4, 2020

I concede the point about identifiers necessitating a new way to mark the feature as used. However, I would rather do this through an overload of checkFeature. I see the value in using two maps for that purpose, but I don't think we need a separate concept naming wise, just an assertion that the identifier variant is used with a persistent feature (which can still be noted in the Feature enum values).

@tvernum tvernum removed the v7.11.0 label Dec 8, 2020
@tvernum
Copy link
Contributor Author

tvernum commented Dec 8, 2020

I've dropped the 7.11.0 label from this PR. There are a number of open questions about feature usage tracking, and we don't anticipate having answers in time for 7.11, we'll select a new release target when we know more.

@tvernum
Copy link
Contributor Author

tvernum commented Jul 30, 2021

Closing this PR. We're taking a different direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :Security/License License functionality for commercial features Team:Security Meta label for security team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants