diff --git a/api/v1alpha1/envoyproxy_tracing_types.go b/api/v1alpha1/envoyproxy_tracing_types.go index 11eaeb686d9..0dfa8bd36ab 100644 --- a/api/v1alpha1/envoyproxy_tracing_types.go +++ b/api/v1alpha1/envoyproxy_tracing_types.go @@ -34,19 +34,39 @@ const ( TracingProviderTypeDatadog TracingProviderType = "Datadog" ) +const ( + // DefaultTracingProviderType is the provider type applied during translation + // when TracingProvider.Type is unset. + DefaultTracingProviderType = TracingProviderTypeOpenTelemetry + // DefaultTracingProviderPort is the provider port applied during translation + // when TracingProvider.Port is unset. + DefaultTracingProviderPort int32 = 4317 +) + // TracingProvider defines the tracing provider configuration. // -// +kubebuilder:validation:XValidation:message="host or backendRefs needs to be set",rule="has(self.host) || self.backendRefs.size() > 0" +// A provider is only required to set host or backendRefs after the +// GatewayClass-level and Gateway-level EnvoyProxy configs are merged +// (see EnvoyProxySpec.MergeType), so completeness is checked during +// translation instead of by a CEL rule here. A provider that is still +// incomplete after the merge turns tracing off for that Gateway. +// // +kubebuilder:validation:XValidation:message="BackendRefs must be used, backendRef is not supported.",rule="!has(self.backendRef)" // +kubebuilder:validation:XValidation:message="BackendRefs only support Service and Backend kind.",rule="has(self.backendRefs) ? self.backendRefs.all(f, f.kind == 'Service' || f.kind == 'Backend') : true" // +kubebuilder:validation:XValidation:message="BackendRefs only support Core and gateway.envoyproxy.io group.",rule="has(self.backendRefs) ? (self.backendRefs.all(f, f.group == \"\" || f.group == 'gateway.envoyproxy.io')) : true" -// +kubebuilder:validation:XValidation:message="openTelemetry can only be used with type OpenTelemetry",rule="has(self.openTelemetry) ? self.type == 'OpenTelemetry' : true" +// +kubebuilder:validation:XValidation:message="openTelemetry can only be used with type OpenTelemetry",rule="has(self.openTelemetry) ? (!has(self.type) || self.type == 'OpenTelemetry') : true" type TracingProvider struct { BackendCluster `json:",inline"` // Type defines the tracing provider type. + // + // Defaults to OpenTelemetry. The default is applied during translation rather + // than by admission, so that a Gateway-level EnvoyProxy overriding only part of + // the provider does not replace the type inherited from the GatewayClass level + // (see EnvoyProxySpec.MergeType). + // // +kubebuilder:validation:Enum=OpenTelemetry;Zipkin;Datadog - // +kubebuilder:default=OpenTelemetry - Type TracingProviderType `json:"type"` + // +optional + Type *TracingProviderType `json:"type,omitempty"` // Host define the provider service hostname. // // Deprecated: Use BackendRefs instead. @@ -55,12 +75,14 @@ type TracingProvider struct { Host *string `json:"host,omitempty"` // Port defines the port the provider service is exposed on. // + // Defaults to 4317. The default is applied during translation rather than by + // admission, for the same reason as Type. + // // Deprecated: Use BackendRefs instead. // // +optional // +kubebuilder:validation:Minimum=0 - // +kubebuilder:default=4317 - Port int32 `json:"port,omitempty"` + Port *int32 `json:"port,omitempty"` // ServiceName defines the service name to use in tracing configuration. // If not set, Envoy Gateway will use a default service name set as // "name.namespace" (e.g., "my-gateway.default"). diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 2a4014dfa8a..54a64efd178 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -8981,11 +8981,21 @@ func (in *Tracing) DeepCopy() *Tracing { func (in *TracingProvider) DeepCopyInto(out *TracingProvider) { *out = *in in.BackendCluster.DeepCopyInto(&out.BackendCluster) + if in.Type != nil { + in, out := &in.Type, &out.Type + *out = new(TracingProviderType) + **out = **in + } if in.Host != nil { in, out := &in.Host, &out.Host *out = new(string) **out = **in } + if in.Port != nil { + in, out := &in.Port, &out.Port + *out = new(int32) + **out = **in + } if in.ServiceName != nil { in, out := &in.ServiceName, &out.ServiceName *out = new(string) diff --git a/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_envoyproxies.yaml b/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_envoyproxies.yaml index a9630d033a6..9d7830b3086 100644 --- a/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_envoyproxies.yaml +++ b/charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_envoyproxies.yaml @@ -18921,10 +18921,12 @@ spec: : true' type: object port: - default: 4317 description: |- Port defines the port the provider service is exposed on. + Defaults to 4317. The default is applied during translation rather than by + admission, for the same reason as Type. + Deprecated: Use BackendRefs instead. format: int32 minimum: 0 @@ -18942,8 +18944,13 @@ spec: - message: serviceName cannot be empty if provided rule: self != "" type: - default: OpenTelemetry - description: Type defines the tracing provider type. + description: |- + Type defines the tracing provider type. + + Defaults to OpenTelemetry. The default is applied during translation rather + than by admission, so that a Gateway-level EnvoyProxy overriding only part of + the provider does not replace the type inherited from the GatewayClass level + (see EnvoyProxySpec.MergeType). enum: - OpenTelemetry - Zipkin @@ -18965,12 +18972,8 @@ spec: id will be used. type: boolean type: object - required: - - type type: object x-kubernetes-validations: - - message: host or backendRefs needs to be set - rule: has(self.host) || self.backendRefs.size() > 0 - message: BackendRefs must be used, backendRef is not supported. rule: '!has(self.backendRef)' - message: BackendRefs only support Service and Backend kind. @@ -18982,8 +18985,8 @@ spec: f.group == "" || f.group == ''gateway.envoyproxy.io'')) : true' - message: openTelemetry can only be used with type OpenTelemetry - rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry'' - : true' + rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type + == ''OpenTelemetry'') : true' samplingFraction: description: |- SamplingFraction represents the fraction of requests that should be diff --git a/charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml b/charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml index 76159da3d20..a59fc47abbe 100644 --- a/charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml +++ b/charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml @@ -18920,10 +18920,12 @@ spec: : true' type: object port: - default: 4317 description: |- Port defines the port the provider service is exposed on. + Defaults to 4317. The default is applied during translation rather than by + admission, for the same reason as Type. + Deprecated: Use BackendRefs instead. format: int32 minimum: 0 @@ -18941,8 +18943,13 @@ spec: - message: serviceName cannot be empty if provided rule: self != "" type: - default: OpenTelemetry - description: Type defines the tracing provider type. + description: |- + Type defines the tracing provider type. + + Defaults to OpenTelemetry. The default is applied during translation rather + than by admission, so that a Gateway-level EnvoyProxy overriding only part of + the provider does not replace the type inherited from the GatewayClass level + (see EnvoyProxySpec.MergeType). enum: - OpenTelemetry - Zipkin @@ -18964,12 +18971,8 @@ spec: id will be used. type: boolean type: object - required: - - type type: object x-kubernetes-validations: - - message: host or backendRefs needs to be set - rule: has(self.host) || self.backendRefs.size() > 0 - message: BackendRefs must be used, backendRef is not supported. rule: '!has(self.backendRef)' - message: BackendRefs only support Service and Backend kind. @@ -18981,8 +18984,8 @@ spec: f.group == "" || f.group == ''gateway.envoyproxy.io'')) : true' - message: openTelemetry can only be used with type OpenTelemetry - rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry'' - : true' + rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type + == ''OpenTelemetry'') : true' samplingFraction: description: |- SamplingFraction represents the fraction of requests that should be diff --git a/internal/gatewayapi/envoyproxy_merge.go b/internal/gatewayapi/envoyproxy_merge.go index 53ab2d4f8b2..9d1ba015c0d 100644 --- a/internal/gatewayapi/envoyproxy_merge.go +++ b/internal/gatewayapi/envoyproxy_merge.go @@ -8,6 +8,8 @@ package gatewayapi import ( "fmt" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/envoyproxy/gateway/internal/utils" ) @@ -90,9 +92,81 @@ func mergeEnvoyProxies( return override, nil } - merged, err := utils.Merge(*base, *override, mergeType) + // The merged object carries the override's metadata, so a backendRef + // inherited from base without a namespace would later be resolved in the + // override's namespace. Pin it to the namespace it was written in first. + merged, err := utils.Merge(*qualifyTelemetryBackendRefs(base), *override, mergeType) if err != nil { return nil, err } return &merged, nil } + +// qualifyTelemetryBackendRefs returns ep with every telemetry backendRef that +// omits a namespace set to ep's own namespace, which is where the ref resolves +// when ep is used on its own. ep itself is not modified: a copy is returned +// when there is anything to set. An EnvoyProxy without a namespace, such as +// the one built from the EnvoyGateway default spec, is returned unchanged. +func qualifyTelemetryBackendRefs(ep *egv1a1.EnvoyProxy) *egv1a1.EnvoyProxy { + if ep.Namespace == "" || !hasUnqualifiedTelemetryBackendRef(ep) { + return ep + } + + ep = ep.DeepCopy() + ns := gwapiv1.Namespace(ep.Namespace) + for _, cluster := range telemetryBackendClusters(ep) { + for i := range cluster.BackendRefs { + if cluster.BackendRefs[i].Namespace == nil { + cluster.BackendRefs[i].Namespace = new(ns) + } + } + } + return ep +} + +func hasUnqualifiedTelemetryBackendRef(ep *egv1a1.EnvoyProxy) bool { + for _, cluster := range telemetryBackendClusters(ep) { + for i := range cluster.BackendRefs { + if cluster.BackendRefs[i].Namespace == nil { + return true + } + } + } + return false +} + +// telemetryBackendClusters returns every BackendCluster under the telemetry +// settings of ep: the tracing provider, the ALS and OpenTelemetry access log +// sinks and the OpenTelemetry metric sinks. The returned pointers alias ep. +func telemetryBackendClusters(ep *egv1a1.EnvoyProxy) []*egv1a1.BackendCluster { + telemetry := ep.Spec.Telemetry + if telemetry == nil { + return nil + } + + var clusters []*egv1a1.BackendCluster + if telemetry.Tracing != nil { + clusters = append(clusters, &telemetry.Tracing.Provider.BackendCluster) + } + if telemetry.AccessLog != nil { + for i := range telemetry.AccessLog.Settings { + for j := range telemetry.AccessLog.Settings[i].Sinks { + sink := &telemetry.AccessLog.Settings[i].Sinks[j] + if sink.ALS != nil { + clusters = append(clusters, &sink.ALS.BackendCluster) + } + if sink.OpenTelemetry != nil { + clusters = append(clusters, &sink.OpenTelemetry.BackendCluster) + } + } + } + } + if telemetry.Metrics != nil { + for i := range telemetry.Metrics.Sinks { + if sink := telemetry.Metrics.Sinks[i].OpenTelemetry; sink != nil { + clusters = append(clusters, &sink.BackendCluster) + } + } + } + return clusters +} diff --git a/internal/gatewayapi/envoyproxy_merge_test.go b/internal/gatewayapi/envoyproxy_merge_test.go index 7642662c093..7303b7e6b73 100644 --- a/internal/gatewayapi/envoyproxy_merge_test.go +++ b/internal/gatewayapi/envoyproxy_merge_test.go @@ -9,6 +9,8 @@ import ( "testing" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" ) @@ -272,3 +274,180 @@ func TestMergeEnvoyProxyConfigs(t *testing.T) { }) } } + +// TestMergeEnvoyProxyConfigsTelemetryBackendRefNamespace checks that telemetry +// backendRefs inherited from the GatewayClass-level EnvoyProxy keep resolving in +// that EnvoyProxy's namespace after the Gateway-level EnvoyProxy is merged over it. +func TestMergeEnvoyProxyConfigsTelemetryBackendRefNamespace(t *testing.T) { + const ( + classNS = "envoy-gateway-system" + gatewayNS = "app-ns" + ) + + ref := func(name string, ns *string) egv1a1.BackendRef { + r := egv1a1.BackendRef{ + BackendObjectReference: gwapiv1.BackendObjectReference{ + Name: gwapiv1.ObjectName(name), + Port: new(gwapiv1.PortNumber(4317)), + }, + } + if ns != nil { + r.Namespace = new(gwapiv1.Namespace(*ns)) + } + return r + } + nsOf := func(r egv1a1.BackendRef) string { + if r.Namespace == nil { + return "" + } + return string(*r.Namespace) + } + + newClassProxy := func() *egv1a1.EnvoyProxy { + return &egv1a1.EnvoyProxy{ + ObjectMeta: metav1.ObjectMeta{Namespace: classNS, Name: "class"}, + Spec: egv1a1.EnvoyProxySpec{ + Telemetry: &egv1a1.ProxyTelemetry{ + Tracing: &egv1a1.ProxyTracing{ + Provider: egv1a1.TracingProvider{ + Type: new(egv1a1.TracingProviderTypeDatadog), + BackendCluster: egv1a1.BackendCluster{ + BackendRefs: []egv1a1.BackendRef{ + ref("datadog-agent", nil), + ref("otel-collector", new("monitoring")), + }, + }, + }, + }, + AccessLog: &egv1a1.ProxyAccessLog{ + Settings: []egv1a1.ProxyAccessLogSetting{{ + Sinks: []egv1a1.ProxyAccessLogSink{ + { + Type: egv1a1.ProxyAccessLogSinkTypeALS, + ALS: &egv1a1.ALSEnvoyProxyAccessLog{ + Type: egv1a1.ALSEnvoyProxyAccessLogTypeHTTP, + BackendCluster: egv1a1.BackendCluster{BackendRefs: []egv1a1.BackendRef{ref("als", nil)}}, + }, + }, + { + Type: egv1a1.ProxyAccessLogSinkTypeOpenTelemetry, + OpenTelemetry: &egv1a1.OpenTelemetryEnvoyProxyAccessLog{ + BackendCluster: egv1a1.BackendCluster{BackendRefs: []egv1a1.BackendRef{ref("otel-logs", nil)}}, + }, + }, + }, + }}, + }, + Metrics: &egv1a1.ProxyMetrics{ + Sinks: []egv1a1.ProxyMetricSink{{ + Type: egv1a1.MetricSinkTypeOpenTelemetry, + OpenTelemetry: &egv1a1.ProxyOpenTelemetrySink{ + BackendCluster: egv1a1.BackendCluster{BackendRefs: []egv1a1.BackendRef{ref("otel-metrics", nil)}}, + }, + }}, + }, + }, + }, + } + } + newGatewayProxy := func(mergeType *egv1a1.MergeType, tracingRefs ...egv1a1.BackendRef) *egv1a1.EnvoyProxy { + return &egv1a1.EnvoyProxy{ + ObjectMeta: metav1.ObjectMeta{Namespace: gatewayNS, Name: "gateway"}, + Spec: egv1a1.EnvoyProxySpec{ + MergeType: mergeType, + Telemetry: &egv1a1.ProxyTelemetry{ + Tracing: &egv1a1.ProxyTracing{ + Provider: egv1a1.TracingProvider{ + ServiceName: new("my-custom-service"), + BackendCluster: egv1a1.BackendCluster{BackendRefs: tracingRefs}, + }, + }, + }, + }, + } + } + + for _, mergeType := range []egv1a1.MergeType{egv1a1.StrategicMerge, egv1a1.JSONMerge} { + t.Run(string(mergeType)+" pins inherited refs to the GatewayClass EnvoyProxy namespace", func(t *testing.T) { + classProxy := newClassProxy() + merged, err := MergeEnvoyProxyConfigs(nil, classProxy, newGatewayProxy(new(mergeType))) + require.NoError(t, err) + + require.Equal(t, gatewayNS, merged.Namespace) + telemetry := merged.Spec.Telemetry + require.Equal(t, "my-custom-service", *telemetry.Tracing.Provider.ServiceName) + require.Equal(t, egv1a1.TracingProviderTypeDatadog, *telemetry.Tracing.Provider.Type) + + tracingRefs := telemetry.Tracing.Provider.BackendRefs + require.Len(t, tracingRefs, 2) + require.Equal(t, classNS, nsOf(tracingRefs[0]), "inherited ref without a namespace") + require.Equal(t, "monitoring", nsOf(tracingRefs[1]), "explicit namespace is kept") + require.Equal(t, classNS, nsOf(telemetry.AccessLog.Settings[0].Sinks[0].ALS.BackendRefs[0])) + require.Equal(t, classNS, nsOf(telemetry.AccessLog.Settings[0].Sinks[1].OpenTelemetry.BackendRefs[0])) + require.Equal(t, classNS, nsOf(telemetry.Metrics.Sinks[0].OpenTelemetry.BackendRefs[0])) + + // The GatewayClass EnvoyProxy is shared across Gateways and must not be modified. + require.Equal(t, newClassProxy(), classProxy) + }) + } + + t.Run("refs set by the Gateway EnvoyProxy keep resolving in its own namespace", func(t *testing.T) { + gatewayProxy := newGatewayProxy(new(egv1a1.StrategicMerge), ref("gateway-collector", nil)) + merged, err := MergeEnvoyProxyConfigs(nil, newClassProxy(), gatewayProxy) + require.NoError(t, err) + + tracingRefs := merged.Spec.Telemetry.Tracing.Provider.BackendRefs + require.Len(t, tracingRefs, 1) + require.Equal(t, "gateway-collector", string(tracingRefs[0].Name)) + require.Empty(t, nsOf(tracingRefs[0])) + // Sinks the Gateway EnvoyProxy did not touch are still inherited and pinned. + require.Equal(t, classNS, nsOf(merged.Spec.Telemetry.Metrics.Sinks[0].OpenTelemetry.BackendRefs[0])) + }) + + t.Run("replace mode leaves the Gateway EnvoyProxy untouched", func(t *testing.T) { + gatewayProxy := newGatewayProxy(nil, ref("gateway-collector", nil)) + merged, err := MergeEnvoyProxyConfigs(nil, newClassProxy(), gatewayProxy) + require.NoError(t, err) + + require.Same(t, gatewayProxy, merged) + require.Empty(t, nsOf(merged.Spec.Telemetry.Tracing.Provider.BackendRefs[0])) + }) + + t.Run("no Gateway EnvoyProxy leaves the GatewayClass EnvoyProxy untouched", func(t *testing.T) { + classProxy := newClassProxy() + merged, err := MergeEnvoyProxyConfigs(nil, classProxy, nil) + require.NoError(t, err) + + require.Same(t, classProxy, merged) + require.Empty(t, nsOf(merged.Spec.Telemetry.Tracing.Provider.BackendRefs[0])) + }) + + t.Run("default spec refs have no namespace to pin", func(t *testing.T) { + defaultSpec := &egv1a1.EnvoyProxySpec{ + Telemetry: &egv1a1.ProxyTelemetry{ + Tracing: &egv1a1.ProxyTracing{ + Provider: egv1a1.TracingProvider{ + BackendCluster: egv1a1.BackendCluster{BackendRefs: []egv1a1.BackendRef{ref("default-collector", nil)}}, + }, + }, + }, + } + classProxy := &egv1a1.EnvoyProxy{ + ObjectMeta: metav1.ObjectMeta{Namespace: classNS, Name: "class"}, + Spec: egv1a1.EnvoyProxySpec{ + MergeType: new(egv1a1.StrategicMerge), + Telemetry: &egv1a1.ProxyTelemetry{ + Tracing: &egv1a1.ProxyTracing{ + Provider: egv1a1.TracingProvider{ServiceName: new("class-service")}, + }, + }, + }, + } + merged, err := MergeEnvoyProxyConfigs(defaultSpec, classProxy, nil) + require.NoError(t, err) + + tracingRefs := merged.Spec.Telemetry.Tracing.Provider.BackendRefs + require.Len(t, tracingRefs, 1) + require.Empty(t, nsOf(tracingRefs[0])) + }) +} diff --git a/internal/gatewayapi/listener.go b/internal/gatewayapi/listener.go index 3d8376553ad..b5cc660d72f 100644 --- a/internal/gatewayapi/listener.go +++ b/internal/gatewayapi/listener.go @@ -1061,14 +1061,27 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En } tracing := envoyproxy.Spec.Telemetry.Tracing + // Type and Port are defaulted here rather than by admission so that a + // Gateway-level EnvoyProxy overriding only part of the provider does not + // replace the values inherited from the GatewayClass level: a defaulted field + // is indistinguishable from an explicit one once the merge patch is built. + // The copy keeps the EnvoyProxy resource itself untouched. + provider := tracing.Provider + if provider.Type == nil { + provider.Type = new(egv1a1.DefaultTracingProviderType) + } + if provider.Port == nil { + provider.Port = new(egv1a1.DefaultTracingProviderPort) + } + // TODO: rename this, so that we can share backend with accesslog? destName := "tracing" settingName := irDestinationSettingName(destName, -1) - ds, traffic, err := t.processBackendRefsForTelemetry(settingName, tracing.Provider.BackendCluster, envoyproxy.Namespace, resources, envoyproxy, gwCtx) + ds, traffic, err := t.processBackendRefsForTelemetry(settingName, provider.BackendCluster, envoyproxy.Namespace, resources, envoyproxy, gwCtx) if err != nil { return nil, err } - if tracing.Provider.Type == egv1a1.TracingProviderTypeOpenTelemetry { + if *provider.Type == egv1a1.TracingProviderTypeOpenTelemetry { // TODO: update when OTLP/HTTP is completely supported (logs, traces, metrics) for _, d := range ds { d.Protocol = ir.GRPC @@ -1080,11 +1093,18 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En // fallback to host and port // TODO: remove support for Host/Port in v1.2 if len(ds) == 0 { - var host string - var port uint32 - if tracing.Provider.Host != nil { - host, port = *tracing.Provider.Host, uint32(tracing.Provider.Port) + // Checked here instead of by a CRD CEL rule so that a partial provider + // (e.g. only serviceName) can be completed by the GatewayClass-level and + // Gateway-level EnvoyProxy merge before the check runs. An incomplete + // provider only turns tracing off, it does not stop the Gateway from + // being provisioned. + if provider.Host == nil { + t.Logger.Info("Disabling tracing because the merged tracing provider sets neither host nor backendRefs", + "gateway", utils.NamespacedName(gwCtx.Gateway).String(), + "envoyProxy", utils.NamespacedName(envoyproxy).String()) + return nil, nil } + host, port := *provider.Host, uint32(*provider.Port) ds = destinationSettingFromHostAndPort(settingName, host, port) authority = host } @@ -1096,8 +1116,8 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En } // Use configured service name if provided - if tracing.Provider.ServiceName != nil { - serviceName = *tracing.Provider.ServiceName + if provider.ServiceName != nil { + serviceName = *provider.ServiceName } return &ir.Tracing{ @@ -1108,15 +1128,15 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En OverallSamplingRate: proxySamplingFractionPtr(tracing.OverallSamplingFraction), CustomTags: ir.CustomTagMapToSlice(tracing.CustomTags), Tags: ir.MapToSlice(tracing.Tags), - ResourceAttributes: ir.MapToSlice(getOpenTelemetryTracingResourceAttributes(&tracing.Provider)), + ResourceAttributes: ir.MapToSlice(getOpenTelemetryTracingResourceAttributes(&provider)), Destination: ir.RouteDestination{ Name: destName, Settings: ds, Metadata: buildResourceMetadata(envoyproxy, nil), }, - Provider: tracing.Provider, + Provider: provider, Traffic: traffic, - Headers: getOpenTelemetryTracingHeaders(&tracing.Provider), + Headers: getOpenTelemetryTracingHeaders(&provider), SpanName: tracing.SpanName, }, nil } diff --git a/internal/gatewayapi/listener_test.go b/internal/gatewayapi/listener_test.go index 0ca43b8b362..e99470cb7ab 100644 --- a/internal/gatewayapi/listener_test.go +++ b/internal/gatewayapi/listener_test.go @@ -916,7 +916,6 @@ func TestProcessTracingServiceName(t *testing.T) { envoyProxy *egv1a1.EnvoyProxy mergeGateways bool expectedServiceName string - expectError bool }{ { name: "no tracing configuration", @@ -951,7 +950,7 @@ func TestProcessTracingServiceName(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -987,7 +986,7 @@ func TestProcessTracingServiceName(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1027,7 +1026,7 @@ func TestProcessTracingServiceName(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1068,7 +1067,7 @@ func TestProcessTracingServiceName(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1088,6 +1087,33 @@ func TestProcessTracingServiceName(t *testing.T) { mergeGateways: true, expectedServiceName: "test-gateway-class", // Should use gateway class name when merging }, + { + name: "tracing provider without backendRefs or host disables tracing", + gateway: &gwapiv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-gateway", + Namespace: "test-namespace", + }, + }, + envoyProxy: &egv1a1.EnvoyProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-proxy", + Namespace: "test-namespace", + }, + Spec: egv1a1.EnvoyProxySpec{ + Telemetry: &egv1a1.ProxyTelemetry{ + Tracing: &egv1a1.ProxyTracing{ + Provider: egv1a1.TracingProvider{ + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), + ServiceName: new("only-name-overridden"), + }, + }, + }, + }, + }, + // An empty expectedServiceName asserts that no tracing config is built. + expectedServiceName: "", + }, } for _, tc := range cases { @@ -1151,11 +1177,6 @@ func TestProcessTracingServiceName(t *testing.T) { Gateway: tc.gateway, }, tc.envoyProxy, tc.mergeGateways, resources) - if tc.expectError { - assert.Error(t, err) - return - } - require.NoError(t, err) if tc.expectedServiceName == "" { @@ -1169,6 +1190,83 @@ func TestProcessTracingServiceName(t *testing.T) { } } +// TestProcessTracingProviderDefaults guards the defaults that used to be applied +// by admission. Applying them there made a partial Gateway-level override +// indistinguishable from an explicit one, so an inherited type or port was +// silently replaced by the defaulted value during the EnvoyProxy merge. +func TestProcessTracingProviderDefaults(t *testing.T) { + cases := []struct { + name string + provider egv1a1.TracingProvider + expectedType egv1a1.TracingProviderType + expectedPort uint32 + }{ + { + name: "unset type and port fall back to the documented defaults", + provider: egv1a1.TracingProvider{ + Host: new("otel-collector.monitoring.svc.cluster.local"), + }, + expectedType: egv1a1.TracingProviderTypeOpenTelemetry, + expectedPort: 4317, + }, + { + name: "type and port inherited from the GatewayClass level are kept", + provider: egv1a1.TracingProvider{ + Host: new("datadog-agent.monitoring.svc.cluster.local"), + Type: new(egv1a1.TracingProviderTypeDatadog), + Port: new(int32(8126)), + }, + expectedType: egv1a1.TracingProviderTypeDatadog, + expectedPort: 8126, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + translator := &Translator{} + envoyProxy := &egv1a1.EnvoyProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-proxy", + Namespace: "test-namespace", + }, + Spec: egv1a1.EnvoyProxySpec{ + Telemetry: &egv1a1.ProxyTelemetry{ + Tracing: &egv1a1.ProxyTracing{ + Provider: tc.provider, + }, + }, + }, + } + + result, err := translator.processTracing(&GatewayContext{ + Gateway: &gwapiv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-gateway", + Namespace: "test-namespace", + }, + }, + }, envoyProxy, false, &resource.Resources{}) + require.NoError(t, err) + require.NotNil(t, result) + + require.NotNil(t, result.Provider.Type) + assert.Equal(t, tc.expectedType, *result.Provider.Type) + require.NotNil(t, result.Provider.Port) + assert.Equal(t, tc.expectedPort, uint32(*result.Provider.Port)) + + // The host/port fallback destination has to agree with the provider port. + require.Len(t, result.Destination.Settings, 1) + require.Len(t, result.Destination.Settings[0].Endpoints, 1) + assert.Equal(t, tc.expectedPort, result.Destination.Settings[0].Endpoints[0].Port) + + // Defaulting works on a copy, so the EnvoyProxy resource keeps whatever + // the user actually set. + assert.Equal(t, tc.provider.Type, envoyProxy.Spec.Telemetry.Tracing.Provider.Type) + assert.Equal(t, tc.provider.Port, envoyProxy.Spec.Telemetry.Tracing.Provider.Port) + }) + } +} + func TestProcessAccessLog(t *testing.T) { tests := []struct { name string diff --git a/internal/gatewayapi/testdata/envoyproxy-otel-backend-custom-ca.out.yaml b/internal/gatewayapi/testdata/envoyproxy-otel-backend-custom-ca.out.yaml index ff5b757e2c6..7926e6d61cf 100644 --- a/internal/gatewayapi/testdata/envoyproxy-otel-backend-custom-ca.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-otel-backend-custom-ca.out.yaml @@ -264,6 +264,7 @@ xdsIR: kind: Backend name: otel-collector namespace: envoy-gateway + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls-per-resource-secret.out.yaml b/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls-per-resource-secret.out.yaml index 6527350a901..c555a091b13 100644 --- a/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls-per-resource-secret.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls-per-resource-secret.out.yaml @@ -261,6 +261,7 @@ xdsIR: kind: Backend name: otel-collector namespace: envoy-gateway + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls.out.yaml b/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls.out.yaml index 9c95ca2bf3d..2c12f551a83 100644 --- a/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-otel-backend-tls.out.yaml @@ -261,6 +261,7 @@ xdsIR: kind: Backend name: otel-collector namespace: envoy-gateway + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-otel-backendtlspolicy.out.yaml b/internal/gatewayapi/testdata/envoyproxy-otel-backendtlspolicy.out.yaml index e15df74b14e..f7018a15b09 100644 --- a/internal/gatewayapi/testdata/envoyproxy-otel-backendtlspolicy.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-otel-backendtlspolicy.out.yaml @@ -292,6 +292,7 @@ xdsIR: kind: Backend name: otel-collector namespace: envoy-gateway + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-tracing-backend-uds.out.yaml b/internal/gatewayapi/testdata/envoyproxy-tracing-backend-uds.out.yaml index c84261d0697..c5e6bcce11c 100644 --- a/internal/gatewayapi/testdata/envoyproxy-tracing-backend-uds.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-tracing-backend-uds.out.yaml @@ -269,6 +269,7 @@ xdsIR: timeout: tcp: connectTimeout: 15s + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-tracing-backend-with-hc-log.out.yaml b/internal/gatewayapi/testdata/envoyproxy-tracing-backend-with-hc-log.out.yaml index db36129747c..d96f1b97ea0 100644 --- a/internal/gatewayapi/testdata/envoyproxy-tracing-backend-with-hc-log.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-tracing-backend-with-hc-log.out.yaml @@ -245,6 +245,7 @@ xdsIR: timeout: 500ms type: HTTP unhealthyThreshold: 3 + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-tracing-backend.out.yaml b/internal/gatewayapi/testdata/envoyproxy-tracing-backend.out.yaml index 66cc26c5040..f9b09b665f1 100644 --- a/internal/gatewayapi/testdata/envoyproxy-tracing-backend.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-tracing-backend.out.yaml @@ -248,6 +248,7 @@ xdsIR: timeout: tcp: connectTimeout: 15s + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-tracing-sampler.out.yaml b/internal/gatewayapi/testdata/envoyproxy-tracing-sampler.out.yaml index 2393104ece4..9c1f5c2ff41 100644 --- a/internal/gatewayapi/testdata/envoyproxy-tracing-sampler.out.yaml +++ b/internal/gatewayapi/testdata/envoyproxy-tracing-sampler.out.yaml @@ -163,6 +163,7 @@ xdsIR: samplingPercentage: numerator: 50 type: ParentBasedTraceIdRatio + port: 4317 type: OpenTelemetry samplingRate: 100 serviceName: gateway-1.envoy-gateway diff --git a/internal/gatewayapi/testdata/envoyproxy-tracing-strategic-merge-inherited-backendref.in.yaml b/internal/gatewayapi/testdata/envoyproxy-tracing-strategic-merge-inherited-backendref.in.yaml new file mode 100644 index 00000000000..2de33feca5b --- /dev/null +++ b/internal/gatewayapi/testdata/envoyproxy-tracing-strategic-merge-inherited-backendref.in.yaml @@ -0,0 +1,89 @@ +gatewayClass: + apiVersion: gateway.networking.k8s.io/v1 + kind: GatewayClass + metadata: + name: envoy-gateway-class + spec: + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parametersRef: + group: gateway.envoyproxy.io + kind: EnvoyProxy + name: class-proxy + namespace: envoy-gateway-system +envoyProxyForGatewayClass: + apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + namespace: envoy-gateway-system + name: class-proxy + spec: + telemetry: + tracing: + provider: + type: Datadog + backendRefs: + - name: datadog-agent + port: 8126 +envoyProxiesForGateways: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + namespace: envoy-gateway + name: gateway-proxy + spec: + mergeType: StrategicMerge + telemetry: + tracing: + provider: + serviceName: my-custom-service +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + namespace: envoy-gateway + name: gateway-1 + spec: + gatewayClassName: envoy-gateway-class + infrastructure: + parametersRef: + group: gateway.envoyproxy.io + kind: EnvoyProxy + name: gateway-proxy + listeners: + - name: http + protocol: HTTP + port: 80 + allowedRoutes: + namespaces: + from: Same +services: +- apiVersion: v1 + kind: Service + metadata: + name: datadog-agent + namespace: envoy-gateway-system + spec: + clusterIP: 10.11.12.13 + ports: + - name: traces + port: 8126 + protocol: TCP + targetPort: 8126 +endpointSlices: +- apiVersion: discovery.k8s.io/v1 + kind: EndpointSlice + metadata: + name: endpointslice-datadog-agent + namespace: envoy-gateway-system + labels: + kubernetes.io/service-name: datadog-agent + addressType: IPv4 + ports: + - name: traces + protocol: TCP + port: 8126 + endpoints: + - addresses: + - 7.7.7.7 + conditions: + ready: true diff --git a/internal/gatewayapi/testdata/envoyproxy-tracing-strategic-merge-inherited-backendref.out.yaml b/internal/gatewayapi/testdata/envoyproxy-tracing-strategic-merge-inherited-backendref.out.yaml new file mode 100644 index 00000000000..fc1a907c7fa --- /dev/null +++ b/internal/gatewayapi/testdata/envoyproxy-tracing-strategic-merge-inherited-backendref.out.yaml @@ -0,0 +1,268 @@ +envoyProxiesForGateways: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + name: gateway-proxy + namespace: envoy-gateway + spec: + logging: {} + mergeType: StrategicMerge + telemetry: + tracing: + provider: + backendRefs: + - name: datadog-agent + namespace: envoy-gateway-system + port: 8126 + serviceName: my-custom-service + type: Datadog + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: envoy-gateway-class + conditions: + - lastTransitionTime: null + message: EnvoyProxy has been accepted. + reason: Accepted + status: "True" + type: Accepted + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: EnvoyProxy has been accepted. + reason: Accepted + status: "True" + type: Accepted +envoyProxyForGatewayClass: + apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + name: class-proxy + namespace: envoy-gateway-system + spec: + logging: {} + telemetry: + tracing: + provider: + backendRefs: + - name: datadog-agent + port: 8126 + type: Datadog + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: envoy-gateway-class + conditions: + - lastTransitionTime: null + message: EnvoyProxy has been accepted. + reason: Accepted + status: "True" + type: Accepted +gatewayClass: + apiVersion: gateway.networking.k8s.io/v1 + kind: GatewayClass + metadata: + name: envoy-gateway-class + spec: + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parametersRef: + group: gateway.envoyproxy.io + kind: EnvoyProxy + name: class-proxy + namespace: envoy-gateway-system + status: + conditions: + - lastTransitionTime: null + message: Valid GatewayClass + reason: Accepted + status: "True" + type: Accepted +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + name: gateway-1 + namespace: envoy-gateway + spec: + gatewayClassName: envoy-gateway-class + infrastructure: + parametersRef: + group: gateway.envoyproxy.io + kind: EnvoyProxy + name: gateway-proxy + listeners: + - allowedRoutes: + namespaces: + from: Same + name: http + port: 80 + protocol: HTTP + status: + listeners: + - attachedRoutes: 0 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute +infraIR: + envoy-gateway/gateway-1: + proxy: + config: + apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + name: gateway-proxy + namespace: envoy-gateway + spec: + logging: {} + mergeType: StrategicMerge + telemetry: + tracing: + provider: + backendRefs: + - name: datadog-agent + namespace: envoy-gateway-system + port: 8126 + serviceName: my-custom-service + type: Datadog + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: envoy-gateway-class + conditions: + - lastTransitionTime: null + message: EnvoyProxy has been accepted. + reason: Accepted + status: "True" + type: Accepted + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: EnvoyProxy has been accepted. + reason: Accepted + status: "True" + type: Accepted + listeners: + - name: envoy-gateway/gateway-1/http + ports: + - containerPort: 10080 + name: http-80 + protocol: HTTP + servicePort: 80 + metadata: + labels: + gateway.envoyproxy.io/owning-gateway-name: gateway-1 + gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway + ownerReference: + kind: GatewayClass + name: envoy-gateway-class + name: envoy-gateway/gateway-1 + namespace: envoy-gateway-system +xdsIR: + envoy-gateway/gateway-1: + accessLog: + json: + - path: /dev/stdout + globalResources: + proxyServiceCluster: + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + settings: + - addressType: IP + endpoints: + - host: 7.6.5.4 + port: 8080 + zone: zone1 + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + protocol: TCP + http: + - address: 0.0.0.0 + externalPort: 80 + hostnames: + - '*' + metadata: + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: http + name: envoy-gateway/gateway-1/http + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10080 + readyListener: + address: 0.0.0.0 + ipFamily: IPv4 + path: /ready + port: 19003 + tracing: + authority: datadog-agent.envoy-gateway-system.svc + destination: + metadata: + kind: EnvoyProxy + name: gateway-proxy + namespace: envoy-gateway + name: tracing + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8126 + metadata: + kind: Service + name: datadog-agent + namespace: envoy-gateway-system + sectionName: "8126" + name: tracing/backend/-1 + protocol: TCP + provider: + backendRefs: + - name: datadog-agent + namespace: envoy-gateway-system + port: 8126 + port: 4317 + serviceName: my-custom-service + type: Datadog + samplingRate: 100 + serviceName: my-custom-service diff --git a/internal/provider/kubernetes/predicates_test.go b/internal/provider/kubernetes/predicates_test.go index f1f6ec1e082..35f908af204 100644 --- a/internal/provider/kubernetes/predicates_test.go +++ b/internal/provider/kubernetes/predicates_test.go @@ -1380,7 +1380,7 @@ func TestValidateServiceForReconcile(t *testing.T) { }, Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { diff --git a/internal/xds/translator/tracing.go b/internal/xds/translator/tracing.go index 203c82bf781..0269439b9eb 100644 --- a/internal/xds/translator/tracing.go +++ b/internal/xds/translator/tracing.go @@ -44,7 +44,11 @@ func buildHCMTracing(tracing *ir.Tracing) (*hcm.HttpConnectionManager_Tracing, e var providerName string var providerConfig typConfigGen - switch tracing.Provider.Type { + // Defaulted here as well as in the Gateway API translation, so that an IR built + // without an explicit provider type still resolves to the documented default. + providerType := ptr.Deref(tracing.Provider.Type, egv1a1.DefaultTracingProviderType) + + switch providerType { case egv1a1.TracingProviderTypeDatadog: providerName = envoyDatadog @@ -99,7 +103,7 @@ func buildHCMTracing(tracing *ir.Tracing) (*hcm.HttpConnectionManager_Tracing, e return proto.ToAnyWithValidation(config) } default: - return nil, fmt.Errorf("unknown tracing provider type: %s", tracing.Provider.Type) + return nil, fmt.Errorf("unknown tracing provider type: %s", providerType) } ocAny, err := providerConfig() diff --git a/internal/xds/translator/tracing_test.go b/internal/xds/translator/tracing_test.go index 932a8d0263c..383a43b8d3c 100644 --- a/internal/xds/translator/tracing_test.go +++ b/internal/xds/translator/tracing_test.go @@ -34,7 +34,7 @@ func TestBuildHCMTracingSampling(t *testing.T) { Name: "tracing", }, Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), }, }, expectedRandomSampling: 10.0, @@ -50,7 +50,7 @@ func TestBuildHCMTracingSampling(t *testing.T) { Name: "tracing", }, Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), }, }, expectedRandomSampling: 10.0, diff --git a/release-notes/current/bug_fixes/9527-tracing-cel-merge.md b/release-notes/current/bug_fixes/9527-tracing-cel-merge.md new file mode 100644 index 00000000000..fda8c0a98d1 --- /dev/null +++ b/release-notes/current/bug_fixes/9527-tracing-cel-merge.md @@ -0,0 +1 @@ +Fixed CRD validation rejecting a per-Gateway EnvoyProxy that overrides only part of the tracing provider (e.g. `serviceName`) when relying on `mergeType` to inherit the rest. The host/backendRefs completeness check now runs after the GatewayClass-level and Gateway-level configs are merged, and a provider that is still incomplete turns tracing off with a log message instead of blocking admission or the Gateway. The tracing provider `type` and `port` are no longer defaulted by admission either, so a partial override no longer replaces the type or port inherited from the GatewayClass level; both still default to `OpenTelemetry` and `4317` during translation. Telemetry `backendRefs` (tracing provider, accessLog and metrics sinks) inherited from the GatewayClass-level EnvoyProxy that do not set a namespace are now resolved in that EnvoyProxy's namespace instead of the Gateway-level EnvoyProxy's namespace. diff --git a/site/content/en/latest/api/extension_types.md b/site/content/en/latest/api/extension_types.md index 5d5f90e9259..28a76b9563e 100644 --- a/site/content/en/latest/api/extension_types.md +++ b/site/content/en/latest/api/extension_types.md @@ -6687,6 +6687,12 @@ _Appears in:_ TracingProvider defines the tracing provider configuration. +A provider is only required to set host or backendRefs after the +GatewayClass-level and Gateway-level EnvoyProxy configs are merged +(see EnvoyProxySpec.MergeType), so completeness is checked during +translation instead of by a CEL rule here. A provider that is still +incomplete after the merge turns tracing off for that Gateway. + _Appears in:_ - [ProxyTracing](#proxytracing) @@ -6695,9 +6701,9 @@ _Appears in:_ | `backendRef` | _[BackendObjectReference](https://gateway-api.sigs.k8s.io/reference/api-spec/1.5/spec/#backendobjectreference)_ | false | | BackendRef references a Kubernetes object that represents the
backend server to which the authorization request will be sent.
Deprecated: Use BackendRefs instead. | | `backendRefs` | _[BackendRef](#backendref) array_ | false | | BackendRefs references a Kubernetes object that represents the
backend server to which the authorization request will be sent. | | `backendSettings` | _[ClusterSettings](#clustersettings)_ | false | | BackendSettings holds configuration for managing the connection
to the backend. | -| `type` | _[TracingProviderType](#tracingprovidertype)_ | true | OpenTelemetry | Type defines the tracing provider type. | +| `type` | _[TracingProviderType](#tracingprovidertype)_ | false | | Type defines the tracing provider type.
Defaults to OpenTelemetry. The default is applied during translation rather
than by admission, so that a Gateway-level EnvoyProxy overriding only part of
the provider does not replace the type inherited from the GatewayClass level
(see EnvoyProxySpec.MergeType). | | `host` | _string_ | false | | Host define the provider service hostname.
Deprecated: Use BackendRefs instead. | -| `port` | _integer_ | false | 4317 | Port defines the port the provider service is exposed on.
Deprecated: Use BackendRefs instead. | +| `port` | _integer_ | false | | Port defines the port the provider service is exposed on.
Defaults to 4317. The default is applied during translation rather than by
admission, for the same reason as Type.
Deprecated: Use BackendRefs instead. | | `serviceName` | _string_ | false | | ServiceName defines the service name to use in tracing configuration.
If not set, Envoy Gateway will use a default service name set as
"name.namespace" (e.g., "my-gateway.default").
Note: This field is only supported for OpenTelemetry and Datadog tracing providers.
For Zipkin, the service name in traces is always derived from the Envoy --service-cluster flag
(typically "namespace/name" format). Setting this field has no effect for Zipkin. | | `zipkin` | _[ZipkinTracingProvider](#zipkintracingprovider)_ | false | | Zipkin defines the Zipkin tracing provider configuration | | `openTelemetry` | _[OpenTelemetryTracingProvider](#opentelemetrytracingprovider)_ | false | | OpenTelemetry defines the OpenTelemetry tracing provider configuration | diff --git a/test/cel-validation/envoyproxy_test.go b/test/cel-validation/envoyproxy_test.go index 406b0bb518e..508c7e2387d 100644 --- a/test/cel-validation/envoyproxy_test.go +++ b/test/cel-validation/envoyproxy_test.go @@ -1243,7 +1243,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1268,7 +1268,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1292,7 +1292,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1311,19 +1311,22 @@ func TestEnvoyProxyProvider(t *testing.T) { }, }, { - desc: "tracing-empty-backend", + // A partial provider must be accepted at admission so it can be + // completed by the GatewayClass/Gateway EnvoyProxy merge; completeness + // is validated during translation instead. + desc: "tracing-partial-provider-for-merge", mutate: func(envoy *egv1a1.EnvoyProxy) { envoy.Spec = egv1a1.EnvoyProxySpec{ Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), + ServiceName: new("my-override"), }, }, }, } }, - wantErrors: []string{"host or backendRefs needs to be set"}, }, { desc: "valid-tracing-service-name", @@ -1332,7 +1335,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1358,7 +1361,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1679,7 +1682,7 @@ func TestEnvoyProxyProvider(t *testing.T) { }, }, }, - Type: egv1a1.TracingProviderTypeZipkin, + Type: new(egv1a1.TracingProviderTypeZipkin), Zipkin: &egv1a1.ZipkinTracingProvider{}, }, }, @@ -1697,7 +1700,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1725,7 +1728,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1756,7 +1759,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1787,7 +1790,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1821,7 +1824,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1855,7 +1858,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1889,7 +1892,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { @@ -1923,7 +1926,7 @@ func TestEnvoyProxyProvider(t *testing.T) { Telemetry: &egv1a1.ProxyTelemetry{ Tracing: &egv1a1.ProxyTracing{ Provider: egv1a1.TracingProvider{ - Type: egv1a1.TracingProviderTypeOpenTelemetry, + Type: new(egv1a1.TracingProviderTypeOpenTelemetry), BackendCluster: egv1a1.BackendCluster{ BackendRefs: []egv1a1.BackendRef{ { diff --git a/test/helm/gateway-crds-helm/all.out.yaml b/test/helm/gateway-crds-helm/all.out.yaml index 197aec2547f..4c449976d56 100644 --- a/test/helm/gateway-crds-helm/all.out.yaml +++ b/test/helm/gateway-crds-helm/all.out.yaml @@ -53015,10 +53015,12 @@ spec: : true' type: object port: - default: 4317 description: |- Port defines the port the provider service is exposed on. + Defaults to 4317. The default is applied during translation rather than by + admission, for the same reason as Type. + Deprecated: Use BackendRefs instead. format: int32 minimum: 0 @@ -53036,8 +53038,13 @@ spec: - message: serviceName cannot be empty if provided rule: self != "" type: - default: OpenTelemetry - description: Type defines the tracing provider type. + description: |- + Type defines the tracing provider type. + + Defaults to OpenTelemetry. The default is applied during translation rather + than by admission, so that a Gateway-level EnvoyProxy overriding only part of + the provider does not replace the type inherited from the GatewayClass level + (see EnvoyProxySpec.MergeType). enum: - OpenTelemetry - Zipkin @@ -53059,12 +53066,8 @@ spec: id will be used. type: boolean type: object - required: - - type type: object x-kubernetes-validations: - - message: host or backendRefs needs to be set - rule: has(self.host) || self.backendRefs.size() > 0 - message: BackendRefs must be used, backendRef is not supported. rule: '!has(self.backendRef)' - message: BackendRefs only support Service and Backend kind. @@ -53076,8 +53079,8 @@ spec: f.group == "" || f.group == ''gateway.envoyproxy.io'')) : true' - message: openTelemetry can only be used with type OpenTelemetry - rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry'' - : true' + rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type + == ''OpenTelemetry'') : true' samplingFraction: description: |- SamplingFraction represents the fraction of requests that should be diff --git a/test/helm/gateway-crds-helm/e2e.out.yaml b/test/helm/gateway-crds-helm/e2e.out.yaml index 32f54c37669..04bf8e27a6d 100644 --- a/test/helm/gateway-crds-helm/e2e.out.yaml +++ b/test/helm/gateway-crds-helm/e2e.out.yaml @@ -28953,10 +28953,12 @@ spec: : true' type: object port: - default: 4317 description: |- Port defines the port the provider service is exposed on. + Defaults to 4317. The default is applied during translation rather than by + admission, for the same reason as Type. + Deprecated: Use BackendRefs instead. format: int32 minimum: 0 @@ -28974,8 +28976,13 @@ spec: - message: serviceName cannot be empty if provided rule: self != "" type: - default: OpenTelemetry - description: Type defines the tracing provider type. + description: |- + Type defines the tracing provider type. + + Defaults to OpenTelemetry. The default is applied during translation rather + than by admission, so that a Gateway-level EnvoyProxy overriding only part of + the provider does not replace the type inherited from the GatewayClass level + (see EnvoyProxySpec.MergeType). enum: - OpenTelemetry - Zipkin @@ -28997,12 +29004,8 @@ spec: id will be used. type: boolean type: object - required: - - type type: object x-kubernetes-validations: - - message: host or backendRefs needs to be set - rule: has(self.host) || self.backendRefs.size() > 0 - message: BackendRefs must be used, backendRef is not supported. rule: '!has(self.backendRef)' - message: BackendRefs only support Service and Backend kind. @@ -29014,8 +29017,8 @@ spec: f.group == "" || f.group == ''gateway.envoyproxy.io'')) : true' - message: openTelemetry can only be used with type OpenTelemetry - rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry'' - : true' + rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type + == ''OpenTelemetry'') : true' samplingFraction: description: |- SamplingFraction represents the fraction of requests that should be diff --git a/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml b/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml index db1da4ef9c1..2fc8d004861 100644 --- a/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml +++ b/test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml @@ -28953,10 +28953,12 @@ spec: : true' type: object port: - default: 4317 description: |- Port defines the port the provider service is exposed on. + Defaults to 4317. The default is applied during translation rather than by + admission, for the same reason as Type. + Deprecated: Use BackendRefs instead. format: int32 minimum: 0 @@ -28974,8 +28976,13 @@ spec: - message: serviceName cannot be empty if provided rule: self != "" type: - default: OpenTelemetry - description: Type defines the tracing provider type. + description: |- + Type defines the tracing provider type. + + Defaults to OpenTelemetry. The default is applied during translation rather + than by admission, so that a Gateway-level EnvoyProxy overriding only part of + the provider does not replace the type inherited from the GatewayClass level + (see EnvoyProxySpec.MergeType). enum: - OpenTelemetry - Zipkin @@ -28997,12 +29004,8 @@ spec: id will be used. type: boolean type: object - required: - - type type: object x-kubernetes-validations: - - message: host or backendRefs needs to be set - rule: has(self.host) || self.backendRefs.size() > 0 - message: BackendRefs must be used, backendRef is not supported. rule: '!has(self.backendRef)' - message: BackendRefs only support Service and Backend kind. @@ -29014,8 +29017,8 @@ spec: f.group == "" || f.group == ''gateway.envoyproxy.io'')) : true' - message: openTelemetry can only be used with type OpenTelemetry - rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry'' - : true' + rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type + == ''OpenTelemetry'') : true' samplingFraction: description: |- SamplingFraction represents the fraction of requests that should be