Add preliminary support for SCTP#20033
Conversation
08d2dde to
eecf2c3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
pchaigno
left a comment
There was a problem hiding this comment.
Thanks for tackling this feature request! It would be nice to finally have SCTP support 😃
My main concern with the current changes is the many limitations. They need to be documented, but I think it would also be best to put the feature behind a flag rather than introduce many runtime checks. (I'm actually surprised you didn't hit any complexity issues because of that.)
The commit history also needs some work. Each commit should contain changes that logically belong together, in a way that facilitates review of the different aspects of this new feature.
|
Thanks for the feedback! For a feature flag, is the is goal that everywhere So, my takeaways from the review are:
In general, does the approach here work or will the code itself need to be reworked? |
b4196a3 to
d72dd0a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@DolceTriade it looks the test failure in https://jenkins.cilium.io/job/Cilium-PR-Runtime-net-next/3250/ is legitimate. |
|
Will debug and push fixes. Thanks for flagging. It would be nice to get all the reviews in first so I don't have to keep debugging test breakages though... |
|
I ran the runtime tests locally and they all passed. PTAL. |
This comment was marked as outdated.
This comment was marked as outdated.
|
I cannot reproduce these failures. Also they are failing in parts unrelated to my changes :-/ |
|
@DolceTriade multicluster is not required to pass currently, and the 4.9 job only failed on one specific test which looks like it was broken recently by #20956 . This PR LGTM from a testing perspective now. Unless @pchaigno / @jrajahalme / @cilium/sig-hubble have further feedback in the short term, I will be OK with merging this. |
pchaigno
left a comment
There was a problem hiding this comment.
A couple changes needed below but they should all be trivial to implement.
pchaigno
left a comment
There was a problem hiding this comment.
I think there was a small mishaps while addressing the changes above.
Allow specifying SCTP as a protocol for policy tracing. Signed-off-by: Harsh Modi <harshmodi@google.com>
This adds support L4 SCTP policies. Note that L7 policies on top of SCTP are unsupported. One behavior change is that policymap will no longer panic on unknown protos. Previously, it used to explicitly ignore UDP and panic on unknown protocols. Now it ignores all unknown protocols. Signed-off-by: Harsh Modi <harshmodi@google.com>
Allow tracing SCTP packets. Signed-off-by: Harsh Modi <harshmodi@google.com>
Signed-off-by: Harsh Modi <harshmodi@google.com> Co-authored-by: Nathan Sweet <nathanjsweet@users.noreply.github.com>
Signed-off-by: Harsh Modi <harshmodi@google.com>
Add docs on how to enable and the limitations of SCTP support. Signed-off-by: Harsh Modi <harshmodi@google.com>
SCTP requires many more branches and paths which will cause the programs to exceed the instruction limit on older kernels. Signed-off-by: Harsh Modi <harshmodi@google.com>
This test is a clone of hairpin_flow, but for SCTP packets. Signed-off-by: Harsh Modi <harshmodi@google.com>
|
Fixed for real :-/ idk what I did last night... |
No worries. You wouldn't believe of many times these sort of things happened to me 😅 I'll trigger the end-to-end tests now and will keep an eye on them. |
|
/test |
|
CI passes and we've had a good number of reviews on the code despite a couple team reviews missing (Hubble and proxy which should be minor here given lack of L7 support). Marking ready to merge. |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Partially implements #5719 and implements M1 for #20490