[Remote clusters] Migrate server code out of legacy by alisonelizabeth · Pull Request #56781 · elastic/kibana

alisonelizabeth · 2020-02-04T19:36:10Z

This PR completes the NP migration for the remote_clusters server-side code. I will address the client code in a separate PR.

Changes include:

Adopted NP router
- I removed a workaround in update_route.ts as the issue has been fixed on the ES side.
Adopted NP license plugin
- I'm happy to change the implementation here once we decide on a common pattern.
Updated route tests using the NP core mocks
Moved server code out of legacy directory

Note: It may be helpful to review the code changes by commit.

How to test

You will need to set up two clusters in order to test remote clusters. For example:

yarn es snapshot
yarn es snapshot -E cluster.name="my_cluster" -E http.port=9201 -E path.data=/tmp/es (in another tab)

Test all routes and confirm behaving as expected (create, edit, delete, get remote clusters)
Test license check (remote clusters is available with Basic license)

elasticmachine · 2020-02-04T19:36:13Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

…sters

elasticmachine · 2020-02-06T22:48:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 6fd2c7f

Test Failures

Kibana Pipeline / kibana-xpack-agent / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/remote_clusters/remote_clusters·js.apis management remote clusters Remote Clusters Add should not allow us to re-add an existing remote cluster

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook
[00:04:17]           └-: management
[00:04:17]             └-> "before all" hook
[00:04:31]             └-: remote clusters
[00:04:31]               └-> "before all" hook
[00:04:31]               └-: Remote Clusters
[00:04:31]                 └-> "before all" hook
[00:04:31]                 └-: Add
[00:04:31]                   └-> "before all" hook
[00:04:31]                   └-> should allow us to add a remote cluster
[00:04:31]                     └-> "before each" hook: global before each
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:04:31]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.skip_unavailable] from [false] to [true]
[00:04:31]                     └- ✓ pass  (65ms) "apis management remote clusters Remote Clusters Add should allow us to add a remote cluster"
[00:04:31]                   └-> should not allow us to re-add an existing remote cluster
[00:04:31]                     └-> "before each" hook: global before each
[00:04:31]                     └- ✖ fail: "apis management remote clusters Remote Clusters Add should not allow us to re-add an existing remote cluster"
[00:04:31]                     │

Stack Trace

Error: expected 409 "Conflict", got 400 "Bad Request"
    at Test._assertStatus (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:268:12)
    at Test._assertFunction (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:283:11)
    at Test.assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:173:18)
    at assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:131:12)
    at /dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:128:5
    at Test.Request.callback (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:718:3)
    at parser (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:906:18)
    at IncomingMessage.res.on (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/parsers/json.js:19:7)
    at endReadableNT (_stream_readable.js:1145:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

Kibana Pipeline / kibana-xpack-agent / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/remote_clusters/remote_clusters·js.apis management remote clusters Remote Clusters Add should not allow us to re-add an existing remote cluster

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]       │
[00:00:00]         └-: apis
[00:00:00]           └-> "before all" hook
[00:04:55]           └-: management
[00:04:55]             └-> "before all" hook
[00:05:09]             └-: remote clusters
[00:05:09]               └-> "before all" hook
[00:05:09]               └-: Remote Clusters
[00:05:09]                 └-> "before all" hook
[00:05:09]                 └-: Add
[00:05:09]                   └-> "before all" hook
[00:05:09]                   └-> should allow us to add a remote cluster
[00:05:09]                     └-> "before each" hook: global before each
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.seeds] from [[]] to [["localhost:6173"]]
[00:05:09]                     │ info [o.e.c.s.ClusterSettings] [kibana-ci-immutable-centos-tests-xl-1581024350419232108] updating [cluster.remote.test_cluster.skip_unavailable] from [false] to [true]
[00:05:09]                     └- ✓ pass  (71ms) "apis management remote clusters Remote Clusters Add should allow us to add a remote cluster"
[00:05:09]                   └-> should not allow us to re-add an existing remote cluster
[00:05:09]                     └-> "before each" hook: global before each
[00:05:09]                     └- ✖ fail: "apis management remote clusters Remote Clusters Add should not allow us to re-add an existing remote cluster"
[00:05:09]                     │

Stack Trace

Error: expected 409 "Conflict", got 400 "Bad Request"
    at Test._assertStatus (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:268:12)
    at Test._assertFunction (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:283:11)
    at Test.assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:173:18)
    at assert (/dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:131:12)
    at /dev/shm/workspace/kibana/node_modules/supertest/lib/test.js:128:5
    at Test.Request.callback (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:718:3)
    at parser (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/index.js:906:18)
    at IncomingMessage.res.on (/dev/shm/workspace/kibana/node_modules/superagent/lib/node/parsers/json.js:19:7)
    at endReadableNT (_stream_readable.js:1145:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

History

💔 Build #24644 failed 0021d90
💔 Build #24390 failed 03d8aa1

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

…sters

elasticmachine · 2020-02-07T05:19:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: c9d0c5d

History

💔 Build #25085 failed 6fd2c7f
💔 Build #24644 failed 0021d90
💔 Build #24390 failed 03d8aa1

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

elasticmachine · 2020-02-07T14:51:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 14cbcb6

History

💔 Build #25155 failed c9d0c5d
💔 Build #25085 failed 6fd2c7f
💔 Build #24644 failed 0021d90
💔 Build #24390 failed 03d8aa1

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

jloleysens

Great work @alisonelizabeth ! I left a few comments. One about typing information that I think should be addressed and one about migrating config to New Platform. I think it would be good to include the migration here if we consider this the final migration step from our side.

I created, updated and deleted a remote cluster(s) and the functionality worked 100% 🎉

jloleysens · 2020-02-10T10:08:47Z

x-pack/legacy/plugins/remote_clusters/index.ts

@@ -7,8 +7,6 @@
 import { Legacy } from 'kibana';


I think if the goal of this contribution is to complete the NP migration we should strip this file down to the bare minimum required to keep functional parity with legacy. For instance:

Migrating config and injectDefaultVars see https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md#configure-plugin

Similarly configPrefix can be removed if we define it in kibana.json I believe

If we use the new platform management service for registering the plugin can we remove managementSections?

Is isEnabled being used somewhere else?

👍 I was planning on addressing these items in a separate PR as part of the public migration.

Also, isEnabled is being used by CCR so I think this needs to stay as is for now.

jloleysens · 2020-02-10T10:30:34Z

x-pack/plugins/remote_clusters/server/routes/api/add_route.ts

+import { RouteDependencies } from '../../types';
+
+export const register = (deps: RouteDependencies): void => {
+  const addHandler: RequestHandler<any, any, any> = async (ctx, request, response) => {


I think it would be good to try and avoid use of any here when we know what the query, params and body will be.

Using unknown explicitly when we know we don't expect a value in one of those slots makes it clear what we expect for a route. Then passing in type information from:

body: schema.object({ name: schema.string(), seeds: schema.arrayOf(schema.string()), skipUnavailable: schema.boolean(), }),

would be really nice!

Good point, fixed!

jloleysens · 2020-02-10T10:35:05Z

x-pack/plugins/remote_clusters/server/routes/api/delete_route.ts

+          const cluster = get(updateClusterResponse, `persistent.cluster.remote.${name}`);
+
+          if (acknowledged && !cluster) {
+            return null;


A short comment here explaining why we are not returning a KibanaResponse for the happy path would be helpful 🙂

Added a comment. The response is being returned on L104.

…sters

alisonelizabeth · 2020-02-10T15:32:22Z

@jloleysens I believe I addressed all of your comments. Mind taking another look? I plan on creating a separate PR to complete the migration of the public code and anything else outstanding in x-pack/legacy/plugins/remote_clusters/index.ts

jloleysens

Happy with the changes - did not test manually again.

alisonelizabeth · 2020-02-10T16:17:13Z

@elasticmachine merge upstream

alisonelizabeth · 2020-02-10T18:56:38Z

@elasticmachine merge upstream

kibanamachine · 2020-02-10T20:22:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a658699

History

💔 Build #25628 failed 4c722fe
💔 Build #25603 failed 276389a
💚 Build #25238 succeeded 14cbcb6
💔 Build #25155 failed c9d0c5d
💔 Build #25085 failed 6fd2c7f

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

* master: (34 commits) [Index management] Server-side NP ready (elastic#56829) Webhook action - make user and password secrets optional (elastic#56823) [DOCS] Removes reference to IRC (elastic#57245) [Monitoring] NP migration: Local angular module (elastic#51823) [SIEM] Adds ECS link to help menu (elastic#57104) Ensure http interceptors are shares across lifecycle methods (elastic#57150) [Remote clusters] Migrate server code out of legacy (elastic#56781) fixes render bug in alert list (elastic#57152) siem 7.6 updates (elastic#57169) Make the update alert API key API work when AAD is out of sync (elastic#56640) fix(NA): MaxListenersExceededWarning on getLoggerStream (elastic#57133) [Metrics UI] Setup commonly used time ranges in timepicker (elastic#56701) [Maps] set filter.meta.key to geoFieldName so query passes filterMatchesIndex when ignoreFilterIfFieldNotInIndex is true (elastic#56692) Create plugin mock for event log plugin (elastic#57048) fix ts error on master (elastic#57236) Don't create API key for disabled alerts when calling create API (elastic#57041) Fix enable and disable API to still work when AAD is out of sync (elastic#56634) [DOCS] Canvas embed objects (elastic#57156) Delete autocomplete namespace (elastic#57187) Security - Inject logout url (elastic#57201) ...

migrate to NP router

03d8aa1

alisonelizabeth added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 4, 2020

move server code out of legacy and integrate NP license plugin

0021d90

alisonelizabeth changed the title ~~[WIP] [Remote clusters] Migrate remote clusters to NP~~ [WIP] [Remote clusters] Migrate server code to NP Feb 5, 2020

alisonelizabeth changed the title ~~[WIP] [Remote clusters] Migrate server code to NP~~ [WIP] [Remote clusters] Migrate server code out of legacy Feb 5, 2020

alisonelizabeth added 2 commits February 6, 2020 11:46

Merge branch 'master' of github.com:elastic/kibana into np/remote_clu…

fc7e21f

…sters

update tests + remove use of callWithRequestFactory()

6fd2c7f

elastic deleted a comment from kibanamachine Feb 6, 2020

alisonelizabeth added 4 commits February 6, 2020 22:16

fix TS

18a36f3

Merge branch 'master' of github.com:elastic/kibana into np/remote_clu…

8dfd03b

…sters

update api integrationt tests

12efed3

remove todo comment

c9d0c5d

alisonelizabeth changed the title ~~[WIP] [Remote clusters] Migrate server code out of legacy~~ [Remote clusters] Migrate server code out of legacy Feb 7, 2020

i18n fix

14cbcb6

alisonelizabeth marked this pull request as ready for review February 7, 2020 14:45

alisonelizabeth requested a review from jloleysens February 7, 2020 14:48

jloleysens mentioned this pull request Feb 10, 2020

[Licensing] Enabling and disabling functionality based on current license in public #55985

Closed

jloleysens reviewed Feb 10, 2020

View reviewed changes

alisonelizabeth added 2 commits February 10, 2020 08:32

Merge branch 'master' of github.com:elastic/kibana into np/remote_clu…

e65ba33

…sters

addres review feedback

276389a

jloleysens approved these changes Feb 10, 2020

View reviewed changes

Merge branch 'master' into np/remote_clusters

4c722fe

Merge branch 'master' into np/remote_clusters

a658699

alisonelizabeth merged commit 82972ef into elastic:master Feb 10, 2020

alisonelizabeth deleted the np/remote_clusters branch February 10, 2020 20:25

alisonelizabeth mentioned this pull request Feb 10, 2020

[7.x] [Remote clusters] Migrate server code out of legacy (#56781) #57263

Merged

alisonelizabeth added a commit that referenced this pull request Feb 10, 2020

[Remote clusters] Migrate server code out of legacy (#56781) (#57263)

b64ad63

alisonelizabeth mentioned this pull request Feb 11, 2020

[Remote clusters] Migrate client-side code out of legacy #57365

Merged

cjcenizal added the Feature:CCR and Remote Clusters label Apr 1, 2020

Conversation

alisonelizabeth commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Uh oh!

elasticmachine commented Feb 4, 2020

Uh oh!

elasticmachine commented Feb 6, 2020

💔 Build Failed

Test Failures

Standard Out

Stack Trace

Standard Out

Stack Trace

History

Uh oh!

elasticmachine commented Feb 7, 2020

💔 Build Failed

History

Uh oh!

elasticmachine commented Feb 7, 2020

💚 Build Succeeded

History

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

jloleysens Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

jloleysens Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

jloleysens Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Feb 10, 2020

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Feb 10, 2020

Uh oh!

alisonelizabeth commented Feb 10, 2020

Uh oh!

kibanamachine commented Feb 10, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alisonelizabeth commented Feb 4, 2020 •

edited

Loading