-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix tproxy listeners with discovery chains. #14751
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good start! Though there is a gotcha here because there are assumptions in the code that discovery chain watches for tproxy only get managed in one place.
I left a comment below explaining.
9f68840
to
0aaf903
Compare
0aaf903
to
58873e4
Compare
if !upstream.CentrallyConfigured { | ||
seenUpstreams[uid] = struct{}{} | ||
} | ||
} | ||
|
||
// Clean up data from services that were not in the update | ||
for uid, targets := range snap.ConnectProxy.WatchedUpstreams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if instead we modify the check for cross-datacenter upstreams to instead check !CentrallyConfigured
? That way we avoid allocating a new map. We're already checking UpstreamConfig in the reconciliation.
if upstream, ok := snap.ConnectProxy.UpstreamConfig[uid]; ok && upstream.Datacenter != "" && upstream.Datacenter != s.source.Datacenter {
continue
}
Could become:
if upstream, ok := snap.ConnectProxy.UpstreamConfig[uid]; ok && !upstream.CentrallyConfigured {
continue
}
This works because a cross-DC watch can only be done explicitly, and not via service-defaults
/CentrallyConfigured
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Applies through L549))
a88b322
to
5bf1075
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Transparent proxy does not spawn outbound listeners for explicit upstreams to service-resolvers. When upstreams are explicitly defined, a corresponding listener should be spawned.
Testing & Reproduction steps
Create a service with the following fields configured:
Create a service resolver that will be used as the upstream.
Check the Envoy admin config dump and observe that no listeners are spawned.
PR Checklist