-
Notifications
You must be signed in to change notification settings - Fork 746
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
[testplan] Macsec static key ciphers #13189
base: master
Are you sure you want to change the base?
Conversation
Catch up on current master
Merge current master
|
@yxieca This test keeps erroring out on the T0 pipeline run, but it is marked as a T2 test so I don't understand why its even trying to run it on T0. Are you able to see more details about what is going on with the pipeline? |
@@ -189,7 +189,7 @@ def cleanup_macsec_configuration(duthost, ctrl_links, profile_name): | |||
for d in devices: | |||
if isinstance(d, EosHost): | |||
continue | |||
assert wait_until(30, 1, 0, lambda d=d: not get_mka_session(d)) | |||
assert wait_until(180, 1, 0, lambda d=d: not get_mka_session(d)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to multiply the waiting time by 6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were seeing a lot of failures in cleanup where it was taking longer for the session to be re-established. Adjusting this timer helped alleviate those issues.
continue | ||
else: | ||
logger.debug("k: {} v: {}".format(k, v)) | ||
setup_macsec_configuration(duthost, int_list, k, v['priority'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can it work if we only change the MACsec profile of the duthost without updating the neighbor's profile? At least, the CAK key will be different. Isn't that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup_macsec_configuration function in use here configured both DUT and the neighbor.
|
||
# use each macsec profile and verify operation | ||
for k, v in list(macsec_profiles.items()): | ||
if duthost.facts["asic_type"] == "vs" and v['send_sci'] == "false": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this topic relevant here? Given that both duthost and neighbor host are SONiC, there is no EOS machine involved, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check was taken from here in the macsec init file:
def startup_macsec(self, request, macsec_duthost, ctrl_links, macsec_profile, tbinfo):
topo_name = tbinfo['topo']['name']
def __startup_macsec():
profile = macsec_profile
if request.config.getoption("neighbor_type") == "eos":
if macsec_duthost.facts["asic_type"] == "vs" and profile['send_sci'] == "false":
# On EOS, portchannel mac is not same as the member port mac (being as SCI),
# then src mac is not equal to SCI in its sending packet. The receiver of vSONIC
# will drop it for macsec kernel module does not correctly handle it.
pytest.skip(
"macsec on dut vsonic, neighbor eos, send_sci false")
```
When those two conditions are met the test fails.
assert "BGP state = Established" in output | ||
|
||
|
||
def test_static_key_ciphers(duthost, nbrhosts, request, profile_name, tbinfo, ctrl_links, rekey_period): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duthost fixture may get you a T2 supervisor. Change to use the enum_rand_one_per_hwsku_frontend_hostname fixture to get duthost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the test to use this fixture.
Description of PR
Summary:
Fixes # (issue)
New test plan and case to test the different macsec ciphers and profiles.
Type of change
Back port request
Approach
What is the motivation for this PR?
To add new test
How did you do it?
Based on manual test our team runs
How did you verify/test it?
Using virtual T2 testbed
Any platform specific information?
Tested on virtual testbed with vsonic neighbors
Supported testbed topology if it's a new test case?
T2
Documentation
Yes, test plan document for macsec is updated.