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

Switch to embeddable factory interface with optional override #61165

Merged

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Mar 24, 2020

The main goal of this PR is to support the ability for another plugin to modify the creation of turning EmbeddableFactoryDefinitions to EmbeddableFactory instances.

The downside is that only one plugin can do it. This is the same limitation the same pattern that we use for savedObjectClient has.

Broken down, this PR:

  • EmbeddableFactory is an interface now, not an abstract class.
  • Introduction of new Embeddable API setEmbeddableFactoryProvider. This is tested in a jest test but not used. There is no sample plugin explaining how to use this. Since we can only have one plugin extend it, and we know AppArch team will be registering the one plugin that we will do this (EmbeddableEnhanced), I don't think we should advertise this extension point.
  • If we can figure out how to support multiple plugins intercepting this creation process, which would be beneficial, then we should add tests/example usage.
  • Many tests needed to be updated to take into account the difference between EmbeddableFactoryDefinition and EmbeddableFactory and now use embeddablePluginMock. This is better for testing anyway.
  • VisualizeEmbeddable and MapEmbeddable exposed new functionality on the EmbeddableFactory that is no longer carried over to the instance of EmbeddableFactory, so those usages needed to be removed/changed.
  • One downside to this new approach is to preserve this context in the definition functions, I have to use const create = async () => semantics instead of create() {....
    • I am now binding all the functions so implementors aren't required to do this.

@stacey-gammon stacey-gammon force-pushed the 2020-03-23-embed-factory-interface branch from 31b0970 to 2bc461b Compare March 25, 2020 16:12
@stacey-gammon stacey-gammon force-pushed the 2020-03-23-embed-factory-interface branch from 2bc461b to 538a748 Compare March 25, 2020 20:03
@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Team:AppArch labels Mar 26, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@stacey-gammon stacey-gammon force-pushed the 2020-03-23-embed-factory-interface branch from 36da384 to fb69ea3 Compare March 26, 2020 15:55
@stacey-gammon stacey-gammon force-pushed the 2020-03-23-embed-factory-interface branch from d671573 to 92f3206 Compare March 27, 2020 13:14
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maps changes LGTM
code review

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIEM code changes look good, however when testing locally I'm seeing the provided layerList duplicated in both error and loading states. All map requests to /internal/search/es also fail with a 400.

Could this be an error in our configuration, or perhaps the provided layerList is getting initialized twice?

Here's the failed request payload

{
  "params": {
    "ignoreThrottled": true,
    "preference": 1585690218105,
    "index": "filebeat-*",
    "body": {
      "size": 0,
      "aggs": {
        "destSplit": {
          "terms": {
            "script": {
              "source": "doc['destination.geo.location'].value.toString()",
              "lang": "painless"
            },
            "order": {
              "_count": "desc"
            },
            "size": 100
          },
          "aggs": {
            "sourceGrid": {
              "geotile_grid": {
                "field": "source.geo.location",
                "precision": 2,
                "size": 500
              },
              "aggs": {
                "sourceCentroid": {
                  "geo_centroid": {
                    "field": "source.geo.location"
                  }
                },
                "sum_of_source.bytes": {
                  "sum": {
                    "field": "source.bytes"
                  }
                },
                "sum_of_destination.bytes": {
                  "sum": {
                    "field": "destination.bytes"
                  }
                }
              }
            }
          }
        }
      },
      "stored_fields": [
        "*"
      ],
      "script_fields": {},
      "docvalue_fields": [
        {
          "field": "@timestamp",
          "format": "date_time"
        },
        {
          "field": "azure.auditlogs.properties.activity_datetime",
          "format": "date_time"
        },
        {
          "field": "azure.enqueued_time",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.agentReceiptTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.deviceCustomDate1",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.deviceCustomDate2",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.deviceReceiptTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.endTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.fileCreateTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.fileModificationTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.flexDate1",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.managerReceiptTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.oldFileCreateTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.oldFileModificationTime",
          "format": "date_time"
        },
        {
          "field": "cef.extensions.startTime",
          "format": "date_time"
        },
        {
          "field": "event.created",
          "format": "date_time"
        },
        {
          "field": "event.end",
          "format": "date_time"
        },
        {
          "field": "event.ingested",
          "format": "date_time"
        },
        {
          "field": "event.start",
          "format": "date_time"
        },
        {
          "field": "file.accessed",
          "format": "date_time"
        },
        {
          "field": "file.created",
          "format": "date_time"
        },
        {
          "field": "file.ctime",
          "format": "date_time"
        },
        {
          "field": "file.mtime",
          "format": "date_time"
        },
        {
          "field": "kafka.block_timestamp",
          "format": "date_time"
        },
        {
          "field": "misp.campaign.first_seen",
          "format": "date_time"
        },
        {
          "field": "misp.campaign.last_seen",
          "format": "date_time"
        },
        {
          "field": "misp.intrusion_set.first_seen",
          "format": "date_time"
        },
        {
          "field": "misp.intrusion_set.last_seen",
          "format": "date_time"
        },
        {
          "field": "misp.observed_data.first_observed",
          "format": "date_time"
        },
        {
          "field": "misp.observed_data.last_observed",
          "format": "date_time"
        },
        {
          "field": "misp.report.published",
          "format": "date_time"
        },
        {
          "field": "misp.threat_indicator.valid_from",
          "format": "date_time"
        },
        {
          "field": "misp.threat_indicator.valid_until",
          "format": "date_time"
        },
        {
          "field": "netflow.collection_time_milliseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.exporter.timestamp",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_end_microseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_end_milliseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_end_nanoseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_end_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_start_microseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_start_milliseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_start_nanoseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.flow_start_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.max_export_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.max_flow_end_microseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.max_flow_end_milliseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.max_flow_end_nanoseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.max_flow_end_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.min_export_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.min_flow_start_microseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.min_flow_start_milliseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.min_flow_start_nanoseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.min_flow_start_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.monitoring_interval_end_milli_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.monitoring_interval_start_milli_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.observation_time_microseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.observation_time_milliseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.observation_time_nanoseconds",
          "format": "date_time"
        },
        {
          "field": "netflow.observation_time_seconds",
          "format": "date_time"
        },
        {
          "field": "netflow.system_init_time_milliseconds",
          "format": "date_time"
        },
        {
          "field": "package.installed",
          "format": "date_time"
        },
        {
          "field": "process.parent.start",
          "format": "date_time"
        },
        {
          "field": "process.start",
          "format": "date_time"
        },
        {
          "field": "suricata.eve.flow.end",
          "format": "date_time"
        },
        {
          "field": "suricata.eve.flow.start",
          "format": "date_time"
        },
        {
          "field": "suricata.eve.timestamp",
          "format": "date_time"
        },
        {
          "field": "suricata.eve.tls.notafter",
          "format": "date_time"
        },
        {
          "field": "suricata.eve.tls.notbefore",
          "format": "date_time"
        },
        {
          "field": "tls.client.not_after",
          "format": "date_time"
        },
        {
          "field": "tls.client.not_before",
          "format": "date_time"
        },
        {
          "field": "tls.server.not_after",
          "format": "date_time"
        },
        {
          "field": "tls.server.not_before",
          "format": "date_time"
        },
        {
          "field": "zeek.kerberos.valid.from",
          "format": "date_time"
        },
        {
          "field": "zeek.kerberos.valid.until",
          "format": "date_time"
        },
        {
          "field": "zeek.ocsp.revoke.time",
          "format": "date_time"
        },
        {
          "field": "zeek.ocsp.update.next",
          "format": "date_time"
        },
        {
          "field": "zeek.ocsp.update.this",
          "format": "date_time"
        },
        {
          "field": "zeek.pe.compile_time",
          "format": "date_time"
        },
        {
          "field": "zeek.smb_files.times.accessed",
          "format": "date_time"
        },
        {
          "field": "zeek.smb_files.times.changed",
          "format": "date_time"
        },
        {
          "field": "zeek.smb_files.times.created",
          "format": "date_time"
        },
        {
          "field": "zeek.smb_files.times.modified",
          "format": "date_time"
        },
        {
          "field": "zeek.smtp.date",
          "format": "date_time"
        },
        {
          "field": "zeek.snmp.up_since",
          "format": "date_time"
        },
        {
          "field": "zeek.x509.certificate.valid.from",
          "format": "date_time"
        },
        {
          "field": "zeek.x509.certificate.valid.until",
          "format": "date_time"
        }
      ],
      "_source": {
        "excludes": []
      },
      "query": {
        "bool": {
          "must": [],
          "filter": [
            {
              "match_all": {}
            },
            {
              "match_all": {}
            },
            {
              "geo_bounding_box": {
                "destination.geo.location": {
                  "top_left": [
                    -180,
                    89.78601
                  ],
                  "bottom_right": [
                    180,
                    -89.78601
                  ]
                }
              }
            },
            {
              "range": {
                "@timestamp": {
                  "gte": "2020-03-30T21:36:06.271Z",
                  "lte": "2020-03-31T21:36:06.271Z",
                  "format": "strict_date_optional_time"
                }
              }
            }
          ],
          "should": [],
          "must_not": []
        }
      }
    },
    "rest_total_hits_as_int": true,
    "ignore_unavailable": true,
    "ignore_throttled": true,
    "timeout": "30000ms"
  },
  "serverStrategy": "es"
}

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Mar 31, 2020

ugghhh, thanks for looking @spong. I don't have an easy way to get SIEM data , the old creds I had for the hosted es instance are causing errors ( FATAL Error: [config validation of [elasticsearch].username]: value of "elastic" is forbidden. This is a superuser account that can obfuscate privilege-related issues. You should use the "kibana" user instead.
). Will try to sync up with you tomorrow so I can debug.

What do you mean by

the provided layerList duplicated in both error and loading states.

btw, probably a good spot to add a functional test for so the error would be caught on ci.

@stacey-gammon
Copy link
Contributor Author

@nreese - would you mind helping me out with this error? I stepped through and setLayerList is being called as I'd expect. This function already existed on the MapEmbeddable, but I don't think it was called anywhere prior to this PR, so maybe there is a bug when setting the layer list outside of the constructor? I'm a bit out of my element digging into the maps code to debug.

@nreese
Copy link
Contributor

nreese commented Apr 1, 2020

I have located the problem that @spong is running into. The problem is that replaceLayerList is getting called before the map has loaded/rendered. This means that layers are getting added to waitingForMapReadyLayerList state instead of layerList. replaceLayerList has a bug and does not clear out waitingForMapReadyLayerList so thats why the layers are showing up multiple times. Replace https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/public/actions/map_actions.js#L175 with

export function replaceLayerList(newLayerList) {
  return (dispatch, getState) => {
    dispatch({
      type: CLEAR_WAITING_FOR_MAP_READY_LAYER_LIST,
    });
    getLayerListRaw(getState()).forEach(({ id }) => {
      dispatch(removeLayerFromLayerList(id));
    });

    newLayerList.forEach(layerDescriptor => {
      dispatch(addLayer(layerDescriptor));
    });
  };
}

@nreese
Copy link
Contributor

nreese commented Apr 1, 2020

created #62202 to address the problem in replaceLayerList action creator that is resulting in duplicated layers.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested dashboard and visualize embeddable flow, works as expected. Had a look at other embeddables, all seems fine to me. Haven't tested other scenarios.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nreese
Copy link
Contributor

nreese commented Apr 2, 2020

#62202 is merged so the problem should be resolved now

@stacey-gammon stacey-gammon requested a review from spong April 2, 2020 16:19
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked out and verified layerList fix is in place. The SIEM Network Map now no longer has duplicate layers in both loading/error states. In testing I found another issue with map requests failing (and thus no data is shown), but have opened #62356 to track.

Approving as base SIEM changes LGTM -- thanks @stacey-gammon and @nreese!

@stacey-gammon stacey-gammon merged commit bb747ab into elastic:master Apr 2, 2020
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Apr 2, 2020
…c#61165)

* wip

* typescript map embeddable

* More updates

* Address code review comments and update some usages in SIEM and uptime to the new types

* More clean up - carry over some of the SIEM types to maps for render tool tip

* fixes

* fixes

* Address more review comments

* fixes

* fixes

* fix jest test

* Fix visualize embeddable

* fixes after master merge

* Fixes

* Prefix variable with name "custom" to make it more obvious

* Remove layerList from input state

* fixes

* Update src/plugins/dashboard/public/embeddable/dashboard_container_factory.tsx

Co-Authored-By: Vadim Dalecky <streamich@users.noreply.github.com>

* review updates

* fixes

* update maps readme

Co-authored-by: Vadim Dalecky <streamich@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 2, 2020
* master:
  Switch to embeddable factory interface with optional override (elastic#61165)
  fix text error in diagrams (elastic#62101)
  [Index management] Prepare support Index template V2 format (elastic#61588)
  Updates dashboard images (elastic#62011)
  [Maps] remove MapBounds type (elastic#62332)
  [Uptime] Convert anomaly Jobs name to lowercase to comply with… (elastic#62293)
  Make d3 place nicely with object values (elastic#62004)
  EMT-287: update schema with elastic agent id (elastic#62252)
  [Maps] fix replaceLayerList to handle case where map is not intialized (elastic#62202)
  Remove support for deprecated xpack.telemetry configurations (elastic#51142)
  [Uptime] Remove static constant for index name completely (elastic#62256)
  [APM] E2E: install dependencies for vanilla workspaces (elastic#62178)
  [backport] Bump to 5.1.3 (elastic#62286)
  Show server name in Remote Cluster detail panel (elastic#62250)
  Rename some alert types (elastic#61693)
  changing duration type to ms, s, m (elastic#62265)
  [ML] Clear Kibana index pattern cache on creation or form reset. (elastic#62184)
  Move `src/legacy/server/index_patterns` to data plugin (server) (Remove step) (elastic#61618)
  [NP] Remove IndexedArray usage in Schemas (elastic#61410)
stacey-gammon added a commit that referenced this pull request Apr 2, 2020
#62360)

* wip

* typescript map embeddable

* More updates

* Address code review comments and update some usages in SIEM and uptime to the new types

* More clean up - carry over some of the SIEM types to maps for render tool tip

* fixes

* fixes

* Address more review comments

* fixes

* fixes

* fix jest test

* Fix visualize embeddable

* fixes after master merge

* Fixes

* Prefix variable with name "custom" to make it more obvious

* Remove layerList from input state

* fixes

* Update src/plugins/dashboard/public/embeddable/dashboard_container_factory.tsx

Co-Authored-By: Vadim Dalecky <streamich@users.noreply.github.com>

* review updates

* fixes

* update maps readme

Co-authored-by: Vadim Dalecky <streamich@users.noreply.github.com>

Co-authored-by: Vadim Dalecky <streamich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes review v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants