[macsec] Parse masec_enabled and macsec_profile from minigraph #10917

judyjoseph · 2022-05-25T01:33:03Z

Why I did it

In the Minigraph the following details are present for macsec configuration

  <LinkMetadataDeclaration>
    <Link xmlns:a="http://schemas.datacontract.org/2004/07/Microsoft.Search.Autopilot.Evolution">
      <a:LinkMetadata>
        <a:Name i:nil="true"/>
        <a:Properties>
          <a:DeviceProperty>
            <a:Name>MacSecEnabled</a:Name>
            <a:Value>True</a:Value>
          </a:DeviceProperty>
        </a:Properties>
        <a:Key>linecard-1:Ethernet1/1;ARISTA01-RH:Ethernet1/1</a:Key>
      </a:LinkMetadata>
    </Link>
    <Properties xmlns:a="http://schemas.datacontract.org/2004/07/Microsoft.Search.Autopilot.Evolution"/>
  </LinkMetadataDeclaration>

<a:DeviceProperty>
  <a:Name>MacSecProfile</a:Name>
  <a:Value>PrimaryKey="macsec-profile" FallbackKey="macsec-profile2" MacsecPolicy=""</a:Value>

Added the following

Support to parse the macsec enabled flag per interface
extract the primary macsec profile name in minigraph and attach it to interfaces where macsecEnabled flag is set.
The PrimaryKey in the MacSecProfile DeviceProperty, is the macsec profile used by default.
The FallbackKey in the MacSecProfile DeviceProperty, is the the macsec profile that will be used during key rotation. This is a manual process for now.

The macsec_profile eg: as below will be pushed into the device as a json file and loaded into the config DB's using "config load command"

1. cat macsec_profile.json

      {
          "MACSEC_PROFILE": {
              "macsec-profile": {
                  "cipher_suite": "GCM-AES-XPN-256",
                  "primary_cak": "cak",
                  "primary_ckn": "ckn",
                  "priority": "0",
                  "rekey_period": "60"
              },
              "macsec-profile2": {
                  "cipher_suite": "GCM-AES-XPN-256",
                  "primary_cak": "cak",  
                  "primary_ckn": "ckn",
                  "priority": "0",
                  "rekey_period": "60"
              }
          }
      }

2. config load macsec_profile.json

How I did it

Added parsing logic in minigraph.py

How to verify it

Verified by generating config_db.json in a multi-asic device.

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

Description for the changelog

Link to config_db schema for YANG module changes

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

arlakshm

Please add unit tests for these ?

src/sonic-config-engine/minigraph.py

judyjoseph · 2022-06-13T02:16:29Z

Please add unit tests for these ?

Added unit tests, but there are some validation failures which needs to be further discussed.

Do we need macsec-profile yang checks enforced here, as we are not populating macsec_profile table contents from minigraph.

    Validating yang schema
sonic_yang(6):Note: Below table(s) have no YANG models: PEER_SWITCH, VOQ_INBAND_INTERFACE, SYSTEM_PORT, CONSOLE_PORT, TUNNEL, MUX_CABLE, DEVICE_NEIGHBOR_METADATA, DHCP_SERVER, TELEMETRY, RESTAPI, CONSOLE_SWITCH
sonic_yang(3):Data Loading Failed:Leafref "/sonic-macsec:sonic-macsec/sonic-macsec:MACSEC_PROFILE/sonic-macsec:MACSEC_PROFILE_LIST/sonic-macsec:name" of value "macsec-profile" points to a non-existing leaf.
yang data generated from /sonic/src/sonic-config-engine/tests/sample-voq-graph.xml is not valid: Data Loading Failed
Leafref "/sonic-macsec:sonic-macsec/sonic-macsec:MACSEC_PROFILE/sonic-macsec:MACSEC_PROFILE_LIST/sonic-macsec:name" of value "macsec-profile" points to a non-existing leaf.

lgtm-com · 2022-06-13T02:42:50Z

This pull request introduces 2 alerts when merging ae13892565ba8d5c710d42000e0ac3635ace61fd into 201792f - view on LGTM.com

new alerts:

2 for Mismatch in multiple assignment

judyjoseph · 2022-06-20T20:09:27Z

Please add unit tests for these ?

Added unit tests, but there are some validation failures which needs to be further discussed.

Do we need macsec-profile yang checks enforced here, as we are not populating macsec_profile table contents from minigraph.

    Validating yang schema
sonic_yang(6):Note: Below table(s) have no YANG models: PEER_SWITCH, VOQ_INBAND_INTERFACE, SYSTEM_PORT, CONSOLE_PORT, TUNNEL, MUX_CABLE, DEVICE_NEIGHBOR_METADATA, DHCP_SERVER, TELEMETRY, RESTAPI, CONSOLE_SWITCH
sonic_yang(3):Data Loading Failed:Leafref "/sonic-macsec:sonic-macsec/sonic-macsec:MACSEC_PROFILE/sonic-macsec:MACSEC_PROFILE_LIST/sonic-macsec:name" of value "macsec-profile" points to a non-existing leaf.
yang data generated from /sonic/src/sonic-config-engine/tests/sample-voq-graph.xml is not valid: Data Loading Failed
Leafref "/sonic-macsec:sonic-macsec/sonic-macsec:MACSEC_PROFILE/sonic-macsec:MACSEC_PROFILE_LIST/sonic-macsec:name" of value "macsec-profile" points to a non-existing leaf.

This issue is fixed by updating the testcases. It is passing now.

arlakshm · 2022-06-21T19:55:47Z

@judyjoseph, LGTM. We will add tests for packet-chassis in another PR ?

src/sonic-config-engine/minigraph.py

qiluo-msft · 2022-06-21T21:45:55Z

src/sonic-config-engine/minigraph.py

    return linkmetas

+def parse_macsec_profile(val_string):
+    macsec_profile = {}
+    values = val_string.strip().split(' ')


val_string

The parsing logic is too strict and not easy to read/extend. Could you explore libraries like https://docs.python.org/3/library/configparser.html?

@Qi I am finding this config parser library used when we have to read from an input file and while parsing it has restrictions like need to have [SECTION] header etc - which I feel is not applicable here.

I checked the following, and the current parsing should take care of them.

the macsec profile name cannot be with spaces
the profile will have PrimaryKey and FallbackKey.

qiluo-msft · 2022-06-21T22:35:40Z

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

@@ -73,6 +73,9 @@ def validate(self, argument):
                cmd += ' -p ' + args.port_config
            if args.namespace is not None:
                cmd += ' -n ' + args.namespace
+            if "-j " in argument:


if "-j " in argument:

if "-j " in argument:

Could you check args? #Closed

Updated to use args itself.

qiluo-msft · 2022-06-21T22:37:11Z

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

@@ -73,6 +73,9 @@ def validate(self, argument):
                cmd += ' -p ' + args.port_config
            if args.namespace is not None:
                cmd += ' -n ' + args.namespace
+            if "-j " in argument:
+                strs = argument.split(' ')
+                cmd += " -j {}".format(next(strs[i+1] for i in range(len(strs)) if strs[i]=='-j'))


{}

cmd += " -j {}".format(next(strs[i+1] for i in range(len(strs)) if strs[i]=='-j'))

Could you use args.json? #Closed

Updated to use args itself.

judyjoseph · 2022-06-27T23:33:12Z

@judyjoseph, LGTM. We will add tests for packet-chassis in another PR ?

yes, I will raise a separate PR for packet-chassis as the sample xml/portconfig.ini needs to be modified a bit which I will confirm, thanks.

* Updates needed to parse the macsec config from minigraph * Add unit tests in tests/test_cfggen.py::TestCfgGen, and updates

…-net#10917) * Updates needed to parse the macsec config from minigraph * Add unit tests in tests/test_cfggen.py::TestCfgGen, and updates

judyjoseph requested a review from lguohan as a code owner May 25, 2022 01:33

judyjoseph requested review from Pterosaur and arlakshm May 25, 2022 02:32

arlakshm reviewed May 30, 2022

View reviewed changes

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

judyjoseph added the Request for 202205 Branch label Jun 9, 2022

judyjoseph requested a review from a team as a code owner June 10, 2022 02:01

judyjoseph force-pushed the macsec_config branch from 2b75155 to ae13892 Compare June 13, 2022 02:22

judyjoseph requested a review from arlakshm June 18, 2022 00:31

arlakshm approved these changes Jun 21, 2022

View reviewed changes

qiluo-msft reviewed Jun 21, 2022

View reviewed changes

src/sonic-config-engine/minigraph.py Show resolved Hide resolved

qiluo-msft reviewed Jun 21, 2022

View reviewed changes

qiluo-msft requested a review from ganglyu June 21, 2022 21:48

qiluo-msft reviewed Jun 21, 2022

View reviewed changes

judyjoseph added 7 commits June 27, 2022 16:20

Updates needed to parse the macsec config from minigraph

573a8e9

fixes while testing

96dba4f

Add unit tests in tests/test_cfggen.py::TestCfgGen, and updates

817c51b

Fix lgtm warnings

1130d23

Update testcfgen using macsec profile json

b7d1153

Fix to remove debug during verification.

0399971

Update for comments.

ea6dadd

judyjoseph force-pushed the macsec_config branch from 7965f02 to ea6dadd Compare June 27, 2022 23:21

ganglyu approved these changes Jun 29, 2022

View reviewed changes

judyjoseph merged commit 05f1c6f into sonic-net:master Jun 29, 2022

yxieca added the Included in 202205 Branch label Jun 30, 2022

yxieca pushed a commit that referenced this pull request Jun 30, 2022

[macsec] Parse masec_enabled and macsec_profile from minigraph (#10917)

37c9b27

* Updates needed to parse the macsec config from minigraph * Add unit tests in tests/test_cfggen.py::TestCfgGen, and updates

judyjoseph deleted the macsec_config branch July 1, 2022 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[macsec] Parse masec_enabled and macsec_profile from minigraph #10917

[macsec] Parse masec_enabled and macsec_profile from minigraph #10917

judyjoseph commented May 25, 2022 •

edited

Loading

arlakshm left a comment

judyjoseph commented Jun 13, 2022

lgtm-com bot commented Jun 13, 2022

judyjoseph commented Jun 20, 2022

arlakshm commented Jun 21, 2022

qiluo-msft Jun 21, 2022

judyjoseph Jun 27, 2022

qiluo-msft Jun 21, 2022 •

edited

Loading

judyjoseph Jun 27, 2022

qiluo-msft Jun 21, 2022 •

edited

Loading

judyjoseph Jun 27, 2022

judyjoseph commented Jun 27, 2022

[macsec] Parse masec_enabled and macsec_profile from minigraph #10917

[macsec] Parse masec_enabled and macsec_profile from minigraph #10917

Conversation

judyjoseph commented May 25, 2022 • edited Loading

Why I did it

How I did it

How to verify it

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

Description for the changelog

Link to config_db schema for YANG module changes

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

arlakshm left a comment

Choose a reason for hiding this comment

judyjoseph commented Jun 13, 2022

lgtm-com bot commented Jun 13, 2022

judyjoseph commented Jun 20, 2022

arlakshm commented Jun 21, 2022

qiluo-msft Jun 21, 2022

Choose a reason for hiding this comment

judyjoseph Jun 27, 2022

Choose a reason for hiding this comment

qiluo-msft Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

if "-j " in argument:

judyjoseph Jun 27, 2022

Choose a reason for hiding this comment

qiluo-msft Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

cmd += " -j {}".format(next(strs[i+1] for i in range(len(strs)) if strs[i]=='-j'))

judyjoseph Jun 27, 2022

Choose a reason for hiding this comment

judyjoseph commented Jun 27, 2022

judyjoseph commented May 25, 2022 •

edited

Loading

qiluo-msft Jun 21, 2022 •

edited

Loading

qiluo-msft Jun 21, 2022 •

edited

Loading