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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9641 +/- ##
==========================================
+ Coverage 76.04% 76.05% +0.01%
==========================================
Files 259 260 +1
Lines 43258 43366 +108
==========================================
+ Hits 32896 32984 +88
- Misses 8174 8182 +8
- Partials 2188 2200 +12 ☔ 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 👍 / 👎.
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>
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. |
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>
|
@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 |
2 participants
What this PR does
Removes the
host or backendRefs needs to be setCEL rule fromTracingProviderand enforces completeness inprocessTracinginstead, after the GatewayClass-level and Gateway-level EnvoyProxy configs are merged. An incomplete merged provider now surfaces as anInvalidParametersGateway condition rather than an admission failure, so a per-Gateway EnvoyProxy can override a single field (e.g.serviceName) and inherit the rest viamergeType: StrategicMerge.Follows the approach suggested by @zirain in the issue.
Fixes #9527
Notes
🤖 Generated with Claude Code