Allows SIGHUP to recreate log file handler (#26675)

Co-authored-by: spalger <email@spalger.com>
Co-authored-by: Tyler Smalley <tyler.smalley@elastic.co>

src/cli/serve/integration_tests/reload_logging_config.test.js

@@ -18,8 +18,11 @@
  */
 
 import { spawn } from 'child_process';
-import { writeFileSync } from 'fs';
-import { relative, resolve } from 'path';
+import fs from 'fs';
+import path from 'path';
+import os from 'os';
+import mkdirp from 'mkdirp';
+import rimraf from 'rimraf';
 
 import { safeDump } from 'js-yaml';
 import { createMapStream, createSplitStream, createPromiseFromStreams } from '../../../utils/streams';
@@ -28,8 +31,14 @@ import { getConfigFromFiles } from '../../../core/server/config/read_config';
 const testConfigFile = follow('__fixtures__/reload_logging_config/kibana.test.yml');
 const kibanaPath = follow('../../../../scripts/kibana.js');
 
+const second = 1000;
+const minute = second * 60;
+
+const tempDir = path.join(os.tmpdir(), 'kbn-reload-test');
+
+
 function follow(file) {
-  return relative(process.cwd(), resolve(__dirname, file));
+  return path.relative(process.cwd(), path.resolve(__dirname, file));
 }
 
 function setLoggingJson(enabled) {
@@ -39,7 +48,7 @@ function setLoggingJson(enabled) {
 
   const yaml = safeDump(conf);
 
-  writeFileSync(testConfigFile, yaml);
+  fs.writeFileSync(testConfigFile, yaml);
 }
 
 describe('Server logging configuration', function () {
@@ -49,6 +58,8 @@ describe('Server logging configuration', function () {
   beforeEach(() => {
     isJson = true;
     setLoggingJson(true);
+
+    mkdirp.sync(tempDir);
   });
 
   afterEach(() => {
@@ -59,6 +70,8 @@ describe('Server logging configuration', function () {
       child.kill();
       child = undefined;
     }
+
+    rimraf.sync(tempDir);
   });
 
   const isWindows = /^win/.test(process.platform);
@@ -128,6 +141,59 @@ describe('Server logging configuration', function () {
       expect(exitCode).toEqual(0);
       expect(sawJson).toEqual(true);
       expect(sawNonjson).toEqual(true);
-    }, 60000);
+    }, minute);
+
+    it.skip('should recreate file handler on SIGHUP', function (done) {
+      expect.hasAssertions();
+
+      const logPath = path.resolve(tempDir, 'kibana.log');
+      const logPathArchived = path.resolve(tempDir, 'kibana_archive.log');
+
+      function watchFileUntil(path, matcher, timeout) {
+        return new Promise((resolve, reject) => {
+          const timeoutHandle = setTimeout(() => {
+            fs.unwatchFile(path);
+            reject(`watchFileUntil timed out for "${matcher}"`);
+          }, timeout);
+
+          fs.watchFile(path, () => {
+            try {
+              const contents = fs.readFileSync(path);
+
+              if (matcher.test(contents)) {
+                clearTimeout(timeoutHandle);
+                fs.unwatchFile(path);
+                resolve(contents);
+              }
+            } catch (e) {
+              // noop
+            }
+          });
+        });
+      }
+
+      child = spawn(process.execPath, [
+        kibanaPath,
+        '--logging.dest', logPath,
+        '--plugins.initialize', 'false',
+        '--logging.json', 'false'
+      ]);
+
+      watchFileUntil(logPath, /Server running at/, 2 * minute)
+        .then(() => {
+          // once the server is running, archive the log file and issue SIGHUP
+          fs.renameSync(logPath, logPathArchived);
+          child.kill('SIGHUP');
+        })
+        .then(() => watchFileUntil(logPath, /Reloaded logging configuration due to SIGHUP/, 10 * second))
+        .then(contents => {
+          const lines = contents.toString().split('\n');
+          // should be the first and only new line of the log file
+          expect(lines).toHaveLength(2);
+          child.kill();
+        })
+        .then(done, done);
+
+    }, 3 * minute);
   }
 });

src/core/server/config/raw_config_service.test.ts

@@ -96,7 +96,7 @@ test('returns config at path as observable', async () => {
   expect(exampleConfig.getFlattenedPaths()).toEqual(['key']);
 });
 
-test("does not push new configs when reloading if config at path hasn't changed", async () => {
+test("pushes new configs when reloading even if config at path hasn't changed", async () => {
   mockGetConfigFromFiles.mockImplementation(() => ({ key: 'value' }));
 
   const configService = new RawConfigService([configFile]);
@@ -113,9 +113,20 @@ test("does not push new configs when reloading if config at path hasn't changed"
 
   configService.reloadConfig();
 
-  expect(valuesReceived).toHaveLength(1);
-  expect(valuesReceived[0].get('key')).toEqual('value');
-  expect(valuesReceived[0].getFlattenedPaths()).toEqual(['key']);
+  expect(valuesReceived).toMatchInlineSnapshot(`
+Array [
+  ObjectToConfigAdapter {
+    "rawConfig": Object {
+      "key": "value",
+    },
+  },
+  ObjectToConfigAdapter {
+    "rawConfig": Object {
+      "key": "value",
+    },
+  },
+]
+`);
 });
 
 test('pushes new config when reloading and config at path has changed', async () => {

src/core/server/config/raw_config_service.ts

@@ -17,9 +17,9 @@
  * under the License.
  */
 
-import { cloneDeep, isEqual, isPlainObject } from 'lodash';
+import { cloneDeep, isPlainObject } from 'lodash';
 import { Observable, ReplaySubject } from 'rxjs';
-import { distinctUntilChanged, map } from 'rxjs/operators';
+import { map } from 'rxjs/operators';
 import typeDetect from 'type-detect';
 
 import { Config } from './config';
@@ -43,8 +43,6 @@ export class RawConfigService {
       new ObjectToConfigAdapter(rawConfig)
   ) {
     this.config$ = this.rawConfigFromFile$.pipe(
-      // We only want to update the config if there are changes to it.
-      distinctUntilChanged(isEqual),
       map(rawConfig => {
         if (isPlainObject(rawConfig)) {
           // TODO Make config consistent, e.g. handle dots in keys

src/core/server/root/index.test.ts

@@ -22,7 +22,7 @@ jest.mock('../logging', () => ({
   LoggingService: jest.fn(() => mockLoggingService),
 }));
 
-const mockConfigService = { atPath: jest.fn() };
+const mockConfigService = { atPath: jest.fn(), getConfig$: jest.fn() };
 jest.mock('../config/config_service', () => ({
   ConfigService: jest.fn(() => mockConfigService),
 }));
@@ -50,6 +50,7 @@ const mockConsoleError = jest.spyOn(console, 'error').mockImplementation(() => {
 
 beforeEach(() => {
   mockLoggingService.asLoggerFactory.mockReturnValue(logger);
+  mockConfigService.getConfig$.mockReturnValue(new BehaviorSubject({}));
   mockConfigService.atPath.mockReturnValue(new BehaviorSubject({ someValue: 'foo' }));
 });
 
@@ -61,6 +62,7 @@ afterEach(() => {
   mockLoggingService.stop.mockReset();
   mockLoggingService.asLoggerFactory.mockReset();
   mockConfigService.atPath.mockReset();
+  mockConfigService.getConfig$.mockReset();
   mockServer.start.mockReset();
   mockServer.stop.mockReset();
 });

src/core/server/root/index.ts

@@ -18,7 +18,7 @@
  */
 
 import { ConnectableObservable, Observable, Subscription } from 'rxjs';
-import { first, map, publishReplay, tap } from 'rxjs/operators';
+import { first, map, publishReplay, switchMap, tap } from 'rxjs/operators';
 
 import { Server } from '..';
 import { Config, ConfigService, Env } from '../config';
@@ -88,7 +88,9 @@ export class Root {
 
   private async setupLogging() {
     // Stream that maps config updates to logger updates, including update failures.
-    const update$ = this.configService.atPath('logging', LoggingConfig).pipe(
+    const update$ = this.configService.getConfig$().pipe(
+      // always read the logging config when the underlying config object is re-read
+      switchMap(() => this.configService.atPath('logging', LoggingConfig)),
       map(config => this.loggingService.upgrade(config)),
       // This specifically console.logs because we were not able to configure the logger.
       // tslint:disable-next-line no-console