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

Index "creation_date" not accurate when created with settings from another index #12854

Closed
wants to merge 2 commits into from

Conversation

HarishAtGitHub
Copy link

issue_topic - Index "creation_date" not accurate when created with settings from another index
issue_id - 12790
issue_url - #12790

PROBLEM STATEMENT:

Create Index operation API is capable of taking the settings as input
and create an index. The problem is that it is also using the
input creation_date and creating new index with same date

APPROACH:

Find where it accepts the creating_date from the request object and
change it so that it is always generated at the time of index creation.
(it should effectively behave like uuid)

Files Changed:

    modified:   core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java
modified:   core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java

modified: core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

Here the part where it looks for creation_date from request object is removed.

modified: core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java

Removed test case that tests that the date given in settings is same as the creation_date of the
newly created index.

RESULT AFTER THE CHANGE:

input settings

{
     'settings':
          {  'index':
                       {'number_of_replicas': '1',
                         'version': {'created': '2000010'},
                         'creation_date': '1439384094544'
                         'uuid': 'ayzHgwB3Sgey-Pk_Okgwyg',
                         'number_of_shards': '5',
                       }
          }
}

new index settings

{
    'settings':
          {'index':
                     {'number_of_replicas': '1',
                      'version': {'created': '2000010'},
                      'creation_date': '1439456346424',
                      'uuid': 'fddpIVxNTTKPzqDakLAL4A',
                      'number_of_shards': '5'
                     }
         }
}

Please give feedback.

Test case to check result

https://github.com/HarishAtGitHub/elasticsearch-tester/blob/master/12790.py

…ttings from another index

issue_id - 12790
issue_url - elastic#12790

PROBLEM STATEMENT:
==================

Create Index operation API is capable of taking the settings as input
and create an index. The problem is that it is also using the
input creation_date and creating new index with same date

APPROACH:
========

Find where it accepts the creating_date from the request object and
change it so that it is always generated at the time of index creation.
(it should effectively behave like uuid)

Files Changed:
=============

        modified:   core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java
	modified:   core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java

> modified:   core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

Here the part where it looks for creation_date from request object is removed.

> modified:   core/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java

Removed test case that tests that the date given in settings is same as the creation_date of the
newly created index.

RESULT AFTER THE CHANGE:
=======================

input settings
```javascript
{
     'settings':
          {  'index':
                       {'number_of_replicas': '1',
                         'version': {'created': '2000010'},
                         'creation_date': '1439384094544'
                         'uuid': 'ayzHgwB3Sgey-Pk_Okgwyg',
                         'number_of_shards': '5',
                       }
          }
}
```

new index settings
```javascript
{
    'settings':
          {'index':
                     {'number_of_replicas': '1',
                      'version': {'created': '2000001'},
                      'creation_date': '1439456346424',
                      'uuid': 'fddpIVxNTTKPzqDakLAL4A',
                      'number_of_shards': '5'
                     }
         }
}
```

Please give feedback.

Test case to check result
=========================

https://github.com/HarishAtGitHub/elasticsearch-tester/blob/master/12790.py
@HarishAtGitHub
Copy link
Author

pull request for the date replication reported in #12790.

@HarishAtGitHub
Copy link
Author

I have just commented out the if loop as so that this change can be easily tracked in case it results in some other bug .(doing this as getting from input settings code has lived for such a long time)

issue url : elastic#12790
issue id : 12790

Problem Statement:
===================

When creating index, we are now able to give the version created as input in the rest api content.
But it is not supposed to be fed by users.
But at the same time we cannot remove it completely as it is used internally as pointed out by
@rjernst in elastic#12790 (comment) and
elastic#12790 (comment).
So we should find a way that only avoids elasticsearch api users from exploiting this.

Approach:
=========

We should not allow users to use this feature. so one way would be to filter the params given
by the user before it is structured and set in CreateIndexRequest.
This is like "Nip it in the Bud" .
Will this affect other internal workings that are dependent on this version.created ?
No. because by doing it in the CreateIndexRequest we are only avoding every one who hits the
top most interface leaving the inner side untouched.

Code:
====

modified:   core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

The filtering is done after the request.content() is processed and
before it is set in the request.settings (as once set we cannot do anything as it is immutable).
This is the last and safe place to filter as we get input in right form to filter.

This is specific to IndexCreation thats why it is done here and only here.
And this is the place where all request.content() processing is done whenever create index is requested.

Did this work ?
==============

Here are the observations:

Manual Test:
************

I tested it with 5 possible settings as follows :

```javascript
settings1            = {'settings': {
                                     'index': {
                                                 'number_of_replicas': '1',
                                                 'version': {'created': '2000009'},
                                                 'creation_date': '1439384094544',
                                                 'uuid': 'ayzHgwB3Sgey-Pk_Okgwyg',
                                                 'number_of_shards': '5'
                                              },
                                     'version': {'created': '2000010'},
                                     'creation_date': '1439384094544',
                                     'uuid': 'ayzHgwB3Sgey-Pk_Okgwyg',
                                     'number_of_shards': '3'
                                   }
                      }

settings2            = {'settings': {
                                     'index': {
                                                 'number_of_replicas': '1',
                                                 'version': {'created': '2000009'},
                                                 'creation_date': '1439384094544',
                                                 'uuid': 'ayzHgwB3Sgey-Pk_Okgwyg',
                                                 'number_of_shards': '5'
                                              }
                                   }
                      }

settings3            = {'settings': {
                                     'version': {'created': '2000009'},
                                     'creation_date': '1439384094544',
                                     'uuid': 'ayzHgwB3Sgey-Pk_Okgwyg',
                                     'number_of_shards': '3'
                                    }
                       }

settings4            = {}

settings5            = { 'settings' : {}}
settings6            = { 'settings' : {'index': {}}}

```

and it seems to ignore the version.created whether it is given within index or outside index.
So it works.

Automated Test:
***************

I built and all tests PASSED except the "Smoke test shaded jar"
see : https://gist.github.com/HarishAtGitHub/aa8539a5c56cb4ad48b9
. and I hope this has nothing to do with my code.
and suspecting some other problem I resumed the build and I landed in https://gist.github.com/HarishAtGitHub/fcc519bd9863ab8a95af

which is reported in elastic#12791 .
@HarishAtGitHub
Copy link
Author

In the changes made to fix the version.created problem , I went with the "filtering params" in the starting place though this would not be ideal in most of the cases. But in our case I thought it might fit because fixing in the MetaDataCreateIndexService(core) might not be appropriate because at this point we will not know who set the settings "is he the user" or "from internal code" ... Fixing in MetaDataCreateIndexService(core) was ok for created_date feature removal as the feature is to be removed for all .But Our aim is to block external users and not internal usage. so the logic would be to filter it at the doorsteps of various inlets.

But this too has a drawback. the drawback is we should find all the possible doors through which users can enter the MetaDataCreateIndexService . one is the fixed one in CreateIndexRequest which is used by RestCreateIndexAction .
Are there any other routes through which users can set settings in CreateIndexRequest and feed it to MetaDataCreateIndexService ? Is CreateIndexRequestBuilder as possible route ?

Also another problem is what if users directly set settings as CreateIndexRequest too has

/**
     * Constructs a new request to create an index with the specified name and settings.
     */
    public CreateIndexRequest(String index, Settings settings) {
        this.index = index;
        this.settings = settings;
    }

@s1monw
Copy link
Contributor

s1monw commented Aug 21, 2015

there are more problems like this, you can also fake the index.version.created etc. yet, I think we need this ability in our tests so I guess there should be a way to by-pass this by the test infrastructure. I think it would make sense to have some switch on MetaDataCreateIndexService to allow overriding these?
The two I think of are:

  • index.creation_date
  • index.version.created

I can take care of this if you want but I wanted to give you a heads-up first

@s1monw s1monw self-assigned this Aug 21, 2015
@HarishAtGitHub
Copy link
Author

@s1monw thanks for the review .

sorry for the delay @s1monw .... I did not notice that you reviewed (pls add the review tag if possible in the issue, I was waiting for review but left it as as there was no tag) ....
I will work on it today nd tmrw (2 days) .... If I cannot find a way out in 2 days I will let you know and you can take it....

@HarishAtGitHub
Copy link
Author

Hi @s1monw, I cannot think of a good acceptable solution for this problem now. Ya may be you or someone else can take it up ... I can look into some other issue ... eager to see the solution ...

@rjernst rjernst assigned rjernst and unassigned s1monw Aug 26, 2015
@rjernst
Copy link
Member

rjernst commented Aug 26, 2015

I'm going to work on this soon.

s1monw added a commit that referenced this pull request Jan 19, 2016
Cut over all index scope settings to the new setting infrastrucuture

This change moves all index.* settings over to the new infrastructure. This means in short that:

 - every setting that has an index scope must be registered up-front
 - index settings are validated on index creation, template creation, index settings update
 - node level settings starting with index.* are validated on node startup
 - settings that are private to ES like index.version.created can only be set by tests when they install a specific test plugin.
 - all index settings can be reset by passing null as their value on update
 - all index settings defaults can be listed via the settings APIs

Closes #12854
Closes #6732
Closes #16032
Closes #12790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants