diff --git a/cmd/extensions/telemetry-policy/api/v1alpha1/telemetrypolicy_types.go b/cmd/extensions/telemetry-policy/api/v1alpha1/telemetrypolicy_types.go index e8c2bef09..3a323546d 100644 --- a/cmd/extensions/telemetry-policy/api/v1alpha1/telemetrypolicy_types.go +++ b/cmd/extensions/telemetry-policy/api/v1alpha1/telemetrypolicy_types.go @@ -29,8 +29,8 @@ import ( //+kubebuilder:object:root=true //+kubebuilder:subresource:status -// TelemetryPolicy enables custom metric labelling for Kuadrant data plane resources -// through the use of dynamically evaluated CEL expressions. +// TelemetryPolicy enables custom metric labelling and log field enrichment for +// Kuadrant data plane resources through the use of dynamically evaluated CEL expressions. type TelemetryPolicy struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` @@ -46,7 +46,12 @@ type TelemetryPolicySpec struct { TargetRef gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName `json:"targetRef"` // Metrics holds the telemetry metrics configuration - Metrics MetricsSpec `json:"metrics"` + // +optional + Metrics *MetricsSpec `json:"metrics,omitempty"` + + // Logging holds the telemetry logging configuration + // +optional + Logging *LoggingSpec `json:"logging,omitempty"` } func (p *TelemetryPolicy) GetName() string { @@ -77,6 +82,22 @@ type MetricsConfig struct { Labels map[string]string `json:"labels"` } +// LoggingSpec defines the configuration for telemetry logging +type LoggingSpec struct { + // Default logging configuration that applies to all requests + Default LoggingConfig `json:"default"` +} + +// LoggingConfig defines reusable logging configuration +type LoggingConfig struct { + // Fields to add to auth decision log records, where keys are field names and values are + // CEL expressions referencing well-known attributes (e.g. auth.identity.sub). + // Only fields whose CEL expressions resolve successfully will be included. + // +kubebuilder:validation:MinProperties=1 + // +kubebuilder:validation:XValidation:rule="self.all(k, !k.contains('.'))",message="field names must not contain periods" + Fields map[string]string `json:"fields"` +} + // TelemetryPolicyStatus defines the observed state of TelemetryPolicy type TelemetryPolicyStatus struct { // ObservedGeneration reflects the generation of the most recently observed spec. diff --git a/cmd/extensions/telemetry-policy/api/v1alpha1/zz_generated.deepcopy.go b/cmd/extensions/telemetry-policy/api/v1alpha1/zz_generated.deepcopy.go index ef4f25941..303b06307 100644 --- a/cmd/extensions/telemetry-policy/api/v1alpha1/zz_generated.deepcopy.go +++ b/cmd/extensions/telemetry-policy/api/v1alpha1/zz_generated.deepcopy.go @@ -25,6 +25,44 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LoggingConfig) DeepCopyInto(out *LoggingConfig) { + *out = *in + if in.Fields != nil { + in, out := &in.Fields, &out.Fields + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LoggingConfig. +func (in *LoggingConfig) DeepCopy() *LoggingConfig { + if in == nil { + return nil + } + out := new(LoggingConfig) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LoggingSpec) DeepCopyInto(out *LoggingSpec) { + *out = *in + in.Default.DeepCopyInto(&out.Default) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LoggingSpec. +func (in *LoggingSpec) DeepCopy() *LoggingSpec { + if in == nil { + return nil + } + out := new(LoggingSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MetricsConfig) DeepCopyInto(out *MetricsConfig) { *out = *in @@ -126,7 +164,16 @@ func (in *TelemetryPolicyList) DeepCopyObject() runtime.Object { func (in *TelemetryPolicySpec) DeepCopyInto(out *TelemetryPolicySpec) { *out = *in in.TargetRef.DeepCopyInto(&out.TargetRef) - in.Metrics.DeepCopyInto(&out.Metrics) + if in.Metrics != nil { + in, out := &in.Metrics, &out.Metrics + *out = new(MetricsSpec) + (*in).DeepCopyInto(*out) + } + if in.Logging != nil { + in, out := &in.Logging, &out.Logging + *out = new(LoggingSpec) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TelemetryPolicySpec. diff --git a/cmd/extensions/telemetry-policy/config/crd/bases/extensions.kuadrant.io_telemetrypolicies.yaml b/cmd/extensions/telemetry-policy/config/crd/bases/extensions.kuadrant.io_telemetrypolicies.yaml index 2546bf8ab..64c8757ab 100644 --- a/cmd/extensions/telemetry-policy/config/crd/bases/extensions.kuadrant.io_telemetrypolicies.yaml +++ b/cmd/extensions/telemetry-policy/config/crd/bases/extensions.kuadrant.io_telemetrypolicies.yaml @@ -18,8 +18,8 @@ spec: schema: openAPIV3Schema: description: |- - TelemetryPolicy enables custom metric labelling for Kuadrant data plane resources - through the use of dynamically evaluated CEL expressions. + TelemetryPolicy enables custom metric labelling and log field enrichment for + Kuadrant data plane resources through the use of dynamically evaluated CEL expressions. properties: apiVersion: description: |- @@ -40,6 +40,31 @@ spec: type: object spec: properties: + logging: + description: Logging holds the telemetry logging configuration + properties: + default: + description: Default logging configuration that applies to all + requests + properties: + fields: + additionalProperties: + type: string + description: |- + Fields to add to auth decision log records, where keys are field names and values are + CEL expressions referencing well-known attributes (e.g. auth.identity.sub). + Only fields whose CEL expressions resolve successfully will be included. + minProperties: 1 + type: object + x-kubernetes-validations: + - message: field names must not contain periods + rule: self.all(k, !k.contains('.')) + required: + - fields + type: object + required: + - default + type: object metrics: description: Metrics holds the telemetry metrics configuration properties: @@ -108,7 +133,6 @@ spec: - message: Invalid targetRef.kind. The only supported value is 'Gateway' rule: self.kind == 'Gateway' required: - - metrics - targetRef type: object status: diff --git a/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler.go b/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler.go index 7f8140a78..a74ffaf66 100644 --- a/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler.go +++ b/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler.go @@ -68,10 +68,21 @@ func (r *TelemetryPolicyReconciler) Reconcile(ctx context.Context, request recon } func (r *TelemetryPolicyReconciler) reconcileSpec(ctx context.Context, pol *v1alpha1.TelemetryPolicy, kuadrantCtx types.KuadrantCtx) (*v1alpha1.TelemetryPolicyStatus, error) { - for binding, expression := range pol.Spec.Metrics.Default.Labels { - if err := kuadrantCtx.AddDataTo(ctx, pol, types.DomainRequest, types.KuadrantMetricBinding(binding), expression); err != nil { - r.Logger.Error(err, "failed to add data to request domain") - return calculateErrorStatus(pol, err), err + if pol.Spec.Metrics != nil { + for binding, expression := range pol.Spec.Metrics.Default.Labels { + if err := kuadrantCtx.AddDataTo(ctx, pol, types.DomainRequest, types.KuadrantMetricBinding(binding), expression); err != nil { + r.Logger.Error(err, "failed to add data to request domain") + return calculateErrorStatus(pol, err), err + } + } + } + + if pol.Spec.Logging != nil { + for binding, expression := range pol.Spec.Logging.Default.Fields { + if err := kuadrantCtx.AddDataTo(ctx, pol, types.DomainRequest, types.KuadrantLoggingBinding(binding), expression); err != nil { + r.Logger.Error(err, "failed to add data to request domain") + return calculateErrorStatus(pol, err), err + } } } diff --git a/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler_test.go b/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler_test.go new file mode 100644 index 000000000..bbde71b77 --- /dev/null +++ b/cmd/extensions/telemetry-policy/internal/controller/telemetrypolicy_reconciler_test.go @@ -0,0 +1,199 @@ +//go:build unit + +package controller + +import ( + "context" + "fmt" + "testing" + + "github.com/go-logr/logr" + celref "github.com/google/cel-go/common/types/ref" + "sigs.k8s.io/controller-runtime/pkg/client" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/kuadrant/kuadrant-operator/cmd/extensions/telemetry-policy/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/pkg/extension/types" +) + +type addDataCall struct { + domain types.Domain + binding string + expression string +} + +type mockKuadrantCtx struct { + calls []addDataCall + failOn string // return error when binding matches this value + failErr error +} + +func (m *mockKuadrantCtx) AddDataTo(_ context.Context, _ types.Policy, domain types.Domain, binding, expression string) error { + m.calls = append(m.calls, addDataCall{domain: domain, binding: binding, expression: expression}) + if m.failOn != "" && binding == m.failOn { + return m.failErr + } + return nil +} + +func (m *mockKuadrantCtx) Resolve(context.Context, types.Policy, string, bool) (celref.Val, error) { + return nil, nil +} +func (m *mockKuadrantCtx) ResolvePolicy(context.Context, types.Policy, string, bool) (types.Policy, error) { + return nil, nil +} +func (m *mockKuadrantCtx) ReconcileObject(context.Context, client.Object, client.Object, types.MutateFn) (client.Object, error) { + return nil, nil +} +func (m *mockKuadrantCtx) RegisterActionMethod(_ context.Context, _ types.Policy, _ types.ActionMethodConfig) error { + return nil +} +func (m *mockKuadrantCtx) NewPipeline(types.Policy) types.Pipeline { return nil } + +func newTestPolicy(metrics map[string]string, loggingFields map[string]string) *v1alpha1.TelemetryPolicy { + pol := &v1alpha1.TelemetryPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "default", + Generation: 1, + }, + Spec: v1alpha1.TelemetryPolicySpec{ + TargetRef: gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName{ + LocalPolicyTargetReference: gatewayapiv1alpha2.LocalPolicyTargetReference{ + Group: "gateway.networking.k8s.io", + Kind: "Gateway", + Name: "my-gw", + }, + }, + }, + } + if metrics != nil { + pol.Spec.Metrics = &v1alpha1.MetricsSpec{Default: v1alpha1.MetricsConfig{Labels: metrics}} + } + if loggingFields != nil { + pol.Spec.Logging = &v1alpha1.LoggingSpec{Default: v1alpha1.LoggingConfig{Fields: loggingFields}} + } + return pol +} + +func TestReconcileSpec_LoggingFieldsOnly(t *testing.T) { + mock := &mockKuadrantCtx{} + r := &TelemetryPolicyReconciler{ + ExtensionBase: types.ExtensionBase{Logger: logr.Discard()}, + } + pol := newTestPolicy(nil, map[string]string{ + "client_identity": "auth.identity.sub", + "request_path": "request.path", + }) + + status, err := r.reconcileSpec(context.Background(), pol, mock) + if err != nil { + t.Fatalf("reconcileSpec returned error: %v", err) + } + if status == nil { + t.Fatal("reconcileSpec returned nil status") + } + + if len(mock.calls) != 2 { + t.Fatalf("expected 2 AddDataTo calls, got %d", len(mock.calls)) + } + + callMap := make(map[string]addDataCall, len(mock.calls)) + for _, c := range mock.calls { + callMap[c.binding] = c + } + + if c, ok := callMap["logging.fields.client_identity"]; !ok { + t.Error("missing AddDataTo call for logging.fields.client_identity") + } else { + if c.expression != "auth.identity.sub" { + t.Errorf("client_identity expression = %q, want %q", c.expression, "auth.identity.sub") + } + if c.domain != types.DomainRequest { + t.Errorf("client_identity domain = %v, want DomainRequest", c.domain) + } + } + + if c, ok := callMap["logging.fields.request_path"]; !ok { + t.Error("missing AddDataTo call for logging.fields.request_path") + } else { + if c.expression != "request.path" { + t.Errorf("request_path expression = %q, want %q", c.expression, "request.path") + } + } +} + +func TestReconcileSpec_MetricsAndLogging(t *testing.T) { + mock := &mockKuadrantCtx{} + r := &TelemetryPolicyReconciler{ + ExtensionBase: types.ExtensionBase{Logger: logr.Discard()}, + } + pol := newTestPolicy( + map[string]string{"model": "responseBodyJSON('/model')"}, + map[string]string{"client_identity": "auth.identity.sub"}, + ) + + _, err := r.reconcileSpec(context.Background(), pol, mock) + if err != nil { + t.Fatalf("reconcileSpec returned error: %v", err) + } + + if len(mock.calls) != 2 { + t.Fatalf("expected 2 AddDataTo calls, got %d", len(mock.calls)) + } + + callMap := make(map[string]addDataCall, len(mock.calls)) + for _, c := range mock.calls { + callMap[c.binding] = c + } + + if _, ok := callMap["metrics.labels.model"]; !ok { + t.Error("missing AddDataTo call for metrics.labels.model") + } + if _, ok := callMap["logging.fields.client_identity"]; !ok { + t.Error("missing AddDataTo call for logging.fields.client_identity") + } +} + +func TestReconcileSpec_LoggingFieldError(t *testing.T) { + expectedErr := fmt.Errorf("binding failed") + mock := &mockKuadrantCtx{ + failOn: "logging.fields.bad_field", + failErr: expectedErr, + } + r := &TelemetryPolicyReconciler{ + ExtensionBase: types.ExtensionBase{Logger: logr.Discard()}, + } + pol := newTestPolicy(nil, map[string]string{ + "bad_field": "invalid.expression", + }) + + status, err := r.reconcileSpec(context.Background(), pol, mock) + if err != expectedErr { + t.Fatalf("expected error %v, got %v", expectedErr, err) + } + if status == nil { + t.Fatal("expected error status, got nil") + } +} + +func TestReconcileSpec_EmptySpec(t *testing.T) { + mock := &mockKuadrantCtx{} + r := &TelemetryPolicyReconciler{ + ExtensionBase: types.ExtensionBase{Logger: logr.Discard()}, + } + pol := newTestPolicy(nil, nil) + + status, err := r.reconcileSpec(context.Background(), pol, mock) + if err != nil { + t.Fatalf("reconcileSpec returned error: %v", err) + } + if status == nil { + t.Fatal("reconcileSpec returned nil status") + } + if len(mock.calls) != 0 { + t.Errorf("expected 0 AddDataTo calls for empty spec, got %d", len(mock.calls)) + } +} diff --git a/internal/wasm/action_spec_test.go b/internal/wasm/action_spec_test.go index 55e9be84f..b9045a1fd 100644 --- a/internal/wasm/action_spec_test.go +++ b/internal/wasm/action_spec_test.go @@ -56,6 +56,8 @@ func TestDomainAndFieldName(t *testing.T) { {"auth.identity.user", "auth.identity", "user"}, {"simple", "", "simple"}, {"a.b.c.d", "a.b.c", "d"}, + {"logging.fields.client_identity", "logging.fields", "client_identity"}, + {"metrics.labels.model", "metrics.labels", "model"}, } for _, tc := range tests { domain, field := DomainAndFieldName(tc.input) @@ -1036,3 +1038,80 @@ func TestAttachBindings_NoBindings(t *testing.T) { } } } + +func TestBuildMetadataContext_LoggingFieldsDomain(t *testing.T) { + bindings := []DataBinding{ + {Domain: "logging.fields", Field: "client_identity", Expression: "auth.identity.sub"}, + {Domain: "logging.fields", Field: "request_path", Expression: "request.path"}, + } + + metadata := buildMetadataContext(bindings) + + if len(metadata.FilterMetadata) != 1 { + t.Fatalf("expected 1 filter_metadata entry, got %d", len(metadata.FilterMetadata)) + } + + entry := metadata.FilterMetadata[0] + if entry.Domain != "io.kuadrant.logging.fields" { + t.Errorf("domain = %q, want %q", entry.Domain, "io.kuadrant.logging.fields") + } + if len(entry.Fields) != 2 { + t.Fatalf("expected 2 fields, got %d", len(entry.Fields)) + } + + fieldMap := make(map[string]string, len(entry.Fields)) + for _, f := range entry.Fields { + fieldMap[f.Key] = f.Expression + } + + if v, ok := fieldMap["client_identity"]; !ok || v != "auth.identity.sub" { + t.Errorf("client_identity field = %q (present=%v), want %q", v, ok, "auth.identity.sub") + } + if v, ok := fieldMap["request_path"]; !ok || v != "request.path" { + t.Errorf("request_path field = %q (present=%v), want %q", v, ok, "request.path") + } +} + +func TestBuildMetadataContext_MixedMetricsAndLogging(t *testing.T) { + bindings := []DataBinding{ + {Domain: "metrics.labels", Field: "model", Expression: "responseBodyJSON('/model')"}, + {Domain: "logging.fields", Field: "client_identity", Expression: "auth.identity.sub"}, + {Domain: "metrics.labels", Field: "user", Expression: "auth.identity.userid"}, + {Domain: "logging.fields", Field: "model", Expression: "request.host"}, + } + + metadata := buildMetadataContext(bindings) + + if len(metadata.FilterMetadata) != 2 { + t.Fatalf("expected 2 filter_metadata entries, got %d", len(metadata.FilterMetadata)) + } + + domainEntries := make(map[string][]MetadataFieldCEL, len(metadata.FilterMetadata)) + for _, e := range metadata.FilterMetadata { + domainEntries[e.Domain] = e.Fields + } + + loggingFields, ok := domainEntries["io.kuadrant.logging.fields"] + if !ok { + t.Fatal("missing io.kuadrant.logging.fields entry") + } + if len(loggingFields) != 2 { + t.Errorf("io.kuadrant.logging.fields: expected 2 fields, got %d", len(loggingFields)) + } + + metricsFields, ok := domainEntries["io.kuadrant.metrics.labels"] + if !ok { + t.Fatal("missing io.kuadrant.metrics.labels entry") + } + if len(metricsFields) != 2 { + t.Errorf("io.kuadrant.metrics.labels: expected 2 fields, got %d", len(metricsFields)) + } + + // Entries should be sorted by domain + if metadata.FilterMetadata[0].Domain != "io.kuadrant.logging.fields" { + t.Errorf("entries[0].Domain = %q, want io.kuadrant.logging.fields (alphabetical sort)", metadata.FilterMetadata[0].Domain) + } + if metadata.FilterMetadata[1].Domain != "io.kuadrant.metrics.labels" { + t.Errorf("entries[1].Domain = %q, want io.kuadrant.metrics.labels (alphabetical sort)", metadata.FilterMetadata[1].Domain) + } +} diff --git a/pkg/extension/types/types.go b/pkg/extension/types/types.go index 30cde684b..55f725779 100644 --- a/pkg/extension/types/types.go +++ b/pkg/extension/types/types.go @@ -24,6 +24,9 @@ const ( // KuadrantMetricsPrefix is the prefix applied to metric bindings injected // via AddDataTo / KuadrantMetricBinding. KuadrantMetricsPrefix = "metrics.labels" + // KuadrantLoggingPrefix is the prefix applied to logging bindings injected + // via AddDataTo / KuadrantLoggingBinding. + KuadrantLoggingPrefix = "logging.fields" ) // Domain enumerates the supported logical domains for mutator injected data. @@ -114,3 +117,9 @@ func (eb *ExtensionBase) Configure(ctx context.Context) error { func KuadrantMetricBinding(binding string) string { return fmt.Sprintf("%s.%s", KuadrantMetricsPrefix, binding) } + +// KuadrantLoggingBinding creates a fully qualified binding name for logging +// enrichment. +func KuadrantLoggingBinding(binding string) string { + return fmt.Sprintf("%s.%s", KuadrantLoggingPrefix, binding) +} diff --git a/pkg/extension/types/types_test.go b/pkg/extension/types/types_test.go new file mode 100644 index 000000000..925a5ec33 --- /dev/null +++ b/pkg/extension/types/types_test.go @@ -0,0 +1,36 @@ +//go:build unit + +package types + +import "testing" + +func TestKuadrantLoggingBinding(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"client_identity", "logging.fields.client_identity"}, + {"model", "logging.fields.model"}, + {"request_path", "logging.fields.request_path"}, + } + for _, tc := range tests { + if got := KuadrantLoggingBinding(tc.input); got != tc.want { + t.Errorf("KuadrantLoggingBinding(%q) = %q, want %q", tc.input, got, tc.want) + } + } +} + +func TestKuadrantMetricBinding(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"model", "metrics.labels.model"}, + {"user", "metrics.labels.user"}, + } + for _, tc := range tests { + if got := KuadrantMetricBinding(tc.input); got != tc.want { + t.Errorf("KuadrantMetricBinding(%q) = %q, want %q", tc.input, got, tc.want) + } + } +}