Skip to content

Add preliminary support for SCTP #20033

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Sep 13, 2022
Merged

Add preliminary support for SCTP #20033

merged 8 commits into from
Sep 13, 2022

Conversation

DolceTriade
Copy link

@DolceTriade DolceTriade commented May 31, 2022 •
edited by joestringer
Loading

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Partially implements #5719 and implements M1 for #20490

Add partial support for SCTP

@DolceTriade DolceTriade requested a review from a team as a code owner May 31, 2022 23:26
@DolceTriade DolceTriade requested review from a team May 31, 2022 23:26
@DolceTriade DolceTriade requested review from a team as code owners May 31, 2022 23:26
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 31, 2022
@DolceTriade DolceTriade changed the title Add preliminary support SCTP Add preliminary support for SCTP May 31, 2022
@DolceTriade DolceTriade requested a review from a team as a code owner June 6, 2022 20:32
@joestringer joestringer added the release-note/major This PR introduces major new functionality to Cilium. label Jun 6, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 6, 2022
@DolceTriade DolceTriade force-pushed the sctp-v2 branch 5 times, most recently from 08d2dde to eecf2c3 Compare June 6, 2022 22:08
@joestringer

This comment was marked as outdated.

Sign in to view
@pchaigno pchaigno self-requested a review June 9, 2022 20:05
pchaigno
pchaigno requested changes Jun 12, 2022
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@DolceTriade
Copy link
Author

DolceTriade commented Jun 13, 2022 •
edited
Loading

Thanks for the feedback!

For a feature flag, is the is goal that everywhere IPPROTO_SCTP is a condition, we need to #ifdef ENABLE_SCTP or something? Is it sufficient to include the #ifdef in https://github.com/cilium/cilium/blob/master/bpf/lib/lb.h#L340? Is it sufficient to feature guard SCTP support only in the BPF side of the code? I worry this will make things more convoluted in the Go side if we have to do a runtime check for SCTP enabled everywhere...

So, my takeaways from the review are:

  • Rebase commit history to be more clear
  • Feature guard SCTP support
  • Add tests

In general, does the approach here work or will the code itself need to be reworked?

@DolceTriade DolceTriade force-pushed the sctp-v2 branch 2 times, most recently from b4196a3 to d72dd0a Compare June 15, 2022 05:56
@DolceTriade DolceTriade requested a review from a team June 15, 2022 05:56
@DolceTriade DolceTriade requested a review from a team as a code owner June 15, 2022 05:56
Harsh Modi and others added 8 commits September 12, 2022 06:37
api: Add support for SCTP.
09ed964 
Allow specifying SCTP as a protocol for policy tracing.

Signed-off-by: Harsh Modi <harshmodi@google.com>
policy: Add support for SCTP policies.
b2f44ee 
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>
hubble: Add SCTP support.
12ce8a9 
Allow tracing SCTP packets.

Signed-off-by: Harsh Modi <harshmodi@google.com>
@nathanjsweet
bpf: Add partial SCTP support.
1a23b83 
Signed-off-by: Harsh Modi <harshmodi@google.com>
Co-authored-by: Nathan Sweet <nathanjsweet@users.noreply.github.com>
helm: Add sctp.enabled and use it to set ENABLE_SCTP.
582dc06 
Signed-off-by: Harsh Modi <harshmodi@google.com>
docs: Add basic docs on SCTP.
1d89310 
Add docs on how to enable and the limitations of SCTP support.

Signed-off-by: Harsh Modi <harshmodi@google.com>
sctp: Error out if SCTP is enabled on older kernels.
1c9032a 
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>
test: Add a BPF test to exercise SCTP.
551d471 
This test is a clone of hairpin_flow, but for SCTP packets.

Signed-off-by: Harsh Modi <harshmodi@google.com>
@DolceTriade
Copy link
Author

Fixed for real :-/ idk what I did last night...

@pchaigno
Copy link
Member

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.

@pchaigno
Copy link
Member

/test

@pchaigno pchaigno added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Sep 12, 2022
@pchaigno
Copy link
Member

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.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 13, 2022
gandro
gandro approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, Hubble still looks good

@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 13, 2022
@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 13, 2022
@nebril nebril merged commit 26d31f0 into cilium:master Sep 13, 2022
@DolceTriade DolceTriade deleted the sctp-v2 branch September 13, 2022 20:32
@joestringer joestringer added this to the v1.13.0-rc1 milestone Sep 30, 2022
@rolinh rolinh mentioned this pull request Jan 25, 2023
Open
@joestringer joestringer mentioned this pull request Mar 2, 2023
Merged
@aojea aojea mentioned this pull request May 29, 2023
Merged
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request Oct 4, 2024
@julianwiedmann
docs: network policy: remove SCTP from missing features list
bd18535 
The `policy enforcement` bullet in the referenced issue
(cilium#5719) is completed. Looks like
the network policy support was added with
cilium#20033.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann mentioned this pull request Oct 4, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 4, 2024
@julianwiedmann @nathanjsweet
docs: network policy: remove SCTP from missing features list
475310f 
The `policy enforcement` bullet in the referenced issue
(#5719) is completed. Looks like
the network policy support was added with
#20033.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qmonnet qmonnet qmonnet left review comments

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@joamaki joamaki joamaki approved these changes

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/major This PR introduces major new functionality to Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
v1.13.0-rc1
Development

Successfully merging this pull request may close these issues.