-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat(kuma-cp) Direct access with forward cluster #790
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.
A couple remarks from me.
pkg/xds/server/components.go
Outdated
@@ -117,7 +122,7 @@ func DefaultDataplaneSyncTracker(rt core_runtime.Runtime, reconciler SnapshotRec | |||
destinations := xds_topology.BuildDestinationMap(dataplane, routes) | |||
|
|||
// resolve all endpoints that match given selectors | |||
outbound, err := xds_topology.GetOutboundTargets(ctx, dataplane, destinations, rt.ReadOnlyResourceManager()) | |||
outbound, err := xds_topology.GetOutboundTargets(ctx, dataplane, destinations, dataplanes) |
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.
Would it make sense to pass only the dataplanes
and not dataplane
?
@@ -0,0 +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.
I guess these are ok to be empty?
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.
Yeah, it does not matter since it is not executed, because transparent proxy is off
|
||
sourceService := proxy.Dataplane.Spec.GetIdentifyingService() | ||
meshName := ctx.Mesh.Resource.GetMeta().GetName() | ||
trafficDirection := "OUTBOUND" |
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 constant that we can use?
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.
Introduced constants
@@ -42,8 +44,10 @@ func (p *PodConverter) DataplaneFor(pod *kube_core.Pod, services []*kube_core.Se | |||
Networking: &mesh_proto.Dataplane_Networking{}, | |||
} | |||
if injector_metadata.HasTransparentProxyingEnabled(pod) { | |||
services := pod.GetAnnotations()[injector_metadata.KumaDirectAccess] |
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.
Shall we have some sanity checks for the services? What if there is some garbage that string.Split can't handle?
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.
How would this sanity check look like? If someone put garbage here like !@#$
, we will just see if there is such service and ignore if not.
// Generating listener for every endpoint will cause XDS snapshot to be large therefore it should be used only if really needed. | ||
// | ||
// Second approach to consider was to use FilterChainMatch on catch_all listener with list of all direct access endpoints | ||
// instead of generating outbound listener, but it seemed to not work with Listener#UseOriginalDst |
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 is a comment in envoy docs about use_original_dst
:
This field is deprecated. Use an original_dst listener filter instead.
Note that hand off to another listener is NOT performed without this flag. Once FilterChainMatch is implemented this flag will be removed, as filter chain matching can be used to select a filter chain based on the restored destination address.
So probably the second approach is workable as well.
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.
Yes, I know. I've seen this. I tried to get rid of use_original_dst
in this PR, but it requires changes in iptables, permissions on K8S, and other stuff. I didn't manage get this to work.
Configure(envoy_clusters.ClientSideMTLS(ctx, proxy.Metadata, "*")). | ||
Build() | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "could not generate cluster: pass_through") |
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.
typo: pass_through
-> direct_access
Summary
Slight change to Direct Access implementation. In the previous version, it was implemented in pod converter. We were generating outbound listener (in DP model) for every instance. The problem is that even if have "backend" cluster with 3 instances and you generate listener for every instance that points to this cluster, Envoy then will do load balancing across all instances with is not what we need.
With this implementation, we generate listener for every instance but everything will go through
direct_access
cluster that will forward request to given destination and encrypt it by mTLS.This functionality will work only on L4 and support mTLS/Traffic Permissions.