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

Mgmt vrf namespace2 #431

Closed
wants to merge 5 commits into from
Closed

Mgmt vrf namespace2 #431

wants to merge 5 commits into from

Conversation

kannankvs
Copy link
Collaborator

@kannankvs kannankvs commented Jan 10, 2019

- What I did
Added support for management VRF using namespace solution.
Requirements that are covered are explained in the design document. Enhancements required to support tacacs and snmptrap are also added. Enhanced the configuration for using --use-mgmt-vrf for tacacs server & snmptrap server configuration on top of namespace based solution for management VRF to configure the required rules for namespace solution.
Two PRs are raised, one for sonic-buildimage (sonic-net/sonic-buildimage#2431) and other for sonic-utilities (this PR#431).
- How I did it
Added commands to enable/disable the management VRF. On enabling, it creates the management namespace, attached eth0 to management namespace, creates the required iptables rules and restarts the networking service. Detailed design is explained in the design document https://github.com/kannankvs/mvrf_namespace/blob/master/Management%20VRF%20Design%20Document%20Namespace.md. Namespace solution requires DNAT as explained in the design document. hostcfgd is enhanced to support maximum of 10 tacacs servers. Mapping between the user configured tacacs server IP/port and internally used local IP/port are maintained in this file for adding and deleting those NAT rules. For supporting snmptrap configuration, enhanced main.py & created sonic_snmp_trap_conf.py to configure the snmptrap server IP address/port and enhanced the file docker_image_ctl.j2 to create the required /usr/bin/snmp.sh script that adds the required DNAT rules during snmp service restart process.
- How to verify it
Use the following commands to enable/disable mgmt vrf and test the basic management VRF features.
config vrf add mgmt
config vrf del mgmt
config interface eth0 ip add ip/mask gatewayIP
Ex: config interface eth0 ip add 10.16.206.11/24 10.16.206.1
Using the above configuration, all applications like Ping, SSH, SCP, apt-get, etc., can be tested on management VRF using “ip netns exec mgmt COMMAND” as explained in the design document.
Use the following steps to test tacacs.

  1. First, checkout all the modified files and build an image with these changes.
  2. With the new image, enable mgmt vrf using command "config vrf add mgmt" and configure tacacs client using following commands.
    (a) config aaa authentication login tacacs+
    (b) config tacacs authtype login
    (c) config tacacs passkey testing123
    (d) config tacacs add --use-mgmt-vrf serveripaddress
  3. Configure the tacacs server accordingly.
  4. Then, do SSH to the device and verify that the user is authenticated using tacacs server via the management VRF port eth0.
    Use the following steps to test snmptrap.
  5. First, checkout all the modified files and build an image with these changes.
  6. Use the command “config snmptrap modify snmp_version snmptrapserver_ipaddress” (ex: config snmptrap modify 2 10.11.150.7) and listen for the traps in the trapserver.
  7. When the above command is configured, it restarts the snmp service. Netsnmp service sends few traps during bootup sequence that can be viewed in the trapserver.

config/main.py Outdated
def vrf_add_management_vrf():
"""Enable management vrf"""

cmd = 'sonic-cfggen -d --var-json "MGMT_VRF_CONFIG"'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov
pavel-shirshov 11 days ago Contributor
Is it possible to read this address from the config DB?
Why do you read using sonic-cfggen, but write directly to DB?

Yes we can read it from config DB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov : Addressed your comment. Removed cfggen and used config DB Read.

config/main.py Outdated

cmd = 'sonic-cfggen -d --var-json "MGMT_VRF_CONFIG"'
p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
res = p.communicate()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov
pavel-shirshov 11 days ago Contributor
I'd extract subprocess.Popen chunk as a function, otherwise you have a lot of duplicate code

Will change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov : Addressed your comment. Removed cfggen and used config DB Read. Hence, there is no duplicate code for Popen chunk.

if address in network:
return True
else:
return False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov
pavel-shirshov 11 days ago Contributor
return address in network ?
This function checks if the address is in network range or not so returning true/false.

config/main.py Outdated
@@ -876,14 +1052,42 @@ def ip(ctx):

@ip.command()
@click.argument("ip_addr", metavar="<ip_addr>", required=True)
@click.argument('gw1', metavar='<default gateway IP address>', required=False)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov
pavel-shirshov 11 days ago Contributor
should we use gw here? Why gw1?

We will change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov : Addressed your comment. Changed gw1 to gw.

config/main.py Outdated
"""Add an IP address towards the interface"""
config_db = ctx.obj["config_db"]
interface_name = ctx.obj["interface_name"]

if interface_name.startswith("Ethernet"):
config_db.set_entry("INTERFACE", (interface_name, ip_addr), {"NULL": "NULL"})
elif interface_name.startswith("eth0"):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov
pavel-shirshov 11 days ago Contributor
It could match interface 'eth01'. I think it's better to compare to 'eth' here

Will address this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov : Addressed your comment. Changed "startswith eth0", to "exact match for eth0"

config/main.py Outdated
@@ -903,6 +1107,8 @@ def remove(ctx, ip_addr):

if interface_name.startswith("Ethernet"):
config_db.set_entry("INTERFACE", (interface_name, ip_addr), None)
elif interface_name.startswith("eth0"):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov
pavel-shirshov 11 days ago Contributor
interface_name ==

Will address this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pavel-shirshov : Addressed your comment. Changed "startswith eth0", to "exact match for eth0"

…n to configDB, (2) changed gw1 to gw and (3) interface_name startswith eth0 changed to compare with eth0
@kannankvs
Copy link
Collaborator Author

no more valid

@kannankvs kannankvs closed this Apr 5, 2023
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.

2 participants