Protect config_db.json from minigraph misconfig #1727

wendani · 2018-05-18T19:07:16Z

- What I did

Update test cases to consider misconfig in minigraph.xml

PortChannelInterfaces that can leak into PORTCHANNEL in config_db.json
EthernetInterface that can leak into PORT in config_db.json
portchannel IPInterface that can leak into PORTCHANNEL_INTERFACE in config_db.json
DeviceInterfaceLinks that can leak into DEVICE_NEIGHBOR and PORT in config_db.json

Add one test case to assert that DEVICE_NEIGHBOR does not have non-port_config.ini port
Disallow non-port_config.ini port to get into PORT list even if it has non-zero Bandwidth attribute in DeviceInterfaceLink
remove non-port_config.ini port from PORTCHANNEL, PORTCHANNEL_INTERFACE, and DEVICE_NEIGHBOR

- How I did it

- How to verify it

pytest tests/test_cfggen.py
load minigraph.py on DUT and confirm that config_db.json generated from a misconfig minigraph.xml does not have the false config.

- Description for the changelog

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

…-graph.xml

Signed-off-by: Wenda <[email protected]>

Add unit test against introducing ports not existing in port_config.ini into DEVICE_NEIGHBOR Signed-off-by: Wenda <[email protected]>

port_config.ini but contraining non-zero Bandwidth attribute Add noice config in simple-sample-graph.xml to capture the case that such a port is leaked into config_db.json Signed-off-by: Wenda <[email protected]>

Signed-off-by: Wenda <[email protected]>

existing in port_config.ini Signed-off-by: Wenda <[email protected]>

Signed-off-by: Wenda <[email protected]>

…raph.xml as it is in PortChannel1001 Signed-off-by: Wenda <[email protected]>

Signed-off-by: Wenda <[email protected]>

existing in port_config.ini Signed-off-by: Wenda <[email protected]>

Signed-off-by: Wenda <[email protected]>

qiluo-msft · 2018-05-18T21:52:52Z

src/sonic-config-engine/minigraph.py

-        # and later swss will pick up and behave on-demand port break-up.
-        # if on-deman port break-up is not supported on a specific platform, swss will return error.
+        # not consider port not in port_config.ini
+        if port_name not in ports:


if port_name not in ports: [](start = 8, length = 26)

Should we treat this as a minigraph generator's bug instead of parser's bug? #Closed

if we can mitigate that in minigraph, it should be ideal #Closed

Discussed with Guohan, this is the good.

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

qiluo-msft · 2018-05-18T21:53:16Z

src/sonic-config-engine/minigraph.py

    results['DEVICE_NEIGHBOR'] = neighbors
+
    results['DEVICE_NEIGHBOR_METADATA'] = { key:devices[key] for key in devices if key.lower() != hostname.lower() }


delete this line? #Closed

Signed-off-by: Wenda <[email protected]>

qiluo-msft

Please also check with other reviewers

lguohan · 2018-05-19T00:01:20Z

src/sonic-config-engine/minigraph.py

-        # if on-deman port break-up is not supported on a specific platform, swss will return error.
+        # not consider port not in port_config.ini
+        if port_name not in ports:
+            continue


can you print out warning here?

port_config.ini Signed-off-by: Wenda <[email protected]>

lguohan · 2018-05-19T09:17:39Z

src/sonic-config-engine/tests/simple-sample-graph.xml

@@ -240,7 +277,20 @@
          <EnableAutoNegotiation>true</EnableAutoNegotiation>
          <EnableFlowControl>true</EnableFlowControl>
          <Index>1</Index>
-          <InterfaceName>fortyGigE0/1</InterfaceName>
+          <InterfaceName>Ethernet1</InterfaceName>


should be fortyGigE0/1

If we use fortyGigE0/1 and fortyGigE0/2, even without your patch, it does not introduce extra port into PORT but only change the order listed in PORT. Because of this bad test case in the first place, it does not catch the extra port that can be introduced in later commit 0e7a7dfa.

I will confirm with other places. But here we should use Ethernet1 and Ethernet2.

lguohan · 2018-05-19T09:17:52Z

src/sonic-config-engine/tests/simple-sample-graph.xml

+          <EnableAutoNegotiation>true</EnableAutoNegotiation>
+          <EnableFlowControl>true</EnableFlowControl>
+          <Index>1</Index>
+          <InterfaceName>Ethernet2</InterfaceName>


-> fortyGigE0/2

lguohan · 2018-05-19T09:18:18Z

src/sonic-config-engine/tests/simple-sample-graph.xml

+        <AutoNegotiation>true</AutoNegotiation>
+        <Bandwidth>10000</Bandwidth>
+        <EndDevice>switch-t0</EndDevice>
+        <EndPort>Ethernet2</EndPort>


-> fortyGigE0/2

lguohan · 2018-05-19T09:18:33Z

src/sonic-config-engine/tests/simple-sample-graph.xml

+        <AutoNegotiation>true</AutoNegotiation>
+        <Bandwidth>10000</Bandwidth>
+        <EndDevice>switch-t0</EndDevice>
+        <EndPort>Ethernet1</EndPort>


-> fortyGigE0/1

lguohan · 2018-05-19T09:18:53Z

src/sonic-config-engine/tests/simple-sample-graph.xml

@@ -125,6 +125,11 @@
          <AttachTo>fortyGigE0/4</AttachTo>
          <SubInterface/>
        </PortChannel>
+        <PortChannel>
+          <Name>PortChannel1001</Name>
+          <AttachTo>Ethernet1;Ethernet2</AttachTo>


fortyGigE0/1;fortyGigE0/2

lguohan · 2018-05-19T09:26:22Z

src/sonic-config-engine/minigraph.py

+    for nghbr in neighbors.keys():
+        # remove port not in port_config.ini
+        if nghbr not in ports:
+            del neighbors[nghbr]


print warning here

lguohan · 2018-05-19T09:26:38Z

src/sonic-config-engine/minigraph.py

+    for pc_intf in pc_intfs.keys():
+        # remove portchannels not in PORTCHANNEL dictionary
+        if pc_intf[0] not in pcs:
+            del pc_intfs[pc_intf]


print warning here.

lguohan · 2018-05-19T09:26:55Z

src/sonic-config-engine/minigraph.py

+            # remove portchannels that contain ports not existing in port_config.ini
+            # when port_config.ini exists
+            if not set(mbr_map['members']).issubset(port_set):
+                del pcs[pc_name]


print warning here.

lguohan · 2018-05-19T09:28:12Z

src/sonic-config-engine/minigraph.py

-        # if on-deman port break-up is not supported on a specific platform, swss will return error.
+        # not consider port not in port_config.ini
+        if port_name not in ports:
+            print >> sys.stderr, "Warning: drop '%s' from enlisting in PORT list" % port_name


Warning: ingore interface '%s' as it is not in the port_config.ini

lguohan

change port name to alias, print more warnings and add unit tests coverage.

Signed-off-by: Wenda <[email protected]>

…es, and device neighbors Update t0-sample-graph.xml with interface name 'fortyGigE0/2' and the ACL_TABLE output Signed-off-by: Wenda <[email protected]>

* Add noise config for PortChannel & EthernetInterface in simple-sample-graph.xml * Add noise config for PORTCHANNEL_INTERFACE in simple-sample-graph.xml Signed-off-by: Wenda <[email protected]> * Add noice config for DEVICE_NEIGHBOR in t0-sample-graph.xml Add unit test against introducing ports not existing in port_config.ini into DEVICE_NEIGHBOR Signed-off-by: Wenda <[email protected]> * DeviceInterfaceLink in minigraph.xml can contain port not existing in port_config.ini but contraining non-zero Bandwidth attribute Add noice config in simple-sample-graph.xml to capture the case that such a port is leaked into config_db.json Signed-off-by: Wenda <[email protected]> * Protect PORTCHANNEL from ports not existing in port_config.ini Signed-off-by: Wenda <[email protected]> * Protect PORTCHANNEL_INTERFACE from portchannels containing ports not existing in port_config.ini Signed-off-by: Wenda <[email protected]> * Protect DEVICE_NEIGHBOR from ports not existing in port_config.ini Signed-off-by: Wenda <[email protected]> * Add noise config Ethernet1 in DeviceInterfaceLinks in simple-sample-graph.xml as it is in PortChannel1001 Signed-off-by: Wenda <[email protected]> * Add noise config Ethernet1 in DeviceInterfaceLinks in simple-sample-graph.xml as it is in PortChannel1001 Signed-off-by: Wenda <[email protected]> * Protect PORTCHANNEL from ports not existing in port_config.ini Signed-off-by: Wenda <[email protected]> * Protect PORTCHANNEL_INTERFACE from portchannels containing ports not existing in port_config.ini Signed-off-by: Wenda <[email protected]> * Protect DEVICE_NEIGHBOR from ports not existing in port_config.ini Signed-off-by: Wenda <[email protected]> * Correct space in minigraph.py Signed-off-by: Wenda <[email protected]> * Does not allow non-port_config.ini port to get into the port list Signed-off-by: Wenda <[email protected]> * Check PORTCHANNEL against PORT list only if port_config_file exists Signed-off-by: Wenda <[email protected]> * Correct format Signed-off-by: Wenda <[email protected]> * print warning when a port coming from DeviceInterfaceLink is not in port_config.ini Signed-off-by: Wenda <[email protected]> * Change Ethernet1 and 2 to fortyGigE0/1 and 2,respectively Signed-off-by: Wenda <[email protected]> * Change Ethernet1 and 2 to fortyGigE0/1 and 2,respectively Signed-off-by: Wenda <[email protected]> * print warning when ignoring ports, portchannels, portchannel interfaces, and device neighbors Update t0-sample-graph.xml with interface name 'fortyGigE0/2' and the ACL_TABLE output Signed-off-by: Wenda <[email protected]>

Include commit - 939c14b | [Submodule][upgrade]Upgrade SAI submodule (sonic-net#1203) updates from SAI repo - 0031470 | improve enum values integration check (sonic-net#1727) (sonic-net#1737) - 4f11c7e | Enable github code scanning to replace LGTM. (sonic-net#1709) Signed-off-by: richardyu-ms <[email protected]>

include changes from sairedis submodule 04d3c41 | [Submodule][upgrade]Upgrade SAI submodule (sonic-net#1204) updates from SAI 0031470 | improve enum values integration check (sonic-net#1727) (sonic-net#1737) 4f11c7e | Enable github code scanning to replace LGTM. (sonic-net#1709) Signed-off-by: richardyu-ms <[email protected]>

include sairedis changes 3a960be | [submodule][SAI]ADvance SAI Header (sonic-net#1206) 7026441 | [Mellanox] Enable DSCP remapping by using SAI attribute (sonic-net#1188) a2c37b8 | [syncd]: Enable port bulk API (sonic-net#1197) include SAI changes 7710e24 | [cherry-pick][202211]Enhance the check enum lock script (sonic-net#1741) (sonic-net#1742) 0031470 | improve enum values integration check (sonic-net#1727) (sonic-net#1737) 4f11c7e | Enable github code scanning to replace LGTM. (sonic-net#1709) 0fd23d2 | [SAI-PTF] Skip test when hit expected error from sai api (sonic-net#1699) aba7612 | [SAI-PTF] API Logger - reformat arg values (sonic-net#1696) 1390cee | [SAI-PTF] API Logger - reformat dict in return value (sonic-net#1690) 3d96a1d | [SAI-PTF]Add return value in the SAI-PTF log (sonic-net#1685) Signed-off-by: richardyu-ms <[email protected]>

include sairedis changes 3a960be | [submodule][SAI]ADvance SAI Header (#1206) 7026441 | [Mellanox] Enable DSCP remapping by using SAI attribute (#1188) a2c37b8 | [syncd]: Enable port bulk API (#1197) include SAI changes 7710e24 | [cherry-pick][202211]Enhance the check enum lock script (#1741) (#1742) 0031470 | improve enum values integration check (#1727) (#1737) 4f11c7e | Enable github code scanning to replace LGTM. (#1709) 0fd23d2 | [SAI-PTF] Skip test when hit expected error from sai api (#1699) aba7612 | [SAI-PTF] API Logger - reformat arg values (#1696) 1390cee | [SAI-PTF] API Logger - reformat dict in return value (#1690) 3d96a1d | [SAI-PTF]Add return value in the SAI-PTF log (#1685) Signed-off-by: richardyu-ms <[email protected]>

Why I did it include changes from sairedis submodule 102d20b | [202211][submodule][SAI]Advance header include 0031470 | improve enum values integration check (#1727) (#1737) 04d3c41 | [Submodule][upgrade]Upgrade SAI submodule (#1204) updates from SAI 7710e24 | [cherry-pick][202211]Enhance the check enum lock script (#1741) (#1742) 0031470 | improve enum values integration check (#1727) (#1737) 4f11c7e | Enable github code scanning to replace LGTM. (#1709) How I did it How to verify it

Related work items: sonic-net#276, sonic-net#305, sonic-net#332, sonic-net#338, sonic-net#339, sonic-net#1188, sonic-net#1192, sonic-net#1197, sonic-net#1206, sonic-net#1685, sonic-net#1690, sonic-net#1696, sonic-net#1699, sonic-net#1709, sonic-net#1727, sonic-net#1737, sonic-net#1741, sonic-net#1742, sonic-net#2511, sonic-net#2512, sonic-net#2532, sonic-net#2559, sonic-net#2626, sonic-net#2638, sonic-net#2645, sonic-net#2649, sonic-net#2660, sonic-net#2669, sonic-net#2670, sonic-net#2678, sonic-net#10084, sonic-net#11442, sonic-net#11873, sonic-net#12047, sonic-net#12110, sonic-net#12207, sonic-net#12529, sonic-net#12678, sonic-net#13235, sonic-net#13287, sonic-net#13372, sonic-net#13395, sonic-net#13456, sonic-net#13497, sonic-net#13522, sonic-net#13545, sonic-net#13547, sonic-net#13552, sonic-net#13569, sonic-net#13572, sonic-net#13578, sonic-net#13591, sonic-net#13611, sonic-net#13647, sonic-net#13649, sonic-net#13660, sonic-net#13710, sonic-net#13716, sonic-net#13724, sonic-net#13726, sonic-net#13732, sonic-net#13735, sonic-net#13739, sonic-net#13757, sonic-net#13786, sonic-net#13792, sonic-net#13800, sonic-net#13801, sonic-net#13802, sonic-net#13805, sonic-net#13806, sonic-net#13812, sonic-net#13814, sonic-net#13822, sonic-net#13831, sonic-net#13834, sonic-net#13847, sonic-net#13870, sonic-net#13882, sonic-net#13884, sonic-net#13885, sonic-net#13894, sonic-net#13895, sonic-net#13926, sonic-net#13932, sonic-net#13935, sonic-net#13942, sonic-net#13951, sonic-net#13953, sonic-net#13964

wendani added 15 commits May 17, 2018 19:29

Add noise config for PortChannel & EthernetInterface in simple-sample…

b9f2080

…-graph.xml

Add noise config for PORTCHANNEL_INTERFACE in simple-sample-graph.xml

696e791

Signed-off-by: Wenda <[email protected]>

Add noice config for DEVICE_NEIGHBOR in t0-sample-graph.xml

b5e43d6

Add unit test against introducing ports not existing in port_config.ini into DEVICE_NEIGHBOR Signed-off-by: Wenda <[email protected]>

DeviceInterfaceLink in minigraph.xml can contain port not existing in

f3c97e0

port_config.ini but contraining non-zero Bandwidth attribute Add noice config in simple-sample-graph.xml to capture the case that such a port is leaked into config_db.json Signed-off-by: Wenda <[email protected]>

Protect PORTCHANNEL from ports not existing in port_config.ini

6518266

Signed-off-by: Wenda <[email protected]>

Protect PORTCHANNEL_INTERFACE from portchannels containing ports not

097f1e9

existing in port_config.ini Signed-off-by: Wenda <[email protected]>

Protect DEVICE_NEIGHBOR from ports not existing in port_config.ini

1f293e2

Signed-off-by: Wenda <[email protected]>

Add noise config Ethernet1 in DeviceInterfaceLinks in simple-sample-g…

6db9a0a

…raph.xml as it is in PortChannel1001 Signed-off-by: Wenda <[email protected]>

Add noise config Ethernet1 in DeviceInterfaceLinks in simple-sample-g…

987bc92

…raph.xml as it is in PortChannel1001 Signed-off-by: Wenda <[email protected]>

Protect PORTCHANNEL from ports not existing in port_config.ini

9021233

Signed-off-by: Wenda <[email protected]>

Protect PORTCHANNEL_INTERFACE from portchannels containing ports not

d604685

existing in port_config.ini Signed-off-by: Wenda <[email protected]>

Protect DEVICE_NEIGHBOR from ports not existing in port_config.ini

c9f22b3

Signed-off-by: Wenda <[email protected]>

Correct space in minigraph.py

955e4be

Signed-off-by: Wenda <[email protected]>

Does not allow non-port_config.ini port to get into the port list

1e79656

Signed-off-by: Wenda <[email protected]>

Merge branch 'config_db_protect_master' into sample_graph_master

f545f41

wendani changed the base branch from sample_graph_master to master May 18, 2018 19:09

wendani changed the title ~~Sample graph master~~ Protect config_db.json from minigraph misconfig May 18, 2018

wendani requested a review from qiluo-msft May 18, 2018 21:43

qiluo-msft reviewed May 18, 2018

View reviewed changes

Check PORTCHANNEL against PORT list only if port_config_file exists

6c3026a

Signed-off-by: Wenda <[email protected]>

wendani requested review from yxieca and lguohan May 18, 2018 22:54

Correct format

8caa011

Signed-off-by: Wenda <[email protected]>

qiluo-msft approved these changes May 18, 2018

View reviewed changes

lguohan reviewed May 19, 2018

View reviewed changes

print warning when a port coming from DeviceInterfaceLink is not in

37f1f81

port_config.ini Signed-off-by: Wenda <[email protected]>

lguohan reviewed May 19, 2018

View reviewed changes

lguohan requested changes May 19, 2018

View reviewed changes

wendani added 5 commits May 22, 2018 23:48

Change Ethernet1 and 2 to fortyGigE0/1 and 2,respectively

472c499

Signed-off-by: Wenda <[email protected]>

Merge branch 'fortyGigE' into sample_graph_master

8efa34d

Change Ethernet1 and 2 to fortyGigE0/1 and 2,respectively

c3c46ba

Signed-off-by: Wenda <[email protected]>

Merge branch 'fortyGigE' into sample_graph_master

75a35fa

print warning when ignoring ports, portchannels, portchannel interfac…

293ac1a

…es, and device neighbors Update t0-sample-graph.xml with interface name 'fortyGigE0/2' and the ACL_TABLE output Signed-off-by: Wenda <[email protected]>

lguohan approved these changes May 24, 2018

View reviewed changes

lguohan merged commit d32c043 into sonic-net:master May 24, 2018

richardyu-ms mentioned this pull request Feb 8, 2023

[202205][sairedis]advance sairedis head #13711

Closed

8 tasks

richardyu-ms mentioned this pull request Feb 8, 2023

[202211][submodule]Advance sairedis head #13712

Merged

8 tasks

richardyu-ms mentioned this pull request Feb 9, 2023

[master][submodule][sairedis]Advance submodule head #13724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect config_db.json from minigraph misconfig #1727

Protect config_db.json from minigraph misconfig #1727

wendani commented May 18, 2018

qiluo-msft May 18, 2018 •

edited

Loading

wendani May 18, 2018 •

edited by qiluo-msft

Loading

qiluo-msft May 18, 2018

qiluo-msft May 18, 2018 •

edited

Loading

qiluo-msft left a comment

lguohan May 19, 2018

wendani May 19, 2018

lguohan May 19, 2018

wendani May 19, 2018 •

edited

Loading

lguohan May 19, 2018

lguohan May 19, 2018

wendani May 23, 2018

lguohan May 19, 2018

wendani May 23, 2018

lguohan May 19, 2018

wendani May 23, 2018

lguohan May 19, 2018

lguohan May 19, 2018

wendani May 23, 2018

lguohan May 19, 2018

wendani May 23, 2018

lguohan May 19, 2018

wendani May 23, 2018

lguohan left a comment

		results['DEVICE_NEIGHBOR'] = neighbors

		results['DEVICE_NEIGHBOR_METADATA'] = { key:devices[key] for key in devices if key.lower() != hostname.lower() }

Protect config_db.json from minigraph misconfig #1727

Protect config_db.json from minigraph misconfig #1727

Conversation

wendani commented May 18, 2018

qiluo-msft May 18, 2018 • edited Loading

Choose a reason for hiding this comment

wendani May 18, 2018 • edited by qiluo-msft Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft May 18, 2018 • edited Loading

Choose a reason for hiding this comment

qiluo-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan left a comment

Choose a reason for hiding this comment

qiluo-msft May 18, 2018 •

edited

Loading

wendani May 18, 2018 •

edited by qiluo-msft

Loading

qiluo-msft May 18, 2018 •

edited

Loading

wendani May 19, 2018 •

edited

Loading