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.

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

Updates for Ebtables and support for multi-asic. #6542

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

abdosi
Copy link
Contributor

@abdosi abdosi commented Jan 24, 2021

Why/What I did:

Following changes were done for ebtables:

  • Support for Multi-asic platforms. Ebtable filters are installed in namespace for multi-asic and not host. On Single asic installed on host.

  • For Multi-asic platforms we don't want to install on host otherwise Namespace-to-Namespace communication does not happens since ARP Request are not forwarded.

  • Updated to use text file to restore ebtables rules then the binary format. Rules are restore as part of Database docker init instead of rc.local

  • Removed the ebtable service files for buster as not needed as filters are restored/installed as part of database docker init.
    All the binaries are pre-installed with ebtables* binary are same as ebatbles-legacy-*

How I verrify:

  • On Single Asic:
admin@str-xxxx-on-2:~$ sudo ebtables -L
Bridge table: filter

Bridge chain: INPUT, entries: 0, policy: ACCEPT

Bridge chain: FORWARD, entries: 3, policy: ACCEPT
-d BGA -j DROP
-p ARP -j DROP
-p 802_1Q --vlan-encap ARP -j DROP

Bridge chain: OUTPUT, entries: 0, policy: ACCEPT
  • On Multi Asic:
admin@str2-xxxx-acs-3:~$ sudo ebtables -L
Bridge table: filter

Bridge chain: INPUT, entries: 0, policy: ACCEPT

Bridge chain: FORWARD, entries: 0, policy: ACCEPT

Bridge chain: OUTPUT, entries: 0, policy: ACCEPT
admin@str2-xxxx-acs-3:~$ sudo ip netns exec asic0 ebtables -L
Bridge table: filter

Bridge chain: INPUT, entries: 0, policy: ACCEPT

Bridge chain: FORWARD, entries: 3, policy: ACCEPT
-d BGA -j DROP
-p ARP -j DROP
-p 802_1Q --vlan-encap ARP -j DROP

Bridge chain: OUTPUT, entries: 0, policy: ACCEPT

Signed-off-by: Abhishek Dosi <[email protected]>
@lguohan lguohan requested a review from prsunny January 24, 2021 21:52
@abdosi abdosi changed the title Updates for Ebtables. Updates for Ebtables and support for multi-asic. Jan 25, 2021
@prsunny
Copy link
Contributor

prsunny commented Jan 25, 2021

I think the ebtable service was added for reboot scenarios to save and restore. @yxieca , do you remember?

@prsunny prsunny requested a review from yxieca January 25, 2021 22:09
@abdosi
Copy link
Contributor Author

abdosi commented Jan 25, 2021

I think the ebtable service was added for reboot scenarios to save and restore. @yxieca , do you remember?

It should get restored with this also as part of database docker init

@abdosi
Copy link
Contributor Author

abdosi commented Jan 26, 2021

@yxieca can you please review this.

@yxieca
Copy link
Contributor

yxieca commented Jan 26, 2021

I think the ebtable service was added for reboot scenarios to save and restore. @yxieca , do you remember?

It should get restored with this also as part of database docker init

@abdosi the rules in dockers will be restored upon database docker start. We don't have scenario to restart database docker, do we have a risk of losing the configuration and not reapplied? I guess the service is an oneshot service so we are not regressing anything.

Also found out that Guohan added the service because ebtable is not enabled by default on buster.

@abdosi
Copy link
Contributor Author

abdosi commented Jan 26, 2021

I think the ebtable service was added for reboot scenarios to save and restore. @yxieca , do you remember?

It should get restored with this also as part of database docker init

@abdosi the rules in dockers will be restored upon database docker start. We don't have scenario to restart database docker, do we have a risk of losing the configuration and not reapplied? I guess the service is an oneshot service so we are not regressing anything.

Also found out that Guohan added the service because ebtable is not enabled by default on buster.

@yxieca Verified load minigraph and config reload where we don't start database docker configuration are not lost.

@abdosi
Copy link
Contributor Author

abdosi commented Jan 26, 2021

@lguohan waiting on this check Azure.sonic-buildimage (Test vstest) ? What need to be done.

@lguohan lguohan merged commit cfa8fbb into sonic-net:master Jan 27, 2021
@lguohan
Copy link
Collaborator

lguohan commented Jan 27, 2021

better to add a test in sonic-mgmt test to compare the ebtables with expected value.

@abdosi abdosi deleted the ebtables branch January 28, 2021 00:47
abdosi added a commit that referenced this pull request Jan 28, 2021
Following changes were done for ebtables:

- Support for Multi-asic platforms. Ebtable filters are installed in namespace for multi-asic and not host. On Single asic installed on  host.

- For Multi-asic platforms we don't want to install on host otherwise Namespace-to-Namespace communication does not happens since ARP Request are not forwarded.

- Updated to use text file to restore ebtables rules then the binary format. Rules are restore as part of Database docker init instead of rc.local

- Removed the ebtable service files for buster as not needed as filters are restored/installed as part of database docker init.
   All the binaries are pre-installed with ebtables* binary are same as ebatbles-legacy-*

Signed-off-by: Abhishek Dosi <[email protected]>
@abdosi
Copy link
Contributor Author

abdosi commented Feb 6, 2021

better to add a test in sonic-mgmt test to compare the ebtables with expected value.

@lguohan sonic-net/sonic-mgmt#2944

lguohan pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Feb 6, 2021
- Added testcase to verify ebtable rules. To verify changes done as part of sonic-net/sonic-buildimage#6542
- Added to T0 kvmtest.sh
- Will enhance for multi-asic in another PR.

Signed-off-by: Abhishek Dosi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants