macsec clarification for PF-1.17 README by steve-goog · Pull Request #4837 · openconfig/featureprofiles

steve-goog · 2025-11-20T21:34:25Z

MACsec updates for DUT to clarify test setup

gemini-code-assist · 2025-11-20T21:34:41Z

Summary of Changes

Hello @steve-goog, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the README.md for the MACsec test setup, providing clearer instructions and definitions for configuring and verifying MACsec policies. The changes aim to improve the accuracy and specificity of the test documentation, particularly regarding key management, the rekey interval, and the behavior of 'must-secure' and 'should-secure' policies under various conditions, ensuring a more robust and understandable test environment.

Highlights

MACsec Configuration Updates: The MACsec configuration instructions in the README have been updated, including a significant reduction in the key rekey interval from 28800 seconds to 30 seconds, and a correction to specify setting the Connectivity Association Key (CAK) as encrypted/hidden instead of the Connectivity Association Key Name (CKN).
Refined Test Scenarios: The descriptions for verifying 'must-secure' and 'should-secure' MACsec policies have been enhanced. Instead of generally 'disabling MACsec', the test now specifies 'changing a key on one side to a mismatch & forcing renegotiation on ATE ports' for a more precise and robust test of session disruption.
New Definitions Section: A new 'Definitions' section has been added to the README, providing clear explanations for 'must-secure' and 'should-secure' MACsec policies, which improves the overall understanding of the test setup and expected behavior.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

OpenConfigBot · 2025-11-20T21:35:10Z

Pull Request Functional Test Report for #4837 / `678e1f2`

Virtual Devices

Device	Test Documentation	Job	Raw Log
Arista cEOS	PF-1.17: MPLSoGRE and MPLSoGUE MACsec	4e02e385	Log
Cisco 8000E	PF-1.17: MPLSoGRE and MPLSoGUE MACsec	4afbd1c5	Log
Cisco XRd	PF-1.17: MPLSoGRE and MPLSoGUE MACsec	9097b2e8	Log
Juniper ncPTX	PF-1.17: MPLSoGRE and MPLSoGUE MACsec	28e0f8b0	Log
Nokia SR Linux	PF-1.17: MPLSoGRE and MPLSoGUE MACsec	1159bedd	Log
Openconfig Lemming	PF-1.17: MPLSoGRE and MPLSoGUE MACsec	3512f3b8	Log

Hardware Devices

Device	Test	Test Documentation	Raw Log
Arista 7808		PF-1.17: MPLSoGRE and MPLSoGUE MACsec
Cisco 8808		PF-1.17: MPLSoGRE and MPLSoGUE MACsec
Juniper PTX10008		PF-1.17: MPLSoGRE and MPLSoGUE MACsec
Nokia 7250 IXR-10e		PF-1.17: MPLSoGRE and MPLSoGUE MACsec

Help

gemini-code-assist

Code Review

This pull request provides several useful clarifications to the MACsec test README. It updates the test procedure for bringing down MACsec sessions to be more specific, adds definitions for must-secure and should-secure policies, and refines several configuration details. The changes improve the clarity and accuracy of the test documentation. I have provided a couple of suggestions to improve consistency and readability within the document.

coveralls · 2025-11-20T21:42:53Z

Pull Request Test Coverage Report for Build 20183581242

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 10.034%

Totals
Change from base Build 20151686211:	0.0%
Covered Lines:	2227
Relevant Lines:	22195

💛 - Coveralls

m26singhvi · 2025-11-21T12:34:49Z

    * Use 256 bit cipher GCM-AES-256-XPN and an associated 64 char CAK-CKN pair
    * Set Key server priority: 15
-    * Set Security association key rekey interval: 28800 seconds
+    * Set Security association key rekey interval: 30 seconds


Do we really want to keep the SAK rekey interval as 30 seconds. Is this value coming from some security requirements ?

This is strictly for the tests so that the tests can validate rekeying (without requring the test to run for 28800 seconds to rekey)

I am still trying to understand this. Once configured, 30 seconds will continue to be in the running configuration. So, SAK will continue to generate at that interval and if that is the intention.

Perhaps to be very clear, we can test two values. One to ensure the re-key occurs in a short timeframe (30sec) and a second value which represents a more realistic production use case (28800sec). In this second scenario, we don't need the test to wait for a re-key in the second scenario but rather simply confirm that the value is accepted by the DUT

sgtm, adding test for that

m26singhvi · 2025-11-21T12:36:44Z

+    * Define first Policy(1) to cover must-secure scenario, as defined below
+    * Define second Policy(2) to cover should-secure scenario, as defined below


The leaf for traffic policy isn't present in public openconfig model for Macsec. Can we please get them added.

dplore · 2025-12-09T19:08:01Z

There are still problems with curly braces and json key. try replacing the whole keychain stanza with:

  "keychains": {
    "keychain": [
      {
        "config": {
          "name": "keychain1"
        },
        "keys": {
          "key": [
            {
              "config": {
                "crypto-algorithm": "AES_256_CMAC",
                "key-id": "keyid1",
                "secret-key": "encrypted-secret-pwd-here"
              },
              "key-id": "keyid1",
              "receive-lifetime": {
                "config": {
                  "end-time": "1765242000",
                  "start-time": "1736298000"
                }
              },
              "send-lifetime": {
                "config": {
                  "end-time": "1765242000",
                  "start-time": "1736298000"
                }
              }
            }
          ]
        },
        "name": "keychain1"
      }
    ]
  }

dplore

removing the security-policy field from the JSON will fix this validation error:

E1209 22:52:41.231000 4717 validate_readme_canonicalocspec.go:145] file feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md: parent container policy (type *oc.Macsec_Mka_Policy): JSON contains unexpected field security-policy

* Add gemini code assist styleguide config * add gemini code assist config * Update styleguide with accessor function comment requirement Added instruction to include a URL link in the accessor function comment for tracking deviations.

* Fix issues in CNTR-2 tests * fix context derivation and typo in log * Fix acl and remove fqdn metadata from test --------- Co-authored-by: Ali Al-Shabibi <alshabibi.ali@gmail.com>

* Add single topology deviation handling for ISIS. * Update ISIS deviation error message * Use SetBatch to configure ISIS single topology deviation. * Make handleSingleTopologyDeviation a private function. * Change Get -> GetOrCreate... --------- Co-authored-by: Suhasini Tiwari <tsuhasini@google.com>

* add atomic test * reformat atomic README * simplify AFT cache logic and remove erroneous fail case during wait * Use test IPs for ISIS routes * Fix import path for gosnappi package * include metadata * update metadata and remove deviation in bgp * verify one next hop for BGP * remove multipath config if deviation set * remove multipath deviation * use common bgp logic to afts_base * use common bgp logic to afts_base * use cfgplugin bgp configuration * configure ALLOW before use * remove unused dutAttrs * Configure ISIS on only one port for proper next-hop count. * Update ATE ISIS Area. * Advertise ISIS routes on both ports * Wait for BGP session-state and ISIS adjacencies on both ports. * Format ISIS system ID for gNMI query. * Add NOKIA metadata. * Only advertise ISIS routes from port1 to ensure single next-hop * Change neighboring ISIS System ID to be unique per port. Also leave a TODO to not spawn a goroutine in aftcache from a returned function. * Create AFT streaming session at beginning of test. To capture all notifications through the config pushes. * change AFT convergence time for delete / stale updates. * update ISIS IP addresses * Move all deviation handling into libraries. * Use cfgplugins.AdvertiseRoutes to advertise BGP routes. * Configure ATE Routes in cfgplugins. * Remove unused variables. Also build CIDR prefixes by hand. * Fix BGP multipath config. * Include port name in ATE Attributes. * fix formatting and comments * Use BGPNeighborsConfig. * Move single topology deviation handling into NewISIS. * Change Get -> GetOrCreate... * move to otg_config_helpers * wait for ISIS before BGP. * fix advertised routes * wait for any ISIS adjacency * update root with single topology in deviation handling. * only configure ISIS on dut Port 1 * advertise ISIS routes over all ports. * wait for single ISIS port. * use returned config * fix cisco KNE * remove vrf change * update metadata * use root instead of batch update for BGP multipath deviation. * move vrf deviation after network instance config * increase gNMI timeout * update cisco metadata * update cisco metadata * revert bgp change * fix set batch * run gofmt * add link-local deviation cisco * revert bgp change * revert bgp change * simplify extra verification * mark configureDUT as helper. * remove duplicate isis deviation --------- Co-authored-by: Suhasini Tiwari <tsuhasini@google.com>

* Added deviation to Nokia: isis_mpls_unsupported * RT-5.6 Use loopback mode TERMINAL ref:PR1155 * . * . * Update interface_loopback_aggregate_test.go * Update interface_loopback_aggregate_test.go * Add README: Feature: <Telemetry: Firewall High Availability> * . * . * . * Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Delete feature/gnmi/tests/telemetry_high_availability_test/metadata.textproto * Rename metadata.proto to metadata.textproto * Update metadata.textproto * Update README.md * Update README.md --------- Co-authored-by: Swetha-haridasula <haridasula@google.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Avnish Singh <singhavnish@google.com>

* Fix sshfs volume source path * Correct default image name in options --------- Co-authored-by: Ali Al-Shabibi <alshabibi.ali@gmail.com>

* Update metadata.textproto * Update breakout_test.go * Update breakout_base_test.go * Update breakout_test.go * Update breakout_test.go * gofmt

* implement carrier-transitions fnt * fix CI checks * add canonical oc spec * update README and use SAMPLE mode

* The isis scale multi adj test * Test folder rename * change of libraries from google to github * some changes * Small change * Update feature/isis/otg_tests/isis_scale_multi_adjacency_test/README.MD Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update feature/isis/otg_tests/isis_scale_multi_adjacency_test/README.MD Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update feature/isis/otg_tests/isis_scale_multi_adjacency_test/README.MD Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update feature/isis/otg_tests/isis_scale_multi_adjacency_test/README.MD Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update feature/isis/otg_tests/isis_scale_multi_adjacency_test/README.MD Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update feature/isis/otg_tests/isis_scale_multi_adjacency_test/README.MD Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * small change * remove source mac address * add ether header to the flow * Rollback the changes done in interface.go * Change of the wait timer of the admin state to 60 sec * minor change in the readme * read me change * readme change --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* add aft reboot test * add metadata to reboot test. * remove unused functions --------- Co-authored-by: Avnish Singh <singhavnish@google.com>

* Add disk space cleanup step in workflow Added a step to free disk space tailored for Go/JS builds in the GitHub Actions workflow. * Fix action reference in GitHub workflow * cleanup space for go test and go static_analysis Added disk space cleanup steps to test and static analysis jobs. * Update disk space management in GitHub Actions Replaced the disk space cleanup action with custom commands to free additional disk space. * Refactor Go setup in GitHub Actions workflow Removed duplicate setup steps for Go in build and test jobs. * Reorganize Go setup in GitHub Actions workflow * Check if cache directory exists before moving Add check for existence before moving cache directory. * Refactor cache handling in Go workflow Removed conditional cache move and added a separate step to move cache in the GitHub Actions workflow. * Refactor Go setup and cache management in workflow Reorganized steps in the GitHub Actions workflow to set up Go after freeing disk space and added a step to move cache.

…mand (#4854) * internal/cfgplugin/qos.go - issue with Arista configs using runCliCommand fixed using much simpler way * Addressing comments * fixing the ci/cd error * fixed comments

m26singhvi · 2025-12-10T18:59:07Z

    * Set MACsec confidentiality offset: 0
-    * Set Replay Protection Window size: 64
-    * Set ICV enabled:True
+    * Set Replay Protection Window (out-of-sequence protection) size: 64


We are still keeping this as 64 ?

Do we need to change it?

In the offiline discussion, it was mentioned that the macsec peers are directly connected and there is no possibility of reordering.

In the normal case, yes - we don't actually expect reordering. But we don't control the link (the far end does) and we are recommending to the far-end that they use 64, so if we don't also use 64 that may be unexpected by that end. (64 allows a tiny bit of reordering, but also works when there is no reordering at all...)

m26singhvi · 2025-12-14T17:02:05Z

 * Traffic equally load-balanced across bundle interfaces in both directions
 * Header fields are as expected in both directions
-* Traffic is dropped (100 percent) when the must-secure MACSec sessions are down by disabling MACsec on ATE ports
+* Traffic is dropped (100 percent) when the must-secure MACSec sessions are down by changing a key on one side to a mismatch & forcing renegotiation on ATE ports


This comment doesn't take care of a scenario where due to connection issues - MKA session goes down. I think the comment should be generic to say that if there are no successful MKA session on the port, then all the traffic will be dropped.

m26singhvi · 2025-12-14T17:08:46Z

 * Traffic equally load-balanced across bundle interfaces in both directions
 * Header fields are as expected in both directions
-* Traffic is not dropped when the should-secure MACSec sessions are down by disabling MACsec on ATE ports
+* Traffic is not dropped when the should-secure MACSec sessions are down by changing a key on one side to a mismatch & forcing renegotiation on ATE ports


Unencrypted traffic is allowed when all the MKA sessions are down.

m26singhvi · 2025-12-14T17:12:21Z

+          "mka": {
+            "config": {
+              "key-chain": "keychain1",
+              "mka-policy": "must_secure"


It will be good if we can include mka policy definition as well here.

This is policy name references the policy later in the JSON:

featureprofiles/feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md

Lines 183 to 202 in 678e1f2

"mka": {

"policies": {

"policy": [

{

"config": {

"confidentiality-offset": "0_BYTES",

"include-icv-indicator": true,

"include-sci": true,

"key-server-priority": 15,

"macsec-cipher-suite": [

"GCM_AES_XPN_256"

],

"name": "must_secure",

"sak-rekey-interval": 30

},

"name": "must_secure"

}

]

}

}

dplore · 2025-12-17T17:36:09Z

This PR was closed due to a code rebase with commits whose CLA checks failed. Please see #4926 for a new PR which continues this work.

macsec clarification for readme

d01de32

steve-goog assigned dplore Nov 20, 2025

steve-goog requested a review from a team as a code owner November 20, 2025 21:34

gemini-code-assist bot reviewed Nov 20, 2025

View reviewed changes

Comment thread feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md Outdated

Comment thread feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md Outdated

m26singhvi reviewed Nov 21, 2025

View reviewed changes

steve-goog added 3 commits November 21, 2025 19:04

rename subtitles, update text, and keep OC config in-line with text

b456116

updated to clarify the macsec test cases

d3e88e2

update timestamps

8d125e6

replace json with regenerated version

1037e35

dplore reviewed Dec 9, 2025

View reviewed changes

Comment thread feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md Outdated

Comment thread feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md Outdated

dplore and others added 16 commits December 9, 2025 22:58

Fix issues in CNTR-2 tests (#4810)

d59536e

* Fix issues in CNTR-2 tests * fix context derivation and typo in log * Fix acl and remove fqdn metadata from test --------- Co-authored-by: Ali Al-Shabibi <alshabibi.ali@gmail.com>

Update bgp_2byte_4byte_asn_policy_test.go (#4831)

b25acfd

Update ondatra version (#4846)

0db1a57

Fix sshfs volume source path and default image name of cntrsrv (#4811)

2a4c482

* Fix sshfs volume source path * Correct default image name in options --------- Co-authored-by: Ali Al-Shabibi <alshabibi.ali@gmail.com>

Fix sampling_and_subscription_check_test import (#4848)

8348c02

Plt 1.1 breakout (#4843)

f8edd22

* Update metadata.textproto * Update breakout_test.go * Update breakout_base_test.go * Update breakout_test.go * Update breakout_test.go * gofmt

Implement carrier-transitions FNT (#4803)

317cd23

* implement carrier-transitions fnt * fix CI checks * add canonical oc spec * update README and use SAMPLE mode

Create README for AFT-1.3: AFTs collector Flap (#4816)

5e51569

add aft reboot test (#4849)

2a26ee7

* add aft reboot test * add metadata to reboot test. * remove unused functions --------- Co-authored-by: Avnish Singh <singhavnish@google.com>

internal/cfgplugin/qos.go - issue with Arista configs using runCliCom…

2325589

…mand (#4854) * internal/cfgplugin/qos.go - issue with Arista configs using runCliCommand fixed using much simpler way * Addressing comments * fixing the ci/cd error * fixed comments

steve-goog requested review from a team as code owners December 9, 2025 22:59

steve-goog added 6 commits December 9, 2025 22:59

remove security-policy as it's not yet in OC

1827dec

Merge branch 'main' into steve-goog/mpls_gre_udp_macsec

31f9908

syntax update

fc09c71

change key-id to a hexstring

8b721f6

update keychain names

7b17d96

add state name

6f3c562

m26singhvi reviewed Dec 10, 2025

View reviewed changes

Comment thread feature/policy_forwarding/otg_tests/mpls_gre_udp_macsec/README.md Outdated

Merge branch 'main' into steve-goog/mpls_gre_udp_macsec

628e0ee

dplore changed the title ~~macsec clarification for readme~~ macsec clarification for PF-1.17 README Dec 11, 2025

dplore and others added 5 commits December 12, 2025 10:36

Merge branch 'main' into steve-goog/mpls_gre_udp_macsec

026c2e6

revise canonical OC for macsec and keychain

fc5678f

add interfaces so leafref will resolve in validator

6970b41

fix curly brace in JSON

13e3a91

add test case for longer SAK key expiration time

678e1f2

steve-goog closed this Dec 13, 2025

steve-goog deleted the steve-goog/mpls_gre_udp_macsec branch December 13, 2025 00:25

m26singhvi reviewed Dec 14, 2025

View reviewed changes

steve-goog added the duplicate This issue or pull request already exists label Dec 18, 2025

		* Define first Policy(1) to cover must-secure scenario, as defined below
		* Define second Policy(2) to cover should-secure scenario, as defined below

	"mka": {
	"policies": {
	"policy": [
	{
	"config": {
	"confidentiality-offset": "0_BYTES",
	"include-icv-indicator": true,
	"include-sci": true,
	"key-server-priority": 15,
	"macsec-cipher-suite": [
	"GCM_AES_XPN_256"
	],
	"name": "must_secure",
	"sak-rekey-interval": 30
	},
	"name": "must_secure"
	}
	]
	}
	}

Conversation

steve-goog commented Nov 20, 2025

Uh oh!

gemini-code-assist bot commented Nov 20, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

OpenConfigBot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Functional Test Report for #4837 / 678e1f2

Virtual Devices

Hardware Devices

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

coveralls commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 20183581242

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dplore commented Dec 9, 2025

Uh oh!

dplore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dplore commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

OpenConfigBot commented Nov 20, 2025 •

edited

Loading

Pull Request Functional Test Report for #4837 / `678e1f2`

coveralls commented Nov 20, 2025 •

edited

Loading