Add checks for localhost drop #200

Merged
merged 1 commit into from Jan 4, 2017

Conversation

Projects
None yet
3 participants
@trescube
Contributor

trescube commented Dec 29, 2016

Previously, only the first host in esclient.hosts was checked for localhost value so if the array contained multiple hosts and the first was not localhost, no warning would be given about dropping a non-localhost index. For example:

{
      "env": "development",
      "protocol": "http",
      "host": "localhost",
      "port": 9200
},
{
      "env": "production",
      "protocol": "http",
      "host": "internal-pelias-production-es2-162482742.us-east-1.elb.amazonaws.com",
      "port": 9200
}

No warning would be given even though it would drop the production index.

@trescube trescube self-assigned this Dec 29, 2016

@trescube trescube added in review and removed in progress labels Dec 29, 2016

@trescube trescube added this to the API Improvements milestone Dec 29, 2016

@orangejulius

Nice catch! I hope there isn't an interesting story I missed about how this was discovered...

@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Jan 3, 2017

Contributor

Nope, no interesting story, just something I saw when add configuration validation.

Contributor

trescube commented Jan 3, 2017

Nope, no interesting story, just something I saw when add configuration validation.

function warnIfNotLocal() {
- if(config.esclient.hosts[0].host !== "localhost") {
+ if (config.esclient.hosts.some((env) => { return env.host !== 'localhost'; } )) {
console.log(colors.red("WARNING: DROPPING SCHEMA NOT ON LOCALHOST"));

This comment has been minimized.

@dianashk

dianashk Jan 4, 2017

Contributor

Sometimes people store various hosts in their config and move them to the top of the list when needed because we always grab the first one (I think). This warning would always yell at those people even though we would only use the first item in the list which they have set to localhost. Is that true?

@dianashk

dianashk Jan 4, 2017

Contributor

Sometimes people store various hosts in their config and move them to the top of the list when needed because we always grab the first one (I think). This warning would always yell at those people even though we would only use the first item in the list which they have set to localhost. Is that true?

This comment has been minimized.

@trescube

trescube Jan 4, 2017

Contributor

But wouldn't having multiple hosts listed cause the index to be dropped in all those hosts?

@trescube

trescube Jan 4, 2017

Contributor

But wouldn't having multiple hosts listed cause the index to be dropped in all those hosts?

This comment has been minimized.

@orangejulius

orangejulius Jan 4, 2017

Member

The elasticsearch-js module is where those config options eventually end up, and it does support multiple hosts. The docs are a little unclear but it feels like it does connect to all of them.

So yes, storing multiple hosts in the config section is probably a bad idea.

I had a similar problem with the whosonfirst datapath where extra values are no longer supported, but I was storing several different paths to different copies of WOF data for different purposes. I just moved them out of the entire whosonfirst section.

@orangejulius

orangejulius Jan 4, 2017

Member

The elasticsearch-js module is where those config options eventually end up, and it does support multiple hosts. The docs are a little unclear but it feels like it does connect to all of them.

So yes, storing multiple hosts in the config section is probably a bad idea.

I had a similar problem with the whosonfirst datapath where extra values are no longer supported, but I was storing several different paths to different copies of WOF data for different purposes. I just moved them out of the entire whosonfirst section.

This comment has been minimized.

@trescube

trescube Jan 4, 2017

Contributor

Confirmed that the elasticsearch client operates on all hosts in pelias.json:

[stephenhess@slumlord schema (add-checks-for-localhost-drop $)]$ node scripts/create_index.js
Elasticsearch INFO: 2017-01-04T16:44:15Z
  Adding connection to http://localhost:9200/

Elasticsearch INFO: 2017-01-04T16:44:15Z
  Adding connection to http://localhost:10200/


--------------
 create index
--------------

[stephenhess@slumlord ~]$ curl --head 'localhost:9200/asdf1234'

HTTP/1.1 200 OK
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

[stephenhess@slumlord ~]$ curl --head 'localhost:10200/asdf1234'

HTTP/1.1 200 OK
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

[stephenhess@slumlord schema (add-checks-for-localhost-drop $)]$ node scripts/drop_index.js
Elasticsearch INFO: 2017-01-04T16:44:45Z
  Adding connection to http://localhost:9200/

Elasticsearch INFO: 2017-01-04T16:44:45Z
  Adding connection to http://localhost:10200/

Are you sure you want to drop the asdf1234 index and delete ALL records? y

[delete mapping] 	 asdf1234 	 { acknowledged: true }

[stephenhess@slumlord ~]$ curl --head 'localhost:10200/asdf1234'

HTTP/1.1 404 Not Found
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

[stephenhess@slumlord ~]$ curl --head 'localhost:9200/asdf1234'

HTTP/1.1 404 Not Found
Content-Type: text/plain; charset=UTF-8
Content-Length: 0
@trescube

trescube Jan 4, 2017

Contributor

Confirmed that the elasticsearch client operates on all hosts in pelias.json:

[stephenhess@slumlord schema (add-checks-for-localhost-drop $)]$ node scripts/create_index.js
Elasticsearch INFO: 2017-01-04T16:44:15Z
  Adding connection to http://localhost:9200/

Elasticsearch INFO: 2017-01-04T16:44:15Z
  Adding connection to http://localhost:10200/


--------------
 create index
--------------

[stephenhess@slumlord ~]$ curl --head 'localhost:9200/asdf1234'

HTTP/1.1 200 OK
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

[stephenhess@slumlord ~]$ curl --head 'localhost:10200/asdf1234'

HTTP/1.1 200 OK
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

[stephenhess@slumlord schema (add-checks-for-localhost-drop $)]$ node scripts/drop_index.js
Elasticsearch INFO: 2017-01-04T16:44:45Z
  Adding connection to http://localhost:9200/

Elasticsearch INFO: 2017-01-04T16:44:45Z
  Adding connection to http://localhost:10200/

Are you sure you want to drop the asdf1234 index and delete ALL records? y

[delete mapping] 	 asdf1234 	 { acknowledged: true }

[stephenhess@slumlord ~]$ curl --head 'localhost:10200/asdf1234'

HTTP/1.1 404 Not Found
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

[stephenhess@slumlord ~]$ curl --head 'localhost:9200/asdf1234'

HTTP/1.1 404 Not Found
Content-Type: text/plain; charset=UTF-8
Content-Length: 0

@trescube trescube merged commit 4f0012f into master Jan 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@trescube trescube deleted the add-checks-for-localhost-drop branch Jan 4, 2017

@trescube trescube removed the in review label Jan 4, 2017

je-l pushed a commit to nlsfi/pelias-schema that referenced this pull request Aug 31, 2017

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