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

[caclmgrd][DualToR] Fix a case where vlan address is not network address for DualToR Active-active configuration #95

Merged
merged 9 commits into from
Dec 12, 2023

Conversation

vdahiya12
Copy link
Contributor

@vdahiya12 vdahiya12 commented Dec 8, 2023

Work item tracking
  • Microsoft ADO (number only):
    26211097

This PR fixes #82 where a corner case is not covered, if vlan address of DUT is not network address.
Basically a src IP is added to the SNAT rule so that only packets originating from ToR with src IP as vlan IP get natted by the rule and change the src IP to LoopBack IP

For multiple vlan IP's only the vlan address coming from configuration is picked as the vlan address and not the network address.

We consider a case where there are multiple vlan IP's are example in such format :

Vlan705                 10.131.113.17/28     up/up         N/A                   N/A
Vlan805                 10.131.111.129/26    up/up         N/A                   N/A

  "VLAN_INTERFACE": {
        "Vlan705|10.131.113.17/28": {},
        "Vlan805|10.131.111.129/26": {},

In such a case we expect iptables rules should be :

target     prot opt source               destination         
SNAT       all  --  10.131.113.17        10.131.113.19        to:10.212.64.1
SNAT       all  --  10.131.111.129       10.131.111.131       to:10.212.64.1

where soc_ip's are such :

10.131.111.131, 10.131.113.19

Before this change the souce IP of the vlan is not correctly picked in the iptables rule

for DualToR Active-active configuration

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12 vdahiya12 changed the title [caclmgrd][DualToR] Fix a case where vlan address is not network addressfor DualToR Active-active configuration [caclmgrd][DualToR] Fix a case where vlan address is not network address for DualToR Active-active configuration Dec 8, 2023
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Please add an example of the issue and config in the description.

@@ -54,8 +54,10 @@ def get_ipv4_networks_from_interface_table(table, intf_name):
iface_name, iface_cidr = key
if iface_name.startswith(intf_name):
ip_ntwrk = ipaddress.ip_network(iface_cidr, strict=False)
if isinstance(ip_ntwrk, ipaddress.IPv4Network):
addresses.append(ip_ntwrk)
ip_str = iface_cidr.split("/")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check if this works if there are secondary IPs in Vlan? Like two or three different IPs in the same Vlan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works as intended, added in description

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@zjswhhh
Copy link

zjswhhh commented Dec 8, 2023

Please add an example of the issue and config in the description.

+1

@vdahiya12
Copy link
Contributor Author

Please add an example of the issue and config in the description.

added

Copy link

@zjswhhh zjswhhh left a comment

Choose a reason for hiding this comment

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

lgtm

@vdahiya12 vdahiya12 merged commit b9600f1 into sonic-net:master Dec 12, 2023
5 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-host-services that referenced this pull request Dec 15, 2023
…ess for DualToR Active-active configuration (sonic-net#95)

* [caclmgrd][DualToR] Fix a case where vlan address is not network address
for DualToR Active-active configuration

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* fix UT

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add all

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add test

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add ut

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add Tests

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add changes

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* fix typo

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add vals

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

---------

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@mssonicbld
Copy link

Cherry-pick PR to 202305: #97

@StormLiangMS
Copy link

@vdahiya12 could you update ADO in the description?

mssonicbld pushed a commit that referenced this pull request Dec 15, 2023
…ess for DualToR Active-active configuration (#95)

* [caclmgrd][DualToR] Fix a case where vlan address is not network address
for DualToR Active-active configuration

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* fix UT

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add all

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add test

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add ut

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add Tests

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add changes

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* fix typo

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

* add vals

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>

---------

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12
Copy link
Contributor Author

@vdahiya12 could you update ADO in the description?

@StormLiangMS added

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