Differentiate UDP and TCP Protocols in Services #12628

nathanjsweet · 2020-07-23T16:40:04Z

Saving protocol information in the lb(x)_key in bpf, as well as
adding protocol information to service maps creation in the
lbmap package ensures that translation, receiving, and forwarding
will always take the protocol into proper account.

Signed-off-by: Nathan Sweet nathanjsweet@pm.me

Fixes: #9207

Differentiate load-balancer keys in the datapath by protocol (in addition to address and port), so that 
cilium can correctly differentiate protocols between services.

bpf/sockops/bpf_sockops.h

coveralls · 2020-07-23T17:19:22Z

Coverage decreased (-0.04%) to 37.081% when pulling 9f59de2 on nathanjsweet/pr/differentiate-protocol-in-services into 3abd218 on master.

joestringer · 2020-07-23T18:05:23Z

Added upgrade-impact label based on the concerns raised in the original issue #9207. Will need to review closely in that area.

pchaigno

LGTM, except for some minor indentation issues and tests.

Maybe a painless way to test this in e2e tests is to simply change test/k8sT/manifests/demo_ds.yaml to switch one of the services to use same ports for TCP and UDP. These services are then used in test/k8sT/Services.go tests for NodePort, ExternalIP, externalTrafficPolicy=Local, etc.

bpf/bpf_sock.c

pchaigno · 2020-07-24T07:14:36Z

pkg/service/service.go

@@ -603,7 +603,7 @@ func (s *Service) upsertServiceIntoLBMaps(svc *svcInfo, onlyLocalBackends bool,

 	err := s.lbmap.UpsertService(
 		uint16(svc.frontend.ID), svc.frontend.L3n4Addr.IP,
-		svc.frontend.L3n4Addr.L4Addr.Port,
+		svc.frontend.L3n4Addr.L4Addr.Port, string(svc.frontend.L3n4Addr.L4Addr.Protocol),


There are some unit tests for this in service_test.go. I think they could be extended to add a new frontend with just the protocol changed.

These look to be unit tests and the lbmap service is just the mock service, so I don't know how much that will really tell us. Is there something else that you're referencing?

Ah, no you're right.

I'd still add a unit test to check whether a svc doesn't get overwritten in pkg/service/service.go internal state if the same svc but with different proto gets added.

brb · 2020-07-24T07:46:52Z

Thinking loud about the upgrade:

Both UDP and TCP services with the same IP/port will end up having the same rev_nat_index (allocated by pkg/service.AcquireID(...)). Same applies to service backends. So, the indices won't change, and thus no changes to existing CT entries are needed.
Last time I looked, the flow is the following:
1. pkg/service.RestoreServices()
2. Wait for k8s watchers to finish restoration (it will call pkg/service.UpsertService(..) for existing services)
3. Reload BPF programs
4. Call pkg/service.SyncWithK8sFinished(). This will remove the obsolete services.

From the first glance it looks that no existing flow will be interrupted, as 1) rev_nat_index doesn't change, 2) the old BPF programs will be able to access old svc entries, 3) the new BPF programs will be accessing new svc entries. However, we should definitely add an upgrade test (we can reuse the existing migrate-svc in test/k8sT/Updates.go) to gain more confidence.

brb

pkg/loadbalancer/loadbalancer.go:L3n4Addr.Hash() should not omit the protocol.
I'd add some unit tests to pkg/service, also integration tests to k8sT/Services.go.
Extend migrate-svc in k8sT/Updates.go to test the upgrade.

bpf/sockops/bpf_sockops.h

christarazi

LGTM overall as a first-pass, just a minor item to address

pkg/service/service_test.go

pkg/k8s/service_cache.go

pkg/loadbalancer/loadbalancer.go

nathanjsweet · 2020-10-02T18:32:58Z

We have full kernel/e2e and runtime passes for this PR. The last commit, I think, doesn't need a full set of tests, as it is a simple matter of deleting a few lines of logging, and adding some integration tests. Assuming @brb is cool with putting off sockops protocol #ifdef work into another PR then I think this PR is safe to merge on 🍏 .

nathanjsweet · 2020-10-02T19:28:53Z

ran into #13262

aanm

I've given a detailed review to all files except the ones in test/

cilium/cmd/service_update.go

pkg/k8s/service.go

pkg/k8s/watchers/service.go

pkg/k8s/service.go

pkg/maps/lbmap/ipv6.go

aanm · 2020-10-05T08:59:56Z

pkg/u8proto/u8proto.go

+func FromNumber(proto uint8) (U8proto, error) {
+	_, ok := protoNames[U8proto(proto)]
+	if !ok {
+		return 0, fmt.Errorf("unknown protocol %d", proto)


nit, any reason why the error on line 66 is quoted but this one is not?

No, but since the primitive causing the error, in this case, isn't a string I think we should leave it as is.

aanm · 2020-10-05T09:00:17Z

test/helpers/cons.go

@@ -22,7 +22,7 @@ import (

 	k8sConst "github.com/cilium/cilium/pkg/k8s/apis/cilium.io"
 	"github.com/cilium/cilium/pkg/versioncheck"
-	"github.com/cilium/cilium/test/ginkgo-ext"
+	ginkgoext "github.com/cilium/cilium/test/ginkgo-ext"


irrelevant change

~~Go fmt~~ goimports does this for me automatically (running go 1.15).

joestringer

Docs, policy changes were trivial. I'll give approval for these two codeowners. CLI and BPF seemed to already be reviewed by others so I didn't look too closely, looks like others already have comments, some of which still need to be addressed.

bpf/sockops/bpf_sockops.c

pkg/policy/portmap_test.go

Saving protocol information in the lb(x)_key in bpf, as well as adding protocol information to service maps creation in the lbmap package ensures that translation, receiving, and forwarding will always take the protocol into proper account. Refactor cilium to always assume a protocol with a service. Refactor tests to account for service protocols. Update Documentation for kubeproxy-free nodeport example Since the output of `cilium service list` has changed the documentation should reflect the change. Fixes: #9207 Signed-off-by: Nate Sweet <nathanjsweet@pm.me>

joestringer · 2020-10-09T19:18:16Z

test-me-please

joestringer · 2020-10-09T22:03:07Z

All green CI, as discussed out-of-band on Slack, feedback was addressed and recently requested changes on this PR have been largely cosmetic. Reviewers, if you still see anything worth following up on then please post those reviews and let @nathanjsweet know.

briantopping · 2022-10-12T15:15:32Z

Does this actually fix #9207? Last time I checked, it's still impossible for a pod to publish both a TCP and UDP as a service (such as 53) with the same port number.

pchaigno · 2022-10-12T15:41:24Z

@briantopping No, this PR was later reverted because of an upgrade issue: #13587. This is now tracked at #9207, as you've seen.

briantopping · 2022-10-12T15:48:13Z

Ah, thanks. I forgot a lot of the history, just noticed the heading on that issue and didn't know if it was getting lost because of it.

pchaigno · 2022-10-12T19:41:24Z

Yeah, that's just the GitHub UI being a bit broken: it doesn't understand when you reopen an issue that should have been fixed by a PR.

nathanjsweet added the area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Jul 23, 2020

nathanjsweet requested review from qmonnet and a team July 23, 2020 16:40

nathanjsweet requested a review from a team as a code owner July 23, 2020 16:40

nathanjsweet requested a review from a team July 23, 2020 16:40

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 23, 2020

nathanjsweet added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 23, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 23, 2020

nathanjsweet commented Jul 23, 2020

View reviewed changes

bpf/sockops/bpf_sockops.h Show resolved Hide resolved

joestringer added the upgrade-impact This PR has potential upgrade or downgrade impact. label Jul 23, 2020

pchaigno added the needs/e2e-test This issue is not covered by existing CI tests, but should be. label Jul 24, 2020

pchaigno requested changes Jul 24, 2020

View reviewed changes

brb requested changes Jul 24, 2020

View reviewed changes

nathanjsweet force-pushed the nathanjsweet/pr/differentiate-protocol-in-services branch 3 times, most recently from b755786 to f0b067e Compare July 25, 2020 00:02

pchaigno reviewed Jul 27, 2020

View reviewed changes

bpf/sockops/bpf_sockops.h Show resolved Hide resolved

nathanjsweet force-pushed the nathanjsweet/pr/differentiate-protocol-in-services branch 2 times, most recently from c954fa9 to db791c9 Compare August 5, 2020 17:38

nathanjsweet requested a review from a team as a code owner August 5, 2020 17:38

nathanjsweet requested a review from a team August 5, 2020 17:38

christarazi reviewed Aug 5, 2020

View reviewed changes

pkg/service/service_test.go Outdated Show resolved Hide resolved

brb reviewed Aug 5, 2020

View reviewed changes

pkg/k8s/service_cache.go Show resolved Hide resolved

brb reviewed Aug 5, 2020

View reviewed changes

pkg/loadbalancer/loadbalancer.go Outdated Show resolved Hide resolved

nathanjsweet force-pushed the nathanjsweet/pr/differentiate-protocol-in-services branch 2 times, most recently from d7d88cc to 359f001 Compare August 6, 2020 03:02

nathanjsweet requested a review from a team as a code owner August 6, 2020 03:02

maintainer-s-little-helper bot requested review from christarazi and pchaigno October 2, 2020 18:28

aanm requested changes Oct 5, 2020

View reviewed changes

joestringer approved these changes Oct 6, 2020

View reviewed changes

bpf/sockops/bpf_sockops.c Show resolved Hide resolved

pkg/policy/portmap_test.go Show resolved Hide resolved

nathanjsweet mentioned this pull request Oct 9, 2020

Add feature flag in sockops bpf code for when the socket object is available for use. #13490

Closed

nathanjsweet force-pushed the nathanjsweet/pr/differentiate-protocol-in-services branch from 6e94a6e to b33559a Compare October 9, 2020 17:39

nathanjsweet force-pushed the nathanjsweet/pr/differentiate-protocol-in-services branch from b33559a to cfdb026 Compare October 9, 2020 17:42

joestringer requested review from aanm and brb October 9, 2020 22:00

joestringer merged commit 107b0f5 into master Oct 9, 2020

joestringer deleted the nathanjsweet/pr/differentiate-protocol-in-services branch October 9, 2020 22:03

joestringer mentioned this pull request Oct 12, 2020

Prepare for release v1.9.0-rc1 #13518

Merged

brb mentioned this pull request Oct 13, 2020

UDP and TCP port differentiation is broken when doing upgrade #13529

Closed

nathanjsweet restored the nathanjsweet/pr/differentiate-protocol-in-services branch October 15, 2020 15:05

nathanjsweet deleted the nathanjsweet/pr/differentiate-protocol-in-services branch October 15, 2020 15:45

nathanjsweet restored the nathanjsweet/pr/differentiate-protocol-in-services branch November 11, 2020 03:49

nathanjsweet deleted the nathanjsweet/pr/differentiate-protocol-in-services branch November 11, 2020 04:08

brb mentioned this pull request Nov 11, 2020

Differentiate UDP and TCP Protocols in Services #13980

Closed

nathanjsweet restored the nathanjsweet/pr/differentiate-protocol-in-services branch February 5, 2021 17:13

aanm deleted the nathanjsweet/pr/differentiate-protocol-in-services branch March 4, 2022 03:12

aojea mentioned this pull request Jun 21, 2023

Differentiate UDP and TCP Protocols in Services #26399

Closed

Differentiate UDP and TCP Protocols in Services #12628

Differentiate UDP and TCP Protocols in Services #12628

Uh oh!

Conversation

nathanjsweet commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer commented Jul 23, 2020

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pchaigno Jul 24, 2020

Choose a reason for hiding this comment

Uh oh!

nathanjsweet Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

pchaigno Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

brb Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

brb commented Jul 24, 2020

Uh oh!

brb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nathanjsweet commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathanjsweet commented Oct 2, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aanm Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

nathanjsweet Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

aanm Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

nathanjsweet Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joestringer commented Oct 9, 2020

Uh oh!

joestringer commented Oct 9, 2020

Uh oh!

nathanjsweet commented Jul 23, 2020 •

edited

Loading

coveralls commented Jul 23, 2020 •

edited

Loading

christarazi left a comment •

edited

Loading

nathanjsweet commented Oct 2, 2020 •

edited

Loading

nathanjsweet Oct 9, 2020 •

edited

Loading