Changes in swss-common submodule to support NAT feature.#304
Changes in swss-common submodule to support NAT feature.#304lguohan merged 5 commits intosonic-net:masterfrom
Conversation
* Add new tables in CONFIG_DB, APP_DB, COUNTERS_DB * Netlink class for abstracting netfilter application socket Signed-off-by: [email protected]
kirankella
force-pushed
the
nat_changes_in_sonic
branch
from
16ef26f to
f4d37fb
Compare
| { | ||
| if (nfnl_ct_add(m_socket, ct, NLM_F_REPLACE) < 0) | ||
| { | ||
| SWSS_LOG_ERROR("Failed to update conntrack object in the kernel"); |
There was a problem hiding this comment.
Suggest to SWSS_LOG_THROW or even better return a boolean whether API succeeded or not?
common/nfnetlink.cpp
Outdated
| int err = nl_socket_add_membership(m_socket, nfnlGroup); | ||
| if (err < 0) | ||
| { | ||
| SWSS_LOG_ERROR("Unable to register to netfilter group %d: %s", nfnlGroup, |
There was a problem hiding this comment.
Is it better to use SWSS_LOG_THROW - shortcut to log error and to throw?
common/nfnetlink.h
Outdated
| NfNetlink(int pri = 0); | ||
| ~NfNetlink() override; | ||
|
|
||
| void registerRecvCallbacks(void); |
There was a problem hiding this comment.
Could you please remove void parameter?. Seems it is legacy from C-code and in this file such style is not used expect this method
| struct nfnl_ct *getCtObject(const IpAddress &sourceIpAddr); | ||
| struct nfnl_ct *getCtObject(uint8_t protoType, const IpAddress &sourceIpAddr, uint16_t srcPort); | ||
|
|
||
| static int onNetlinkRcv(struct nl_msg *msg, void *arg); |
There was a problem hiding this comment.
put onNetlinkRcv declaration in #ifdef NETFILTER_UNIT_TEST
common/nfnetlink.h
Outdated
|
|
||
| private: | ||
| struct nfnl_ct *getCtObject(const IpAddress &sourceIpAddr); | ||
| struct nfnl_ct *getCtObject(uint8_t protoType, const IpAddress &sourceIpAddr, uint16_t srcPort); |
There was a problem hiding this comment.
These are private and not used here?
There was a problem hiding this comment.
Were added initially during development, now these can be removed. Good catch.
|
|
||
| namespace swss { | ||
|
|
||
| class NfNetlink : public Selectable { |
There was a problem hiding this comment.
A lot of code duplicates Netlink class, did you consider to inherit from Netlink?
There was a problem hiding this comment.
Inheriting from Netlink class was considered, but the Netlink class is specific to NETLINK_ROUTE and initializes m_socket member to route socket in its constructor. But NfNetlink class is specific to NETFILTER socket and Netlink class constructor invocation would result in unnecessary calls specific Route socket.
There was a problem hiding this comment.
I think the difference is not big, one thing you can do is to define virtual private "connect" method and override it for NfNetlink to call nfnl_connect.
There was a problem hiding this comment.
@stepanblyschak Are you saying, we have connect() as virtual function which is overridden by derived NfNetlink class, and call connect() in NetLink() construtor in the place of nl_connect() in the current code?
If so, since the virtual function connect() is called in the NetLink constructor, it would resolve to NetLink::connect() instead of NfNetlink::connect().
We need to get the connect() call out of the NetLink constructor and have the users of the NetLink class call the connect() explicitly.
| nl_socket_set_nonblocking(m_socket); | ||
|
|
||
| /* Set socket buffer size to 10 MB */ | ||
| nl_socket_set_buffer_size(m_socket, 10485760, 0); |
There was a problem hiding this comment.
Make the buffer configurable from class user?
There was a problem hiding this comment.
added an API to the class for setting the send/recv socket buffer size.
- Added natsyncd and warmboot related changes. Link to NAT HLD : https://github.com/Azure/SONiC/blob/master/doc/nat/nat_design_spec.md Depends on: sonic-swss : 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
- Added natsyncd and warmboot related changes. Link to NAT HLD : https://github.com/Azure/SONiC/blob/master/doc/nat/nat_design_spec.md Depends on: sonic-swss : 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
Add warning/critical thresholds for PSU power Signed-off-by: Stephen Sun <[email protected]>
6 participants
Link to NAT HLD:
https://github.com/kirankella/SONiC/blob/nat_doc_changes/doc/nat/nat_design_spec.md
Signed-off-by: [email protected]