Changes in swss-utilities submodule to support NAT feature. #645

AkhileshSamineni · 2019-09-16T12:57:49Z

Added new commands to supports NAT feature

Config Commands:
config nat add static basic {global-ip} {local-ip} -nat_type {snat/dnat} -twice_nat_id {value}
config nat remove static basic {global-ip} {local-ip}
config nat add static {tcp | udp} {global-ip} {global-port} {local-ip} {local-port} -nat_type {snat | dnat} -twice_nat_id {value}
config nat remove static {tcp | udp} {global-ip} {global-port} {local-ip} {local-port}
config nat remove static all
config nat add pool {pool-name} {global-ip-range} {global-port-range}
config nat remove pool {pool-name}
config nat remove pools
config nat add binding {binding-name} {pool-name} {acl-name} -nat_type {snat | dnat} -twice_nat_id {value}
config nat remove binding {binding-name}
config nat remove bindings
config nat add interface {interface-name} {-nat_zone {zone-value}}
config nat remove interface {interface-name}
config nat remove interfaces
config nat set timeout {secs}
config nat reset timeout
config nat feature {enable | disable}
config nat set udp-timeout {secs}
config nat reset udp-timeout
config nat set tcp-timeout {secs}
config nat reset tcp-timeout

Show Commands:
show nat translations
show nat statistics
show nat config static
show nat config pool
show nat config bindings
show nat config globalvalues
show nat config zones
show nat translations count
show nat config

Clear Commands:
sonic-clear nat translations
sonic-clear nat statistics

Link to NAT HLD : https://github.com/kirankella/SONiC/blob/nat_doc_changes/doc/nat/nat_design_spec.md

Signed-off-by: akhilesh.samineni@broadcom.com

msftclas · 2019-09-16T12:58:04Z

All CLA requirements met.

arlakshm · 2019-10-01T17:46:49Z

config/nat.py

+    """Add Static NAT-related configutation"""
+
+    # Verify the ip address format 
+    if is_valid_ipv4_address(local_ip) is False:


The function is_valid_ipv4_address rejects if the ip is reserved IP address. Will this block use of some private ip ranges as local ip address ?

@arlakshm No, It won't block the private ip addresses. Function "is_reserved()" will check for IETF reserved ip addresses (Ex. 240.0.0.0/4). For private address checking, "is_private()" function is used.

Signed-off-by: akhilesh.samineni@broadcom.com

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

arlakshm · 2019-10-17T00:38:43Z

config/nat.py

+            if 'SNAT_ENTRIES' in counter_entry:
+                snat_entries = counter_entry['SNAT_ENTRIES']
+
+        if int(snat_entries) >= 1024:


This limit of 1k entries is for SNAT or DNAT or overall static nat entry limit ?
Is this a Software limit or hardware limit ?

1K is hardware limitation only, Both SNAT and DNAT limits the entries to 1k.
Single Static NAT entry creates a SNAT and DNAT entry is hardware.

Do we have a way to query this limit from HW?

Yes, Added "MAX_ENTRIES" in counter_db which will be queried from HW at natorch code.

arlakshm · 2019-10-17T00:42:27Z

config/nat.py

+    """Enbale the NAT feature """
+    config_db = ConfigDBConnector()
+    config_db.connect()
+    config_db.mod_entry("NAT_GLOBAL", "Values", {"admin_mode": "enabled"})


When NAT is enabled but no timeout is configured, what will be the default timeout value ?

For NAT entries, the default timeout is 600 seconds.
For NAPT entries, the default tcp timeout is 86400 seconds and udp timeout is 300 seconds.
When NAT is enabled, default timeouts are set in NatMgr code if timeouts are not configured before NAT enabled.

stepanblyschak

Could you please add?

unittest for new CLI commands
update command reference

stepanblyschak · 2019-10-18T09:22:10Z

config/nat.py

+from swsssdk import ConfigDBConnector
+from swsssdk import SonicV2Connector
+
+TABLE_NAME_SEPARATOR = '|'


I think TABLE_NAME_SEPERATOR should be defined somewhere in swsssdk

Yes, TABLE_NAME_SEPERATOR is defined in ConfigDBConnector from swsssdk.
Not using the TABLE_NAME_SEPERATOR, removed this variable.

stepanblyschak · 2019-10-18T09:22:54Z

config/nat.py

+        ip = ipaddress.IPv4Address(address)
+        if (ip.is_reserved) or (ip.is_multicast) or (ip.is_loopback) or (address in invalid_list):
+            return False
+    except:


Only IPv4Address creation can throw here, could you be more specific in except statement?

Added "AddressValueError" exception here.

stepanblyschak · 2019-10-18T09:23:42Z

config/nat.py

+        return False
+    try:
+        netaddr.IPNetwork(val)
+    except:


It will be good to be more specific in except

Not needed this function, removed it.

stepanblyschak · 2019-10-18T09:25:46Z

config/nat.py

+    except ValueError:
+        return False
+
+    if ((port_address < 1) or (port_address > 65535)):


if port_address not in xrange(1, 65535):

looks more python-way

stepanblyschak · 2019-10-18T09:29:19Z

config/nat.py

@@ -0,0 +1,1104 @@
+#!/usr/bin/env python -u


Why '-u' flag is needed ?

Not needed actually, removed it.

stepanblyschak · 2019-10-18T09:31:42Z

config/nat.py

+    elif interface_name.startswith("Vlan"):
+        interface_dict = config_db.get_table('VLAN')
+    elif interface_name.startswith("Loopback"):
+        return True


Don't we need to check LOOPBACK_INTERFACE table?

In this function, checking for PORT, VLAN, PORTCHANNEL tables and for Loopback we don't have a LOOPBACK table, so returning True for it.
The verification of INTERFACE tables like LOOPBACK_INTERFACE tables are happens at add_interface(..) and remove_interface(..) functions.

stepanblyschak · 2019-10-18T11:41:12Z

config/nat.py

+            if 'SNAT_ENTRIES' in counter_entry:
+                snat_entries = counter_entry['SNAT_ENTRIES']
+
+        if int(snat_entries) >= 1024:


Do we have a way to query this limit from HW?

stepanblyschak · 2019-10-18T13:17:03Z

scripts/dump_nat_entries.py

+import subprocess
+
+def main():
+    ctdumpcmd = 'conntrack -L -j > /host/warmboot/nat/nat_entries.dump'


'conntrack -L -j' output may have a lot of entries. I'm looking at https://github.com/Azure/sonic-buildimage/pull/3494/files#diff-dc200de8bc4641995c83a89e61e3c32cR51 where it is parsed using a complex regular expressions during warm start.
My concern is that parsing that output during system start can consume a lot of cpu in case of huge amount of entries. Did you test this part and impact on system warm reboot?
Right now sonic seems to have performance degradation comparing to 201811 release during system warm start causing LAG/BGP flapping on some platforms.
IMO, dumping this output in a fast-loadable format (like json) during warm start is preferred.

This option can be explored. We can do it in a separate PR ?

stepanblyschak · 2019-10-18T13:18:05Z

scripts/fast-reboot

+# Kill nat docker after saving the conntrack table
+debug "Stopping nat ..."
+/usr/bin/dump_nat_entries.py
+docker kill nat > /dev/null


To avoid systemd restart the service automatically after kill you should add 'systemctl stop nat'

stepanblyschak · 2019-10-18T13:19:04Z

scripts/fast-reboot

@@ -410,6 +410,12 @@ docker exec -i bgp pkill -9 zebra
 docker exec -i bgp pkill -9 bgpd || [ $? == 1 ]
 debug "Stopped  bgp ..."

+# Kill nat docker after saving the conntrack table
+debug "Stopping nat ..."
+/usr/bin/dump_nat_entries.py


Any reason to put this after stopping radv, bgp ? If we stop before it will have no impact on bgp restart timeout

No specific reason. moved it to before radv.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

AkhileshSamineni · 2019-11-12T07:58:13Z

Could you please add?

unittest for new CLI commands

update command reference

@stepanblyschak
Will add unit tests in separate PR.
Will add command reference in next patch.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

AkhileshSamineni · 2020-01-13T10:43:57Z

Retest this please.

AkhileshSamineni · 2020-01-20T00:16:03Z

Retest this please.

AkhileshSamineni · 2020-01-20T13:41:08Z

Retest this please.

AkhileshSamineni · 2020-01-21T13:39:49Z

Retest this please.

rlhui · 2020-01-28T02:58:39Z

retest this please

Signed-off-by: akhilesh.samineni@broadcom.com

kirankella mentioned this pull request Sep 24, 2019

Changes in sonic-sairedis repo to support the NAT feature. sonic-net/sonic-sairedis#519

Merged

arlakshm reviewed Oct 1, 2019

View reviewed changes

AkhileshSamineni added 4 commits October 16, 2019 03:58

Changes in swss-utilities submodule to support NAT feature.

638d931

Signed-off-by: akhilesh.samineni@broadcom.com

Corrected changes in setup.py file.

9745e8f

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

Minor changes to nat/py file.

59bcb26

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

Removed space to rerun the test

a3273d5

AkhileshSamineni force-pushed the nat_changes_in_sonic branch from fe51425 to a3273d5 Compare October 16, 2019 10:59

arlakshm reviewed Oct 17, 2019

View reviewed changes

stepanblyschak suggested changes Oct 18, 2019

View reviewed changes

Addressed few review comments.

72a45b5

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

AkhileshSamineni and others added 6 commits November 12, 2019 13:30

Merge branch 'master' into nat_changes_in_sonic

b66fe05

Added command Reference and addressed review comments.

b84d679

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

Merge branch 'master' into nat_changes_in_sonic

fd78867

Addressed review comments.

7761091

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>

Merge branch 'master' into nat_changes_in_sonic

a7c3274

Merge branch 'master' into nat_changes_in_sonic

6d2f1e0

rlhui assigned arlakshm Jan 27, 2020

arlakshm approved these changes Jan 28, 2020

View reviewed changes

rlhui merged commit 17c708b into sonic-net:master Jan 29, 2020

rlhui added the Request for 201911 Branch label Jan 31, 2020

lguohan added a commit that referenced this pull request Feb 3, 2020

Changes in swss-utilities submodule to support NAT feature. (#645)

6b23f19

Signed-off-by: akhilesh.samineni@broadcom.com

abdosi added the Included in 201911 Branch label Feb 4, 2020

abdosi pushed a commit that referenced this pull request Feb 4, 2020

Changes in swss-utilities submodule to support NAT feature. (#645)

2f1b644

Signed-off-by: akhilesh.samineni@broadcom.com

Changes in swss-utilities submodule to support NAT feature. #645

Changes in swss-utilities submodule to support NAT feature. #645

Conversation

AkhileshSamineni commented Sep 16, 2019

msftclas commented Sep 16, 2019 • edited Loading

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

stepanblyschak 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

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

AkhileshSamineni commented Nov 12, 2019 • edited Loading

AkhileshSamineni commented Jan 13, 2020

AkhileshSamineni commented Jan 20, 2020

AkhileshSamineni commented Jan 20, 2020

AkhileshSamineni commented Jan 21, 2020

rlhui commented Jan 28, 2020

msftclas commented Sep 16, 2019 •

edited

Loading

AkhileshSamineni commented Nov 12, 2019 •

edited

Loading