Skip to content

Commit f0fff20

Browse files
kadircanyildirm-cryptoclaude
kadircanyildirm-crypto
and
claude
committed
fix: stop defaulting the tracing provider type and port at admission
A Gateway-level EnvoyProxy that overrides only part of the tracing provider still had `type` and `port` materialized by admission, because both carried a kubebuilder default. Once those values are stored they are indistinguishable from an explicit override, so StrategicMerge and JSONMerge replaced the type and port inherited from the GatewayClass level: a Datadog provider became OpenTelemetry, and an OpenTelemetry provider on a custom port such as 4318 was redirected to 4317. Both fields are now optional, and the defaults are applied during translation instead, where the merge has already happened. The defaults themselves are unchanged (OpenTelemetry and 4317) and live next to the type as DefaultTracingProviderType and DefaultTracingProviderPort. The openTelemetry CEL rule is relaxed to tolerate an absent type, and the xDS translator resolves the default again so an IR built without an explicit provider type keeps working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent ac71f9f commit f0fff20

23 files changed

Lines changed: 234 additions & 77 deletions

‎api/v1alpha1/envoyproxy_tracing_types.go‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,15 @@ const (
3434
TracingProviderTypeDatadog TracingProviderType = "Datadog"
3535
)
3636

37+
const (
38+
// DefaultTracingProviderType is the provider type applied during translation
39+
// when TracingProvider.Type is unset.
40+
DefaultTracingProviderType = TracingProviderTypeOpenTelemetry
41+
// DefaultTracingProviderPort is the provider port applied during translation
42+
// when TracingProvider.Port is unset.
43+
DefaultTracingProviderPort int32 = 4317
44+
)
45+
3746
// TracingProvider defines the tracing provider configuration.
3847
//
3948
// A provider is only required to set host or backendRefs after the
@@ -45,13 +54,19 @@ const (
4554
// +kubebuilder:validation:XValidation:message="BackendRefs must be used, backendRef is not supported.",rule="!has(self.backendRef)"
4655
// +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"
4756
// +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"
48-
// +kubebuilder:validation:XValidation:message="openTelemetry can only be used with type OpenTelemetry",rule="has(self.openTelemetry) ? self.type == 'OpenTelemetry' : true"
57+
// +kubebuilder:validation:XValidation:message="openTelemetry can only be used with type OpenTelemetry",rule="has(self.openTelemetry) ? (!has(self.type) || self.type == 'OpenTelemetry') : true"
4958
type TracingProvider struct {
5059
BackendCluster `json:",inline"`
5160
// Type defines the tracing provider type.
61+
//
62+
// Defaults to OpenTelemetry. The default is applied during translation rather
63+
// than by admission, so that a Gateway-level EnvoyProxy overriding only part of
64+
// the provider does not replace the type inherited from the GatewayClass level
65+
// (see EnvoyProxySpec.MergeType).
66+
//
5267
// +kubebuilder:validation:Enum=OpenTelemetry;Zipkin;Datadog
53-
// +kubebuilder:default=OpenTelemetry
54-
Type TracingProviderType `json:"type"`
68+
// +optional
69+
Type *TracingProviderType `json:"type,omitempty"`
5570
// Host define the provider service hostname.
5671
//
5772
// Deprecated: Use BackendRefs instead.
@@ -60,12 +75,14 @@ type TracingProvider struct {
6075
Host *string `json:"host,omitempty"`
6176
// Port defines the port the provider service is exposed on.
6277
//
78+
// Defaults to 4317. The default is applied during translation rather than by
79+
// admission, for the same reason as Type.
80+
//
6381
// Deprecated: Use BackendRefs instead.
6482
//
6583
// +optional
6684
// +kubebuilder:validation:Minimum=0
67-
// +kubebuilder:default=4317
68-
Port int32 `json:"port,omitempty"`
85+
Port *int32 `json:"port,omitempty"`
6986
// ServiceName defines the service name to use in tracing configuration.
7087
// If not set, Envoy Gateway will use a default service name set as
7188
// "name.namespace" (e.g., "my-gateway.default").

‎api/v1alpha1/zz_generated.deepcopy.go‎

Lines changed: 10 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_envoyproxies.yaml‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18644,10 +18644,12 @@ spec:
1864418644
: true'
1864518645
type: object
1864618646
port:
18647-
default: 4317
1864818647
description: |-
1864918648
Port defines the port the provider service is exposed on.
1865018649

18650+
Defaults to 4317. The default is applied during translation rather than by
18651+
admission, for the same reason as Type.
18652+
1865118653
Deprecated: Use BackendRefs instead.
1865218654
format: int32
1865318655
minimum: 0
@@ -18665,8 +18667,13 @@ spec:
1866518667
- message: serviceName cannot be empty if provided
1866618668
rule: self != ""
1866718669
type:
18668-
default: OpenTelemetry
18669-
description: Type defines the tracing provider type.
18670+
description: |-
18671+
Type defines the tracing provider type.
18672+
18673+
Defaults to OpenTelemetry. The default is applied during translation rather
18674+
than by admission, so that a Gateway-level EnvoyProxy overriding only part of
18675+
the provider does not replace the type inherited from the GatewayClass level
18676+
(see EnvoyProxySpec.MergeType).
1867018677
enum:
1867118678
- OpenTelemetry
1867218679
- Zipkin
@@ -18688,8 +18695,6 @@ spec:
1868818695
id will be used.
1868918696
type: boolean
1869018697
type: object
18691-
required:
18692-
- type
1869318698
type: object
1869418699
x-kubernetes-validations:
1869518700
- message: BackendRefs must be used, backendRef is not supported.
@@ -18703,8 +18708,8 @@ spec:
1870318708
f.group == "" || f.group == ''gateway.envoyproxy.io''))
1870418709
: true'
1870518710
- message: openTelemetry can only be used with type OpenTelemetry
18706-
rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry''
18707-
: true'
18711+
rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type
18712+
== ''OpenTelemetry'') : true'
1870818713
samplingFraction:
1870918714
description: |-
1871018715
SamplingFraction represents the fraction of requests that should be

‎charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18643,10 +18643,12 @@ spec:
1864318643
: true'
1864418644
type: object
1864518645
port:
18646-
default: 4317
1864718646
description: |-
1864818647
Port defines the port the provider service is exposed on.
1864918648

18649+
Defaults to 4317. The default is applied during translation rather than by
18650+
admission, for the same reason as Type.
18651+
1865018652
Deprecated: Use BackendRefs instead.
1865118653
format: int32
1865218654
minimum: 0
@@ -18664,8 +18666,13 @@ spec:
1866418666
- message: serviceName cannot be empty if provided
1866518667
rule: self != ""
1866618668
type:
18667-
default: OpenTelemetry
18668-
description: Type defines the tracing provider type.
18669+
description: |-
18670+
Type defines the tracing provider type.
18671+
18672+
Defaults to OpenTelemetry. The default is applied during translation rather
18673+
than by admission, so that a Gateway-level EnvoyProxy overriding only part of
18674+
the provider does not replace the type inherited from the GatewayClass level
18675+
(see EnvoyProxySpec.MergeType).
1866918676
enum:
1867018677
- OpenTelemetry
1867118678
- Zipkin
@@ -18687,8 +18694,6 @@ spec:
1868718694
id will be used.
1868818695
type: boolean
1868918696
type: object
18690-
required:
18691-
- type
1869218697
type: object
1869318698
x-kubernetes-validations:
1869418699
- message: BackendRefs must be used, backendRef is not supported.
@@ -18702,8 +18707,8 @@ spec:
1870218707
f.group == "" || f.group == ''gateway.envoyproxy.io''))
1870318708
: true'
1870418709
- message: openTelemetry can only be used with type OpenTelemetry
18705-
rule: 'has(self.openTelemetry) ? self.type == ''OpenTelemetry''
18706-
: true'
18710+
rule: 'has(self.openTelemetry) ? (!has(self.type) || self.type
18711+
== ''OpenTelemetry'') : true'
1870718712
samplingFraction:
1870818713
description: |-
1870918714
SamplingFraction represents the fraction of requests that should be

‎internal/gatewayapi/listener.go‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,14 +1036,27 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En
10361036
}
10371037
tracing := envoyproxy.Spec.Telemetry.Tracing
10381038

1039+
// Type and Port are defaulted here rather than by admission so that a
1040+
// Gateway-level EnvoyProxy overriding only part of the provider does not
1041+
// replace the values inherited from the GatewayClass level: a defaulted field
1042+
// is indistinguishable from an explicit one once the merge patch is built.
1043+
// The copy keeps the EnvoyProxy resource itself untouched.
1044+
provider := tracing.Provider
1045+
if provider.Type == nil {
1046+
provider.Type = ptr.To(egv1a1.DefaultTracingProviderType)
1047+
}
1048+
if provider.Port == nil {
1049+
provider.Port = ptr.To(egv1a1.DefaultTracingProviderPort)
1050+
}
1051+
10391052
// TODO: rename this, so that we can share backend with accesslog?
10401053
destName := "tracing"
10411054
settingName := irDestinationSettingName(destName, -1)
1042-
ds, traffic, err := t.processBackendRefsForTelemetry(settingName, tracing.Provider.BackendCluster, envoyproxy.Namespace, resources, envoyproxy, gwCtx)
1055+
ds, traffic, err := t.processBackendRefsForTelemetry(settingName, provider.BackendCluster, envoyproxy.Namespace, resources, envoyproxy, gwCtx)
10431056
if err != nil {
10441057
return nil, err
10451058
}
1046-
if tracing.Provider.Type == egv1a1.TracingProviderTypeOpenTelemetry {
1059+
if *provider.Type == egv1a1.TracingProviderTypeOpenTelemetry {
10471060
// TODO: update when OTLP/HTTP is completely supported (logs, traces, metrics)
10481061
for _, d := range ds {
10491062
d.Protocol = ir.GRPC
@@ -1060,13 +1073,13 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En
10601073
// Gateway-level EnvoyProxy merge before the check runs. An incomplete
10611074
// provider only turns tracing off, it does not stop the Gateway from
10621075
// being provisioned.
1063-
if tracing.Provider.Host == nil {
1076+
if provider.Host == nil {
10641077
t.Logger.Info("Disabling tracing because the merged tracing provider sets neither host nor backendRefs",
10651078
"gateway", utils.NamespacedName(gwCtx.Gateway).String(),
10661079
"envoyProxy", utils.NamespacedName(envoyproxy).String())
10671080
return nil, nil
10681081
}
1069-
host, port := *tracing.Provider.Host, uint32(tracing.Provider.Port)
1082+
host, port := *provider.Host, uint32(*provider.Port)
10701083
ds = destinationSettingFromHostAndPort(settingName, host, port)
10711084
authority = host
10721085
}
@@ -1078,8 +1091,8 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En
10781091
}
10791092

10801093
// Use configured service name if provided
1081-
if tracing.Provider.ServiceName != nil {
1082-
serviceName = *tracing.Provider.ServiceName
1094+
if provider.ServiceName != nil {
1095+
serviceName = *provider.ServiceName
10831096
}
10841097

10851098
return &ir.Tracing{
@@ -1090,15 +1103,15 @@ func (t *Translator) processTracing(gwCtx *GatewayContext, envoyproxy *egv1a1.En
10901103
OverallSamplingRate: proxySamplingFractionPtr(tracing.OverallSamplingFraction),
10911104
CustomTags: ir.CustomTagMapToSlice(tracing.CustomTags),
10921105
Tags: ir.MapToSlice(tracing.Tags),
1093-
ResourceAttributes: ir.MapToSlice(getOpenTelemetryTracingResourceAttributes(&tracing.Provider)),
1106+
ResourceAttributes: ir.MapToSlice(getOpenTelemetryTracingResourceAttributes(&provider)),
10941107
Destination: ir.RouteDestination{
10951108
Name: destName,
10961109
Settings: ds,
10971110
Metadata: buildResourceMetadata(envoyproxy, nil),
10981111
},
1099-
Provider: tracing.Provider,
1112+
Provider: provider,
11001113
Traffic: traffic,
1101-
Headers: getOpenTelemetryTracingHeaders(&tracing.Provider),
1114+
Headers: getOpenTelemetryTracingHeaders(&provider),
11021115
SpanName: tracing.SpanName,
11031116
}, nil
11041117
}

‎internal/gatewayapi/listener_test.go‎

Lines changed: 82 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -950,7 +950,7 @@ func TestProcessTracingServiceName(t *testing.T) {
950950
Telemetry: &egv1a1.ProxyTelemetry{
951951
Tracing: &egv1a1.ProxyTracing{
952952
Provider: egv1a1.TracingProvider{
953-
Type: egv1a1.TracingProviderTypeOpenTelemetry,
953+
Type: ptr.To(egv1a1.TracingProviderTypeOpenTelemetry),
954954
BackendCluster: egv1a1.BackendCluster{
955955
BackendRefs: []egv1a1.BackendRef{
956956
{
@@ -986,7 +986,7 @@ func TestProcessTracingServiceName(t *testing.T) {
986986
Telemetry: &egv1a1.ProxyTelemetry{
987987
Tracing: &egv1a1.ProxyTracing{
988988
Provider: egv1a1.TracingProvider{
989-
Type: egv1a1.TracingProviderTypeOpenTelemetry,
989+
Type: ptr.To(egv1a1.TracingProviderTypeOpenTelemetry),
990990
BackendCluster: egv1a1.BackendCluster{
991991
BackendRefs: []egv1a1.BackendRef{
992992
{
@@ -1026,7 +1026,7 @@ func TestProcessTracingServiceName(t *testing.T) {
10261026
Telemetry: &egv1a1.ProxyTelemetry{
10271027
Tracing: &egv1a1.ProxyTracing{
10281028
Provider: egv1a1.TracingProvider{
1029-
Type: egv1a1.TracingProviderTypeOpenTelemetry,
1029+
Type: ptr.To(egv1a1.TracingProviderTypeOpenTelemetry),
10301030
BackendCluster: egv1a1.BackendCluster{
10311031
BackendRefs: []egv1a1.BackendRef{
10321032
{
@@ -1067,7 +1067,7 @@ func TestProcessTracingServiceName(t *testing.T) {
10671067
Telemetry: &egv1a1.ProxyTelemetry{
10681068
Tracing: &egv1a1.ProxyTracing{
10691069
Provider: egv1a1.TracingProvider{
1070-
Type: egv1a1.TracingProviderTypeOpenTelemetry,
1070+
Type: ptr.To(egv1a1.TracingProviderTypeOpenTelemetry),
10711071
BackendCluster: egv1a1.BackendCluster{
10721072
BackendRefs: []egv1a1.BackendRef{
10731073
{
@@ -1104,7 +1104,7 @@ func TestProcessTracingServiceName(t *testing.T) {
11041104
Telemetry: &egv1a1.ProxyTelemetry{
11051105
Tracing: &egv1a1.ProxyTracing{
11061106
Provider: egv1a1.TracingProvider{
1107-
Type: egv1a1.TracingProviderTypeOpenTelemetry,
1107+
Type: ptr.To(egv1a1.TracingProviderTypeOpenTelemetry),
11081108
ServiceName: new("only-name-overridden"),
11091109
},
11101110
},
@@ -1190,6 +1190,83 @@ func TestProcessTracingServiceName(t *testing.T) {
11901190
}
11911191
}
11921192

1193+
// TestProcessTracingProviderDefaults guards the defaults that used to be applied
1194+
// by admission. Applying them there made a partial Gateway-level override
1195+
// indistinguishable from an explicit one, so an inherited type or port was
1196+
// silently replaced by the defaulted value during the EnvoyProxy merge.
1197+
func TestProcessTracingProviderDefaults(t *testing.T) {
1198+
cases := []struct {
1199+
name string
1200+
provider egv1a1.TracingProvider
1201+
expectedType egv1a1.TracingProviderType
1202+
expectedPort uint32
1203+
}{
1204+
{
1205+
name: "unset type and port fall back to the documented defaults",
1206+
provider: egv1a1.TracingProvider{
1207+
Host: ptr.To("otel-collector.monitoring.svc.cluster.local"),
1208+
},
1209+
expectedType: egv1a1.TracingProviderTypeOpenTelemetry,
1210+
expectedPort: 4317,
1211+
},
1212+
{
1213+
name: "type and port inherited from the GatewayClass level are kept",
1214+
provider: egv1a1.TracingProvider{
1215+
Host: ptr.To("datadog-agent.monitoring.svc.cluster.local"),
1216+
Type: ptr.To(egv1a1.TracingProviderTypeDatadog),
1217+
Port: ptr.To(int32(8126)),
1218+
},
1219+
expectedType: egv1a1.TracingProviderTypeDatadog,
1220+
expectedPort: 8126,
1221+
},
1222+
}
1223+
1224+
for _, tc := range cases {
1225+
t.Run(tc.name, func(t *testing.T) {
1226+
translator := &Translator{}
1227+
envoyProxy := &egv1a1.EnvoyProxy{
1228+
ObjectMeta: metav1.ObjectMeta{
1229+
Name: "test-proxy",
1230+
Namespace: "test-namespace",
1231+
},
1232+
Spec: egv1a1.EnvoyProxySpec{
1233+
Telemetry: &egv1a1.ProxyTelemetry{
1234+
Tracing: &egv1a1.ProxyTracing{
1235+
Provider: tc.provider,
1236+
},
1237+
},
1238+
},
1239+
}
1240+
1241+
result, err := translator.processTracing(&GatewayContext{
1242+
Gateway: &gwapiv1.Gateway{
1243+
ObjectMeta: metav1.ObjectMeta{
1244+
Name: "test-gateway",
1245+
Namespace: "test-namespace",
1246+
},
1247+
},
1248+
}, envoyProxy, false, &resource.Resources{})
1249+
require.NoError(t, err)
1250+
require.NotNil(t, result)
1251+
1252+
require.NotNil(t, result.Provider.Type)
1253+
assert.Equal(t, tc.expectedType, *result.Provider.Type)
1254+
require.NotNil(t, result.Provider.Port)
1255+
assert.Equal(t, tc.expectedPort, uint32(*result.Provider.Port))
1256+
1257+
// The host/port fallback destination has to agree with the provider port.
1258+
require.Len(t, result.Destination.Settings, 1)
1259+
require.Len(t, result.Destination.Settings[0].Endpoints, 1)
1260+
assert.Equal(t, tc.expectedPort, result.Destination.Settings[0].Endpoints[0].Port)
1261+
1262+
// Defaulting works on a copy, so the EnvoyProxy resource keeps whatever
1263+
// the user actually set.
1264+
assert.Equal(t, tc.provider.Type, envoyProxy.Spec.Telemetry.Tracing.Provider.Type)
1265+
assert.Equal(t, tc.provider.Port, envoyProxy.Spec.Telemetry.Tracing.Provider.Port)
1266+
})
1267+
}
1268+
}
1269+
11931270
func TestProcessAccessLog(t *testing.T) {
11941271
tests := []struct {
11951272
name string

‎internal/gatewayapi/testdata/envoyproxy-otel-backend-custom-ca.out.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ xdsIR:
264264
kind: Backend
265265
name: otel-collector
266266
namespace: envoy-gateway
267+
port: 4317
267268
type: OpenTelemetry
268269
samplingRate: 100
269270
serviceName: gateway-1.envoy-gateway

‎internal/gatewayapi/testdata/envoyproxy-otel-backend-tls-per-resource-secret.out.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ xdsIR:
261261
kind: Backend
262262
name: otel-collector
263263
namespace: envoy-gateway
264+
port: 4317
264265
type: OpenTelemetry
265266
samplingRate: 100
266267
serviceName: gateway-1.envoy-gateway

0 commit comments

Comments
 (0)