-
Notifications
You must be signed in to change notification settings - Fork 339
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): deduplicate dataplane inbounds by address and port combination #12760
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Marcin Skalski <[email protected]>
Reviewer Checklist🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
|
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
…rom-pod # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_controller_test.go # pkg/plugins/runtime/k8s/controllers/testdata/01.dataplane.yaml
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
ifaces = append(ifaces, inboundForServiceless(zone, pod, name, nodeLabels)) | ||
} | ||
|
||
// Right now we will build multiple inbounds for each service selecting port, but later on |
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.
The code will be left here for future usage, so readers are from the future. Should it be changed to "We are only create one listener for last inbound now, but in the previous version, LegacyInboundInterfacesFor, we built multiple inbounds for each service selecting port"
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.
Looks good to me.
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
Signed-off-by: Marcin Skalski <[email protected]>
@@ -166,6 +168,7 @@ func (i *InboundConverter) LegacyInboundInterfacesFor(ctx context.Context, zone | |||
return ifaces, nil | |||
} | |||
|
|||
// Should be used when MeshService mode is Exclusive | |||
func (i *InboundConverter) InboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) { |
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.
Do we need to sort services []*kube_core.Service
before iterate over it? To keep consistant results when deduplicated in multiple runs.
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.
we sort inbounds at the end
} | ||
|
||
func deduplicateInboundsByAddressAndPort(ifaces []*mesh_proto.Dataplane_Networking_Inbound) []*mesh_proto.Dataplane_Networking_Inbound { | ||
inboundKey := func(iface *mesh_proto.Dataplane_Networking_Inbound) string { |
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.
In one of my testing, I found when the service port
is different than the targetPort
, the two inbounds generated on the DP has the same name and port, but they have different k8s.kuma.io/service-port
tags: one of them is incorrect. So in this case, we need to remove the one with incorrect tag. Could you also verify this?
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.
do you mean the situation when we have service with two ports?
With this approach we don't care about tags, inbound tags will be gone after we fully switch to MeshService exclusive. I think in this situation we will just create inbound from pod port
Signed-off-by: Marcin Skalski <[email protected]>
Motivation
We now can have multiple inbounds if multiple services select the same address and port, but we will generate Envoy resources only for one of these. We should deduplicate them on creation so that we don't have inbounds that don't correlate with Envoy resources. This can be also useful for GUI
xref #13108