[ftr] rework kibana arg parsing, extend loggers correctly (#135944)

packages/kbn-test/src/functional_tests/lib/kibana_cli_args.test.ts

@@ -0,0 +1,97 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import {
+  getArgValue,
+  getKibanaCliArg,
+  getKibanaCliLoggers,
+  parseRawFlags,
+} from './kibana_cli_args';
+
+describe('parseRawFlags()', () => {
+  it('produces a sorted list of flags', () => {
+    expect(parseRawFlags(['--foo=bar', '--a=b', '--c.b.a=0', '--a.b.c=1'])).toMatchInlineSnapshot(`
+      Array [
+        "--a=b",
+        "--foo=bar",
+        "--a.b.c=1",
+        "--c.b.a=0",
+      ]
+    `);
+  });
+
+  it('validates that bare values are not used', () => {
+    expect(() => parseRawFlags(['--foo', 'bar'])).toThrowErrorMatchingInlineSnapshot(
+      `"invalid CLI arg [bar], all args must start with \\"--\\" and values must be specified after an \\"=\\" in a single string per arg"`
+    );
+  });
+
+  it('deduplciates --base-path, --no-base-path, and --server.basePath', () => {
+    expect(parseRawFlags(['--no-base-path', '--server.basePath=foo', '--base-path=bar']))
+      .toMatchInlineSnapshot(`
+      Array [
+        "--base-path=bar",
+      ]
+    `);
+  });
+
+  it('allows duplicates for --plugin-path', () => {
+    expect(parseRawFlags(['--plugin-path=foo', '--plugin-path=bar'])).toMatchInlineSnapshot(`
+      Array [
+        "--plugin-path=foo",
+        "--plugin-path=bar",
+      ]
+    `);
+  });
+});
+
+describe('getArgValue()', () => {
+  const args = parseRawFlags(['--foo=bar', '--bar=baz', '--foo=foo']);
+
+  it('extracts the value of a specific flag by name', () => {
+    expect(getArgValue(args, 'foo')).toBe('foo');
+  });
+});
+
+describe('getKibanaCliArg()', () => {
+  it('parses the raw flags and then extracts the value', () => {
+    expect(getKibanaCliArg(['--foo=bar', '--foo=foo'], 'foo')).toBe('foo');
+  });
+
+  it('parses the value as JSON if valid', () => {
+    expect(getKibanaCliArg(['--foo=["foo"]'], 'foo')).toEqual(['foo']);
+    expect(getKibanaCliArg(['--foo=null'], 'foo')).toBe(null);
+    expect(getKibanaCliArg(['--foo=1'], 'foo')).toBe(1);
+    expect(getKibanaCliArg(['--foo=10.10'], 'foo')).toBe(10.1);
+  });
+
+  it('returns an array for flags which are valid duplicates', () => {
+    expect(getKibanaCliArg(['--plugin-path=foo', '--plugin-path=bar'], 'plugin-path')).toEqual([
+      'foo',
+      'bar',
+    ]);
+  });
+});
+
+describe('getKibanaCliLoggers()', () => {
+  it('parses the --logging.loggers value to an array', () => {
+    expect(getKibanaCliLoggers(['--logging.loggers=[{"foo":1}]'])).toEqual([
+      {
+        foo: 1,
+      },
+    ]);
+  });
+
+  it('returns an array for invalid values', () => {
+    expect(getKibanaCliLoggers([])).toEqual([]);
+    expect(getKibanaCliLoggers(['--logging.loggers=null'])).toEqual([]);
+    expect(getKibanaCliLoggers(['--logging.loggers.foo=name'])).toEqual([]);
+    expect(getKibanaCliLoggers(['--logging.loggers={}'])).toEqual([]);
+    expect(getKibanaCliLoggers(['--logging.loggers=1'])).toEqual([]);
+  });
+});

packages/kbn-test/src/functional_tests/lib/kibana_cli_args.ts

@@ -0,0 +1,147 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+/**
+ * These aliases are used to ensure the values for different flags are collected in a single set.
+ */
+const ALIASES = new Map([
+  // --base-path and --no-base-path inform the same config as `server.basePath`, so deduplicate them
+  // by treating "base-path" as an alias for "server.basePath"
+  ['base-path', 'server.basePath'],
+]);
+
+/**
+ * These are the only flag names that allow duplicate definitions
+ */
+const ALLOW_DUPLICATES = new Set(['plugin-path']);
+
+export type KibanaCliArg = string & {
+  readonly __cliArgBrand: unique symbol;
+};
+
+/**
+ * Ensure that cli args are always specified as ["--flag=value", "--other-flag"] and not ["--flag", "value"]
+ */
+function assertCliArg(arg: string): asserts arg is KibanaCliArg {
+  if (!arg.startsWith('--')) {
+    throw new Error(
+      `invalid CLI arg [${arg}], all args must start with "--" and values must be specified after an "=" in a single string per arg`
+    );
+  }
+}
+
+/**
+ * Get the name of an arg, stripping the `--` and `no-` prefixes, and values
+ *
+ *  --no-base-path => base-path
+ *  --base-path => base-path
+ *  --server.basePath=foo => server.basePath
+ */
+function argName(arg: KibanaCliArg) {
+  const unflagged = arg.slice(2);
+  const i = unflagged.indexOf('=');
+  const withoutValue = i === -1 ? unflagged : unflagged.slice(0, i);
+  return withoutValue.startsWith('no-') ? withoutValue.slice(3) : withoutValue;
+}
+
+export type ArgValue = boolean | string | number | Record<string, unknown> | unknown[] | null;
+
+const argToValue = (arg: KibanaCliArg): ArgValue => {
+  if (arg.startsWith('--no-')) {
+    return false;
+  }
+
+  const i = arg.indexOf('=');
+  if (i === -1) {
+    return true;
+  }
+
+  const value = arg.slice(i + 1);
+  try {
+    return JSON.parse(value);
+  } catch (error) {
+    return value;
+  }
+};
+
+/**
+ * Get the value of an arg from the CliArg flags.
+ */
+export function getArgValue(args: KibanaCliArg[], name: string): ArgValue | ArgValue[] | undefined {
+  if (ALLOW_DUPLICATES.has(name)) {
+    return args.filter((a) => argName(a) === name).map(argToValue);
+  }
+
+  for (const arg of args) {
+    if (argName(arg) === name) {
+      return argToValue(arg);
+    }
+  }
+}
+
+export function parseRawFlags(rawFlags: string[]) {
+  // map of CliArg values by their name, this allows us to deduplicate flags and ensure
+  // that the last flag wins
+  const cliArgs = new Map<string, KibanaCliArg | KibanaCliArg[]>();
+
+  for (const arg of rawFlags) {
+    assertCliArg(arg);
+    let name = argName(arg);
+    const alias = ALIASES.get(name);
+    if (alias !== undefined) {
+      name = alias;
+    }
+
+    const existing = cliArgs.get(name);
+    const allowsDuplicate = ALLOW_DUPLICATES.has(name);
+
+    if (!existing || !allowsDuplicate) {
+      cliArgs.set(name, arg);
+      continue;
+    }
+
+    if (Array.isArray(existing)) {
+      existing.push(arg);
+    } else {
+      cliArgs.set(name, [existing, arg]);
+    }
+  }
+
+  return [...cliArgs.entries()]
+    .sort(([a], [b]) => {
+      const aDot = a.includes('.');
+      const bDot = b.includes('.');
+      return aDot === bDot ? a.localeCompare(b) : aDot ? 1 : -1;
+    })
+    .map((a) => a[1])
+    .flat();
+}
+
+/**
+ * Parse a list of Kibana CLI Arg flags and find the flag with the given name. If the flag has no
+ * value then a boolean will be returned (assumed to be a switch flag). If the flag does have a value
+ * that can be parsed by `JSON.stringify()` the parsed result is returned. Otherwise the raw string
+ * value is returned.
+ */
+export function getKibanaCliArg(rawFlags: string[], name: string) {
+  return getArgValue(parseRawFlags(rawFlags), name);
+}
+
+/**
+ * Parse the list of Kibana CLI Arg flags and extract the loggers config so that they can be extended
+ * in a subsequent FTR config
+ */
+export function getKibanaCliLoggers(rawFlags: string[]) {
+  const value = getKibanaCliArg(rawFlags, 'logging.loggers');
+
+  if (Array.isArray(value)) {
+    return value;
+  }
+
+  return [];
+}

packages/kbn-test/src/functional_tests/lib/paths.ts

@@ -16,6 +16,6 @@ function resolveRelative(path: string) {
 }
 
 export const KIBANA_EXEC = 'node';
-export const KIBANA_EXEC_PATH = resolveRelative('scripts/kibana');
+export const KIBANA_SCRIPT_PATH = resolveRelative('scripts/kibana');
 export const KIBANA_ROOT = REPO_ROOT;
 export const KIBANA_FTR_SCRIPT = resolve(KIBANA_ROOT, 'scripts/functional_test_runner');

packages/kbn-test/src/functional_tests/lib/run_kibana_server.ts

@@ -5,17 +5,21 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
+
+import Path from 'path';
+
 import type { ProcRunner } from '@kbn/dev-proc-runner';
-import { resolve, relative } from 'path';
-import { KIBANA_ROOT, KIBANA_EXEC, KIBANA_EXEC_PATH } from './paths';
+
+import { KIBANA_ROOT, KIBANA_EXEC, KIBANA_SCRIPT_PATH } from './paths';
 import type { Config } from '../../functional_test_runner';
+import { parseRawFlags } from './kibana_cli_args';
 
 function extendNodeOptions(installDir?: string) {
   if (!installDir) {
     return {};
   }
 
-  const testOnlyRegisterPath = relative(
+  const testOnlyRegisterPath = Path.relative(
     installDir,
     require.resolve('./babel_register_for_test_plugins')
   );
@@ -40,15 +44,31 @@ export async function runKibanaServer({
 }) {
   const runOptions = config.get('kbnTestServer.runOptions');
   const installDir = runOptions.alwaysUseSource ? undefined : options.installDir;
-  const env = config.get('kbnTestServer.env');
+  const extraArgs = options.extraKbnOpts ?? [];
 
+  const buildArgs: string[] = config.get('kbnTestServer.buildArgs') || [];
+  const sourceArgs: string[] = config.get('kbnTestServer.sourceArgs') || [];
+  const serverArgs: string[] = config.get('kbnTestServer.serverArgs') || [];
+
+  const args = parseRawFlags([
+    // When installDir is passed, we run from a built version of Kibana which uses different command line
+    // arguments. If installDir is not passed, we run from source code.
+    ...(installDir
+      ? [...buildArgs, ...serverArgs.filter((a: string) => a !== '--oss')]
+      : [...sourceArgs, ...serverArgs]),
+
+    // We also allow passing in extra Kibana server options, tack those on here so they always take precedence
+    ...extraArgs,
+  ]);
+
+  // main process
   await procs.run('kibana', {
     cmd: getKibanaCmd(installDir),
-    args: filterCliArgs(collectCliArgs(config, installDir, options.extraKbnOpts)),
+    args: installDir ? args : [KIBANA_SCRIPT_PATH, ...args],
     env: {
       FORCE_COLOR: 1,
       ...process.env,
-      ...env,
+      ...config.get('kbnTestServer.env'),
       ...extendNodeOptions(installDir),
     },
     cwd: installDir || KIBANA_ROOT,
@@ -60,88 +80,9 @@ export async function runKibanaServer({
 function getKibanaCmd(installDir?: string) {
   if (installDir) {
     return process.platform.startsWith('win')
-      ? resolve(installDir, 'bin/kibana.bat')
-      : resolve(installDir, 'bin/kibana');
+      ? Path.resolve(installDir, 'bin/kibana.bat')
+      : Path.resolve(installDir, 'bin/kibana');
   }
 
   return KIBANA_EXEC;
 }
-
-/**
- * When installDir is passed, we run from a built version of Kibana,
- * which uses different command line arguments. If installDir is not
- * passed, we run from source code. We also allow passing in extra
- * Kibana server options, so we tack those on here.
- */
-function collectCliArgs(config: Config, installDir?: string, extraKbnOpts: string[] = []) {
-  const buildArgs: string[] = config.get('kbnTestServer.buildArgs') || [];
-  const sourceArgs: string[] = config.get('kbnTestServer.sourceArgs') || [];
-  const serverArgs: string[] = config.get('kbnTestServer.serverArgs') || [];
-
-  return pipe(
-    serverArgs,
-    (args) => (installDir ? args.filter((a: string) => a !== '--oss') : args),
-    (args) => (installDir ? [...buildArgs, ...args] : [KIBANA_EXEC_PATH, ...sourceArgs, ...args]),
-    (args) => args.concat(extraKbnOpts)
-  );
-}
-
-/**
- * Filter the cli args to remove duplications and
- * overridden options
- */
-function filterCliArgs(args: string[]) {
-  return args.reduce((acc, val, ind) => {
-    // If original argv has a later basepath setting, skip this val.
-    if (isBasePathSettingOverridden(args, val, ind)) {
-      return acc;
-    }
-
-    // Check if original argv has a later setting that overrides
-    // the current val. If so, skip this val.
-    if (
-      !allowsDuplicate(val) &&
-      findIndexFrom(args, ++ind, (opt) => opt.split('=')[0] === val.split('=')[0]) > -1
-    ) {
-      return acc;
-    }
-
-    return [...acc, val];
-  }, [] as string[]);
-}
-
-/**
- * Apply each function in fns to the result of the
- * previous function. The first function's input
- * is the arr array.
- */
-function pipe(arr: any[], ...fns: Array<(...args: any[]) => any>) {
-  return fns.reduce((acc, fn) => {
-    return fn(acc);
-  }, arr);
-}
-
-/**
- * Checks whether a specific parameter is allowed to appear multiple
- * times in the Kibana parameters.
- */
-function allowsDuplicate(val: string) {
-  return ['--plugin-path'].includes(val.split('=')[0]);
-}
-
-function isBasePathSettingOverridden(args: string[], val: string, index: number) {
-  const key = val.split('=')[0];
-  const basePathKeys = ['--no-base-path', '--server.basePath'];
-
-  if (basePathKeys.includes(key)) {
-    if (findIndexFrom(args, ++index, (opt) => basePathKeys.includes(opt.split('=')[0])) > -1) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
-function findIndexFrom(array: string[], index: number, predicate: (element: string) => boolean) {
-  return [...array].slice(index).findIndex(predicate);
-}

packages/kbn-test/src/index.ts

@@ -27,6 +27,8 @@ export { runTests, startServers } from './functional_tests/tasks';
 // @internal
 export { KIBANA_ROOT } from './functional_tests/lib/paths';
 
+export { getKibanaCliArg, getKibanaCliLoggers } from './functional_tests/lib/kibana_cli_args';
+
 export type {
   CreateTestEsClusterOptions,
   EsTestCluster,

test/common/config.js

@@ -57,11 +57,19 @@ export default function () {
         ...(!!process.env.CODE_COVERAGE
           ? [`--plugin-path=${path.join(__dirname, 'fixtures', 'plugins', 'coverage')}`]
           : []),
-        '--logging.appenders.deprecation.type=console',
-        '--logging.appenders.deprecation.layout.type=json',
-        '--logging.loggers[0].name=elasticsearch.deprecation',
-        '--logging.loggers[0].level=all',
-        '--logging.loggers[0].appenders[0]=deprecation',
+        `--logging.appenders.deprecation=${JSON.stringify({
+          type: 'console',
+          layout: {
+            type: 'json',
+          },
+        })}`,
+        `--logging.loggers=${JSON.stringify([
+          {
+            name: 'elasticsearch.deprecation',
+            level: 'all',
+            appenders: ['deprecation'],
+          },
+        ])}`,
       ],
     },
     services,

x-pack/test/fleet_api_integration/config.ts

@@ -7,8 +7,11 @@
 
 import path from 'path';
 
-import { FtrConfigProviderContext } from '@kbn/test';
-import { defineDockerServersConfig } from '@kbn/test';
+import {
+  FtrConfigProviderContext,
+  defineDockerServersConfig,
+  getKibanaCliLoggers,
+} from '@kbn/test';
 
 // Docker image to use for Fleet API integration tests.
 // This hash comes from the latest successful build of the Snapshot Distribution of the Package Registry, for
@@ -67,10 +70,17 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
         ...(registryPort ? [`--xpack.fleet.registryUrl=http://localhost:${registryPort}`] : []),
         `--xpack.fleet.developer.bundledPackageLocation=${BUNDLED_PACKAGE_DIR}`,
         '--xpack.cloudSecurityPosture.enabled=true',
-        // Enable debug fleet logs by default
-        `--logging.loggers[0].name=plugins.fleet`,
-        `--logging.loggers[0].level=debug`,
-        `--logging.loggers[0].appenders=${JSON.stringify(['default'])}`,
+
+        `--logging.loggers=${JSON.stringify([
+          ...getKibanaCliLoggers(xPackAPITestsConfig.get('kbnTestServer.serverArgs')),
+
+          // Enable debug fleet logs by default
+          {
+            name: 'plugins.fleet',
+            level: 'debug',
+            appenders: ['default'],
+          },
+        ])}`,
       ],
     },
   };

x-pack/test/fleet_cypress/config.ts

@@ -5,7 +5,7 @@
  * 2.0.
  */
 
-import { FtrConfigProviderContext } from '@kbn/test';
+import { FtrConfigProviderContext, getKibanaCliLoggers } from '@kbn/test';
 
 import { CA_CERT_PATH } from '@kbn/dev-utils';
 
@@ -38,10 +38,17 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
         '--csp.strict=false',
         // define custom kibana server args here
         `--elasticsearch.ssl.certificateAuthorities=${CA_CERT_PATH}`,
-        // Enable debug fleet logs by default
-        `--logging.loggers[0].name=plugins.fleet`,
-        `--logging.loggers[0].level=debug`,
-        `--logging.loggers[0].appenders=${JSON.stringify(['default'])}`,
+
+        `--logging.loggers=${JSON.stringify([
+          ...getKibanaCliLoggers(xpackFunctionalTestsConfig.get('kbnTestServer.serverArgs')),
+
+          // Enable debug fleet logs by default
+          {
+            name: 'plugins.fleet',
+            level: 'debug',
+            appenders: ['default'],
+          },
+        ])}`,
       ],
     },
   };

x-pack/test/fleet_functional/config.ts

@@ -6,7 +6,7 @@
  */
 
 import { resolve } from 'path';
-import { FtrConfigProviderContext } from '@kbn/test';
+import { FtrConfigProviderContext, getKibanaCliLoggers } from '@kbn/test';
 import { pageObjects } from './page_objects';
 import { services } from './services';
 
@@ -33,10 +33,17 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
       ...xpackFunctionalConfig.get('kbnTestServer'),
       serverArgs: [
         ...xpackFunctionalConfig.get('kbnTestServer.serverArgs'),
-        // Enable debug fleet logs by default
-        `--logging.loggers[0].name=plugins.fleet`,
-        `--logging.loggers[0].level=debug`,
-        `--logging.loggers[0].appenders=${JSON.stringify(['default'])}`,
+
+        `--logging.loggers=${JSON.stringify([
+          ...getKibanaCliLoggers(xpackFunctionalConfig.get('kbnTestServer.serverArgs')),
+
+          // Enable debug fleet logs by default
+          {
+            name: 'plugins.fleet',
+            level: 'debug',
+            appenders: ['default'],
+          },
+        ])}`,
       ],
     },
     layout: {

x-pack/test/functional_execution_context/config.ts

@@ -6,7 +6,7 @@
  */
 import Path from 'path';
 import { CA_CERT_PATH } from '@kbn/dev-utils';
-import { FtrConfigProviderContext } from '@kbn/test';
+import { FtrConfigProviderContext, getKibanaCliLoggers } from '@kbn/test';
 import { logFilePath } from './test_utils';
 
 const alertTestPlugin = Path.resolve(__dirname, './fixtures/plugins/alerts');
@@ -42,23 +42,32 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
         `--elasticsearch.hosts=${servers.elasticsearch.protocol}://${servers.elasticsearch.hostname}:${servers.elasticsearch.port}`,
         `--elasticsearch.ssl.certificateAuthorities=${CA_CERT_PATH}`,
 
+        `--xpack.alerting.rules.minimumScheduleInterval.value="1s"`,
+
         '--server.requestId.allowFromAnyIp=true',
         '--logging.appenders.file.type=file',
         `--logging.appenders.file.fileName=${logFilePath}`,
         '--logging.appenders.file.layout.type=json',
 
-        '--logging.loggers[0].name=elasticsearch.query',
-        '--logging.loggers[0].level=all',
-        `--logging.loggers[0].appenders=${JSON.stringify(['file'])}`,
+        `--logging.loggers=${JSON.stringify([
+          ...getKibanaCliLoggers(functionalConfig.get('kbnTestServer.serverArgs')),
 
-        '--logging.loggers[1].name=execution_context',
-        '--logging.loggers[1].level=debug',
-        `--logging.loggers[1].appenders=${JSON.stringify(['file'])}`,
-
-        '--logging.loggers[2].name=http.server.response',
-        '--logging.loggers[2].level=all',
-        `--logging.loggers[2].appenders=${JSON.stringify(['file'])}`,
-        `--xpack.alerting.rules.minimumScheduleInterval.value="1s"`,
+          {
+            name: 'elasticsearch.query',
+            level: 'all',
+            appenders: ['file'],
+          },
+          {
+            name: 'execution_context',
+            level: 'debug',
+            appenders: ['file'],
+          },
+          {
+            name: 'http.server.response',
+            level: 'all',
+            appenders: ['file'],
+          },
+        ])}`,
       ],
     },
   };