Access logs API proposal

strekm commented 7 months ago

Description

Based on POC that was already done propose enhancement to Istio API to allow user to configure access logs format. User should be able to provide key value map for format. In addition user should set strategy how format will be applied to default configuration.

ACs:

[x] proposal accepted by Kyma architecture
[x] ADR finalised

Reasons

User should be able to configure access log format that suits his requirements

DoD: ~~- [ ] Provide unit and integration tests.~~

[x] Provide documentation. ~~- [ ] Verify if the solution works for both open-source Kyma and SAP BTP, Kyma runtime.~~ ~~- [ ] If you changed the resource limits, explain why it was needed.~~ ~~- [ ] Verify that your contributions don't decrease code coverage. If they do, explain why this is the case.~~ ~~- [ ] Add release notes.~~

Attachments https://github.com/kyma-project/istio/issues/489#issuecomment-1941262909 part of: https://github.com/kyma-project/istio/issues/489 ADR: https://github.com/kyma-project/istio/issues/641

Ressetkk commented 7 months ago

/assign

Ressetkk commented 7 months ago

Created ADR based on proposal: https://github.com/kyma-project/istio/issues/641 /unassign

triffer commented 7 months ago

I think we can provide more information about why this is relevant in the Context. We already have a lot of information in the description of this issue that we can use.

The decision only mentions handling of EnvoyFileAccessLogProvider but not that the log format of OpenTelemetryTracingProvider is also updated. I also see that you simplified the ADR and removed the strategy field mentioned in the proposal. This is important and should be part of the decision. Usually in the Context of the ADR you would describe pros and cons of different approaches/options and in the decision you would add what was agreed on and why. I would also move User Scenarios and Examples to the Context as this gives more information about what we want to agree on. We should keep our top-level ADR structure of Status, Context, Decision and Consequences.

If this configuration is provided, it overwrites the default values provided by the module. The configuration of handling labels in log format is inspired on how Envoy handles logging for its default ExtensionProvider named envoy - if labels field is empty, apply default configuration, otherwise apply values from defined field.

As far as I understand this would only support the scenario of replacing the existing logFormat with your own format. How do we support only the update of single labels or adding a new custom label?

Additionally, we should list a consequence that we allow the user to completely change the default access log. This means if we want to investigate an issue by enabling access logs for stdout, we might not all the labels that we need. This means we need to apply an EnvoyFilter to enable additional access logs with the log format that we might need. The question is also if we are allowed to do it, because depending on how the user uses the logging, this additional logs might be visible to the user and mess up with their logging.

Ressetkk commented 7 months ago

/cc @videlov Please take a look since you were the person that took participation in the decision.

videlov commented 7 months ago

User Scenario:

Module updates IstioOperator CR and sets kyma-default-logger fields to updated configuration.

We do not need this, its not customer relevant how it gets applied. What actually matters to Istio is istio ConfigMap (where extension providers are defined in the mesh config) and not IstioOperator CR.

Module reads the configuration and alters default kyma-default-logger based on user-provided configuration

We do alter configuration for all predefined extension providers kyma-default-logger AND kyma-default-otel-logger. The user activates an extension provider for specific workloads or the entire service mesh using the telemetry API. You may keep this sentence simple, e.g.: module reads and alters default logging configuration for all ext providers.

Decision:

We extend Istio CR config field to include configuration for default EnvoyFileAccessLogProvider.

We alter default logging configuration for all ext providers as mentioned above. Would be better to refer to the extension providers by name (its an unique identifier in Istio).

If any of the fields in accessLog struct is empty, module applies default settings defined in the code.

What does this mean? Can you be exact what is allowed not allowed. Since strategy is optional this specifies what happens only if I have empty labels case? What if I have, e.g.:
```
config:
accessLog:
  labels:
    tenant: ""
```
this should be valid right?

The accessLog field applies log format to both EnvoyFileAccessLogProvider and EnvoyOpenTelemetryLogProvider.

Perhaps it is better to refer to the ext providers by name and not type, since we create 2 at the moment but may have more of the same type in the future.

Examples:

The access logs would then contain single field as follows: {"tenant": "%REQ(X-TENANT)%"}

The access logs from Envoy will actually not have the template/format but the actual value, e.g.: {"tenant": "sap-tenant-xxx"} https://www.envoyproxy.io/docs/envoy/latest/configuration/observability/access_log/usage#format-dictionaries

IstioConfig struct is extended with AccessLogConfig.

We do not need to explicitly define golang API structs but if we do please specify how this extents Istio CR's Spec.Config struct and more specifically define whats required / optional.

Open:

Please keep existing ADR template structure: Status, Context, Decision and Consequences. Adding stuff as sub-abstracts (###).
We currently have kyma-traces extension provider. Can we clarify if we replace it with kyma-default-otel-logger

Ressetkk commented 7 months ago

@videlov

We do not need this, its not customer relevant how it gets applied. What actually matters to Istio is istio ConfigMap (where extension providers are defined in the mesh config) and not IstioOperator CR.

I didn't know that. Can you provide how Istio manages the reconciliation of the extension providers with example how it's working? I've only found references that they are configured in IstioOperator CR.

We do alter configuration for all predefined extension providers kyma-default-logger AND kyma-default-otel-logger. The user activates an extension provider for specific workloads or the entire service mesh using the telemetry API. You may keep this sentence simple, e.g.: module reads and alters default logging configuration for all ext providers.

Yup, correct. That is supposed to be for both of the loggers.

We alter default logging configuration for all ext providers as mentioned above. Would be better to refer to the extension providers by name (it's a unique identifier in Istio).

That is also correct. I only suspect that the providers will be referenced by name in the code. For te ADR scope I decided to reference them by type.

What does this mean? Can you be exact what is allowed not allowed. Since strategy is optional this specifies what happens only if I have empty labels case? What if I have, e.g.: config: accessLog: labels: tenant: "" this should be valid right?

Yes, then the tenant value will be empty. Default values are only applied when labels is empty or not defined. We can think about the sanitization, but I think that will be misleading for the user.

Perhaps it is better to refer to the ext providers by name and not type, since we create 2 at the moment but may have more of the same type in the future.

That is also correct. I only suspect that the providers will be referenced by name in the code. For te ADR scope I decided to reference them by type.

The access logs from Envoy will actually not have the template/format but the actual value, e.g.: {"tenant": "sap-tenant-xxx"} https://www.envoyproxy.io/docs/envoy/latest/configuration/observability/access_log/usage#format-dictionaries

The values will be injected by istio based on envoy markers. If you want hands-on example then sure I will update the doc.

We do not need to explicitly define golang API structs but if we do please specify how this extents Istio CR's Spec.Config struct and more specifically define whats required / optional.

I proposed an example implementation. It's up to the developer implementing the feature.

We currently have kyma-traces extension provider. Can we clarify if we replace it with kyma-default-otel-logger

Completely different structure. Please refer to the official Istio MeshConfig documentation.

Ressetkk commented 6 months ago

ADR is updated and accepted /close

kyma-bot commented 6 months ago

@Ressetkk: Closing this issue.

In response to [this](https://github.com/kyma-project/istio/issues/624#issuecomment-1964169366): >ADR is updated and accepted >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

kyma-project / istio

Access logs API proposal #624