Bpf iface auto detection #8803

sridhartigera · 2024-05-09T02:19:04Z

Description

This PR adds the following

bond* added to the default value of dataIfacePattern Regex.
Handle dynamic addition of interfaces to the bond.
IFstate map changes to add more info about the type of each interface.

This PR does not handle attaching xdp programs to the bond slaves. Will be done subsequently.

Related issues/PRs

Todos

Tests
Documentation
Release note

Release Note

ebpf:  If a bond master device is part of the bpfDataIfacePattern regexp, calico attaches to it and not to the slaves

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

tomastigera

As discussed offline, it would be good/possible not to introduce the new option, deprecate the l3 regexp and just see of ia device is l3 or l2 and or slave/master in a bond. That would let us exclude device using the pattern that we do not care and we woul dnot need to include bondX inthe patter to include bonds.

tomastigera · 2024-05-15T20:21:35Z

felix/config/config_params.go

@@ -204,6 +204,7 @@ type Config struct {
 BPFForceTrackPacketsFromIfaces []string `config:"iface-filter-slice;docker+"`
 BPFDisableGROForIfaces *regexp.Regexp `config:"regexp;"`
 BPFExcludeCIDRsFromNAT []string `config:"cidr-list;;"`
+ BPFInterfaceAutoDetection string `config:"oneof(TCP,Enabled,Disabled);Enabled;non-zero"`


Suggested change

BPFInterfaceAutoDetection string `config:"oneof(TCP,Enabled,Disabled);Enabled;non-zero"`

BPFInterfaceAutoDetection string `config:"oneof(Enabled,Disabled);Enabled;non-zero"`

tomastigera · 2024-05-15T20:25:18Z

felix/fv/bpf_attach_test.go

+ tc.Felixes[0].Exec("ip", "link", "add", "dummy0", "type", "dummy")
+ tc.Felixes[0].Exec("ip", "link", "add", "dummy1", "type", "dummy")


what if you named these devices eth3 and eth4 or somethign else that could interfere with the regexp?

tomastigera · 2024-05-15T20:47:00Z

felix/dataplane/linux/bpf_ep_mgr.go

+ // Check if the interface is bond. If so, update the interface type for
+ // slaves, master.


what does this comment relates to?

tomastigera · 2024-05-15T20:49:22Z

felix/dataplane/linux/bpf_ep_mgr.go

+
+func (m *bpfEndpointManager) autoDetectInterfaceType(intf *net.Interface) IfaceType {
+ name := intf.Name
+ if name == "tunl0" {


what if ipip tunnel has a different name?

tomastigera · 2024-05-15T21:12:35Z

felix/dataplane/linux/bpf_ep_mgr.go

+}
+
+func isBondIface(name string) bool {
+ bonding := fmt.Sprintf("/sys/class/net/%s/bonding", name)


could this be queried via netlink?

tomastigera · 2024-05-23T18:37:17Z

felix/dataplane/linux/bpf_ep_mgr.go

+ log.Debugf("Failed to get master interface details for '%s'", update.Name)
+ }
+ if !m.isDataIface(masterIfa.Name) {
+ log.Warnf("Master interface '%s' ignored. Add it to the config", masterIfa.Name)


I think the better behaviour would be to say "ok, master is not in the regexp but the slave is, seems like the user wants us to attach to the slave and not to the bond. User may know better, why not, lets to it. Attach to slave and ignore the master" WDYT? 🤔

tomastigera · 2024-05-23T18:51:02Z

felix/fv/bpf_attach_test.go

@@ -92,4 +93,71 @@ var _ = infrastructure.DatastoreDescribe("_BPF-SAFE_ Felix bpf reattach object",
 return out
 }, "15s", "1s").ShouldNot(ContainSubstring("eth0"))
 })
+
+ It("should attach programs to the bond interfaces", func() {


btw can this be tested in the unit test in bpf_ep_mgr_test.go? Woudn't that be a better place? Perhaps those cases with slave in regexp and master not in addition to this test? There is a mock for netlink.

tomastigera

lgtm, some nits around error handling

tomastigera · 2024-05-28T16:11:15Z

felix/bpf/ifstate/map.go

@@ -42,13 +42,27 @@ const (
 FlgWEP = uint32(0x1)
 FlgIPv4Ready = uint32(0x2)
 FlgIPv6Ready = uint32(0x4)
- FlgMax = uint32(0x7)
+ FlgHost = uint32(0x8)


So far, not having FlgWEP meant host. Is there any need for the new flag? We cannot have both WEP and Host. Also the name should probably we FlgHEP.

I changed it to FlgHEP. Want these flags to make sure iface detection is working fine.

tomastigera · 2024-05-28T18:23:31Z

felix/dataplane/linux/bpf_ep_mgr_test.go

+ JustAfterEach(func() {
+ err := dp.deleteIface("bond0")
+ Expect(err).NotTo(HaveOccurred())
+ err = dp.deleteIface("eth10")
+ Expect(err).NotTo(HaveOccurred())
+ err = dp.deleteIface("eth20")
+ Expect(err).NotTo(HaveOccurred())
+ })


why do you this clean up? Isn't dp recreated?

tomastigera · 2024-05-28T18:26:51Z

felix/dataplane/linux/bpf_ep_mgr_test.go

+ err = dp.deleteIface("eth11")
+ Expect(err).NotTo(HaveOccurred())
+ err = dp.deleteIface("foo0")
+ Expect(err).NotTo(HaveOccurred())
+ genIfaceUpdate("eth11", ifacemonitor.StateNotPresent, 21)()
+ genIfaceUpdate("foo0", ifacemonitor.StateNotPresent, 11)()


what does this test? Would it make sense to add this to where bond matches the when bond is deleted slaves get the attachment?

Just want to make sure things are fine when interface is deleted.

tomastigera · 2024-05-28T18:28:44Z

felix/dataplane/linux/bpf_ep_mgr.go

+ // update the ifaceType, master ifindex if bond slave.
+ link, err := m.dp.getIfaceLink(update.Name)
+ if err != nil {
+ log.Panicf("Failed to get interface information via netlink '%s'", update.Name)


Panic does not seem right? What if we just ignore the iface and log an error?

tomastigera · 2024-05-28T18:33:50Z

felix/dataplane/linux/bpf_ep_mgr.go

+ if err != nil {
+ log.WithError(err).Warn("Failed to list attached programs")
+ } else {
+ if err := m.cleanupOldAttach(update.Name, ai[update.Name]); err != nil {


could there be no error and ai[] to be empty as if the programs were not attached before?

yes. cleanupAttached handles that by checking if ai.xdp, ai.ingress, ai.egress != nil

tomastigera · 2024-05-28T18:34:53Z

felix/dataplane/linux/bpf_ep_mgr.go

+ // throw a warning and continue to attach to slave.
+ masterIfa, err := m.dp.interfaceByIndex(val.info.masterIfIndex)
+ if err != nil {
+ log.Debugf("Failed to get master interface details for '%s'. Continuing to attach program", iface)


Suggested change

log.Debugf("Failed to get master interface details for '%s'. Continuing to attach program", iface)

log.Warnf("Failed to get master interface details for '%s'. Continuing to attach program", iface)

feels like this should be reported

tomastigera · 2024-05-28T18:36:28Z

felix/dataplane/linux/bpf_ep_mgr.go

+ if err != nil {
+ log.Debugf("Failed to get master interface details for '%s'. Continuing to attach program", iface)
+ } else if !m.isDataIface(masterIfa.Name) {
+ log.Warnf("Master interface '%s' ignored. Add it to the config", masterIfa.Name)


Suggested change

log.Warnf("Master interface '%s' ignored. Add it to the config", masterIfa.Name)

log.Warnf("Master interface '%s' ignored. Add it to the <config name> in config", masterIfa.Name)

tomastigera

I still think FlgHEP is redundant, but well, doesn't hurt either. There is some static check error. When you fix it, could you squash all the commits locally (not through GH UI) and force push it before merging? 🙏

marvin-tigera added this to the Calico v3.29.0 milestone May 9, 2024

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels May 9, 2024

sridhartigera marked this pull request as ready for review May 14, 2024 01:08

sridhartigera requested a review from a team as a code owner May 14, 2024 01:08

sridhartigera changed the title ~~Bpf iface detect~~ Bpf iface auto detection May 14, 2024

tomastigera reviewed May 15, 2024

View reviewed changes

tomastigera reviewed May 23, 2024

View reviewed changes

tomastigera reviewed May 28, 2024

View reviewed changes

tomastigera approved these changes May 29, 2024

View reviewed changes

Interface autodetection - ebpf

d6c2cbf

sridhartigera force-pushed the bpf-iface-detect branch from 06e7edb to d6c2cbf Compare May 30, 2024 01:04

sridhartigera merged commit dfa5f61 into projectcalico:master May 30, 2024
2 checks passed

sridhartigera deleted the bpf-iface-detect branch May 30, 2024 03:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bpf iface auto detection #8803

Bpf iface auto detection #8803

sridhartigera commented May 9, 2024 •

edited by tomastigera

tomastigera left a comment

tomastigera May 15, 2024

tomastigera May 15, 2024

tomastigera May 15, 2024

tomastigera May 15, 2024

tomastigera May 15, 2024

tomastigera May 23, 2024

tomastigera May 23, 2024

tomastigera left a comment

tomastigera May 28, 2024

sridhartigera May 28, 2024

tomastigera May 28, 2024

tomastigera May 28, 2024

sridhartigera May 28, 2024

tomastigera May 28, 2024

tomastigera May 28, 2024

sridhartigera May 28, 2024

tomastigera May 28, 2024

tomastigera May 28, 2024

tomastigera left a comment

	BPFInterfaceAutoDetection string `config:"oneof(TCP,Enabled,Disabled);Enabled;non-zero"`
	BPFInterfaceAutoDetection string `config:"oneof(Enabled,Disabled);Enabled;non-zero"`

		tc.Felixes[0].Exec("ip", "link", "add", "dummy0", "type", "dummy")
		tc.Felixes[0].Exec("ip", "link", "add", "dummy1", "type", "dummy")

		// Check if the interface is bond. If so, update the interface type for
		// slaves, master.

	log.Debugf("Failed to get master interface details for '%s'. Continuing to attach program", iface)
	log.Warnf("Failed to get master interface details for '%s'. Continuing to attach program", iface)

	log.Warnf("Master interface '%s' ignored. Add it to the config", masterIfa.Name)
	log.Warnf("Master interface '%s' ignored. Add it to the <config name> in config", masterIfa.Name)

Bpf iface auto detection #8803

Bpf iface auto detection #8803

Conversation

sridhartigera commented May 9, 2024 • edited by tomastigera

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

tomastigera 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

tomastigera 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

tomastigera left a comment

Choose a reason for hiding this comment

sridhartigera commented May 9, 2024 •

edited by tomastigera