feat: Reconcile BackendRefs on EnvoyProxy #3190
Conversation
Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
davidalger
force-pushed
the
algerdev/feat-ep-backendref-reconcile
branch
from
2899491
to
da8d2d3
Compare
…'t be needed Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
…hat wont't be needed" Signed-off-by: David Alger <davidmalger@gmail.com>
Signed-off-by: David Alger <davidmalger@gmail.com>
davidalger
force-pushed
the
algerdev/feat-ep-backendref-reconcile
branch
from
76a4700
to
8491367
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3190 +/- ##
==========================================
+ Coverage 66.51% 66.65% +0.14%
==========================================
Files 161 162 +1
Lines 22673 22962 +289
==========================================
+ Hits 15080 15306 +226
- Misses 6720 6781 +61
- Partials 873 875 +2 ☔ View full report in Codecov by Sentry. |
|
/retest |
| // TODO (davidalger) Handle case where Service referenced by backendRef doesn't exist | ||
| serviceNamespace := NamespaceDerefOr(backendRef.Namespace, envoyproxy.Namespace) | ||
| service := resources.GetService(serviceNamespace, string(backendRef.Name)) | ||
| for _, port := range service.Spec.Ports { |
There was a problem hiding this comment.
This will panic if the backendRef refers to a Service which cannot be found. Ideally this would surface as a message on the status somewhere, similar to how it does on XRoutes.
Status isn't currently being used on EnvoyProxy as far as I can tell. It seems it could make sense to put errors such as this on the Gateway rather than the EnvoyProxy since it's something which will arise when processing the Gateway.
@arkodg @zirain Do either of you have thoughts on where errors such as this should be surfaced?
There was a problem hiding this comment.
ah
- any EnvoyProxy errors are surfaced in the
GatewayClass, lets tackle this separately, and raise a issue to track this because moving it toAccepted=Falsewill break everything in the next reconciliation :) - For now lets stick a Direct Response that returns
500similar togateway/internal/gatewayapi/route.go
Line 492 in 2c4d385
| @@ -141,6 +141,7 @@ func GetRenderedBootstrapConfig(opts *RenderBootsrapConfigOptions) (string, erro | |||
| StatsMatcher StatsMatcherParameters | |||
| ) | |||
|
|
|||
| // TODO (davidalger) push these through XDS? | |||
There was a problem hiding this comment.
@zirain @arkodg Reconciling BackendRefs on EnvoyProxy for OTel Tracing and Access Logs is working, although there is a little more work to do to polish things up. OTel metrics is outstanding and I wanted to get thoughts on it before proceeding.
Since stats_sinks is used to configure metrics statically in the bootstrap, reconciling the clusters and feeding endpoint IPs in via XDS is probably not ideal as it would prevent metrics from being captured in the event Envoy fails to fetch XDS config at startup. My thinking is it may be ideal to keep this one as-is, preferring the current DNS discovery using an FQDN derived from the backendRef. If we agree on this, I'll remove reconciling backendRefs for metrics and leave this one as-is.
There was a problem hiding this comment.
@zirain is there a way to dynamically setup metrics cluster via xds ?
# Conflicts: # internal/gatewayapi/listener.go # internal/gatewayapi/listener_test.go # internal/xds/translator/accesslog.go # internal/xds/translator/tracing.go
Signed-off-by: David Alger <davidmalger@gmail.com>
|
hey @davidalger @zirain, just realized that there's a massive overlap with this PR and #3293 , is this anything from this PR that is still required ? |
What this PR does / why we need it:
This adds support for BackendRefs on EnvoyProxies to the reconciler so the same endpoint discovery used for HTTPRoute refs will be used rather than DNS based discovery using service FQDNs.