feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Automaat · 2025-02-05T14:37:22Z

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

Fix: #12123

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

github-actions · 2025-02-05T14:45:43Z

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
IPv6 is taken into account (.e.g: no string concatenation of host port)
Tests (Unit test, E2E tests, manual test on universal and k8s)
- Don't forget ci/ labels to run additional/fewer tests
Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

…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 <skalskimarcin33@gmail.com>

jijiechen · 2025-02-24T07:53:12Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+		ifaces = append(ifaces, inboundForServiceless(zone, pod, name, nodeLabels))
+	}
+
+	// Right now we will build multiple inbounds for each service selecting port, but later on


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"

jijiechen

Looks good to me.

not ready yet

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

pkg/plugins/runtime/k8s/controllers/pod_controller.go

jijiechen · 2025-03-21T08:48:45Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

@@ -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) {


Do we need to sort services []*kube_core.Service before iterate over it? To keep consistant results when deduplicated in multiple runs.

we sort inbounds at the end

jijiechen · 2025-03-21T08:59:49Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+}
+
+func deduplicateInboundsByAddressAndPort(ifaces []*mesh_proto.Dataplane_Networking_Inbound) []*mesh_proto.Dataplane_Networking_Inbound {
+	inboundKey := func(iface *mesh_proto.Dataplane_Networking_Inbound) string {


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?

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 <skalskimarcin33@gmail.com>

lobkovilya · 2025-03-24T10:49:17Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+// to not change order of inbounds.
+// For gateway we pick first inbound to take tags from. Delegated gateway identity relies on this.
+// For Dataplanes when MeshService is disabled we base identity and routing
+// TODO: We should revisit this when we rework identity


you can reference the issue #3339

lobkovilya · 2025-03-24T10:50:07Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

 	return ifaces, nil
 }

+// Should be used when MeshService mode is Exclusive


nit: it's a public method, please use a proper doc comments that starts with // InboundInterfacesFor ...

lobkovilya · 2025-03-24T10:58:03Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

 	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) {


Please create a private method inboundInterfaceFor so that methods looks like

func (i *InboundConverter) LegacyInboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) { return i.inboundInterfacesFor(ctx, zone, pod, services) } func (i *InboundConverter) InboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) { inbounds, err := i.inboundInterfacesFor(ctx, zone, pod, services) if err != nil { return err } return deduplicateInboundsByAddressAndPort(ifaces), nil }

Because in the future if duplicated code in LegacyInboundInterfacesFor and InboundInterfacesFor diverges it'll be hard to understand what it takes to remove LegacyInboundInterfacesFor

lobkovilya · 2025-03-24T11:02:31Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+	for _, v := range inboundsPerName {
+		deduplicatedInbounds = append(deduplicatedInbounds, v)
+	}
+	sort.Slice(deduplicatedInbounds, func(i, j int) bool {


Probably should be moved out from deduplicateInbounds method, "deduplicate" usually implies:

the complexity is O(N)

the order of the input slice is preserved in the output slice

lobkovilya · 2025-03-24T11:05:57Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+		return fmt.Sprintf("%s:%d", iface.Address, iface.Port)
+	}
+
+	inboundsPerName := map[string]*mesh_proto.Dataplane_Networking_Inbound{}


nit: inboundsPerAddressPort?

lobkovilya · 2025-03-24T11:08:29Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+	inboundsPerName := map[string]*mesh_proto.Dataplane_Networking_Inbound{}
+	for _, iface := range ifaces {
+		if inboundsPerName[inboundKey(iface)] == nil {
+			inboundsPerName[inboundKey(iface)] = iface


append to the result slice here, that way you preserve the original order and you won't need another loop on inboundsPerName

feat(kuma-cp): inbound name should be taken from pod instead of service

30d0259

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

Automaat added the ci/run-full-matrix label Feb 5, 2025

Automaat requested a review from a team as a code owner February 5, 2025 14:37

Automaat requested review from jijiechen and Icarus9913 February 5, 2025 14:37

Automaat added 2 commits February 6, 2025 15:18

feat(kuma-cp): fix tests

fc93dba

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

feat(kuma-cp): fix tests

514fe96

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

Automaat mentioned this pull request Feb 7, 2025

feat(kuma-cp): take inbound name from pod instead of service #12783

Merged

Automaat added 2 commits February 12, 2025 09:16

Merge remote-tracking branch 'origin/master' into feat/inbound-name-f…

dfe4edb

…rom-pod # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_controller_test.go # pkg/plugins/runtime/k8s/controllers/testdata/01.dataplane.yaml

fix(e2e): fix picking right interface for delegated gateway

8817d6d

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

Automaat changed the title ~~feat(kuma-cp): inbound name should be taken from pod instead of service~~ feat(kuma-cp): deduplicate dataplane inbounds by address and port conmbination Feb 12, 2025

Automaat changed the title ~~feat(kuma-cp): deduplicate dataplane inbounds by address and port conmbination~~ feat(kuma-cp): deduplicate dataplane inbounds by address and port combination Feb 12, 2025

Automaat added 2 commits February 12, 2025 13:56

fix(e2e): fix delegated gateway behaviour

aca4d0a

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

fix(e2e): keep first inbound when deduplicating

dab5391

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

jijiechen reviewed Feb 24, 2025

View reviewed changes

jijiechen previously approved these changes Feb 24, 2025

View reviewed changes

Automaat marked this pull request as draft March 17, 2025 10:28

Automaat and others added 7 commits March 17, 2025 11:45

Merge branch 'master' into feat/inbound-name-from-pod

b020271

Merge branch 'master' into feat/inbound-name-from-pod

d3a5a65

feat(inbounds): fix unit tests

3ad8235

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

feat(inbounds): deduplicate inbounds only for MeshService exclusive

653f1e6

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

feat(inbounds): add test for deduplication

21f1407

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

feat(inbounds): fix check

bb69589

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

feat(inbounds): update comments

2972853

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

Automaat marked this pull request as ready for review March 21, 2025 06:57

jijiechen reviewed Mar 21, 2025

View reviewed changes

pkg/plugins/runtime/k8s/controllers/pod_controller.go Outdated Show resolved Hide resolved

jijiechen reviewed Mar 21, 2025

View reviewed changes

feat(inbounds): remove unneeded log

35f8d6d

Signed-off-by: Marcin Skalski <skalskimarcin33@gmail.com>

lobkovilya reviewed Mar 24, 2025

View reviewed changes

lukidzi assigned Automaat Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Automaat commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025

jijiechen Feb 24, 2025

jijiechen left a comment

jijiechen Mar 21, 2025

Automaat Mar 21, 2025

jijiechen Mar 21, 2025

Automaat Mar 21, 2025

lobkovilya Mar 24, 2025

lobkovilya Mar 24, 2025

lobkovilya Mar 24, 2025

lobkovilya Mar 24, 2025

lobkovilya Mar 24, 2025

lobkovilya Mar 24, 2025

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Are you sure you want to change the base?

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Conversation

Automaat commented Feb 5, 2025 • edited Loading

Motivation

github-actions bot commented Feb 5, 2025

Reviewer Checklist

Choose a reason for hiding this comment

jijiechen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Automaat commented Feb 5, 2025 •

edited

Loading