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

[SNMP]: SONiC SNMP Changes to support IPv6 #1457

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

SuvarnaMeenakshi
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi commented Sep 1, 2023

Document captures the changes required to support SNMP over IPv6 for single asic platforms.

  1. [SNMP][IPv6]: Fix SNMP IPv6 reachability issue in certain scenarios [SNMP][IPv6]: Fix SNMP IPv6 reachability issue in certain scenarios sonic-buildimage#15487
  2. [SNMP][IPv6]: Fix to use link local IPv6 address as snmp agentAddress [SNMP][IPv6]: Fix to use link local IPv6 address as snmp agentAddress sonic-buildimage#16013
  3. [SNMP]: Modify minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG [SNMP]: Modify minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG sonic-buildimage#17045

Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>


### Effects of this change ###
1. By default, SNMP will listen on Management and Loopback0 IPs configured via config_db.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This new change may not work for all SONiC users, can introduce backward compatibility issues as we dont have any issue with IPv4 case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified to be backward compatible


**Possible solution**

Update STATE_DB with DHCP configured management IP address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hope you are fixing this as part of the initial pull request itself, otherwise we're breaking the working feature when the DHCP is desired for the mgmt interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required with current suggested change

@zhangyanzhao
Copy link
Collaborator

Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>
Copy link
Collaborator

@venkatmahalingam venkatmahalingam left a comment

Choose a reason for hiding this comment

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

We should make sure that the backward compatibility issues are addressed properly before merging this HLD.

SuvarnaMeenakshi added a commit to sonic-net/sonic-mgmt that referenced this pull request Sep 28, 2023
What is the motivation for this PR?
Skip SNMP IPv6 related test cases in 202211,202205 and 202305 branches until the approach to fix IPv6 issue is fixed.
PR contains details of the issue and approach sonic-net/SONiC#1457

How did you do it?
Skip single asic IPv6 SNMP loopback test case and link local test case in branches with the testcase added.

How did you verify/test it?
Tested on 202205 single asic VS image
SuvarnaMeenakshi added a commit to SuvarnaMeenakshi/sonic-mgmt that referenced this pull request Oct 6, 2023
…#10097)

What is the motivation for this PR?
Skip SNMP IPv6 related test cases in 202211,202205 and 202305 branches until the approach to fix IPv6 issue is fixed.
PR contains details of the issue and approach sonic-net/SONiC#1457

How did you do it?
Skip single asic IPv6 SNMP loopback test case and link local test case in branches with the testcase added.

How did you verify/test it?
Tested on 202205 single asic VS image

(cherry picked from commit a72a4db)
SuvarnaMeenakshi added a commit to SuvarnaMeenakshi/sonic-mgmt that referenced this pull request Oct 6, 2023
…t#10097)

What is the motivation for this PR?
Skip SNMP IPv6 related test cases in 202211,202205 and 202305 branches until the approach to fix IPv6 issue is fixed.
PR contains details of the issue and approach sonic-net/SONiC#1457

How did you do it?
Skip single asic IPv6 SNMP loopback test case and link local test case in branches with the testcase added.

How did you verify/test it?
Tested on 202205 single asic VS image

(cherry picked from commit a72a4db)
qiluo-msft pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Oct 6, 2023
…10238)

What is the motivation for this PR?
cherry pick of #10097
Skip SNMP IPv6 related test cases in 202211,202205 and 202305 branches until the approach to fix IPv6 issue is fixed. PR contains details of the issue and approach sonic-net/SONiC#1457

How did you do it?
Skip single asic IPv6 SNMP loopback test case and link local test case in branches with the testcase added.

How did you verify/test it?
Tested on 202205 single asic VS image

(cherry picked from commit a72a4db)
@SuvarnaMeenakshi
Copy link
Contributor Author

SuvarnaMeenakshi commented Oct 10, 2023

@venkatmahalingam , @dgsudharsan , @qiluo-msft , thank you for reviewing.
Based on the feedback, one approach that I would like your input for is instead of modifying snmpd.conf.j2 default behavior to get the MGMT and Loopback IP, if we keep the snmpd.conf.j2 as it was and modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db.
This way, if we load a minigraph with Management and Looopback IP, that will get used and snmpd will listen on those IPs.
If we use config_db, then snmpd will listen on IPs in SNMP_AGENT_ADDRESS_CONFIG if that table is present, if not it will listen on any IP. This was the initial behavior.
Here is a diff for the proposal SuvarnaMeenakshi/sonic-buildimage@d1a9fa3

wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Oct 10, 2023
…10244)

(cherry picked from commit a72a4db)

What is the motivation for this PR?
Skip SNMP IPv6 related test cases in 202211,202205 and 202305 branches until the approach to fix IPv6 issue is fixed.
PR contains details of the issue and approach sonic-net/SONiC#1457

How did you do it?
Skip single asic IPv6 SNMP loopback test case and link local test case in branches with the testcase added.

How did you verify/test it?
Tested on 202205 single asic VS image

(cherry picked from commit a72a4db)
@zhangyanzhao
Copy link
Collaborator

@SuvarnaMeenakshi can you please add the code PRs to this HLD? Thanks.

@dgsudharsan
Copy link
Contributor

@venkatmahalingam , @dgsudharsan , @qiluo-msft , thank you for reviewing. Based on the feedback, one approach that I would like your input for is instead of modifying snmpd.conf.j2 default behavior to get the MGMT and Loopback IP, if we keep the snmpd.conf.j2 as it was and modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db. This way, if we load a minigraph with Management and Looopback IP, that will get used and snmpd will listen on those IPs. If we use config_db, then snmpd will listen on IPs in SNMP_AGENT_ADDRESS_CONFIG if that table is present, if not it will listen on any IP. This was the initial behavior. Here is a diff for the proposal SuvarnaMeenakshi/sonic-buildimage@d1a9fa3

Hi @SuvarnaMeenakshi
If I understand, this flow will get executed only when someone calls config load minigraph. If there is an upgrade flow, this will not get executed, so backward compatibility is maintained. Similarly if someone creates config_db.json directly and performs reboot, this flow will not get triggered. Can you confirm this?

@SuvarnaMeenakshi
Copy link
Contributor Author

rly if someone creates config_db.json directly and perf

@venkatmahalingam , @dgsudharsan , @qiluo-msft , thank you for reviewing. Based on the feedback, one approach that I would like your input for is instead of modifying snmpd.conf.j2 default behavior to get the MGMT and Loopback IP, if we keep the snmpd.conf.j2 as it was and modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db. This way, if we load a minigraph with Management and Looopback IP, that will get used and snmpd will listen on those IPs. If we use config_db, then snmpd will listen on IPs in SNMP_AGENT_ADDRESS_CONFIG if that table is present, if not it will listen on any IP. This was the initial behavior. Here is a diff for the proposal SuvarnaMeenakshi/sonic-buildimage@d1a9fa3

Hi @SuvarnaMeenakshi If I understand, this flow will get executed only when someone calls config load minigraph. If there is an upgrade flow, this will not get executed, so backward compatibility is maintained. Similarly if someone creates config_db.json directly and performs reboot, this flow will not get triggered. Can you confirm this?

During upgrade_flow, if the config_db.json file is present, then backward compatibility will be maintained.
Yes, if someone is using config_db.json directly and performing a reboot then this will not be triggered and snmp will listen on any IP.
Only when load_minigraph is issued this flow will be triggered and snmp will listen on loopback and mgmt IPs configured in the minigraph.xml.

@venkatmahalingam
Copy link
Collaborator

modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db.

Thanks @SuvarnaMeenakshi, this looks good.

@SuvarnaMeenakshi
Copy link
Contributor Author

modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db.

Thanks @SuvarnaMeenakshi, this looks good.

@dgsudharsan @venkatmahalingam thank you, I have code PR here sonic-net/sonic-buildimage#17045

Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>
@SuvarnaMeenakshi
Copy link
Contributor Author

@SuvarnaMeenakshi can you please add the code PRs to this HLD? Thanks.

Updated PR in the document.

modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db.

Thanks @SuvarnaMeenakshi, this looks good.

@dgsudharsan @venkatmahalingam thank you, I have code PR here sonic-net/sonic-buildimage#17045

modify the minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG config table with MGMT and LOOPBACK IP in config_db.

Thanks @SuvarnaMeenakshi, this looks good.

@dgsudharsan @venkatmahalingam thank you, I have code PR here sonic-net/sonic-buildimage#17045

@dgsudharsan @venkatmahalingam Please help review this document PR

@SuvarnaMeenakshi
Copy link
Contributor Author

@qiluo-msft please help review and merge this.

## Motivation ##

SNMP query over IPv6 address fails in certain scenarios on single asic platforms.
Ideally, SNMP over IPv6 should be successful over both IPv4 and IPv6.
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 28, 2023

Choose a reason for hiding this comment

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

Could you rephase "SNMP over IPv6 should be successful over both IPv4 and IPv6"? #Closed


1. [SNMP][IPv6]: Fix SNMP IPv6 reachability issue in certain scenarios https://github.com/sonic-net/sonic-buildimage/pull/15487
2. [SNMP][IPv6]: Fix to use link local IPv6 address as snmp agentAddress https://github.com/sonic-net/sonic-buildimage/pull/16013
3. [SNMP]: Modify minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG https://github.com/sonic-net/sonic-buildimage/pull/17045
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 28, 2023

Choose a reason for hiding this comment

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

Since this issue is only on single asic, why fixing multi asic in this PR? #Closed

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, only on single-asic, not fixing for multi-asic in this PR.

Rephrase incorrect statement in motivation section.

Signed-off-by: Suvarna Meenakshi <sumeenak@microsoft.com>
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM, please also check with other reviewers.

@SuvarnaMeenakshi
Copy link
Contributor Author

@qiluo-msft can you help merge this PR?
We have approval from Dell team.

qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Dec 6, 2023
…able (#17045)

#### Why I did it
SNMP query over IPv6 does not work due to issue in net-snmp where IPv6 query does not work on multi-nic environment.
To get around this, if snmpd listens on specific ipv4 or ipv6 address, then the issue is not seen.
We plan to configure Management IP and Loopback IP configured in minigraph.xml as SNMP_AGENT_ADDRESS in config_db., based on changes discussed in sonic-net/SONiC#1457.

##### Work item tracking
- Microsoft ADO **(number only)**:26091228

#### How I did it
Modify minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG with management and Loopback0 IP addresses.
Modify snmpd.conf.j2 to use SNMP_AGENT_ADDRESS_CONFIG table if it is present in config_db, if not listen on any IP.
Main change:
1. if minigraph.xml is used to configure the device, then snmpd will listen on mgmt and loopback IP addresses,
2. if config_db is used to configure the device, snmpd will listen IP present in SNMP_AGENT_ADDRESS_CONFIG  if that table is present, if table is not present snmpd will listen on any IP.
#### How to verify it
config_db.json created from minigraph.xml for single asic VS image with mgmt and Loopback IP addresses.
```
    "SNMP_AGENT_ADDRESS_CONFIG": {
        "10.1.0.32|161|": {},
        "10.250.0.101|161|": {},
        "FC00:1::32|161|": {},
        "fec0::ffff:afa:1|161|": {}
    },
 .....
 
 snmpd listening on the above IP addresses:
 admin@vlab-01:~$ sudo netstat -tulnp | grep 161
tcp        0      0 127.0.0.1:3161          0.0.0.0:*               LISTEN      71522/snmpd         
udp        0      0 10.250.0.101:161        0.0.0.0:*                           71522/snmpd         
udp        0      0 10.1.0.32:161           0.0.0.0:*                           71522/snmpd         
udp6       0      0 fec0::ffff:afa:1:161    :::*                                71522/snmpd         
udp6       0      0 fc00:1::32:161          :::*                                71522/snmpd  
```
@SuvarnaMeenakshi
Copy link
Contributor Author

@dgsudharsan can you approve the doc PR

@SuvarnaMeenakshi
Copy link
Contributor Author

@dgsudharsan can you approve the doc PR

Merging the PR based on discussion in the PR, thanks.

@SuvarnaMeenakshi SuvarnaMeenakshi merged commit fb78b2a into sonic-net:master Dec 7, 2023
1 check passed
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Feb 2, 2024
…able (sonic-net#17045)

#### Why I did it
SNMP query over IPv6 does not work due to issue in net-snmp where IPv6 query does not work on multi-nic environment.
To get around this, if snmpd listens on specific ipv4 or ipv6 address, then the issue is not seen.
We plan to configure Management IP and Loopback IP configured in minigraph.xml as SNMP_AGENT_ADDRESS in config_db., based on changes discussed in sonic-net/SONiC#1457.

##### Work item tracking
- Microsoft ADO **(number only)**:26091228

#### How I did it
Modify minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG with management and Loopback0 IP addresses.
Modify snmpd.conf.j2 to use SNMP_AGENT_ADDRESS_CONFIG table if it is present in config_db, if not listen on any IP.
Main change:
1. if minigraph.xml is used to configure the device, then snmpd will listen on mgmt and loopback IP addresses,
2. if config_db is used to configure the device, snmpd will listen IP present in SNMP_AGENT_ADDRESS_CONFIG  if that table is present, if table is not present snmpd will listen on any IP.
#### How to verify it
config_db.json created from minigraph.xml for single asic VS image with mgmt and Loopback IP addresses.
```
    "SNMP_AGENT_ADDRESS_CONFIG": {
        "10.1.0.32|161|": {},
        "10.250.0.101|161|": {},
        "FC00:1::32|161|": {},
        "fec0::ffff:afa:1|161|": {}
    },
 .....
 
 snmpd listening on the above IP addresses:
 admin@vlab-01:~$ sudo netstat -tulnp | grep 161
tcp        0      0 127.0.0.1:3161          0.0.0.0:*               LISTEN      71522/snmpd         
udp        0      0 10.250.0.101:161        0.0.0.0:*                           71522/snmpd         
udp        0      0 10.1.0.32:161           0.0.0.0:*                           71522/snmpd         
udp6       0      0 fec0::ffff:afa:1:161    :::*                                71522/snmpd         
udp6       0      0 fc00:1::32:161          :::*                                71522/snmpd  
```
mssonicbld pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Feb 2, 2024
…able (#17045)

#### Why I did it
SNMP query over IPv6 does not work due to issue in net-snmp where IPv6 query does not work on multi-nic environment.
To get around this, if snmpd listens on specific ipv4 or ipv6 address, then the issue is not seen.
We plan to configure Management IP and Loopback IP configured in minigraph.xml as SNMP_AGENT_ADDRESS in config_db., based on changes discussed in sonic-net/SONiC#1457.

##### Work item tracking
- Microsoft ADO **(number only)**:26091228

#### How I did it
Modify minigraph parser to update SNMP_AGENT_ADDRESS_CONFIG with management and Loopback0 IP addresses.
Modify snmpd.conf.j2 to use SNMP_AGENT_ADDRESS_CONFIG table if it is present in config_db, if not listen on any IP.
Main change:
1. if minigraph.xml is used to configure the device, then snmpd will listen on mgmt and loopback IP addresses,
2. if config_db is used to configure the device, snmpd will listen IP present in SNMP_AGENT_ADDRESS_CONFIG  if that table is present, if table is not present snmpd will listen on any IP.
#### How to verify it
config_db.json created from minigraph.xml for single asic VS image with mgmt and Loopback IP addresses.
```
    "SNMP_AGENT_ADDRESS_CONFIG": {
        "10.1.0.32|161|": {},
        "10.250.0.101|161|": {},
        "FC00:1::32|161|": {},
        "fec0::ffff:afa:1|161|": {}
    },
 .....
 
 snmpd listening on the above IP addresses:
 admin@vlab-01:~$ sudo netstat -tulnp | grep 161
tcp        0      0 127.0.0.1:3161          0.0.0.0:*               LISTEN      71522/snmpd         
udp        0      0 10.250.0.101:161        0.0.0.0:*                           71522/snmpd         
udp        0      0 10.1.0.32:161           0.0.0.0:*                           71522/snmpd         
udp6       0      0 fec0::ffff:afa:1:161    :::*                                71522/snmpd         
udp6       0      0 fc00:1::32:161          :::*                                71522/snmpd  
```
@zhangyanzhao
Copy link
Collaborator

back ported to previous releases as well

@skg-net
Copy link
Member

skg-net commented Feb 6, 2024

@SuvarnaMeenakshi Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md
Thanks

@SuvarnaMeenakshi
Copy link
Contributor Author

@SuvarnaMeenakshi Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md Thanks

Sure, will check the quality metric and add the required details. Is this a new additional requirement for new features or changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants