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

Natmgrd changes in sonic-swss sub module to support NAT feature. #1059

Merged
merged 12 commits into from
Jan 16, 2020

Conversation

AkhileshSamineni
Copy link
Contributor

@AkhileshSamineni AkhileshSamineni commented Sep 18, 2019

Added natmgr, and intfmgr changes.

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

Depends on:
sonic-swss : #1126 and #1125
sonic-swss-common : sonic-net/sonic-swss-common#304
sonic-linux-kernel : sonic-net/sonic-linux-kernel#100
sonic-sairedis : sonic-net/sonic-sairedis#519

Signed-off-by: Akhilesh Samineni [email protected]

{
FieldValueTuple t("nat_zone", nat_zone);
fvVector.push_back(t);
m_appIntfTableProducer.set(alias, fvVector);
Copy link
Collaborator

Choose a reason for hiding this comment

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

set is at the end of outer if (!is_lo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set it specific to nat_zone only, so using new "fvVector" variable for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After resolved merge conflicts, doing set at the end of outer if(!is_lo).

m_cfgNatPoolTable(cfgDb, CFG_NAT_POOL_TABLE_NAME),
m_cfgNatBindingsTable(cfgDb, CFG_NAT_BINDINGS_TABLE_NAME),
m_cfgNatGlobalTable(cfgDb, CFG_NAT_GLOBAL_TABLE_NAME),
m_cfgInterfaceTable(cfgDb, CFG_INTF_TABLE_NAME),
Copy link
Collaborator

Choose a reason for hiding this comment

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

intfmgr is responsible for that table

Copy link
Contributor Author

@AkhileshSamineni AkhileshSamineni Nov 11, 2019

Choose a reason for hiding this comment

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

I have removed these lines, but NatMgr is also a consumer for the Interface Tables, to get the configured nat_zone value on every interface, which will be used as a mark value (zone +1 ) in iptables rules.

Comment on lines 42 to 46
m_cfgLagInterfaceTable(cfgDb, CFG_LAG_INTF_TABLE_NAME),
m_cfgVlanInterfaceTable(cfgDb, CFG_VLAN_INTF_TABLE_NAME),
m_cfgLoopbackInterfaceTable(cfgDb, CFG_LOOPBACK_INTERFACE_TABLE_NAME),
m_cfgNatAclTable(cfgDb, CFG_ACL_TABLE_NAME),
m_cfgNatAclRuleTable(cfgDb, CFG_ACL_RULE_TABLE_NAME),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Each table has a single manager that should handle it, and these table don't belong to NAT manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed these lines, but NatMgr is also a consumer for the Interface Tables, to get the configured nat_zone value on every interface, which will be used as a mark value (zone +1 ) in iptables rules and ACL tables as well.


/* Clean the NAT iptables */
std::string res;
const std::string cmds = std::string("") + IPTABLES_CMD + " -F -t nat ";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why clear iptables before start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the NAT application, wanted to clearing any stale nat iptables rules.

}

/* To check the give global_ip is withing the Prefix subnet or not */
bool NatMgr::isGlobalIpMatching(const string &prefix, const string &global_ip)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would fit as a method in IpPrefix class

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, will move it to IpPrefix class in next patch.

char ipAddr[INET_ADDRSTRLEN];
std::vector<swss::FieldValueTuple> values;

if (!port_range.empty() and (port_range != "NULL"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

First check if not NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, When pool is created with only Ip address, "nat_port" value is set to "NULL" string like below

root@sonic:/home/admin# config nat add pool p1 65.55.42.5
root@sonic:/home/admin# redis-cli -n 4
127.0.0.1:6379[4]> keys NA

  1. "NAT_POOL|p1"
    127.0.0.1:6379[4]> hgetall NAT_POOL|p1
  2. "nat_port"
  3. "NULL"
  4. "nat_ip"
  5. "65.55.42.5"
    127.0.0.1:6379[4]> exit
    root@sonic:/home/admin#

ipv4_addr_low = ntohl(ipv4_addr_low);
}

for (ip = ipv4_addr_low; ip <= ipv4_addr_high; ip++)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why set like this and not as a range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here m_appNaptPoolIpTable is used by NatSyncd code to know whether the packet notified by conntrack table is NAPT'ed or NAT'ed.

For example, Create a NAT pool to do NAPT'ed like "config nat add pool p1 65.55.42.5 100-200"
If packet comes to cpu with Src Ip as 12.0.0.1 and Src port as 100, it get's SNAPT'ed to Src Ip as 65.55.42.5 and Src Port as 100, kernel picks the "100" as it is available from Pool.
So kernel doesn't set any separate flags in the conntrack entry if it is NAPT'ed. Only SNAT/DNAT flags are set in the conntrack entry that is notified to natsyncd.
When the translated port number is the same, Natsyncd will not be able to say if it is NAT'ed or NAPT'ed. So the NaptPoolIpTable is added for every IP address in the pool to help NatSyncd pick the conntrack notifications as NAPT and add them as NAPT entries in the APP_DB.

Copy link
Collaborator

@marian-pritsak marian-pritsak left a comment

Choose a reason for hiding this comment

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

The PR is very substantial. Can you please split it by the daemon it is related to? I think it won't break it.

@AkhileshSamineni
Copy link
Contributor Author

The PR is very substantial. Can you please split it by the daemon it is related to? I think it won't break it.

@marian-pritsak Yes right, Created PRs for orchanget (#1125 ) and Natsyncd (#1126 ).

@AkhileshSamineni AkhileshSamineni changed the title Changes in sonic-swss sub module to support NAT feature. Natmgrd changes in sonic-swss sub module to support NAT feature. Nov 11, 2019
@lguohan
Copy link
Contributor

lguohan commented Nov 13, 2019

        if (inet_pton(AF_INET6, ipPrefixKeys[0].c_str(), &ipv6_addr))

we have ipaddress class in sonic swss common, can you use that?


Refers to: cfgmgr/natmgr.cpp:6628 in d7113d6. [](commit_id = d7113d6, deletion_comment = False)

@AkhileshSamineni
Copy link
Contributor Author

        if (inet_pton(AF_INET6, ipPrefixKeys[0].c_str(), &ipv6_addr))

we have ipaddress class in sonic swss common, can you use that?

Refers to: cfgmgr/natmgr.cpp:6628 in d7113d6. [](commit_id = d7113d6, deletion_comment = False)

Yes, added changes to use IpAddress class here.

Copy link
Contributor

@arlakshm arlakshm left a comment

Choose a reason for hiding this comment

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

Are this pytest changes present in PR 1125. I could not fine them. Kindly confirm.

@AkhileshSamineni
Copy link
Contributor Author

Are this pytest changes present in PR 1125. I could not fine them. Kindly confirm.

Added pytest changes in PR 1125.
Here is the link - 69d1b68

  - Added natmgr and natorch changes.
  - Added nat Zone related changes.
  - Added Warm reboot changes.

  Signed-off-by: Akhilesh Samineni <[email protected]>
Signed-off-by: Akhilesh Samineni <[email protected]>
Signed-off-by: Akhilesh Samineni <[email protected]>
Signed-off-by: Akhilesh Samineni <[email protected]>
Signed-off-by: Akhilesh Samineni <[email protected]>
Signed-off-by: Akhilesh Samineni <[email protected]>
@AkhileshSamineni
Copy link
Contributor Author

Retest this please.

@AkhileshSamineni
Copy link
Contributor Author

retest this please.

@AkhileshSamineni
Copy link
Contributor Author

Retest this please.

@rlhui rlhui merged commit 85036df into sonic-net:master Jan 16, 2020
lguohan added a commit that referenced this pull request Jan 30, 2020
abdosi pushed a commit that referenced this pull request Feb 4, 2020
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…#1059)

Filter fdb was wiping out IPv4 entries when both IPv4 and IPv6
are associated with VLan interface. The reason is IPv6 network
was overwriting IPv4 network. This pr add support to filter
both IPv4 and IPv6 addresses

signed-off-by: Tamer Ahmed <[email protected]>
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
The previous regex can only match one device so that the original MACsec devices cannot been cleanup by config reload.
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.

7 participants