nhrpd: fix passphrase handling, add topotest for resolution request #17115
Conversation
|
Looks like frrbot and verify source are finding some issues that need to be cleaned up in the commit. Please take a second and do so. |
|
Can we also rewrite the commit message to first discuss the code change and then the topotest change especially in more detail. When I started looking at the subject line I thought I was going to be looking at topotest changes only and frankly it's a bit confusing |
jmuthiilabn
force-pushed
the
jmuthii/nhrpd-retry-resolution-topotest
branch
from
32ddfd1
to
c4c2808
Compare
jmuthiilabn
force-pushed
the
jmuthii/nhrpd-retry-resolution-topotest
branch
3 times, most recently
from
ce91fd8
to
c644d59
Compare
|
@donaldsharp How's the commit message now? I tried to be a little more descriptive but don't mind changing it again if it's still a little vague. |
|
I don't think these lint errors need to be fixed, but I'd like @donaldsharp or @ton31337 to make certain before pushing |
jmuthiilabn
force-pushed
the
jmuthii/nhrpd-retry-resolution-topotest
branch
from
c644d59
to
a3fb903
Compare
|
@donaldsharp @ton31337 Do these changes look good to you guys? I tried to fix up the commit message and fix up any of the lint errors. Of course I'll try to be receptive to anything other changes you guys recommend. |
|
I really like to see topotest changes split into their own commit. Can you please do that? |
|
Also, can you change the topic of the PR to reflect the added feature. The main addition is the the feature, the topotest is there just to make sure the new feature works. |
|
@Jafaral Yeah, I can split the commits up for the bug fix and the topotest, no problem. Just to be clear though, this PR is for a topotest for a feature that has already been merged: #16788 . The code change mentioned in this PR is just a bug I came across while writing the topotest. It's a bug that was causing issues with the topotest (as in the topotest doesn't run with this bug present) so I figured it makes sense to both fix the bug and add the topotest in the same PR. But the main thing I was going for here was the topotest. Do you still think I should change it? I'm sure it wouldn't be hard to make the PR focus on the code change if that's what you want. |
jmuthiilabn
force-pushed
the
jmuthii/nhrpd-retry-resolution-topotest
branch
from
a3fb903
to
c410aec
Compare
Modified nhrp_topo topotest to test for newly added resolution request retry feature. Changes to the topotest include adding a spoke to the existing nhrp_topo topotest so that a topology with two spokes and hub can be used to create shortcuts and test the sending/resending of resolution requests and responses between spoke and hub. The resolution request retry feature was tested by blocking incoming resolution requests on a receiving nodes to stop the creation of a successful shortcut - which then triggered the sending spoke to retry sending resolution requests Signed-off-by: Joshua Muthii <[email protected]>
Modified nhrp_connection_authorized(). Initially, when writing debug information about incoming NHRP packets with authentication enabled, the nhrp_connection_authorized() function would print the passphrase of the incoming packet as if it were a null terminated string. This meant that if the passphrase on the incoming packet had non ASCII-complient bytes in it, it would attempt to print those bytes anyway. There was also no check that the size of the passphrase in the incoming packet matched the size of the passphrase on the interface. The changes in this commit log the passphrase on the incoming packet as well as the passphrase on interface in HEX to avoid issues with ASCII. It also performs a check that accounts for the sizes of the two different passphrases Moved CISCO_PASS_LENGTH_LEN from nhrp_vty.c to nhrp_protocol.h for easier access to the macro in other files Signed-off-by: Joshua Muthii <[email protected]>
jmuthiilabn
force-pushed
the
jmuthii/nhrpd-retry-resolution-topotest
branch
from
c410aec
to
5718ee3
Compare
Jafaral
changed the title
|
Thanks @jmuthiilabn. I did change the title to capture the scope of the changes. Feel free to change as you see fit. |
|
@Mergifyio backport dev/10.2 |
✅ Backports have been created
|
nhrpd: fix passphrase handling, add topotest for resolution request (backport #17115)
Modified nhrp_topo topotest to test for newly added resolution request retry feature
Modified nhrp_connection_authorized() function so that debugging messages don't read into NHRP authentication extension as if it were a null-terminated string - which can lead to printing of non ASCII-compliant bytes.
Moved CISCO_PASS_LENGTH_LEN from nhrp_vty.c to nhrp_protocol.h for easier access to the macro in other files