fix: validate tracing provider completeness after EnvoyProxy merge instead of at admission - #9641
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9641 +/- ##
==========================================
- Coverage 81.31% 81.31% -0.01%
==========================================
Files 263 263
Lines 40870 40911 +41
==========================================
+ Hits 33233 33265 +32
- Misses 7637 7646 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
| // Validated 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. | ||
| if tracing.Provider.Host == nil { |
There was a problem hiding this comment.
will this block the whole fleet provisioning, can we add a warning message instead of blocking it?
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ded8cc6f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 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 validated during | ||
| // translation instead of by a CEL rule here. |
There was a problem hiding this comment.
With this admission relaxation, a Gateway EnvoyProxy can now set only provider.serviceName and rely on merge to inherit the rest, but provider.type is still defaulted to OpenTelemetry before the EnvoyProxy merge. If the GatewayClass provider is Datadog or Zipkin and the Gateway override omits type, the stored override carries type: OpenTelemetry, so the merge changes the provider kind instead of only overriding serviceName; xDS generation then builds the wrong tracing provider. The merge path needs to distinguish an omitted/defaulted type from an intentional override, or this partial-provider path should not be allowed for inherited non-OpenTelemetry providers.
Useful? React with 👍 / 👎.
| // Validated 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. |
There was a problem hiding this comment.
This new partial-provider path is documented as being completed by the GatewayClass/Gateway EnvoyProxy merge, but inherited backendRefs with no explicit namespace are later resolved relative to the merged EnvoyProxy's namespace. When a Gateway override now sets only serviceName and inherits a GatewayClass provider whose backendRef omits namespace, the merged object has the Gateway-level EnvoyProxy namespace, so the collector is looked up in the Gateway namespace and the Gateway is marked invalid even though the class-level reference was valid. Preserve the source EnvoyProxy namespace for inherited refs or rewrite/default namespaces during merge.
Useful? React with 👍 / 👎.
4ded8cc to
1e85b90
Compare
|
Rebased onto current main, no content change. The red checks on the previous run were not from this PR. @zirain this is ready whenever you have time. |
|
@zirain switched to a warning — an incomplete provider now logs and skips tracing for that Gateway instead of setting Codex's two findings are both real. I reproduced them with the exact config from #9527: class proxy with
Do you want the |
|
/retest |
14f4852 to
ac71f9f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac71f9fdf3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 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. |
There was a problem hiding this comment.
When a Gateway override supplies only serviceName, admission also materializes the provider's port default of 4317, so both StrategicMerge and JSONMerge treat that value as an explicit override. Consequently, a GatewayClass OpenTelemetry provider using host with a custom port such as 4318 is completed by the merge but silently redirected to port 4317. The partial-provider path needs to distinguish the defaulted port from an intentional override, just as it must for other defaulted provider fields.
Useful? React with 👍 / 👎.
|
Went ahead with the Both are optional now and default during translation instead ( The backendRef-namespace finding is still not addressed here — it applies to accessLog and metrics backendRefs too, so it still looks like a separate PR to me. @zirain if you would rather see the |
|
is it possible fix all merge issue in one PR? if so, prefer to see them happen here. |
|
@zirain sorry for the slow reply. Yes, I will fix the backendRef namespace issue here as well. Plan: before the merge, stamp the GatewayClass EnvoyProxy's namespace onto its telemetry backendRefs that do not set one (tracing, accessLog and metrics sinks), so inherited refs keep resolving where they were defined. Will rebase onto main and push shortly. |
f0fff20 to
82ba8b8
Compare
|
Drop the 'host or backendRefs needs to be set' CEL rule from TracingProvider so a per-Gateway EnvoyProxy can override a single field (e.g. serviceName) and inherit the rest via mergeType. The completeness check now runs in processTracing after the GatewayClass-level and Gateway-level configs are merged, surfacing an InvalidParameters Gateway condition instead of an admission error. Fixes envoyproxy#9527 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
An incomplete tracing provider now turns tracing off for that Gateway instead of setting Accepted=False, so an observability misconfiguration does not stop the proxy from being provisioned. Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
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> Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
main now rejects ptr.To through forbidigo in favour of the new(expr) builtin, so the tracing provider defaults and their tests follow suit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbWe5JJNQYR3HvVFAPheNP Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
…re defined in The merged EnvoyProxy carries the Gateway-level object's metadata, so a backendRef inherited from the GatewayClass-level EnvoyProxy without an explicit namespace was looked up in the Gateway's namespace, and the Gateway was marked invalid even though the class-level reference was valid. This affected the tracing provider as well as the accessLog and metrics sinks. Before merging, every telemetry backendRef of the base EnvoyProxy that omits a namespace is now set to the base's own namespace, which is where it resolves when that EnvoyProxy is used on its own. Refs written on the Gateway-level EnvoyProxy are untouched and keep resolving in the Gateway's namespace. The shared GatewayClass object is not modified; a copy is merged instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DbWe5JJNQYR3HvVFAPheNP Signed-off-by: Kadir Can Yildirim <252162627+kadircanyildirm-crypto@users.noreply.github.com>
82ba8b8 to
6fdac97
Compare
| // +kubebuilder:default=OpenTelemetry | ||
| Type TracingProviderType `json:"type"` | ||
| // +optional | ||
| Type *TracingProviderType `json:"type,omitempty"` |
There was a problem hiding this comment.
let's do this first in a seperated PR, WDYT?
4 participants
What this PR does
Fixes the three merge problems that stop a per-Gateway EnvoyProxy from overriding a single tracing field (e.g.
serviceName) and inheriting the rest viamergeType:host or backendRefs needs to be setCEL rule is removed fromTracingProvider. Completeness is checked inprocessTracingafter the GatewayClass-level and Gateway-level configs are merged. A provider that is still incomplete logs and skips tracing for that Gateway instead of blocking admission or provisioning.typeandportare no longer defaulted by admission, so a partial override no longer replaces the type or port inherited from the GatewayClass level. Both still default toOpenTelemetry/4317during translation.backendRefs(tracing provider, accessLog and metrics sinks) inherited from the GatewayClass-level EnvoyProxy that omit a namespace are pinned to that EnvoyProxy's namespace before the merge, so they resolve where they were defined instead of in the Gateway's namespace.Follows the approach suggested by @zirain in the issue.
Fixes #9527
Notes
🤖 Generated with Claude Code