-
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
Update proxycfg state management and xDS generation for transparent proxy #9894
Conversation
By accepting a name the function can be used for other inbound listeners, like the one for TransparentProxy.
} | ||
|
||
if chain == nil || chain.IsDefault() { | ||
// TODO(rb): make this do the old school stuff too |
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 old stuff was this?
if u.CentrallyConfigured { | ||
continue | ||
} | ||
snap.ConnectProxy.UpstreamConfig[u.Identifier()] = &u |
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.
Note you are taking the address of the loop variable here, not the spot in the slice.
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 now you're taking the address of a copy of the slice item.
I think the fix would be = &s.proxyCfg.Upstreams[i]
@@ -54,35 +58,225 @@ func (s *Server) listenersFromSnapshot(cInfo connectionInfo, cfgSnap *proxycfg.C | |||
|
|||
// listenersFromSnapshotConnectProxy returns the "listeners" for a connect proxy service | |||
func (s *Server) listenersFromSnapshotConnectProxy(cInfo connectionInfo, cfgSnap *proxycfg.ConfigSnapshot) ([]proto.Message, error) { | |||
// One listener for each upstream plus the public one | |||
resources := make([]proto.Message, len(cfgSnap.Proxy.Upstreams)+1) | |||
resources := make([]proto.Message, 1) |
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.
Is there a better guesstimate that we can use here now?
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.
There isn't a great one. If in TProxy mode there might only be 2 listeners, since it relies on per-upstream filter chains rather than per-upstream listeners. Outside of TProxy mode it could be the size of the DiscoverChain map.
Not sure if it's worth doing having a variable starting capacity based on whether TProxy is on or not.
cfgSnap *proxycfg.ConfigSnapshot, | ||
tlsContext *envoy_tls_v3.DownstreamTlsContext, | ||
) (*envoy_listener_v3.FilterChain, error) { | ||
// TODO (freddy) Make this actually legible |
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 feel like there's some work we could do during ConfigSnapshot construction to alleviate some of this pain down here in the xDS code.
|
||
// Only create the outbound listener when there are upstreams and filter chains are present | ||
if outboundListener != nil && hasChains { | ||
// Filter chains are stable sorted to avoid draining if the list is provided out of order |
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.
👍
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.
Left more comments, but LGTM for the alpha.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/339575. |
This PR makes a few changes: