Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ARP Snooping for Switch NI to learn Interface IP #3203

Merged
merged 1 commit into from
May 17, 2023
Merged

ARP Snooping for Switch NI to learn Interface IP #3203

merged 1 commit into from
May 17, 2023

Conversation

naiming-zededa
Copy link
Contributor

@naiming-zededa naiming-zededa commented May 11, 2023 •
edited
Loading

This patch fixes an issue of EdgeView treats EVE App on switch-NI with static Intf IP
configuration inside the App as 'External' IP address which can be disallowed by EdgeView policy.
The patch extends the EVE switch-NI snooping operation currently of DNS and DHCP,
to now including the ARP request and reply. This gives us the capability of learning the
App IP addresses on switch-NI in all the conditions. In rare cases, if a user does
not want this ARP snooping, it can be disabled by a configitem knob, but it may require
a device reboot if the App is already running.

milan-zededa
milan-zededa reviewed May 11, 2023
View reviewed changes
pkg/pillar/nistate/linux_flow.go
if arp == nil {
return nil, false
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below this point you can return isARP as true - however, currently it is irrelevant since there is no other packet processing after this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but the 'real' meaning is we got an 'arp IP address'. We'll see later when there is more items

Copy link
Contributor

@milan-zededa milan-zededa May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case you can drop isARP and just do len(addrUpdates) != 0 instead.
In processDHCPPacket the boolean is used to decide if after processDHCPPacket we should try to run processDADProbe. Because even if processDHCPPacket learned nothing about IP assignments but confirmed that the packet is DHCP, there is no point trying to run processDADProbe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

milan-zededa
milan-zededa reviewed May 11, 2023
View reviewed changes
docs/CONFIG-PROPERTIES.md Outdated
@@ -51,6 +51,7 @@
| netdump.topic.postonboard.interval | integer in seconds | 1 day | how frequently (in seconds) can be netdumps of the same topic published after device has been onboarded |
| netdump.topic.maxcount | integer | 10 | maximum number of netdumps that can be published for each topic. The oldest netdump is unpublished should a new netdump exceed the limit.
| netdump.downloader.with.pcap | boolean | false | include packet captures inside netdumps for download requests. However, even if enabled, TCP segments carrying non-empty payload (i.e. content which is being downloaded) are excluded and the overall PCAP size is limited to 64MB. |
| disable.switch.ni.arpsnoop | boolean | false | disable ARP Snooping on switch Network Instance, may need device reboot to take effect |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that most boolean config options vaguely follow template: <topic>.<item>.enable.<feature>
I would therefore propose something like network.switch.enable.arpsnoop
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @milan-zededa

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if you do it as an enable.X.Y knob you can set the default value to enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

milan-zededa
milan-zededa approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@milan-zededa milan-zededa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we want to support change in disable.switch.ni.arpsnoop also for already deployed network instances. It can be done but it would make things little bit more complicated and it is probably overkill and not necessary, so PR LGTM.

@naiming-zededa
Copy link
Contributor Author

Right that was my concern. I think it will be a very corner case someone wants to disable it, and to handle dynamically seems quite complex to me.

eriknordmark
eriknordmark reviewed May 11, 2023
View reviewed changes
pkg/pillar/types/global.go Outdated
@@ -849,6 +851,7 @@ func NewConfigItemSpecMap() ConfigItemSpecMap {
configItemSpecMap.AddBoolItem(DisableDHCPAllOnesNetMask, false)
configItemSpecMap.AddBoolItem(ProcessCloudInitMultiPart, false)
configItemSpecMap.AddBoolItem(ConsoleAccess, true) // Controller likely default to false
configItemSpecMap.AddBoolItem(DisableARPSnoop, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to update global_test.go and run make test;
The unittest fails with
=== FAIL: types TestNewConfigItemSpecMap (0.00s)
time="2023-05-11T16:58:25Z" level=error msg="getEveMemoryLimitInBytes failed: open /hostfs/sys/fs/cgroup/memory/eve/memory.soft_limit_in_bytes: no such file or directory"
global_test.go:206: GlobalSettings has more (53) than expected keys (52)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that. will add and run test.

eriknordmark
eriknordmark requested changes May 11, 2023
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix unit test failure

@naiming-zededa
Copy link
Contributor Author

naiming-zededa commented May 11, 2023 •
edited
Loading

ran 'make test', seems to be fine:
=== SKIP: cmd/tpmmgr TestSealUnseal (0.00s)
tpmmgr_test.go:208: TPM is not available, skipping the test.

=== SKIP: cmd/zedrouter TestRunDnsmasq (0.00s)
dnsmasq_test.go:404: Could not create bridge device, probably not enough permissions

=== SKIP: hypervisor TestGetDomsCPUMem (10.00s)
time="2023-05-11T18:21:19Z" level=info msg=NewContainerdClient
time="2023-05-11T18:21:29Z" level=error msg="NewContainerdClient: could not create containerd client. failed to dial "/run/containerd/containerd.sock": context deadline exceeded: connection error: desc = "transport: error while dialing: dial unix:///run/containerd/containerd.sock: timeout""
containerd_test.go:10: test must be run on a system with a functional containerd

DONE 215 tests, 3 skipped in 767.007s

but above unit tests still failed in this PR.

@eriknordmark
Copy link
Contributor

ran 'make test', seems to be fine:

Try cd pkg/pillar/types; go test

I know from experience that adding a configItem requires touching global_test.go

eriknordmark
eriknordmark approved these changes May 15, 2023
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eriknordmark
Copy link
Contributor

@naiming-zededa @milan-zededa @uncleDecart do we have any current tests in Eden which cover DHCP snooping which we can extend to cover ARP snooping?

@naiming-zededa
Copy link
Contributor Author

@naiming-zededa @milan-zededa @uncleDecart do we have any current tests in Eden which cover DHCP snooping which we can extend to cover ARP snooping?

I'm not aware of that.

@naiming-zededa
Copy link
Contributor Author

I think i found a bug in this code, let me add a fix to this.

@milan-zededa
Copy link
Contributor

milan-zededa commented May 16, 2023 •
edited
Loading

@naiming-zededa @milan-zededa @uncleDecart do we have any current tests in Eden which cover DHCP snooping which we can extend to cover ARP snooping?

Yes we do: https://raw.githubusercontent.com/lf-edge/eden/master/tests/eclient/testdata/app_dhcp.txt
Although I plan to modify the test to work with Eden-SDN rather than deploying DHCP server as another app, but that can be done later..

@naiming-zededa
Copy link
Contributor Author

Updated for the fix. The App's side can be either the sender or receiver side since we allow both request and reply.

@naiming-zededa
ARP Snooping for Switch NI to learn Interface IP
63ea9f1 
Signed-off-by: Naiming Shen <naiming@zededa.com>
@eriknordmark
Copy link
Contributor

Yes we do: https://raw.githubusercontent.com/lf-edge/eden/master/tests/eclient/testdata/app_dhcp.txt
Although I plan to modify the test to work with Eden-SDN rather than deploying DHCP server as another app, but that can be done later..

@naiming-zededa can you add an item to jira to add an ARP snooping test along the lines of the current/future dhcp snooping test?

eriknordmark
eriknordmark approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run eden again

@naiming-zededa
Copy link
Contributor Author

@eriknordmark will do. thanks.

@eriknordmark eriknordmark merged commit 02e8a1e into lf-edge:master May 17, 2023
@milan-zededa milan-zededa mentioned this pull request May 17, 2023
Merged
@naiming-zededa naiming-zededa deleted the naiming-arp-snoop branch May 17, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milan-zededa milan-zededa milan-zededa approved these changes

@eriknordmark eriknordmark eriknordmark approved these changes

@rvs rvs Awaiting requested review from rvs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naiming-zededa @eriknordmark @milan-zededa