Migrate instance uuid logic to new platform #52060

pgayvallet · 2019-12-03T10:38:28Z

Summary

Migrate the code that was managing the instance uuid and writing it to file (formerly in src/legacy/core_plugins/kibana/server/lib/manage_uuid.js) to new platform.

As the only public API exposed from this feature is a constant-like, I tried to expose it to plugins via the PluginInitializerContext (something like pluginInitializerContext.instanceUuid). However the plugins PluginInitializerContext is created during plugin discovery, which is done before configuration validation. As we decided in #49378 that no config access should be performed before config validation, and as the uuid logic relies on reading the config, I had to fallback to a more classic service approach (core.uuid)

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-12-03T11:07:35Z

💔 Build Failed

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

pgayvallet · 2019-12-03T14:27:03Z

Waiting for #51478 to be merged (need path.data access in NP config)

pgayvallet · 2019-12-06T16:15:19Z

src/legacy/server/kbn_server.d.ts

-export type KibanaConfig = LegacyConfig;
+// lot of legacy code was assuming this type only had these two methods
+export type KibanaConfig = Pick<LegacyConfig, 'get' | 'has'>;


Added set method in LegacyConfig to set the uuid in it for legacy compatibility. However a LOT of code in legacy made the assumption that KibanaConfig only had get and has.

I'm surprised having set would break anything?

Lots of TS errors, mostly (but not only) in test code, similars to:

Type '() => { get<T>(key: string): any; has(key: string): boolean; }' is not assignable to type '() => LegacyConfig'. Property 'set' is missing in type '{ get<T>(key: string): any; has(key: string): boolean; }' but required in type 'LegacyConfig'

As set isn't used in legacy anyway, It felt like is was more pragmatic to restrict the type than to adapt something like 25 usages in multiple solutions code.

pgayvallet · 2019-12-06T16:20:07Z

src/core/server/http/http_config.ts

@@ -91,6 +91,7 @@ export const config = {
          )
        ),
      }),
+      uuid: schema.maybe(schema.string()),


I don't like having to add this here, as the server config namespace is now more like only the http config, However the current property is server.uuid, so I don't really see another way.

We could deprecates/rename that once the config deprecation PR lands if we want to.

Can you add a note to the config refactoring issue just so we don't forget?

In the legacy config, we validated this as a valid guid. Can we add that validation here as well? Should be able to specify a custom validate option to schema.string()

Oh good catch, totally forgot about that

pgayvallet · 2019-12-06T16:38:29Z

src/core/server/uuid/uuid_service.ts

+    // propagate the instance uuid to the legacy config, as it was the legacy way to access it.
+    legacyPlugins.pluginExtendedConfig.set('server.uuid', this.uuid);


The legacy way to access the instance uuid was via the config... the legacy mixin was writing it during server init.

I tried to migrates the legacy usages of config.get('server.uuid'), however some calls are very deeply nested in places where the server, and therefor the NP API, is not accessible. So to preserve retro-compatibility, I was kinda forced to do that.

Tell me if we think this is not acceptable and if I should migrate all calls instead.

Would it be possible to move this code into the LegacyService instead? That way we can more easily delete legacy code without having to modify this as well.

Done. The legacy service tests mocks requirements are growing though. It was harder than expected to move the legacy config set test here.

pgayvallet · 2019-12-06T16:41:11Z

src/core/server/uuid/uuid_service.ts

+
+  public async setup({ legacyPlugins }: SetupDeps) {
+    this.uuid = await manageInstanceUuid(this.configService);
+    this.log.info(`Kibana instance UUID: ${this.uuid}`);


The legacy was logging different (but very similar) messages depending on where the uuid was read from. I think we don't really care, and that the only important info is the actual UUID, so I simplified. Please tell me if I'm wrong.

I'm not sure if we necessarily care, but seeing how this does affect startup and some critical functions (task_manager) maybe it would be good to retain some insight. That said, I think downgrading the detailed messages to debug would be fine.

Ok, adapted to use the exact same messages we had in legacy, and used debug instead of info

elasticmachine · 2019-12-06T17:08:06Z

💚 Build Succeeded

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

elasticmachine · 2019-12-06T17:13:30Z

Pinging @elastic/kibana-platform (Team:Platform)

joshdover · 2019-12-11T19:47:46Z

src/core/server/uuid/uuid_service.ts

+    // propagate the instance uuid to the legacy config, as it was the legacy way to access it.
+    legacyPlugins.pluginExtendedConfig.set('server.uuid', this.uuid);


Would it be possible to move this code into the LegacyService instead? That way we can more easily delete legacy code without having to modify this as well.

joshdover · 2019-12-11T19:50:07Z

src/core/server/uuid/manage_uuid.ts

+    configService
+      .atPath<PathConfigType>('path')
+      .pipe(take(1))
+      .toPromise(),
+    configService
+      .atPath<HttpConfigType>('server')
+      .pipe(take(1))
+      .toPromise(),
+  ]);


nit: use config.path from the original modules rather than hardcoded strings.

joshdover · 2019-12-11T19:52:12Z

src/core/server/uuid/manage_uuid.ts

+
+  const uuidFilePath = join(pathConfig.data, FILE_NAME);
+
+  const uuidFromFile = await readUUIDFromFile(uuidFilePath);


super nit: can we capitalize all these UUID names with the same convention used elsewhere: readUuidFromFile

Thanks. I'm more of a UUID person myself, so I had to rename all theses after asking about our conventions. Missed some, will fix that.

joshdover · 2019-12-11T19:55:20Z

src/core/server/uuid/manage_uuid.ts

+const readFile = promisify(fs.readFile);
+const writeFile = promisify(fs.writeFile);
+
+export async function manageInstanceUuid(configService: IConfigService): Promise<string> {


Suggested change

export async function manageInstanceUuid(configService: IConfigService): Promise<string> {

export async function resolveInstanceUuid(configService: IConfigService): Promise<string> {

seems more descriptive than manage?

Probably better. Wasn't that inspired with the name. Changed.

joshdover · 2019-12-11T19:56:14Z

src/legacy/server/kbn_server.d.ts

-export type KibanaConfig = LegacyConfig;
+// lot of legacy code was assuming this type only had these two methods
+export type KibanaConfig = Pick<LegacyConfig, 'get' | 'has'>;


I'm surprised having set would break anything?

joshdover · 2019-12-11T19:58:46Z

src/core/server/uuid/uuid_service.ts

+
+  public async setup({ legacyPlugins }: SetupDeps) {
+    this.uuid = await manageInstanceUuid(this.configService);
+    this.log.info(`Kibana instance UUID: ${this.uuid}`);


I'm not sure if we necessarily care, but seeing how this does affect startup and some critical functions (task_manager) maybe it would be good to retain some insight. That said, I think downgrading the detailed messages to debug would be fine.

rudolf · 2019-12-13T13:21:15Z

src/core/server/http/http_config.ts

@@ -91,6 +91,7 @@ export const config = {
          )
        ),
      }),
+      uuid: schema.maybe(schema.string()),


Can you add a note to the config refactoring issue just so we don't forget?

src/core/server/uuid/uuid_service.ts

src/core/server/uuid/resolve_uuid.ts

rudolf · 2019-12-13T13:53:01Z

src/core/server/uuid/resolve_uuid.ts

+      // non-existent uuid file is ok, we will create it.
+      return undefined;
+    }
+    throw e;


It's possible that we get a EACCESS error here and in writeUuidToFile we might also get EISDIR. I think it would be worth catching these errors and throwing a more useful error to users, maybe something like:

Unable to write Kibana UUID file, please check the uuid.server configuration value in kibana.yml and ensure Kibana has sufficient permissions to read / write to this file.

I think it would also be worth testing these error conditions

Explicit error message is indeed better.

rudolf · 2019-12-16T10:38:18Z

src/core/server/uuid/resolve_uuid.test.ts

+      await expect(
+        resolveInstanceUuid(configService, logger)
+      ).rejects.toThrowErrorMatchingInlineSnapshot(
+        `"Unable to write Kibana UUID file, please check the uuid.server configurationvalue in kibana.yml and ensure Kibana has sufficient permissions to read / write to this file. Error was: undefined"`


"Error was: undefined" doesn't seem right.

Damn. thanks you. That's the actual main reason why I hate snapshot testing. That makes developer lazy and leads to that kind of errors.

elasticmachine · 2019-12-17T08:09:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 16e9a2f

History

💚 Build #15744 succeeded 9f27f5a
💚 Build #15660 succeeded 1b8e6b6
💚 Build #15530 succeeded 83eb19f
💚 Build #15447 succeeded 1844093
💚 Build #15336 succeeded 8bcc718

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

@public

* move and migrate uuid code to new platform * create and wire uuid service * handle legacy compatibility * update generated docs * add `set` to LegacyConfig interface * Fix types * fix config access * respect naming conventions for uuid * remove hardcoded config paths * rename manageInstanceUuid to resolveInstanceUuid * moves legacy config uuid set from uuid to legacy service * log specific message depending on how uuid was resolved * resolve merge conflicts * use fs.promises * add forgotten @public in uuid contract * add explicit errors and tests * ensure uuid is valid in configuration * fix read/write tests

@public

* move and migrate uuid code to new platform * create and wire uuid service * handle legacy compatibility * update generated docs * add `set` to LegacyConfig interface * Fix types * fix config access * respect naming conventions for uuid * remove hardcoded config paths * rename manageInstanceUuid to resolveInstanceUuid * moves legacy config uuid set from uuid to legacy service * log specific message depending on how uuid was resolved * resolve merge conflicts * use fs.promises * add forgotten @public in uuid contract * add explicit errors and tests * ensure uuid is valid in configuration * fix read/write tests

pgayvallet added 5 commits December 6, 2019 14:45

move and migrate uuid code to new platform

732ccf2

create and wire uuid service

bb7c5f0

handle legacy compatibility

958b28e

update generated docs

3ed08c5

add set to LegacyConfig interface

6964d1e

pgayvallet force-pushed the kbn-48956-uuid-NP branch from a35888f to 6964d1e Compare December 6, 2019 13:55

This comment has been minimized.

Sign in to view

pgayvallet added 2 commits December 6, 2019 16:18

Fix types

88f9790

fix config access

c8a808d

pgayvallet commented Dec 6, 2019

View reviewed changes

pgayvallet added v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:New Platform labels Dec 6, 2019

pgayvallet added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Dec 6, 2019

pgayvallet marked this pull request as ready for review December 6, 2019 17:13

pgayvallet requested a review from a team as a code owner December 6, 2019 17:13

pgayvallet added 2 commits December 9, 2019 09:18

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

81a0dc0

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

7bbd025

joshdover reviewed Dec 11, 2019

View reviewed changes

pgayvallet added 5 commits December 12, 2019 07:49

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

b850444

respect naming conventions for uuid

a499ff1

remove hardcoded config paths

afcdf9a

rename manageInstanceUuid to resolveInstanceUuid

6c0c880

moves legacy config uuid set from uuid to legacy service

6139832

log specific message depending on how uuid was resolved

26719cb

This was referenced Dec 12, 2019

Implements config deprecation in New Platform #52251

Merged

ClusterManager is created with incorrect parameters #52860

Closed

pgayvallet added 3 commits December 13, 2019 10:02

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

1783d71

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

ba3a23f

resolve merge conflicts

8bcc718

rudolf reviewed Dec 13, 2019

View reviewed changes

pgayvallet added 6 commits December 13, 2019 16:51

use fs.promises

f4b11d8

add forgotten @public in uuid contract

fa6ea93

add explicit errors and tests

a6f5980

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

1844093

ensure uuid is valid in configuration

83eb19f

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

1b8e6b6

rudolf reviewed Dec 16, 2019

View reviewed changes

pgayvallet added 2 commits December 16, 2019 14:56

fix read/write tests

8767b73

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

9f27f5a

joshdover approved these changes Dec 16, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-48956-uuid-NP

16e9a2f

pgayvallet merged commit 2426811 into elastic:master Dec 17, 2019

pgayvallet mentioned this pull request Dec 17, 2019

[7.x] Migrate instance uuid logic to new platform (#52060) #53260

Merged

jbudz mentioned this pull request Feb 24, 2021

Server mocha test failures #41678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate instance uuid logic to new platform #52060

Migrate instance uuid logic to new platform #52060

pgayvallet commented Dec 3, 2019 •

edited

Loading

elasticmachine commented Dec 3, 2019

pgayvallet commented Dec 3, 2019

This comment has been minimized.

pgayvallet Dec 6, 2019

joshdover Dec 11, 2019

pgayvallet Dec 12, 2019 •

edited

Loading

pgayvallet Dec 6, 2019

rudolf Dec 13, 2019

joshdover Dec 13, 2019

pgayvallet Dec 13, 2019

pgayvallet Dec 6, 2019

joshdover Dec 11, 2019

pgayvallet Dec 12, 2019

pgayvallet Dec 6, 2019

joshdover Dec 11, 2019

pgayvallet Dec 12, 2019

elasticmachine commented Dec 6, 2019

elasticmachine commented Dec 6, 2019

joshdover Dec 11, 2019

joshdover Dec 11, 2019

joshdover Dec 11, 2019

pgayvallet Dec 12, 2019

joshdover Dec 11, 2019

pgayvallet Dec 12, 2019

joshdover Dec 11, 2019

joshdover Dec 11, 2019

rudolf Dec 13, 2019

rudolf Dec 13, 2019

rudolf Dec 13, 2019

pgayvallet Dec 13, 2019

rudolf Dec 16, 2019

pgayvallet Dec 16, 2019

elasticmachine commented Dec 17, 2019

		// propagate the instance uuid to the legacy config, as it was the legacy way to access it.
		legacyPlugins.pluginExtendedConfig.set('server.uuid', this.uuid);


		const uuidFilePath = join(pathConfig.data, FILE_NAME);

		const uuidFromFile = await readUUIDFromFile(uuidFilePath);

	export async function manageInstanceUuid(configService: IConfigService): Promise<string> {
	export async function resolveInstanceUuid(configService: IConfigService): Promise<string> {

Migrate instance uuid logic to new platform #52060

Migrate instance uuid logic to new platform #52060

Conversation

pgayvallet commented Dec 3, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Dec 3, 2019

💔 Build Failed

pgayvallet commented Dec 3, 2019

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 6, 2019

💚 Build Succeeded

elasticmachine commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 17, 2019

💚 Build Succeeded

History

pgayvallet commented Dec 3, 2019 •

edited

Loading

pgayvallet Dec 12, 2019 •

edited

Loading