caclmgrd: monitor state_db to update dhcp acl · Pull Request #8222 · sonic-net/sonic-buildimage

ghost · 2021-07-19T17:23:28Z

caclmgrd: monitor mux_cable_table in state_db to update dhcp acl

if the state changes to 'standby', add acl to block dhcp packets based on ingress interfaces
if the state changes to 'active', delete acl
if the state changes to 'unknown', also delete acl to avoid potential disconnect
both addition and deletion follow checking the existence of the rules

The change has been verified on a virtual switch based testbed.

lguohan · 2021-07-19T20:28:37Z

missing unit test.

prsunny

Please provide details of the setting up dhcp chain/acl rule in the description. Need more clarifications on the approach taken.

prsunny · 2021-07-19T22:58:31Z

src/sonic-host-services/scripts/caclmgrd

-
-        # Add iptables command to delete all non-default chains
-        iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + "iptables -X")
+        # Add iptables command to flush the current rules and delete all non-default chains


what is this change for?, if caclmgrd restarts, can we flush as before?

This is to shield DHCP related rule in separate chain list. When flushing, exclude DHCP chain and flush/delete all other chains other than built-in chains.

yes, if caclmgrd restarts, we flush all.

prsunny · 2021-07-19T23:01:20Z

src/sonic-host-services/scripts/caclmgrd


+        # Add iptables commands to link the DCHP chain to block dhcp packets based on ingress interfaces
+        if self.DualToR:
+            iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + "iptables -A INPUT -p udp --dport 67 -j DHCP")


Are the below rules required for dualtor? Also why not port 68?

this is to direct matching into DHCP chain, dhcp discovers/requests use dport 67

tahmed-dev · 2021-07-20T00:40:15Z

src/sonic-host-services/scripts/caclmgrd

-
-        # Add iptables command to delete all non-default chains
-        iptables_cmds.append(self.iptables_cmd_ns_prefix[namespace] + "iptables -X")
+        # Add iptables command to flush the current rules and delete all non-default chains


This is to shield DHCP related rule in separate chain list. When flushing, exclude DHCP chain and flush/delete all other chains other than built-in chains.

src/sonic-host-services/scripts/caclmgrd

tahmed-dev · 2021-07-20T00:45:54Z

src/sonic-host-services/scripts/caclmgrd


+    def setup_dhcp_chain(self, namespace):
+        all_chains = self.get_chain_list(self.iptables_cmd_ns_prefix[namespace], [""])
+        dhcp_chain_exist = 1 if "DHCP" in all_chains else 0


what about using boolean?
dhcp_chain_exist = "DHCP" in all_chains

tahmed-dev · 2021-07-20T00:46:11Z

src/sonic-host-services/scripts/caclmgrd


    UPDATE_DELAY_SECS = 0.5

+    DualToR = 0


ghost · 2021-08-03T00:45:51Z

retest this please

ghost · 2021-08-03T00:47:46Z

/Azp run

azure-pipelines · 2021-08-03T00:48:03Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2021-08-03T18:02:48Z

/Azp run

azure-pipelines · 2021-08-03T18:03:07Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2021-08-05T15:08:01Z

src/sonic-host-services/scripts/caclmgrd

+                            break
+                        self.log_info("mux cable update : '%s'" % str((key, op, fvs)))
+                        self.update_dhcp_acl(key, op, dict(fvs))
+                continue


Can you confirm this flow is tested on a non-dualtor testbed?

yes, i tested it with both subtype=dualtor and none on a vs testbed

prsunny · 2021-08-06T15:47:17Z

Is unit test separate PR? or are you planning to add to this?

prsunny · 2021-08-06T15:51:35Z

Is unit test separate PR? or are you planning to add to this?

Ok, I see this PR - #8359. You could have it in same PR, FYI

ghost · 2021-08-07T04:41:04Z

/Azp run

azure-pipelines · 2021-08-07T04:41:21Z

Azure Pipelines successfully started running 1 pipeline(s).

caclmgrd: monitor mux_cable_table in state_db to update dhcp acl - if the state changes to 'standby', add acl to block dhcp packets based on ingress interfaces - if the state changes to 'active', delete acl - if the state changes to 'unknown', also delete acl to avoid potential disconnect - both addition and deletion follow checking the existence of the rules The change has been verified on a virtual switch based testbed. Port to 202012 branch from #8222

Fix #8672 add two missing commits in caclmgrd: monitor state_db to update dhcp acl #8222 when porting to 202012 branch

dhcp acl

7ff74c1

ghost requested a review from lguohan as a code owner July 19, 2021 17:23

ghost requested review from abdosi, prsunny and tahmed-dev July 19, 2021 17:23

ghost added DHCP Relay Control Plane ACL labels Jul 19, 2021

prsunny reviewed Jul 19, 2021

View reviewed changes

tahmed-dev reviewed Jul 20, 2021

View reviewed changes

update

0ae67a0

ghost requested review from prsunny and tahmed-dev August 2, 2021 22:45

Tianrong Zhang added 2 commits August 2, 2021 18:26

use state as key

b629ff8

update subprocess call

1078f75

tahmed-dev previously approved these changes Aug 4, 2021

View reviewed changes

prsunny reviewed Aug 5, 2021

View reviewed changes

ghost requested a review from prsunny August 5, 2021 20:47

ghost added the Request for 202012 Branch label Aug 6, 2021

prsunny previously approved these changes Aug 6, 2021

View reviewed changes

Merge branch 'master' into dt_bdr

f7c4ad7

ghost dismissed stale reviews from prsunny and tahmed-dev via f7c4ad7 August 6, 2021 17:59

ghost requested review from prsunny and tahmed-dev August 6, 2021 18:01

tahmed-dev approved these changes Aug 6, 2021

View reviewed changes

ghost merged commit 8608af7 into sonic-net:master Aug 7, 2021

SavchukRomanLv mentioned this pull request Aug 9, 2021

caclmgrd.service FAILURE status for single ASIC device #8388

Closed

ghost deleted the dt_bdr branch August 9, 2021 18:24

ghost mentioned this pull request Aug 14, 2021

caclmgrd: monitor state_db to update dhcp acl #8477

Merged

qiluo-msft removed the Request for 202012 Branch label Aug 14, 2021

ghost mentioned this pull request Sep 3, 2021

Caclmgrd: port two commits from master to 202012 branch #8673

Merged

lguohan pushed a commit that referenced this pull request Sep 14, 2021

Caclmgrd: port two commits from master to 202012 branch (#8673)

4bdd315

Fix #8672 add two missing commits in caclmgrd: monitor state_db to update dhcp acl #8222 when porting to 202012 branch

This pull request was closed.

Conversation

ghost commented Jul 19, 2021

Uh oh!

lguohan commented Jul 19, 2021

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 3, 2021

Uh oh!

ghost commented Aug 3, 2021

Uh oh!

azure-pipelines bot commented Aug 3, 2021

Uh oh!

ghost commented Aug 3, 2021

Uh oh!

azure-pipelines bot commented Aug 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Aug 5, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Aug 6, 2021

Uh oh!

prsunny commented Aug 6, 2021

Uh oh!

ghost commented Aug 7, 2021

Uh oh!

azure-pipelines bot commented Aug 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost Aug 5, 2021 •

edited by ghost

Loading