Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
README PF-1.3: Policy-based Static GUE Encapsulation to IPv4 tunnel #3482
base: main
Are you sure you want to change the base?
README PF-1.3: Policy-based Static GUE Encapsulation to IPv4 tunnel #3482
Changes from 3 commits
28bc7a4
dd24235
a6246c7
6f6a599
1be7b5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think this diagram needs to be updated with the change to have no DUM.
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.
instead, consider ATE port 2 is just the next hop for the DUT. The encapsulated packet can be captured at ATE port 2 and decoded to ensure it has the expected format.
Make decap testing a separate test.
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.
Ack, updated the test verifications. Decap will being done in a separate test.
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.
Is there a required number of source addresses? (A random combination could mean that the test just picks one source address.)
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.
Updated to specify fixed packet count and number of distinct flows.
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.
Can we clarify which counter this is expected to be? I assume that it is the
matched-packets
path that is defined below?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.
Yes, it's matched-packets, updated the test cases to reflect this.
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 seems to be harder to verify if we have
DUM
in the topology -- in the scenario that we are using ATE here I think we can do this with ingress accounting or pcap, but otherwise don't we need to pcap onDUM
?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.
Updated the topology to use one device only and we can do ingress accounting on ATE.
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.
Perhaps just clarify that the 'two Singleton ports' here are the individual ports within
LAG1
andLAG2
-- you are not saying that we need to check the ATE-DUT and ATE-DUM interfaces AIUI.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.
Ack, done.
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.
Same comments here as above w.r.t clarifications.
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.
So these are the un-encapsulated packets from ATE:Port1, did you mean to state again in the bullets?
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.
Again, I think it's worth clarifying the minimum entropy that you expect here. Something like "ensuring that there are at least X unique source-destination pairs".
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 would specify the number of 5 tuple flows and a fixed number of packets to send from ATE port 1. Then the validation should have some number of packets received on each ATE port 2 interface +/- some tolerance.
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.
Updated to specify fixed packet count and number of distinct flows.
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.
Ditto re: clarifications from the IPv4 case.
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.
These are the un-encapsulated packets from ATE:Port1, did you mean to state again in the bullets?
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 just meant to ensure that we make the same clarifications that were expressed above in this section. It looks like you did.
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.
Ok cool.
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.
IMO, it is ok to say, modify the flows in PF-1.3.5 to using IPv6 and repeat the traffic generation and validation. This is likely how one would write the code for this anyways.
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.
Ack, updated the IPv6 versions to be this way.
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.
Are all of the parameters that we want to specify the same between IPv4 and IPv6? It seems OK to do this as long as we need no modifications and (of course) we assume that IPv4 field names are translated to v6 ones. (I didn't mind the repetition for clarity.)
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.
Yes, all the parameters are the same. IPv4 ToS/TTL fields will be translated to IPv6 Traffic Class/Hop Limit respectively. Hope this works, otherwise let me know and I'll repeat with just these two field names being different.
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.
It is true, new OC paths will be needed. FYI, they should follow the pattern being established in the /afts tree, but using /policy-forwarding instead.
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.
Ack, thanks for the pointer. I'll follow this to send proposal for the OC paths.