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

Use alias name from rollover request to query indices stats #40774

Merged
merged 10 commits into from Apr 16, 2019

Conversation

Projects
None yet
7 participants
@bizybot
Copy link
Contributor

bizybot commented Apr 3, 2019

In TransportRolloverAction before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.

Closes #40771

Use alias name from rollover request to query indices stats
In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of what is in the request.
This fails when user is assigned role with index privilege on the
alias instead of concrete index. This commit fixes this by using
the alias from the request.

Closes #40771
@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 3, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 3, 2019

@albertzaharovits
Copy link
Contributor

albertzaharovits left a comment

I am game with the main code change.
I would love if you'd add an IntegTest so that we don't catch authorization errors in Rest tests...

However, I think we should make the Rollover action use the client for index and alias creation. WDYT?

bizybot added some commits Apr 4, 2019

Check whether index is same and only one write index exists for alias
When same alias points to multiple indices we can write to only one index
with `is_write_index` value `true`. The special handling for
PutMappingRequest filtered out such aliases making the request unauthorized.
The check has been modified to consider write index flag and only when the
requested index matches with the one with write index alias.

Closes #40831
@bizybot

This comment has been minimized.

Copy link
Contributor Author

bizybot commented Apr 4, 2019

Hi @albertzaharovits, Thanks for your comment

I am game with the main code change.
I would love if you'd add an IntegTest so that we don't catch authorization errors in Rest tests...

I did not see one for Rollover Action but I can add unit test to cover this. Thanks.

However, I think we should make the Rollover action use the client for index and alias creation. WDYT?

Could you please elaborate? as I did not understand what you have proposed here. Thank you.

@albertzaharovits

This comment has been minimized.

Copy link
Contributor

albertzaharovits commented Apr 4, 2019

Ah, I'm sorry, I mean createIndexService.createIndex to be replaced with client.indices().create() . I think the user of the rollup action should necessitate the create index privilege, at least this is my perception of the authorization with "action granularity".
This is a discussion suggestion for an issue as a follow-up, not to be addressed here.

@tvernum

This comment has been minimized.

Copy link
Contributor

tvernum commented Apr 5, 2019

@talevy Are you able to review the (1 line) change to the transport action? It looks like you were the last person to make actual functional changes to that action, and I'd appreciate a review from someone who knows that code better than we do.

@tvernum tvernum requested a review from talevy Apr 5, 2019

@talevy
Copy link
Contributor

talevy left a comment

I still need to test my comments, but I figure sending this out earlier is better. let me know if
what I said makes sense!

@talevy
Copy link
Contributor

talevy left a comment

I just ran this check to assert my theory:

PUT alpha
{
  "aliases": {
    "my_alias": {
      "is_write_index": true
    }
  }
}

PUT beta
{
  "aliases": {
    "my_alias": {
      "is_write_index": false
    }
  }
}

PUT alpha/_doc/1?refresh
{
  "foo": "bar"
}

PUT beta/_doc/1?refresh
{
  "foo": "bar"
}

POST /my_alias/_rollover/new_index?dry_run
{
  "conditions": {
    "max_docs":  2
  }
}

this returns an incorrect rollover evaluation (the write index "alpha" only has one document):

{
  "acknowledged" : false,
  "shards_acknowledged" : false,
  "old_index" : "alpha",
  "new_index" : "new_index",
  "rolled_over" : false,
  "dry_run" : true,
  "conditions" : {
    "[max_docs: 2]" : true
  }
}

I think the additional change that would make this work would be this:

@@ -249,7 +249,7 @@ public class TransportRolloverAction extends TransportMasterNodeAction<RolloverR
 
     static Map<String, Boolean> evaluateConditions(final Collection<Condition<?>> conditions, final IndexMetaData metaData,
                                                     final IndicesStatsResponse statsResponse) {
-        return evaluateConditions(conditions, statsResponse.getPrimaries().getDocs(), metaData);
+        return evaluateConditions(conditions, statsResponse.getIndex(metaData.getIndex().getName()).getPrimaries().getDocs(), metaData);
     }

The issue is that the code was looking at the total DocStats of all the indices returned in the stats-response. This change should make it so that we are looking at the sourceIndex only. The non-docstats condition (index age) is done on the correct IndexMetaData of the sourceIndex, so this should not be a problem.

Evaluate conditions by looking at source index stats only
When alias is used, we retrieve all the stats (including write + read indexes)
so the earlier commit, made the stats evaluation on collective stats instead
of considering only source index.
@talevy
Copy link
Contributor

talevy left a comment

Changes to Rollover look good to me. I'm not against
getting another opinion regarding my comments around
performance concerns.

Due to the difficulty of controlling time for the additional test in RolloverIT,
what do you think of moving these additional tests as unit tests
in TransportRolloverActionTests. That way we can control the
state of the indices such that only the write-index meets the
relevant criteria (max_age, max_docs, max_size).

bizybot added some commits Apr 8, 2019

Address review feedback
- Add UT instead of IT to minimize impact on build times.
@talevy

talevy approved these changes Apr 8, 2019

Copy link
Contributor

talevy left a comment

LGTM

@polyfractal polyfractal removed the v7.0.0 label Apr 9, 2019

@tvernum
Copy link
Contributor

tvernum left a comment

LGTM

@bizybot bizybot merged commit 3c66cff into elastic:master Apr 16, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

bizybot added a commit to bizybot/elasticsearch that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (elastic#…
…40774)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes elastic#40771

bizybot added a commit to bizybot/elasticsearch that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (elastic#…
…40774)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes elastic#40771

@bizybot bizybot added the v7.0.1 label Apr 17, 2019

bizybot added a commit to bizybot/elasticsearch that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (elastic#…
…40774)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes elastic#40771

bizybot added a commit to bizybot/elasticsearch that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (elastic#…
…40774)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes elastic#40771

bizybot added a commit that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (#40774) (#…
…41284)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes #40771

bizybot added a commit that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (#40774) (#…
…41285)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes #40771

bizybot added a commit to bizybot/elasticsearch that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (elastic#…
…40774)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes elastic#40771

bizybot added a commit that referenced this pull request Apr 17, 2019

Use alias name from rollover request to query indices stats (#40774) (#…
…41286)

In `TransportRolloverAction` before doing rollover we resolve
source index name (write index) from the alias in the rollover request.
Before evaluating the conditions and executing rollover action, we
retrieve stats, but to do so we used the source index name
resolved from the alias instead of alias from the index.
This fails when the user is assigned a role with index privilege on the
alias instead of the concrete index. This commit fixes this by using
the alias from the request.
After this change, verified that when we retrieve all the stats (including write + read indexes)
we are considering only source index.

Closes #40771
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.