Allow plugins to extend logging config

[joshdover]

Allow plugins to dynamically modify the logging config for the contexts that they own.

Blocker for Audit Logging (#52125)

[elasticmachine]

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

[pgayvallet]

Allow plugins to dynamically modify the logging config for the contexts that they own

That mean that a plugin should be allowed to update anything included in the plugins.{pluginId} context, right?

[joshdover]

Thinking about the API design here a bit, my current plan to is to change the PluginInitializerContext interface to add a new configure method:

export interface PluginInitializerContext<ConfigSchema = unknown> {
   opaqueId: PluginOpaqueId;
   env: {
     mode: EnvironmentMode;
     packageInfo: Readonly<PackageInfo>;
   };
+  logging: {
+    logger: LoggerFactory;
+    configure(config$: Observable<LoggerConfig>): void;
+  };
-  logger: LoggerFactory;
   config: {
     legacy: { globalConfig$: Observable<SharedGlobalConfig> };
     create: <T = ConfigSchema>() => Observable<T>;
     createIfExists: <T = ConfigSchema>() => Observable<T | undefined>;
   };
 }

Of course this will be a breaking change, though it should be trivial. If desired, we could keep the existing logger key and mark it deprecated.

The LoggerConfig type will be similar to the existing LoggingConfigType, but without the root property:

interface LoggerConfig {
  appenders: Map<string, AppendersType>;
  loggers: Array<{
    appenders: string[];
    context: string;
    level: 'all' | 'fatal' | 'error' | 'warn' | 'info' | 'debug' | 'trace' | 'off';
  }>;
}

One limitation is that the context keys defined in loggers can only contain contexts that are children of plugins.<pluginId>. I'm not sure how to type this, so I see two options:

1. Throw a runtime exceptions if a plugin calls this API with a different context; or
2. Automatically prepend the plugin's context to whatever context the plugin specifies. This would allow plugins to use a "relative" context. Not sure if the complexity of that is worth it though.

I lean towards (1) on this due to the simplicity, though it does introduce a "caveat" or "gotcha" in the consumption of this API.

---

Another question is how to deal with the user-defined configuration and the plugin-defined configuration, potential options:

1. Plugin-defined configuration always overrides user-defined configuration
   • This gives the plugin complete control and is the most simple option. If a plugin wishes to expose user-configurable options, they may do so via their own regular config mechanism
   • We may need to emit a warning if the user defines logging config for a context that is overridden by a plugin
2. Plugin-defined configuration always overrides user-defined, but we give the plugin access to the user-defined config
   • Basically this changes the API to take a factory function configure((userConfig$: Observable<LoggerConfig>) => Observable<LoggerConfig>): void;
   • This option may have some user-facing benefits by keeping all logging-related configuration in the same part of the kibana.yml structure. However, it is up to the plugin to merge these configs together, which may lead to unexpected or inconsistent behaviors between plugins.
3. Attempt to come up with general configuration merging algorithm
   • This is both somewhat tricky to implement as well as hard to document & explain to users

I lean towards option (1) for the simplicity and explicitness.

[mshustov]

Could there be any cases when the logging system emits a logging record before the plugin initialized and adjusts the config? In this case, we have to configure the logging context in a declarative manner via static exports.

logging: {
  logger: LoggerFactory;
  configure(config$: Observable<LoggerConfig>): void;
};

What are the benefits of using PluginInitializerContext for this purpose?
Why not setup?
It introduces another place to expose API besides setup & start contracts.

Another question is how to deal with the user-defined configuration and the plugin-defined configuration, potential options:

I'd expect that the user's configuration always takes precedence. However, a plugin can provide its own configuration to adjust the logging format.

but we give the plugin access to the user-defined config

Wouldn't they exist in the same Audit Logging context? It means that the user-defined config will be a sub-set of Audit Logging config.

setup(core){
  const loggingConfig$ = this.config$.pipe(map(cfg => {
    return { layout: { pattern: cfg.pattern || '{date} {level}'...
  }));
  core.logging.configure(loggingConfig$);

[pgayvallet]

1. throw a runtime exceptions if a plugin calls this API with a different context
   or
2. Automatically prepend the plugin's context to whatever context the plugin specifies. This would allow plugins to use a "relative" context. Not sure if the complexity of that is worth it though.

As a plugin's context is already implicitly prepended by plugins.XXX (aka logger.get('foo') returns the plugins.XXX.foo context, I'd say that options 2 would be consistent. But It's not a strong opinion

I'd expect that the user's configuration always takes precedence

+1 on that. Main argument would be that if the user explicitly configured a silent or very verbose logging, plugins shouldn't be able to override it.

However, it is up to the plugin to merge these configs together, which may lead to unexpected or inconsistent behaviors between plugins

We could expose helper APIs to help them, as webpack is doing with webpack-merge. I agree that this would still be quite complex both for us to implement and for plugins to use.

[joshdover]

Could there be any cases when the logging system emits a logging record before the plugin initialized and adjusts the config? In this case, we have to configure the logging context in a declarative manner via static exports.
What are the benefits of using PluginInitializerContext for this purpose?
Why not setup?
It introduces another place to expose API besides setup & start contracts.

Core does have access to a plugin's log context, so it is possible, but we have control over that. Right now, the only messages I am aware of in Core are the lifecycle debug logs. However, these are currently only emitted after the plugin constructor has been created.

This was one of my motivations for including this API in the PluginInitializerContext. The other was that config is also exposed in this API, and I suspect will drive the customizations that a plugin will be doing to it's logging config.

Wouldn't they exist in the same Audit Logging context? It means that the user-defined config will be a sub-set of Audit Logging config.

setup(core){
  const loggingConfig$ = this.config$.pipe(map(cfg => {
    return { layout: { pattern: cfg.pattern || '{date} {level}'...
  }));
  core.logging.configure(loggingConfig$);

I think maybe we're talking about different parts of the config here. For extra clarity, I see two options of where the user can configure this logging context: in the main logging config, or in the config of the audit logging plugin.

1. Have the user directly modify the audit logging context & require the plugin to merge this with it's own configuration:

logging:
  appenders:
    custom_audit_log:
      kind: file
      path: /path/to/audit.log
      layout:
        kind: json
  loggers:
    - context: plugins.audit_logger
      appenders: [custom_audit_log]
      level: 'all'

2. Or the plugin itself could expose an appenders config and any appender configured here would be added to the list of appenders that the plugin configures for its logging context. Any customizations configured via logging.loggers for this context would be ignored/overridden:

xpack.audit_logger:
  appenders:
    custom_audit_log:
      kind: file
      path: /path/to/audit.log
      layout:
        kind: json

If we went with option (1) then the user-defined config will be a sub-set of Audit Logging config would be true and the "merging" could just be adding additional loggers items on top of what the user has configured. The audit logging plugin would not necessarily need access to the user-defined configuration. The user's appender would only need to override the default audit appender if they referenced the same filename.

This is probably fine, but the audit logger may need to also expose a disable_default_appender config option as well to opt-out of the default log file. So the downside of this approach is that disabling the default appender is in a different part of the kibana.yml file from the rest of the logging config.

[mshustov]

Core does have access to a plugin's log context, so it is possible, but we have control over that. Right now, the only messages I am aware of in Core are the lifecycle debug logs. However, these are currently only emitted after the plugin constructor has been created.

I'd suggest removing those logs. It looks redundant since plugins-system already logs the lifecycle events:

[plugins-system] Setting up plugin "advancedSettings"...
[advancedSettings][plugins] Initializing plugin
[advancedSettings][plugins] Setting up plugin

Having removed them, we can switch to the regular API exposed via setup contract.

Have the user directly modify the audit logging context & require the plugin to merge this with it's own configuration:

This option is closer to the regular logging configuration, so 👍 . However, I'm concerned that the user config doesn't override the plugin config completely. For example, a user provides a customized config, and then in logs they will find 2 different files containing the same records in different formats. It sounds like a permanent source of confusion.
@jportner Could you clarify what approach is preferable? Should yml file be the only source of audit logging configuration? Do we want to allow users to modify audit logging config from UI or REST API in the next iteration?

[jportner]

@jportner Could you clarify what approach is preferable? Should yml file be the only source of audit logging configuration? Do we want to allow users to modify audit logging config from UI or REST API in the next iteration?

It's not a requirement and not in scope for the MVP of the audit logging overhaul. However, it would be nice to eventually be able to modify the audit logging config from the UI and REST API. Elasticsearch's audit logging can be configured via REST API today, and we'll want Kibana to eventually reach parity with it, I assume. Perhaps @arisonl has something else to add.

With regard to @joshdover's proposals on how to configure audit logging -- option (1) or option (2) -- I lean towards option (1) as well. We will need to expose additional configuration settings that aren't generic logging settings, so it makes sense to me to have all of the audit logging settings exposed by the plugin so they're in the same space (with the same prefix).

[joshdover]

I'd suggest removing those logs. It looks redundant since plugins-system already logs the lifecycle events:

Agreed 👍

I lean towards option (1) as well. We will need to expose additional configuration settings that aren't generic logging settings, so it makes sense to me to have all of the audit logging settings exposed by the plugin so they're in the same space (with the same prefix).

@jportner If I understand correctly, this is what option 2 does, not option 1.

FWIW, I think option 2 is easier to implement correctly and possibly easier for the user to understand. The primary downside is that audit logging will be configured in a different part of the yml file from all other logging. But all of the audit logging options will be in the same place rather than split between the global logging config and the audit logging config.

Option 2 seems like the better tradeoff to me: easier to implement correctly + keeps all the config for audit logging in the same place (however it is separate from the rest of logging config).

[arisonl]

Should yml file be the only source of audit logging configuration? Do we want to allow users to modify audit logging config from UI or REST API in the next iteration?

Quick note in support of Joe's vision, we are ultimately aiming to create a unified audit logging capability between ES and Kibana, one which users are able to find the information they are looking for, easily in one place. UX is very important. So we would definitely want the ability to config through API and UI in the future, beyond the MVP.

[mshustov]

FWIW, I think option 2 is easier to implement correctly and possibly easier for the user to understand. The primary downside is that audit logging will be configured in a different part of the yml file from all other logging

Another disadvantage of this approach is a divergence from elasticsearch audit logging configuration, which it is closer to the 1st option https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/config/log4j2.properties

[jportner]

@jportner If I understand correctly, this is what option 2 does, not option 1.

@joshdover sorry, I was trying to digest all of the earlier comments and I think I got my numbers mixed up with what was said before. But yes, re-reading this, I intended to say option 2.

Another disadvantage of this approach is a divergence from elasticsearch audit logging configuration, which it is closer to the 1st option elastic/elasticsearch:x-pack/plugin/core/src/main/config/log4j2.properties@master

@restrry good catch... there's no mention of those settings on the Elasticsearch Auditing Settings docs page, either.

I appreciate sticking with convention most of the time, but I feel this is one situation where it makes sense to diverge. Having audit logging settings in two different places is a pretty confusing/bad UX. So, I'll reiterate my vote of support for option 2!

[joshdover]

I appreciate sticking with convention most of the time, but I feel this is one situation where it makes sense to diverge. Having audit logging settings in two different places is a pretty confusing/bad UX.

Agreed, and I also think implementing the API-based configuration will be easier if the config is completely driven by the plugin rather than merged with the user-defined config. With this option, we should at least emit a warning log if the user does define config in the logging.loggers key.

@arisonl is consistency between ES and Kibana on the config critical here? If so, who on the ES side could we talk to about getting alignment on this?

If not, and there are no "fundamental flaw" objections, I will proceed with implementing option 2.