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.

Sign up for GitHub

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

Jump to bottom

Generalize remote license checker #32971

Merged
merged 24 commits into from
Aug 20, 2018
Merged

Generalize remote license checker #32971

merged 24 commits into from
Aug 20, 2018

Conversation

jasontedor
Copy link
Member

Machine learning has baked a remote license checker for use in checking license compatibility of a remote license. This remote license checker has general usage for any feature that relies on a remote cluster. For example, cross-cluster replication will pull changes from a remote cluster and require that the local and remote clusters have platinum licenses. This commit generalizes the remote cluster license check for use in cross-cluster replication.

@jasontedor
Generalize remote license checker
a9c5d43 
Machine learning has baked a remote license checker for use in checking
license compatibility of a remote license. This remote license checker
has general usage for any feature that relies on a remote cluster. For
example, cross-cluster replication will pull changes from a remote
cluster and require that the local and remote clusters have platinum
licenses. This commit generalizes the remote cluster license check for
use in cross-cluster replication.
@jasontedor jasontedor added review v7.0.0 :Security/License License functionality for commercial features v6.5.0 labels Aug 18, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member Author

@davidkyle Your review would be appreciated here as the original author of this functionality.

@tbrooks8 Your review would be appreciated here as you are in license purgatory. ❤️

@jasontedor
Fix forbidden API violations
c28c950
@jasontedor
Make class final
87ad218
@jasontedor
Add missing newline
3744702
@jasontedor
Merge branch 'master' into generalize-remote-license-check
844830d 
* master:
  NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (elastic#32764)
  [DOCS] Splits the users API documentation into multiple pages (elastic#32825)
  [DOCS] Splits the token APIs into separate pages (elastic#32865)
  [DOCS] Creates redirects for role management APIs page
  Bypassing failing test PainlessDomainSplitIT#testHRDSplit (elastic#32966)
  TEST: Mute testRetentionPolicyChangeDuringRecovery
  [DOCS] Fixes more broken links to role management APIs
  [Docs] Tweaks and fixes to rollup docs
  [DOCS] Fixes links to role management APIs
  [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature
  [DOCS] Splits the roles API documentation into multiple pages (elastic#32794)
  [TEST]  Run pre 6.4 nodes in non-FIPS JVMs (elastic#32901)
  Make Geo Context Mapping Parsing More Strict (elastic#32821)
@jasontedor
Simplify error message
2f8d5ad
@jasontedor
Remove import
0ed0b7e
@jasontedor
Fix line length violation
e16617a
@jasontedor
Fix terminology
952d501
@jasontedor
Fix bug in getting remote client!
8f25930
@jasontedor
Fix ML error handling
901acdc
@jasontedor
Preserve context after remote license check
222dea9
jasontedor
jasontedor commented Aug 19, 2018
View reviewed changes
x-pack/plugin/core/src/main/java/org/elasticsearch/license/RemoteClusterLicenseChecker.java

private void remoteClusterLicense(final String clusterAlias, final ActionListener<XPackInfoResponse> listener) {
final ThreadContext threadContext = client.threadPool().getThreadContext();
final ContextPreservingActionListener<XPackInfoResponse> contextPreservingActionListener =
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a bug in the existing implementation. This bug does not impact the existing usage of checking the license of a remote cluster. This is because the existing context is not needed when starting a data feed in ML. However, in CCR after we check the remote license we will execute another action (the follow index action). Prior to this fix, that action would execute under the system context. The system context runs with limited privileges. This means that invoking that additional action would not have privileges to execute. Instead, this action should be executed with the original context to preserve the user that initiated the action. This is why we must preserve the context here. There is a test for this behavior in the unit tests for this class.

@jasontedor
Copy link
Member Author

@davidkyle @tbrooks8 This is ready for review now. I have built a proof-of-concept in CCR that uses this refactoring and all tests pass. In particular, we have a test that does cross-cluster replication across two clusters with security and attempts to do cross-cluster replication. This test was failing prior to the context-preserving change that I made in 222dea9. With that change, the entire CCR test suite passes now, including this cross-cluster with security test.

davidkyle
davidkyle approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM, left a comment on constructing one of the error messages

.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDatafeedAction.java Outdated
return new ElasticsearchStatusException(message, RestStatus.BAD_REQUEST, new Exception(cause.getMessage()));
private ElasticsearchStatusException createUnknownLicenseError(
final String datafeedId, final List<String> remoteIndices, final Exception cause) {
final String remoteClusterQualifier = remoteIndices.size() == 1 ? "a remote cluster" : "remote clusters";
Copy link
Member

Choose a reason for hiding this comment

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

There could be a number indices on a single remote cluster or several remote clusters remoteIndices.size() is not a measure of the number of remote clusters RemoteClusterLicenseChecker.remoteClusterAliases(remoteIndices).size() should be used instead.

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 for that @davidkyle! I pushed 5c097f3.

davidkyle
davidkyle approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM thank for making the change and indeed, the entire refactoring

@jasontedor jasontedor merged commit 9050c7e into elastic:master Aug 20, 2018
jasontedor added a commit that referenced this pull request Aug 20, 2018
@jasontedor
Generalize remote license checker (#32971)
9088d81 
Machine learning has baked a remote license checker for use in checking
license compatibility of a remote license. This remote license checker
has general usage for any feature that relies on a remote cluster. For
example, cross-cluster replication will pull changes from a remote
cluster and require that the local and remote clusters have platinum
licenses. This commit generalizes the remote cluster license check for
use in cross-cluster replication.
jasontedor added a commit that referenced this pull request Aug 20, 2018
@jasontedor
Merge branch 'master' into ccr
853eb1c 
* master:
  Generalize remote license checker (#32971)
  Trim translog when safe commit advanced (#32967)
  Fix an inaccuracy in the dynamic templates documentation. (#32890)
  Logging: Use settings when building daemon threads (#32751)
  All Translog inner closes should happen after tragedy exception is set (#32674)
  HLREST: AwaitsFix ML Test
  Pass DiscoveryNode to initiateChannel (#32958)
  Add mzn and dz to unsupported locales (#32957)
  Use settings from the context in BootstrapChecks (#32908)
  Update docs for node specifications (#30468)
  HLRC: Forbid all Elasticsearch logging infra (#32784)
  Only configure publishing if it's applied externally (#32351)
  Fixes libs:dissect when in eclipse
  Protect ScriptedMetricIT test cases against failures on 0-doc shards (#32959) (#32968)
  [Kerberos] Add documentation for Kerberos realm (#32662)
  Watcher: Properly find next valid date in cron expressions (#32734)
  Fix some small issues in the getting started docs (#30346)
  Set forbidden APIs target compatibility to compiler java version   (#32935)
  Move connection listener to ConnectionManager (#32956)
@jasontedor jasontedor deleted the generalize-remote-license-check branch August 20, 2018 19:54
@jasontedor jasontedor mentioned this pull request Aug 20, 2018
Merged
@jasontedor
Copy link
Member Author

Thanks for reviewing @davidkyle. The motivation for this change was #33002. There's no need for you to review that PR, only keeping you in the loop on the change.

@Tim-Brooks
Copy link
Contributor

LGTM

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 21, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into allowed-index-stor…
be8ce3b 
…e-types

* elastic/master: (89 commits)
  Fix assertion in AbstractSimpleTransportTestCase (elastic#32991)
  [DOC] Splits role mapping APIs into separate pages (elastic#32797)
  HLRC: ML Close Job (elastic#32943)
  Generalize remote license checker (elastic#32971)
  Trim translog when safe commit advanced (elastic#32967)
  Fix an inaccuracy in the dynamic templates documentation. (elastic#32890)
  Logging: Use settings when building daemon threads (elastic#32751)
  All Translog inner closes should happen after tragedy exception is set (elastic#32674)
  HLREST: AwaitsFix ML Test
  Pass DiscoveryNode to initiateChannel (elastic#32958)
  Add mzn and dz to unsupported locales (elastic#32957)
  Use settings from the context in BootstrapChecks (elastic#32908)
  Update docs for node specifications (elastic#30468)
  HLRC: Forbid all Elasticsearch logging infra (elastic#32784)
  Only configure publishing if it's applied externally (elastic#32351)
  Fixes libs:dissect when in eclipse
  Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968)
  [Kerberos] Add documentation for Kerberos realm (elastic#32662)
  Watcher: Properly find next valid date in cron expressions (elastic#32734)
  Fix some small issues in the getting started docs (elastic#30346)
  ...
jasontedor added a commit that referenced this pull request Aug 21, 2018
@jasontedor
Generalize remote license checker (#32971)
2dd8018 
Machine learning has baked a remote license checker for use in checking
license compatibility of a remote license. This remote license checker
has general usage for any feature that relies on a remote cluster. For
example, cross-cluster replication will pull changes from a remote
cluster and require that the local and remote clusters have platinum
licenses. This commit generalizes the remote cluster license check for
use in cross-cluster replication.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 24, 2018
@jasontedor
Merge branch '6.x' into ccr-6.x
d894679 
* commit '9088d811ce9cff922e6ef1befbeb0f1e0c27016a': (23 commits)
  Generalize remote license checker (elastic#32971)
  Trim translog when safe commit advanced (elastic#32967)
  Fix an inaccuracy in the dynamic templates documentation. (elastic#32890)
  HLREST: AwaitsFix ML Test
  Make Geo Context Mapping Parsing More Strict (elastic#32862)
  Add mzn and dz to unsupported locales (elastic#32957)
  Use settings from the context in BootstrapChecks (elastic#32908)
  Update docs for node specifications (elastic#30468)
  HLRC: Forbid all Elasticsearch logging infra (elastic#32784)
  Fix use of deprecated apis
  Only configure publishing if it's applied externally (elastic#32351)
  Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968)
  Scripted metric aggregations: add deprecation warning and system (elastic#32944)
  Watcher: Properly find next valid date in cron expressions (elastic#32734)
  Fix some small issues in the getting started docs (elastic#30346)
  Set forbidden APIs target compatibility to compiler java version   (elastic#32935)
  [TEST] Add "ne" as an unsupported SimpleKdc locale (elastic#32700)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841) (elastic#32974)
  INGEST: Create Index Before Pipeline Execute (elastic#32786) (elastic#32975)
  NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (elastic#32764) (elastic#32976)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidkyle davidkyle davidkyle approved these changes

@Tim-Brooks Tim-Brooks Awaiting requested review from Tim-Brooks

Assignees
No one assigned
Labels
>non-issue :Security/License License functionality for commercial features v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasontedor @elasticmachine @Tim-Brooks @davidkyle @colings86