kyma-project / istio

Apache License 2.0
3 stars 19 forks source link

Configurable access logs #489

Open a-thaler opened 10 months ago

a-thaler commented 10 months ago

Description The istio module ships a hardcoded set of istio telemetry extensionProviders. One of these providers configures access logging based on stdout using a fixed JSON format. Using the extensionProvider mechanism users can selectively enable access logging on workloads or a gateway.

Users have custom request and response headers used across solutions and with that like to have them included in the access logs as well. As far as I can see is the existing approach of logging to stdout accepted, still istio already supports logging via OTLP. In combination with the strategy of the telemetry module (https://github.com/kyma-project/telemetry-manager/issues/556), any customization should be also possible following the OTLP approach in future.

So one aspect is the actual format configuration, the other the actual backend configuration (here stdout) and the having all options of istio available. Thinking of traces and metrics, it becomes relevant on what the focus of the configurability will be, should we really support using a different tracer implementation? Of course it will be cool to have all options, but the goal is to provide a tested and manageable setup so that the user don't need to start bothering with the topic so much.

Goal Turn the access logging into a configurable feature, supporting at least additional request and response headers. In best case you can define your own format. The feature should work with a solution based on OTLP as well. Aspects like traces and metrics could be supported in a similar way.

Criterias As a devops I want to configure which HTTP req and res headers should appear in our monitoring tool (mainly SAP Cloud Logging, but also Dynatrace comes into mind) in order to use them for troubleshooting

Questions needed to get answered:

Actions

Reasons Access logs are often a key element for monitoring. If relevant context is encoded in headers, users require to see that context in the logs to improve the observability.

Attachments

a-thaler commented 7 months ago

After a first discussion round, it seems most valuable to focus on supporting well-defined and preconfigured extensionProviders which can be configured via the modules API. So taking the existing provider based on json and stdout and make the format extensible, having in mind that additional providers might be added later for further scenarios which again can be configured. Hereby, we see it more important to support well-defined integration scenarios which are tested very well instead of providing full flexibility leveraging providers like envoyHttpAls which are barely tested by the module. A more concrete API proposal will follow as a next step.

videlov commented 7 months ago

We brainstormed different options and came up with this simple proposal. We would like to extend Istio CR's spec with additional configuration for Istio's MeshConfig.

We will have two default Istio extension providers definitions for EnvoyFileAccessLogProvider and EnvoyOpenTelemetryLogProvider to emit access logs either in JSON to stdout or OTLP.

The defined access log format is applied to all extension providers. The user activates an extension provider for specific workloads or the entire service mesh using the telemetry API.

Example

apiVersion: operator.kyma-project.io/v1alpha1
kind: Istio
metadata:
  name: default
  namespace: kyma-system
spec:
  config:
    accessLog:
      strategy: "merge" | "replace"
      format:
        tenant: "%REQ(X-TENANT)%"
a-thaler commented 7 months ago

Additional background to the discussion: Activating multiple providers for access logs at the same time is unlikely as the outputs are usually conflicting (on stdout for sure, on OTLP potentially). So the user mainly wants to influence the format and then select the pre-defined output format. We do not see the need at the moment that other output types need to be configurable. Leveraging an OTLP integration via the telemetry module in future will even provide manipulation options at a later stage.

a-thaler commented 6 months ago

In case you cannot wait for the feature, there is a workaround to have an additional access log configuration in the EnvoyFilter. This will be a configuration independent to the one which you can enable by the telemetry resource, so the easiest way to avoid duplicate log entries is to not enable access logging using the istio telemetry resources and to add all relevant fields in the EnvoyFilter configuration taken from here.

The following example will add a new log attribute with the fields tenant_id, method, path, traceparent, tracestate for HTTP requests that are inbound to the sidecar with the label app: httpbin.

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: tenant-access-log
  namespace: istio-system
spec:
  workloadSelector:
    labels:
      app: httpbin
  configPatches:
    - applyTo: NETWORK_FILTER
      match:
        context: SIDECAR_INBOUND
        listener:
          filterChain:
            filter:
              name: "envoy.filters.network.http_connection_manager"
      patch:
        operation: MERGE
        value:
          typed_config:
            "@type": "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager"
            access_log:
              - name: envoy.access_loggers.file
                typed_config:
                  "@type": "type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog"
                  path: /dev/stdout
                  logFormat:
                    jsonFormat:
                      tenant_id: "%REQ(X-Tenant-ID)%"
                      method: "%REQ(:METHOD)%"
                      path: "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%"
                      traceparent: "%REQ(TRACEPARENT)%"
                      tracestate: "%REQ(TRACESTATE)%"

Sending a http request to the workload with the header X-TENANT-ID:3213 set will result in the log for the default log configuration and an additional log containing the tenant ID.

{"method":"GET","traceparent":null,"tenant_id":"3213","tracestate":null,"path":"/anything"}
vweckerle commented 3 weeks ago

I am not sure if it is technically possible with istio. But if there would be a way to make the access log format specific per namespace or even per deployment, that would be a nice extra. I.e. application A might use custom headers X, Y and Z and application B custom headers A, B and C. Having only one central log format would mean that it has to contain A, B, C, X, Y and Z. This does not scale very well. But even with just a single log format, it would already be a very helpful extension over the status quo.

a-thaler commented 3 weeks ago

The current Isio API allows to specify many "extensionProviders" which can be of type "accessLog". Via the Istio Telemetry API is is possible to then activate an extensionProvider per workload or namespace. So indeed you could specify all your log formats centrally (repeating the base format over and over again) and them apply them via the API to the different workloads. There is no way yet to modify a central format dynamically by only adding a single expression.

The planned concept was considering to adjust the singleton accessLog format only. A question is if we should make that accessLog config more flexible and support more logFormats with new extensionProvider names (using the same base settings as the existing provider), or if we stay with the concept of adjusting the default format only and additionally support generic definition of extensionProviders.