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

[CCR] Make auto follow patterns work with security #33501

Merged
merged 16 commits into from
Sep 17, 2018

Conversation

martijnvg
Copy link
Member

Relates to #33007

@martijnvg martijnvg added review v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.5.0 labels Sep 7, 2018
@martijnvg martijnvg requested a review from jasontedor September 7, 2018 11:14
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@@ -404,7 +404,9 @@ void start(
final AtomicReferenceArray<Object> responses = new AtomicReferenceArray<>(followIndexMetadata.getNumberOfShards());
Map<String, String> filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream()
.filter(e -> ShardFollowTask.HEADER_FILTERS.contains(e.getKey()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));for (int i = 0; i < numShards; i++) {
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
Copy link
Member Author

Choose a reason for hiding this comment

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

just added a new lines here for readability.

@martijnvg
Copy link
Member Author

In a follow up PR, I like integrate the has_privileges api into the put auto follow patterns api, so that we can validate whether the user in the leader cluster has sufficient privileges to follow indices specified in the auto follow pattern's leader index patterns.

The AutoFollowCoordinator will retry indefinitely (in subsequent executions) to try to follow indices for which the user that has specified the auto follow pattern has no sufficient privileges. I think this behavior is okay, assuming that at the time the auto follow pattern was stored the user did have sufficient privileges.

@jasontedor
Copy link
Member

@martijnvg Would you merge master into this and resolve the conflicts?

@martijnvg
Copy link
Member Author

martijnvg commented Sep 10, 2018

@jasontedor I merged it in tomorrow the morning, but I forgot to push.

@@ -105,12 +116,14 @@ public boolean isCcrAllowed() {
*/
public <T> void checkRemoteClusterLicenseAndFetchClusterState(
final Client client,
final Map<String, String> headers,
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'm not super happy with the changes made to this file. Maybe the caller should provide the leaderClient? So that headers and clusterAlias does not need to be provided?

Copy link
Member

Choose a reason for hiding this comment

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

As we discussed offline, this is okay for now, we can always refactor this later. The implementation is fine, we just want to take a step back and look at how ML uses the remote license checker too (multiple clusters) which has kind of forced how we implement this.

@@ -168,6 +182,33 @@ public void onFailure(final Exception e) {
});
}

public static Client wrapClient(Client client, Map<String, String> headers) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved it here, because I did not want that this code depends on ccr action code.

martijnvg and others added 6 commits September 10, 2018 16:31
* master: (43 commits)
  [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603)
  Update AWS SDK to 1.11.406  in repository-s3 (elastic#30723)
  Expose CCR stats to monitoring (elastic#33617)
  [Docs] Update match-query.asciidoc (elastic#33610)
  TEST: Adjust rollback condition when shard is empty
  [CCR] Improve shard follow task's retryable error handling (elastic#33371)
  Forbid negative `weight` in Function Score Query (elastic#33390)
  Clarify context suggestions filtering and boosting (elastic#33601)
  Disable CCR REST endpoints if CCR disabled (elastic#33619)
  Lower version on full cluster restart settings test
  Upgrade remote cluster settings (elastic#33537)
  NETWORKING: http.publish_host Should Contain CNAME (elastic#32806)
  Add test coverage for global checkpoint listeners
  Reset replica engine to global checkpoint on promotion (elastic#33473)
  HLRC: ML Delete Forecast API (elastic#33526)
  Remove debug logging in full cluster restart tests (elastic#33612)
  Expose CCR to the transport client (elastic#33608)
  Mute testIndexDeletionWhenNodeRejoins
  SQL: Make Literal a NamedExpression (elastic#33583)
  [DOCS] Adds missing built-in user information (elastic#33585)
  ...
@martijnvg
Copy link
Member Author

Jenkins, run sample packaging tests

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@martijnvg martijnvg merged commit 481f8a9 into elastic:master Sep 17, 2018
martijnvg added a commit that referenced this pull request Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants