Encode the Kibana saved objects during packaging by ruflin · Pull Request #157 · elastic/package-registry

ruflin · 2019-11-20T13:45:06Z

In the packages, the saved objects are stored with decoded JSON fields. The reason is that this makes versioning of it much easier and the diffs are simpler. But inside Kibana some of the fields are stored as encoded json strings (this might change in the future elastic/kibana#43673). To not require special logic on the Package Manager to encode the strings, this is done directly during packaging.

One thing not too nice about this PR is that it includes now a dependency on common.MapStr from Beats. Reason is that it makes the code much simpler.

Part of #42

In the packages, the saved objects are stored with decoded JSON fields. The reason is that this makes versioning of it much easier and the diffs are simpler. But inside Kibana some of the fields are stored as encoded json strings (this might change in the future elastic/kibana#43673). To not require special logic on the Package Manager to encode the strings, this is done directly during packaging. One thing not too nice about this PR is that it includes now a dependency on `common.MapStr` from Beats. Reason is that it makes the code much simpler. Part of elastic#42

ruflin · 2019-11-20T13:59:29Z


 require (
 	github.com/blang/semver v3.5.1+incompatible
+	github.com/elastic/beats v7.4.2+incompatible


@urso @exekias I started to use common.MapStr in this PR which had not such a nice side effect dependency wise. I haven't check yet in detail but I guess it is because many things are in the common package so it adds all these :-(

Should we extract common.MapStr in its own package?

I'm actually thinking the same. Move often used functions/types into separate packages and use aliases to not break too much code. But MapStr is not one of my favorite types :), you really need it?

Really need it is a bendable term ;-) The GetValue and PutValue that traverse the tree for dotted keys just make my code much simpler, otherwise I would have to reimplement that.

ruflin · 2019-11-21T12:55:12Z

@@ -0,0 +1,287 @@
+// Licensed to Elasticsearch B.V. under one or more contributor


@urso Found an "easy" solution. I copied over the mapstr file from libbeat and remove the methods I don't need. Not nice but works.

Maybe create a tech-debt issue for us to know that might need to clean this up in the future.

I filed elastic/beats#14734

ruflin · 2019-11-25T10:07:56Z

@jfsiii After this changed, I assumed I could just get rid ofensureJsonValues here and things would work as expected: https://github.com/elastic/kibana/blob/feature-ingest/x-pack/legacy/plugins/epm/server/packages/get_objects.ts#L79 But I get mapping errors which I'm not sure yet why. I tried to load the saved objects manually and it works as expected. Is there some other magic here that I'm missing?

The error I get is:

   │ info [o.e.a.b.TransportShardBulkAction] [xodoa.local] [.kibana_1][0] failed to execute bulk item (index) index {[.kibana][dashboard:ceefb9e0-1f51-11e9-93ed-f7e068f4aebb-ecs], source[n/a, actual length: [2.3kb], max length: 2kb]}
   │      org.elasticsearch.index.mapper.MapperParsingException: failed to parse field [dashboard.kibanaSavedObjectMeta.searchSourceJSON] of type [text] in document with id 'dashboard:ceefb9e0-1f51-11e9-93ed-f7e068f4aebb-ecs'. Preview of field's value: '{filter=[], query={query=, language=kuery}}'

And the file in question looks as following:

{
  "attributes": {
    "description": "Overview of the iptables events dashboard.",
    "hits": 0,
    "kibanaSavedObjectMeta": {
      "searchSourceJSON": "{\"filter\":[],\"query\":{\"language\":\"kuery\",\"query\":\"\"}}"
    },
    "optionsJSON": "{\"darkTheme\":false,\"hidePanelTitles\":false,\"useMargins\":true}",
    "panelsJSON": "[{\"embeddableConfig\":{\"vis\":{\"legendOpen\":false}},\"gridData\":{\"h\":15,\"i\":\"1\",\"w\":37,\"x\":0,\"y\":0},\"panelIndex\":\"1\",\"panelRefName\":\"panel_0\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{},\"gridData\":{\"h\":15,\"i\":\"2\",\"w\":11,\"x\":37,\"y\":0},\"panelIndex\":\"2\",\"panelRefName\":\"panel_1\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{\"mapCenter\":[47.15984001304432,-47.02148437500001],\"mapZoom\":2},\"gridData\":{\"h\":15,\"i\":\"3\",\"w\":24,\"x\":0,\"y\":15},\"panelIndex\":\"3\",\"panelRefName\":\"panel_2\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{\"mapCenter\":[49.15296965617042,-27.949218750000004],\"mapZoom\":2},\"gridData\":{\"h\":15,\"i\":\"4\",\"w\":24,\"x\":24,\"y\":15},\"panelIndex\":\"4\",\"panelRefName\":\"panel_3\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{},\"gridData\":{\"h\":15,\"i\":\"5\",\"w\":19,\"x\":0,\"y\":30},\"panelIndex\":\"5\",\"panelRefName\":\"panel_4\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{},\"gridData\":{\"h\":15,\"i\":\"6\",\"w\":18,\"x\":19,\"y\":30},\"panelIndex\":\"6\",\"panelRefName\":\"panel_5\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{},\"gridData\":{\"h\":15,\"i\":\"7\",\"w\":11,\"x\":37,\"y\":30},\"panelIndex\":\"7\",\"panelRefName\":\"panel_6\",\"version\":\"6.6.0\"},{\"embeddableConfig\":{},\"gridData\":{\"h\":19,\"i\":\"8\",\"w\":48,\"x\":0,\"y\":45},\"panelIndex\":\"8\",\"panelRefName\":\"panel_7\",\"version\":\"6.6.0\"}]",
    "timeRestore": false,
    "title": "[Filebeat Iptables] Overview ECS",
    "version": 1
  },
  "migrationVersion": {
    "dashboard": "7.3.0"
  },
  "references": [
    {
      "id": "4c913eb0-1f51-11e9-93ed-f7e068f4aebb-ecs",
      "name": "panel_0",
      "type": "visualization"
    },
    {
      "id": "2599f5e0-1e98-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_1",
      "type": "visualization"
    },
    {
      "id": "c4394ec0-1efd-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_2",
      "type": "visualization"
    },
    {
      "id": "d8cea010-1efd-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_3",
      "type": "visualization"
    },
    {
      "id": "b57b7370-1f1d-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_4",
      "type": "visualization"
    },
    {
      "id": "35fe0910-1f26-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_5",
      "type": "visualization"
    },
    {
      "id": "683402b0-1f29-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_6",
      "type": "visualization"
    },
    {
      "id": "b3f1b010-1f26-11e9-8ec4-cf5d91a864b3-ecs",
      "name": "panel_7",
      "type": "search"
    }
  ]
}

ruflin · 2019-11-25T10:13:26Z

Investigating the above further, I stumbled over the following console output:

{ type: 'dashboard',
  id: 'ceefb9e0-1f51-11e9-93ed-f7e068f4aebb-ecs',
  attributes:
   { description: 'Overview of the iptables events dashboard.',
     hits: 0,
     kibanaSavedObjectMeta: { searchSourceJSON: [Object] },
     optionsJSON:
      { darkTheme: false, hidePanelTitles: false, useMargins: true },
     panelsJSON:
      [ [Object]
    ....

This is outputting the savedObject after creating on the Kibana side. I wonder why it marks and object instead of a string for searchSourceJSON and the others? Need to investigate further what JSON.parse does.

jfsiii · 2019-11-25T17:52:24Z

@ruflin After our video chat and some look at the code here's what I think it happening. TL;DR; Comments should be updated for ensureJsonValues but the function should likely remain

The asset from #157 (comment) is valid JSON

However, that alone isn't enough to submit to the Saved Object HTTP API. I assume you added the required type parameter (dashboard in this case) based on your knowledge of what the asset was, not based on information stored in the asset.

EPM is using the Saved Objects JS API which takes a JS object (as opposed to the JSON string of the asset). It calls JSON.parse to convert the stored JSON string into a JS object that the SO API can use.

In doing the JSON.parse, the escaped values stored in the asset are lost. That's why ensureJsonValues goes back through and restores them to JSON strings

EPM adds the required type param based on the convention for asset location, and the optional (but required by EPM) id param for now based on the assets' filename, but eventually based on some other id.

JS SO API requires JS objects. JS objects require calling JSON.parse to convert the asset from a string. That conversion loses the intent for certain of those values to be JSON vs simply strings.

I don't think we can avoid converting from a string to a JS object since we want to use the SO JS API. We could use the SO HTTP API directly, but that makes EPM much more brittle (duplicating the route locations, losing any validation, etc).

We might be able to do something to make JSON.parse less destructive by supplying our own reviver function but I haven't tested that out.

I think we should remove the outdated/misleading comments and replace them with something more accurate. Initially, this function was working around bad input (invalid JSON due to golang literals in templates, etc) but now it's for the reasons above.

ruflin · 2019-11-25T19:48:46Z

@jfsiii Thanks for all the investigation and details. I found the actual error and it was on the registry side. The files were properly encoded but the .tar.gz was created before the encoding happened, so the fix is here: https://github.com/elastic/package-registry/pull/157/files#diff-9421fdf5cec2a108353534181652b35fR167

I tested this with the current Kibana implementation and the cleaned up one. The great thing is both versions of the format work with the current implementation, so if we merge this PR, things should keep working.

jfsiii · 2019-11-25T20:06:30Z

+// DecodeExported decodes an exported dashboard
+//func EncodeKibanaAssets(result MapStr) MapStr {
+
+func TestFoo(t *testing.T) { // Read file from json


If I'm reading this correctly, the test ensures no errors occur when a) reading the test file b) calling encodedSavedObject.

If it doesn't already, it would be useful to test if the output of the function is what's expected; not only that it doesn't fail.

Agree. My plan is to have a more static test file and check the generated output to compare it. At the moment I link to an existing file which will probably not be there anymore. Will merge as is and follow up.

jfsiii

Left two questions, but happy to see this merge.

ruflin force-pushed the encode-saved-objects branch from 93cff70 to 687ce86 Compare November 20, 2019 13:55

ruflin changed the title ~~encode saved objects on packaging~~ Encode the Kibana saved objects during packaging Nov 20, 2019

ruflin commented Nov 20, 2019

View reviewed changes

ruflin marked this pull request as ready for review November 20, 2019 13:59

ruflin self-assigned this Nov 21, 2019

ruflin added 2 commits November 21, 2019 13:49

local mapstr.go

95732bf

remove dependency

7a2644e

ruflin commented Nov 21, 2019

View reviewed changes

ruflin added 3 commits November 21, 2019 13:59

update framework

af2281b

reset mod changes

d8131f8

update code comment

964655f

ruflin mentioned this pull request Nov 25, 2019

Extract common.MapStr to its own package elastic/beats#14734

Closed

update path grabbing

9ae16ab

move creation of the tar.gz after the modification of the objects

6ea603a

ruflin requested a review from jfsiii November 25, 2019 19:49

jfsiii reviewed Nov 25, 2019

View reviewed changes

Comment thread dev/generator/main_test.go Outdated

jfsiii reviewed Nov 25, 2019

View reviewed changes

jfsiii approved these changes Nov 25, 2019

View reviewed changes

Cleanup tests

361bd8f

ruflin merged commit 70fc57e into elastic:master Nov 26, 2019

ruflin deleted the encode-saved-objects branch November 26, 2019 08:24

This was referenced Nov 26, 2019

Issues with some example package values #42

Closed

[EPM] Remove encoding of Kibana objects as not needed anymore elastic/kibana#51694

Merged

		@@ -0,0 +1,287 @@
		// Licensed to Elasticsearch B.V. under one or more contributor

Conversation

ruflin commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

ruflin commented Nov 25, 2019

Uh oh!

ruflin commented Nov 25, 2019

Uh oh!

jfsiii commented Nov 25, 2019

Uh oh!

ruflin commented Nov 25, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfsiii left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ruflin commented Nov 20, 2019 •

edited

Loading