[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

tsullivan · 2021-11-01T20:15:22Z

Closes #113832

Summary

0.0.0.0 is not a valid value for xpack.reporting.kibanaServer.hostname. This setting does not work on windows.
If the server.host is used as the xpack.reporting.kibanaServer.hostname value, the value must be converted to localhost.

Checklist

Test on Windows

Release Note

Fixed an issue where PDF/PNG reporting would break on Windows if the Kibana server hostname was 0.0.0.0

…is "0.0.0.0"

elasticmachine · 2021-11-01T20:17:31Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

jloleysens

Thanks for adding this change @tsullivan , I'm sure this will prevent some bad config!

I have one question regarding re-checking the config that I'd like to get your thoughts on before approving.

x-pack/plugins/reporting/server/config/schema.ts

x-pack/plugins/reporting/server/config/create_config.ts

x-pack/plugins/reporting/server/config/create_config.test.ts

x-pack/plugins/reporting/server/config/schema.ts

…tname

kibanamachine · 2021-11-03T04:51:35Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #3864 failed 3d91c33
💚 Build #3393 succeeded 3b4cd51

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

dokmic · 2021-11-03T08:54:41Z

x-pack/plugins/reporting/server/config/create_config.test.ts

+  for (const hostname of [
+    '0',
+    '0.0',
+    '0.0.0',
+    '0.0.0.0',
+    '0000:0000:0000:0000:0000:0000:0000:0000',
+    '::',
+  ]) {
+    it(`apply failover logic when hostname is given as ${hostname}`, async () => {


Jest provides tooling for such cases. Please use it.each instead.

Suggested change

for (const hostname of [

'0',

'0.0',

'0.0.0',

'0.0.0.0',

'0000:0000:0000:0000:0000:0000:0000:0000',

'::',

]) {

it(`apply failover logic when hostname is given as ${hostname}`, async () => {

it.each`

hostname

${'0'}

${'0.0'}

${'0.0.0'}

${'0.0.0.0'}

${'0000:0000:0000:0000:0000:0000:0000:0000'}

${'::'}

`('apply failover logic when hostname is given as $hostname', async ({ hostname }) => {

dokmic · 2021-11-03T09:00:46Z

x-pack/plugins/reporting/server/config/create_config.test.ts

+    expect(mockLogger.warn.mock.calls.length).toBe(1);
+    expect(mockLogger.warn.mock.calls[0]).toMatchObject([
      'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',
    ]);


I understand that the change was about removing the any. Since I have more relevant feedback below, maybe we can commit the following to prettify the code even more.

Suggested change

expect(mockLogger.warn.mock.calls.length).toBe(1);

expect(mockLogger.warn.mock.calls[0]).toMatchObject([

'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',

]);

expect(mockLogger.warn).toHaveBeenCalledTimes(1);

expect(mockLogger.warn).toHaveBeenCalledWith(

'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.'

);

dokmic · 2021-11-03T09:01:20Z

x-pack/plugins/reporting/server/config/create_config.test.ts

    const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();
    expect(result.encryptionKey).toMatch('iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii');
-    expect((mockLogger.warn as any).mock.calls.length).toBe(0);
+    expect(mockLogger.warn.mock.calls.length).toBe(0);


Suggested change

expect(mockLogger.warn.mock.calls.length).toBe(0);

expect(mockLogger.warn).not.toHaveBeenCalled();

dokmic · 2021-11-03T09:01:43Z

x-pack/plugins/reporting/server/config/create_config.test.ts

@@ -108,7 +103,7 @@ describe('Reporting server createConfig$', () => {
        },
      }
    `);
-    expect((mockLogger.warn as any).mock.calls.length).toBe(0);
+    expect(mockLogger.warn.mock.calls.length).toBe(0);


Suggested change

expect(mockLogger.warn.mock.calls.length).toBe(0);

expect(mockLogger.warn).not.toHaveBeenCalled();

dokmic · 2021-11-03T09:02:09Z

x-pack/plugins/reporting/server/config/create_config.test.ts

    const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

    expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: false });
-    expect((mockLogger.warn as any).mock.calls.length).toBe(0);
+    expect(mockLogger.warn.mock.calls.length).toBe(0);


Suggested change

expect(mockLogger.warn.mock.calls.length).toBe(0);

expect(mockLogger.warn).not.toHaveBeenCalled();

dokmic · 2021-11-03T09:02:36Z

x-pack/plugins/reporting/server/config/create_config.test.ts

    const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

    expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: true });
-    expect((mockLogger.warn as any).mock.calls.length).toBe(0);
+    expect(mockLogger.warn.mock.calls.length).toBe(0);


Suggested change

expect(mockLogger.warn.mock.calls.length).toBe(0);

expect(mockLogger.warn).not.toHaveBeenCalled();

dokmic · 2021-11-03T09:02:58Z

x-pack/plugins/reporting/server/config/create_config.test.ts

    const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

    expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: expect.any(Boolean) });
-    expect((mockLogger.warn as any).mock.calls.length).toBe(0);
+    expect(mockLogger.warn.mock.calls.length).toBe(0);


Suggested change

expect(mockLogger.warn.mock.calls.length).toBe(0);

expect(mockLogger.warn).not.toHaveBeenCalled();

dokmic · 2021-11-03T09:06:59Z

x-pack/plugins/reporting/server/config/create_config.test.ts

+      mockCoreSetup.http.getServerInfo = jest.fn().mockImplementation(
+        (): HttpServerInfo => ({
+          name: 'cool server',
+          hostname,
+          port: 5601,
+          protocol: 'http',
+        })
+      );


Could be simplified a bit.

Suggested change

mockCoreSetup.http.getServerInfo = jest.fn().mockImplementation(

(): HttpServerInfo => ({

name: 'cool server',

hostname,

port: 5601,

protocol: 'http',

})

);

mockCoreSetup.http.getServerInfo = jest.fn((): HttpServerInfo => ({

name: 'cool server',

hostname,

port: 5601,

protocol: 'http',

}));

dokmic · 2021-11-03T09:13:33Z

x-pack/plugins/reporting/server/config/create_config.test.ts

+      const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();
+      expect(result.kibanaServer).toMatchObject({
+        hostname: 'localhost',
+        port: 5601,
+        protocol: 'http',
+      });


I think it would be better to check that the promise is resolved and the result is not empty and has the property. In case when the test fails, jest can provide additional logging of where exactly it went wrong (e.g. promises rejects or empty result).

Suggested change

const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.kibanaServer).toMatchObject({

hostname: 'localhost',

port: 5601,

protocol: 'http',

});

await expect(

createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise()

).resolves.toHaveProperty(

'kibanaServer',

expect.objectContaining({

hostname: 'localhost',

port: 5601,

protocol: 'http',

})

);

dokmic · 2021-11-03T09:19:48Z

x-pack/plugins/reporting/server/config/schema.test.ts

+  for (const address of ['0', '0.0', '0.0.0']) {
+    it(`fails to validate "kibanaServer.hostname" with an invalid hostname: "${address}"`, () => {
+      expect(() =>
+        ConfigSchema.validate({
+          kibanaServer: { hostname: address },
+        })
+      ).toThrowError(`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`);
+    });
+  }


Suggested change

for (const address of ['0', '0.0', '0.0.0']) {

it(`fails to validate "kibanaServer.hostname" with an invalid hostname: "${address}"`, () => {

expect(() =>

ConfigSchema.validate({

kibanaServer: { hostname: address },

})

).toThrowError(`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`);

});

}

it.each`

hostname

${'0'}

${'0.0'}

${'0.0.0'}

`('fails to validate "kibanaServer.hostname" with an invalid hostname: "$hostname"', ({ hostname }) => {

expect(() =>

ConfigSchema.validate({

kibanaServer: { hostname },

})

).toThrowError(`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`);

});

dokmic · 2021-11-03T09:21:57Z

x-pack/plugins/reporting/server/config/schema.test.ts

+  for (const address of ['0.0.0.0', '0000:0000:0000:0000:0000:0000:0000:0000', '::']) {
+    it(`fails to validate "kibanaServer.hostname" hostname as zero: "${address}"`, () => {
+      expect(() =>
+        ConfigSchema.validate({
+          kibanaServer: { hostname: address },
+        })
+      ).toThrowError(
+        `[kibanaServer.hostname]: cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`
+      );
+    });
+  }


Suggested change

for (const address of ['0.0.0.0', '0000:0000:0000:0000:0000:0000:0000:0000', '::']) {

it(`fails to validate "kibanaServer.hostname" hostname as zero: "${address}"`, () => {

expect(() =>

ConfigSchema.validate({

kibanaServer: { hostname: address },

})

).toThrowError(

`[kibanaServer.hostname]: cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`

);

});

}

it.each`

hostname

${'0.0.0.0'}

${'0000:0000:0000:0000:0000:0000:0000:0000'}

${'::'}

`('fails to validate "kibanaServer.hostname" hostname as zero: "$hostname"', ({ hostname }) => {

expect(() =>

ConfigSchema.validate({

kibanaServer: { hostname },

})

).toThrowError(

`[kibanaServer.hostname]: cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`

);

});

dokmic · 2021-11-03T09:24:34Z

x-pack/plugins/reporting/server/config/schema.ts

+      validate(value) {
+        if (ipaddr.isValid(value) && !sum(ipaddr.parse(value).toByteArray())) {
+          // prevent setting a hostname that fails in Chromium on Windows
+          return `cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`;


Could we substitute the actual value here? It is going to be a valid IP address, so that it should not print anything insecure.

Suggested change

return `cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`;

return `cannot use '${value}' as Kibana host name, consider using the default (localhost) instead`;

dokmic

I left some minor suggestions regarding the tests code, but the change itself looks good to me. Great job 👍

….host" is "0.0.0.0" (elastic#117022) * [Reporting] Internal correction for server hostname if "server.host" is "0.0.0.0" * add schema validation * correction to the failover logic the test is validation * update per feedback * more check against invalid hostnames * remove silly translations for server logs * remove unused translations * improve the tests

….host" is "0.0.0.0" (#117022) (#117441) * [Reporting] Internal correction for server hostname if "server.host" is "0.0.0.0" * add schema validation * correction to the failover logic the test is validation * update per feedback * more check against invalid hostnames * remove silly translations for server logs * remove unused translations * improve the tests

tsullivan added 2 commits November 1, 2021 12:38

[Reporting] Internal correction for server hostname if "server.host" …

42a1296

…is "0.0.0.0"

add schema validation

3b4cd51

tsullivan requested review from a team as code owners November 1, 2021 20:15

tsullivan added release_note:fix v8.0.0 v8.1.0 Team:AppServicesUx labels Nov 1, 2021

tsullivan added the SharedUX/fix-it-week Bugs that have been groomed and queued up for the team's next fix it week label Nov 1, 2021

jloleysens reviewed Nov 2, 2021

View reviewed changes

x-pack/plugins/reporting/server/config/schema.ts Outdated Show resolved Hide resolved

x-pack/plugins/reporting/server/config/create_config.ts Outdated Show resolved Hide resolved

x-pack/plugins/reporting/server/config/create_config.test.ts Outdated Show resolved Hide resolved

dokmic reviewed Nov 2, 2021

View reviewed changes

x-pack/plugins/reporting/server/config/schema.ts Outdated Show resolved Hide resolved

tsullivan added 7 commits November 2, 2021 16:35

Merge remote-tracking branch 'elastic/main' into reporting/usable-hos…

987abef

…tname

correction to the failover logic the test is validation

86fd8ce

update per feedback

7af5772

more check against invalid hostnames

6480620

remove silly translations for server logs

3d91c33

remove unused translations

5ca5a4d

improve the tests

82053bc

tsullivan enabled auto-merge (squash) November 3, 2021 03:00

dokmic reviewed Nov 3, 2021

View reviewed changes

dokmic approved these changes Nov 3, 2021

View reviewed changes

tsullivan merged commit e1bf92c into elastic:main Nov 3, 2021

tsullivan deleted the reporting/usable-hostname branch November 3, 2021 17:26

tsullivan mentioned this pull request Nov 3, 2021

[Reporting] clean up config tests #117411

Merged

tsullivan mentioned this pull request Nov 3, 2021

[8.0] [Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" (#117022) #117441

Merged

sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

tsullivan commented Nov 1, 2021 •

edited

Loading

elasticmachine commented Nov 1, 2021

jloleysens left a comment

kibanamachine commented Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic Nov 3, 2021

dokmic left a comment

	expect(mockLogger.warn.mock.calls.length).toBe(0);
	expect(mockLogger.warn).not.toHaveBeenCalled();

	return `cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`;
	return `cannot use '${value}' as Kibana host name, consider using the default (localhost) instead`;

[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

Conversation

tsullivan commented Nov 1, 2021 • edited Loading

Summary

Checklist

Release Note

elasticmachine commented Nov 1, 2021

jloleysens left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 3, 2021

💚 Build Succeeded

Metrics [docs]

History

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

dokmic left a comment

Choose a reason for hiding this comment

tsullivan commented Nov 1, 2021 •

edited

Loading