Add verification IPv4 address on LLDP conf Jinja2 Template #5699

bratashX · 2020-10-22T13:52:59Z

Signed-off-by: Petro Bratash petrox.bratash@intel.com

- Why I did it

LLDP conf Jinja2 Template does not verify IPv4 address and can use IPv6 version. This issue does not effect control LLDP daemon. Issue can be reproduced via test_snmp_lldp test. LLDP conf Jinja2 Template selects first item from the list of mgmt interfaces.

TESTBED_1 LLDP conf

# cat /etc/lldpd.conf 
configure ports eth0 lldp portidsubtype local eth0
configure system ip management pattern FC00:3::32
configure system hostname dut-1

TESTBED_2 LLDP conf

# cat /etc/lldpd.conf
configure ports eth0 lldp portidsubtype local eth0
configure system ip management pattern 10.22.24.61
configure system hostname dut-2

TESTBED_1 MGMT_INTERFACE

$ redis-cli -n 4 keys "*" | grep MGMT_INTERFACE
MGMT_INTERFACE|eth0|10.22.24.53/23
MGMT_INTERFACE|eth0|FC00:3::32/64

TESTBED_2 MGMT_INTERFACE

$ redis-cli -n 4 keys "*" | grep MGMT_INTERFACE
MGMT_INTERFACE|eth0|FC00:3::32/64
MGMT_INTERFACE|eth0|10.22.24.61/23

- How I did it

Skip IP address if it includes ":" characters

- How to verify it

Run:
docker exec lldp cat /etc/lldpd.conf
Check the type of IP-address

- Which release branch to backport (provide reason below if selected)

201811
201911
202006

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

bratashX · 2020-10-22T13:56:33Z

@akokhan, please review

liat-grozovik · 2020-10-22T14:56:17Z

@shlomibitton can you please review as well?

pavel-shirshov

Thank you for the contrinbution
Can you please

Describe more what is the problem is? The way to reproduce the issue and so on. Currently it is not clear from the PR description
Add tests for your change,

dockers/docker-lldp/lldpd.conf.j2

akokhan · 2020-10-23T05:30:07Z

Thank you for the contrinbution
Can you please

Describe more what is the problem is? The way to reproduce the issue and so on. Currently it is not clear from the PR description

Add tests for your change,

#5603

lguohan · 2020-10-23T07:50:57Z

can you add test in sonic-cfggen to validate the template results.

shlomibitton · 2020-10-28T15:37:03Z

LGTM

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

bratashX · 2020-10-29T14:13:52Z

@lguohan @pavel-shirshov
Currently test for testing lldp.conf exists
https://github.com/Azure/sonic-buildimage/blob/ad2001f972e27034b876a79c6108f47420544bf5/src/sonic-config-engine/tests/test_j2files.py#L69
Test take last interface in ManagementIPInterfaces from t0-sample-graph.xml file.
Previously test passed because the last interface was IPv4 https://github.com/Azure/sonic-buildimage/blob/ad2001f972e27034b876a79c6108f47420544bf5/src/sonic-config-engine/tests/t0-sample-graph.xml#L202

I think this commit will fix the test(change the order of the interfaces): ad2001f

Could you say, how to run this tests?

pavel-shirshov · 2020-10-29T17:58:33Z

Hi @bratashX

I'd suggest you revert the last commit. It breaks other tests. Usually we have ipv6 as second address on the interface in the minigraph.

How to run the tests:

cd sonic-buildimage/src/sonic-config-engine
pytest tests

This reverts commit ad2001f.

dockers/docker-lldp/lldpd.conf.j2

qiluo-msft · 2020-10-30T22:47:22Z

dockers/docker-lldp/lldpd.conf.j2

 {% if MGMT_INTERFACE %}
+{% for interface in MGMT_INTERFACE.keys()|list %}
+{% if interface[1]|ipv4 %}


ipv4 [](start = 19, length = 4)

Could you add a test case similar to https://github.com/Azure/sonic-buildimage/blob/781188f54941a2eb9e4a23a96f05986ec51ff106/src/sonic-config-engine/tests/test_j2files.py#L69? So it will capture regression like this.

The test was added, please review

Thanks for adding test! I think another useful test case is 1 ipv4 + 1 ipv6 mgmt addresses, which are common to in normal use case.

In reply to: 515413789 [](ancestors = 515413789)

I think the test must verify 2 cases:

IPv4 mgmt interface exists

IPv4 mgmt interface doesn't exist

But now I add 3 cases, when:

IPv4 mgmt interface only exists

IPv6 mgmt interface only exists

IPv4 + IPv6 mgmt interface exists

Please review

qiluo-msft

As comment

bratashX · 2020-11-02T06:49:12Z

2. pytest tests

@pavel-shirshov Previously I have never been run SONiC test. Could you please say what environment needs use before test running? Maybe is some docker for testing or some README file?

lguohan · 2020-11-05T02:30:47Z

retest vsimage please

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

bratashX · 2020-11-05T12:48:53Z

@lguohan Can I duplicate src/sonic-config-engine/tests/t0-sample-graph.xml with changes from these commit: ad2001f
(it fix test_lldp test and doesn't affect other tests)? Is it good solution?

Unblock, the test could be further improved.

pavel-shirshov · 2020-11-06T00:44:34Z

@bratashX
Currently I don't have such documentation.
What I did to run the tests: manually installed all dependency which is required by using pip.
I'd suggest start with

pip install pytest
pip install pytest-runner

Also you need to install sonic-py-common and swsssdk for that

git clone https://github.com/Azure/sonic-buildimage.git
cd sonic-buildimage/src/sonic-py-common
sudo python setup.py install
git clone https://github.com/Azure/sonic-py-swsssdk.git
sido python setup.py install

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

src/sonic-config-engine/tests/test_j2files.py

lguohan · 2020-11-06T10:40:55Z

@lguohan Can I duplicate src/sonic-config-engine/tests/t0-sample-graph.xml with changes from these commit: ad2001f
(it fix test_lldp test and doesn't affect other tests)? Is it good solution?

a good solution is to use test_ndppd_conf test as an example, you do not need to introduce a new minigraph.

lguohan · 2020-11-06T16:51:43Z

retest mellanox please

pavel-shirshov · 2020-11-07T01:48:18Z

retest vsimage please

…5699) Fix #5812 LLDP conf Jinja2 Template does not verify IPv4 address and can use IPv6 version. This issue does not effect control LLDP daemon. Issue can be reproduced via `test_snmp_lldp` test. LLDP conf Jinja2 Template selects first item from the list of mgmt interfaces. TESTBED_1 LLDP conf ``` configure ports eth0 lldp portidsubtype local eth0 configure system ip management pattern FC00:3::32 configure system hostname dut-1 ``` TESTBED_2 LLDP conf ``` configure ports eth0 lldp portidsubtype local eth0 configure system ip management pattern 10.22.24.61 configure system hostname dut-2 ``` TESTBED_1 MGMT_INTERFACE ``` $ redis-cli -n 4 keys "*" | grep MGMT_INTERFACE MGMT_INTERFACE|eth0|10.22.24.53/23 MGMT_INTERFACE|eth0|FC00:3::32/64 ``` TESTBED_2 MGMT_INTERFACE ``` $ redis-cli -n 4 keys "*" | grep MGMT_INTERFACE MGMT_INTERFACE|eth0|FC00:3::32/64 MGMT_INTERFACE|eth0|10.22.24.61/23 ``` Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

…onic-net#5699) Fix sonic-net#5812 LLDP conf Jinja2 Template does not verify IPv4 address and can use IPv6 version. This issue does not effect control LLDP daemon. Issue can be reproduced via `test_snmp_lldp` test. LLDP conf Jinja2 Template selects first item from the list of mgmt interfaces. TESTBED_1 LLDP conf ``` # cat /etc/lldpd.conf configure ports eth0 lldp portidsubtype local eth0 configure system ip management pattern FC00:3::32 configure system hostname dut-1 ``` TESTBED_2 LLDP conf ``` # cat /etc/lldpd.conf configure ports eth0 lldp portidsubtype local eth0 configure system ip management pattern 10.22.24.61 configure system hostname dut-2 ``` TESTBED_1 MGMT_INTERFACE ``` $ redis-cli -n 4 keys "*" | grep MGMT_INTERFACE MGMT_INTERFACE|eth0|10.22.24.53/23 MGMT_INTERFACE|eth0|FC00:3::32/64 ``` TESTBED_2 MGMT_INTERFACE ``` $ redis-cli -n 4 keys "*" | grep MGMT_INTERFACE MGMT_INTERFACE|eth0|FC00:3::32/64 MGMT_INTERFACE|eth0|10.22.24.61/23 ``` Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

Add verification IPv4 address on LLDP conf Jinja2 Template

d42ae43

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

bratashX mentioned this pull request Oct 22, 2020

There is only IPv6 address(es) in lldp_loc_man_addr in Redis #5603

Closed

gechiang requested review from qiluo-msft, tahmed-dev, jleveque and mykolaf October 22, 2020 14:58

akokhan previously approved these changes Oct 22, 2020

View reviewed changes

jleveque requested a review from pavel-shirshov October 22, 2020 17:36

pavel-shirshov suggested changes Oct 22, 2020

View reviewed changes

tahmed-dev reviewed Oct 22, 2020

View reviewed changes

dockers/docker-lldp/lldpd.conf.j2 Outdated Show resolved Hide resolved

Code improvement

bda9e7d

bratashX dismissed akokhan’s stale review via bda9e7d October 23, 2020 09:31

Fix config file for correct work test_j2files.py(test_lldp)

ad2001f

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

Revert "Fix config file for correct work test_j2files.py(test_lldp)"

de774cb

This reverts commit ad2001f.

tahmed-dev reviewed Oct 30, 2020

View reviewed changes

dockers/docker-lldp/lldpd.conf.j2 Outdated Show resolved Hide resolved

qiluo-msft reviewed Oct 30, 2020

View reviewed changes

qiluo-msft previously requested changes Oct 30, 2020

View reviewed changes

qiluo-msft mentioned this pull request Nov 5, 2020

SNMP missing expected LLDP data in master image #5812

Closed

daall linked an issue Nov 5, 2020 that may be closed by this pull request

SNMP missing expected LLDP data in master image #5812

Closed

bratashX added 2 commits November 4, 2020 23:19

Merge branch 'master' into fix_lldp_jinja2_template

a5f32d2

Add test for lldp.conf

ad58479

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

Code refactoring

0e81cc3

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

lguohan added the Bug 🐛 label Nov 5, 2020

bratashX force-pushed the fix_lldp_jinja2_template branch from 052b382 to 0e81cc3 Compare November 5, 2020 19:35

bratashX requested review from tahmed-dev, qiluo-msft and pavel-shirshov November 5, 2020 19:37

bratashX added 2 commits November 5, 2020 18:13

Add new case to test_lldp and remove link to output files

8464938

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

Use json-file instead of minigraph

3281d2f

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>

lguohan reviewed Nov 6, 2020

View reviewed changes

src/sonic-config-engine/tests/test_j2files.py Outdated Show resolved Hide resolved

bratashX requested a review from lguohan November 6, 2020 14:12

lguohan approved these changes Nov 6, 2020

View reviewed changes

pavel-shirshov approved these changes Nov 6, 2020

View reviewed changes

qiluo-msft approved these changes Nov 6, 2020

View reviewed changes

lguohan merged commit 32a832a into sonic-net:master Nov 7, 2020

tbgowda mentioned this pull request Feb 1, 2021

lldpRemManAddrTable SNMP failure due to DHCP on Mgmt interface #6636

Closed

abdosi mentioned this pull request Feb 11, 2021

[snmp | lldp] table is no present in SNMP facts #6737

Closed

liat-grozovik added Issue for 201911 Request for 202012 Branch Request for 201911 Branch and removed Issue for 201911 labels Feb 18, 2021

daall added the Included in 202012 Branch label Feb 18, 2021

abdosi added the Included in 201911 Branch label Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verification IPv4 address on LLDP conf Jinja2 Template #5699

Add verification IPv4 address on LLDP conf Jinja2 Template #5699

bratashX commented Oct 22, 2020 •

edited

Loading

bratashX commented Oct 22, 2020

liat-grozovik commented Oct 22, 2020

pavel-shirshov left a comment

akokhan commented Oct 23, 2020

lguohan commented Oct 23, 2020

shlomibitton commented Oct 28, 2020

bratashX commented Oct 29, 2020 •

edited

Loading

pavel-shirshov commented Oct 29, 2020

qiluo-msft Oct 30, 2020

bratashX Nov 5, 2020

qiluo-msft Nov 5, 2020

bratashX Nov 6, 2020

qiluo-msft left a comment

bratashX commented Nov 2, 2020

lguohan commented Nov 5, 2020

bratashX commented Nov 5, 2020

pavel-shirshov commented Nov 6, 2020

lguohan commented Nov 6, 2020

lguohan commented Nov 6, 2020

pavel-shirshov commented Nov 7, 2020

Add verification IPv4 address on LLDP conf Jinja2 Template #5699

Add verification IPv4 address on LLDP conf Jinja2 Template #5699

Conversation

bratashX commented Oct 22, 2020 • edited Loading

bratashX commented Oct 22, 2020

liat-grozovik commented Oct 22, 2020

pavel-shirshov left a comment

Choose a reason for hiding this comment

akokhan commented Oct 23, 2020

lguohan commented Oct 23, 2020

shlomibitton commented Oct 28, 2020

bratashX commented Oct 29, 2020 • edited Loading

pavel-shirshov commented Oct 29, 2020

qiluo-msft Oct 30, 2020

Choose a reason for hiding this comment

bratashX Nov 5, 2020

Choose a reason for hiding this comment

qiluo-msft Nov 5, 2020

Choose a reason for hiding this comment

bratashX Nov 6, 2020

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

bratashX commented Nov 2, 2020

lguohan commented Nov 5, 2020

bratashX commented Nov 5, 2020

pavel-shirshov commented Nov 6, 2020

lguohan commented Nov 6, 2020

lguohan commented Nov 6, 2020

pavel-shirshov commented Nov 7, 2020

bratashX commented Oct 22, 2020 •

edited

Loading

bratashX commented Oct 29, 2020 •

edited

Loading