[savedObjects] fix error handling when Kibana index is missing by spalger · Pull Request #14141 · elastic/kibana

spalger · 2017-09-23T00:10:48Z

Currently when the Kibana index is missing the savedObjectsClient and APIs produce unexpected results. This PR tests and formalizes the following behaviors:

get, update, delete
- generic 404, intentionally obfuscating the fact that it's caused by a missing index
bulk_get
- 200 bulk response, generic 404s for individuals
find
- 200 and an empty result set
create
- creates an invalid Kibana index (not new, see Writes to a non-existent kibana index can break healthcheck #12819)

For those unfamiliar, run the API integration tests and dependencies in one go with:

npx grunt test:api

To run the server/runner combo and for debugging run the following two commands in separate tabs:

# start es/kibana, will auto restart on changes
npx grunt test:api:server

# run the ftr once, supports `--debug-brk`, `--inspect`, `--grep` and the like
node scripts/functional_test_runner --config test/api_integration/config.js

…cts/mget404

kimjoar

Just a couple nitpicks, but I'm good with merging as-is. LGTM

kimjoar · 2017-09-29T13:11:46Z

src/server/saved_objects/client/saved_objects_client.js

+    }
+
+    // 404 might be because document is missing or index is missing,
+    // don't leak that implementation detail to the user


Would be great if we could add an explanation here. (Basically answering the question: Why don't we want to leak this to the user?)

We've got this same comment in multiple places. Not sure if we should add an explanation in one place and "link" there, but it feels like we should make it clear here, at least.

kimjoar · 2017-09-29T13:14:29Z

test/api_integration/apis/saved_objects/bulk_get.js

+          .expect(200)
+          .then(resp => {
+            expect(resp.body).to.eql({
+              saved_objects: [


Would be 🎉 to have snapshot tests for these, but that's of course not possible yet

These are basically snapshot tests, no? What would be different with a snapshot test?

++ Yeah, it's basically the same (I just prefer working with snapshots over large objects, e.g. when changing code etc)

Shit, I totally meant to delete my comment, thought it failed, and accidentally deleted yours 😿

kimjoar · 2017-09-29T13:16:32Z

test/api_integration/apis/saved_objects/update.js

+          });
+      });
+
+      describe('unkown id', () => {


typo, unknown

…cts/mget404

spalger · 2017-09-29T22:14:44Z

src/server/saved_objects/client/saved_objects_client.js

  }

+  /**
+   * ## SavedObjectsClient errors


Alright, you asked for some docs @kjbekkelund :) I don't think it's bad to have some of these thought written down but let me know if you think I went overboard

❤️ I like this! It gives tons of context and makes it way easier to understand why we do what we do.

spalger · 2017-09-29T22:16:01Z

src/server/saved_objects/client/saved_objects_client.js

+    const indexNotFound = response.error && response.error.type === 'index_not_found_exception';
+    if (docNotFound || indexNotFound) {
+      // see "404s from missing index" above
+      throw errors.createGenericNotFoundError();


I also fixed the lib/errors module. Including a decorateNotFoundError contradicts the goal of not exposing where NotFound errors come from, so now it has a createGenericNotFoundError() that does the same thing these lines were doing.

spalger · 2017-09-29T22:43:08Z

@epixa any more feedback?

azasypkin

LGTM, just few nits.

azasypkin · 2017-10-02T07:13:54Z

src/server/saved_objects/client/saved_objects_client.js

+   *      by the decorators in `./lib/errors`
+   *
+   * Type 1 errors are inevitable, but since all expected/handle-able errors
+   * should be Type 2 the `isXYZError()` helpers exposed at `SavedObjectsClient.errors`


typo: is this a typo (double mentioning of SavedObjectsClient.errors)?

azasypkin · 2017-10-02T07:22:01Z

src/server/saved_objects/client/saved_objects_client.js

+   * ### 404s from missing index
+   *
+   * From the perspective of application code and APIs the SavedObjectsClient is
+   * a black box that persists objects. One of the internal details that the client


question: just for my understanding, that the client has no control over - the client here is the consumer of SavedObjectsClient or the SavedObjectsClient itself?

Yeah, I use client here to mean SavedObjectsClient and browser/user client. I'll more clearly distinguish those.

azasypkin · 2017-10-02T07:29:10Z

src/server/saved_objects/client/saved_objects_client.js

+   *
+   * At the time of writing we are in the process of transitioning away from the
+   * operating assumption that the SavedObjects index is always available. Part of
+   * this transition is handling errors resulting from an index missing. These use


typo: these use to trigger --> these used to trigger?

azasypkin · 2017-10-02T07:47:33Z

src/server/saved_objects/client/lib/__tests__/errors.js

-          expect(error.output.payload).to.have.property('message', 'biz: foobar');
+        it('Uses "Not Found" message', () => {
+          const error = createGenericNotFoundError();
+          expect(error.output.payload).to.have.property('message', 'Not Found');


question: there is no need to change anything here, just wondering why not to use to.be?

expect(error.output.payload.message).to.be('Not Found')

the error message shows the properties that do exist, that is all

azasypkin · 2017-10-02T07:57:26Z

src/server/saved_objects/client/__tests__/saved_objects_client.js

+      });
      await savedObjectsClient.delete('index-pattern', 'logstash-*');

      expect(callAdminCluster.calledOnce).to.be(true);


nit: since you're here already :)

sinon.assert.calledOnce(onBeforeWrite); sinon.assert.calledOnce(callAdminCluster); sinon.assert.calledWithExactly(callAdminCluster, 'delete', { type: 'doc', id: 'index-pattern:logstash-*', refresh: 'wait_for', index: '.kibana-test', ignore: [404], });

There is a lot of that in this file, but if you like!

Pretty sure I'm falling in love with sinon's matcher API. I'm going to merge if the tests pass, but if you wouldn't mind double checking this commit @azasypkin, I would appreciate it

Looks good, thanks!

azasypkin · 2017-10-02T10:42:37Z

tasks/config/run.js

        ...stdDevArgs,
        '--optimize.enabled=false',
        '--elasticsearch.url=' + esTestConfig.getUrl(),
+        '--elasticsearch.healthCheck.delay=360000',


question: just curious, why exactly 6 minutes? :)

Haha, I'm horrible at math

azasypkin · 2017-10-02T11:01:11Z

test/api_integration/apis/saved_objects/delete.js

+
+      it('returns generic 404 when kibana index is missing', async () => (
+        await supertest
+          .delete(`/api/saved_objects/dashboard/not-a-real-id`)


nit: maybe not-a-real-id ---> be3733a0-9efe-11e7-acb3-3dab96693fab just to be sure we get 404 because of missing index and not because of non-existing id.

epixa · 2017-10-02T20:31:20Z

@spalger I don't have anything new to add beyond @azasypkin's latest round of comments.

…cts/mget404

…xist

…lastic#14202

* [savedObjects/delete+bulk_get] add failing tests * [savedObjects/delete+bulk_get] improve 404 handling * [savedObjects/client] fix mocha tests * [savedObjects/tests] remove extra test wrapper * [apiIntegration/kbnServer] basically disable es healthcheck * [savedObjects/create] add integration test * [savedObjects/find] add failing integration tests * [savedObjects/find] fix failing test * [savedObjects/client] explain reason for generic 404s * [savedObjects/get] add integration tests * [savedObjects/find] test request with unkown type * [savedObjects/find] add some more weird param tests * [savedObjects/find] test that weird params pass when no index * [savedObjects/update] use generic 404 * fix typos * [savedObjects/update] add integration tests * remove debugging uncomment * [savedObjects/tests] move backup kibana index delete out of tests * [savedObjects/tests/esArchives] remove logstash data * [savedObjects] update test * [uiSettings] remove detailed previously leaked from API * [functional/dashboard] wrap check that is only failing on Jenkins * [savedObjects/error] replace decorateNotFound with createGenericNotFound * fix typo * [savedObjectsClient/errors] fix decorateEsError() test * [savedObjectsClient] fix typos * [savedObjects/tests/functional] delete document that would normally exist * [savedObjectsClient/tests] use sinon assertions * [savedObjects/apiTests] create without index responds with 503 after #14202

spalger · 2017-10-03T01:52:39Z

6.x: ddc1d9c

spalger added 22 commits September 21, 2017 13:57

[savedObjects/delete+bulk_get] add failing tests

4b2f6cb

[savedObjects/delete+bulk_get] improve 404 handling

859a5ce

[savedObjects/client] fix mocha tests

cf1b5a7

Merge branch 'master' of github.com:elastic/kibana into fix/savedObje…

5032648

…cts/mget404

[savedObjects/tests] remove extra test wrapper

b99bf09

[apiIntegration/kbnServer] basically disable es healthcheck

00aaac6

[savedObjects/create] add integration test

a93cae0

[savedObjects/find] add failing integration tests

ec21fe4

[savedObjects/find] fix failing test

3c6ee2f

[savedObjects/client] explain reason for generic 404s

de1a3fc

[savedObjects/get] add integration tests

8d2e623

[savedObjects/find] test request with unkown type

bcd3880

[savedObjects/find] add some more weird param tests

dae161d

[savedObjects/find] test that weird params pass when no index

058e66d

[savedObjects/update] use generic 404

69272c7

fix typos

4353cd2

[savedObjects/update] add integration tests

7381d0c

remove debugging uncomment

56842cf

[savedObjects/tests] move backup kibana index delete out of tests

64e7bfb

[savedObjects/tests/esArchives] remove logstash data

73df8dd

[savedObjects] update test

ba595f8

[uiSettings] remove detailed previously leaked from API

d80cf10

spalger added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// review v6.0.0 v6.1.0 v7.0.0 labels Sep 23, 2017

spalger requested review from azasypkin, epixa and kimjoar September 23, 2017 00:10

epixa added v6.0.0-rc2 and removed v6.0.0-rc2 labels Sep 28, 2017

Merge branch 'master' of github.com:elastic/kibana into fix/savedObje…

47dde37

…cts/mget404

kimjoar approved these changes Sep 29, 2017

View reviewed changes

spalger added 3 commits September 29, 2017 14:01

Merge branch 'master' of github.com:elastic/kibana into fix/savedObje…

36386dc

…cts/mget404

[savedObjects/error] replace decorateNotFound with createGenericNotFound

baa9e07

fix typo

b976a21

spalger commented Sep 29, 2017

View reviewed changes

elastic deleted a comment from kimjoar Sep 29, 2017

[savedObjectsClient/errors] fix decorateEsError() test

aaf4bb4

azasypkin approved these changes Oct 2, 2017

View reviewed changes

spalger added 4 commits October 2, 2017 14:51

Merge branch 'master' of github.com:elastic/kibana into fix/savedObje…

4b683cb

…cts/mget404

[savedObjectsClient] fix typos

eaa739f

[savedObjects/tests/functional] delete document that would normally e…

977728f

…xist

[savedObjectsClient/tests] use sinon assertions

f98c009

spalger force-pushed the fix/savedObjects/mget404 branch from 6852b15 to f98c009 Compare October 2, 2017 22:24

[savedObjects/apiTests] create without index responds with 503 after e…

9b2f127

…lastic#14202

spalger merged commit e847612 into elastic:master Oct 3, 2017

spalger removed the v6.0.0 label Oct 3, 2017

spalger deleted the fix/savedObjects/mget404 branch October 3, 2017 01:52

This was referenced Jan 26, 2021

Migrations v2: don't auto-create indices + FTR/esArchiver support #85778

Merged

Saved objects API should return 500 for all methods when the index doesn't exist #89642

Open

rudolf mentioned this pull request Feb 16, 2021

Unrevert "Migrations v2: don't auto-create indices + FTR/esArchiver support (#85778)" #89992

Merged

9 tasks

Conversation

spalger commented Sep 23, 2017

Uh oh!

kimjoar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger commented Sep 29, 2017

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa commented Oct 2, 2017

Uh oh!

spalger commented Oct 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spalger Oct 2, 2017 •

edited

Loading