Support SCTP port mapping #1825

ishidawataru · 2017-06-29T07:01:46Z

Introducing support for SCTP(Stream Control Transmission Protocol) port mapping.

SCTP support is necessary to run S1AP and Diameter applications in docker containers.

Related issue : moby/moby#9689

AkihiroSuda · 2017-07-05T01:50:37Z

cc @aboch @mavenugo @sanimej @fcrisciani PTAL

AkihiroSuda · 2017-07-06T06:13:09Z

also cc @aaronlehmann (moby/swarmkit#2298)

ishidawataru · 2017-07-13T05:13:20Z

@aboch @mavenugo @sanimej @fcrisciani @aaronlehmann Any comments?
Lack of SCTP port mapping support prevents from running most of the mobile telecom applications on docker. With this feature, we will be able to deploy/test http://git.osmocom.org/ or https://gitlab.eurecom.fr/oai/openair-cn much more easily using docker.

sanimej · 2017-07-14T16:57:39Z

@ishidawataru Thanks. Does SCTP have any dependencies on kernel version ? What distro/kernel version has this been tested with ?

ishidawataru · 2017-07-15T02:12:32Z

@sanimej Thanks for your comment.

Linux has built-in support for the SCTP since the 2.6 kernel, so I believe there is no dependency issue for docker users.
I'm testing with Ubuntu 16.04.2. If there is other distro I should test, please let me know.

ishidawataru · 2017-07-19T19:16:32Z

@sanimej ping

thaJeztah · 2017-07-21T08:52:11Z

agent.pb.go

 size := m.Size()
- data = make([]byte, size)
- n, err := m.MarshalTo(data)
+ dAtA = make([]byte, size)


was this a deliberate change?

oh, never mind this is generated code, my eye just fell on it 😅

@thaJeztah Thanks for your review! Do you have any other comments?

Not right now; it's not really my expertise, but I was checking up on the status of this PR (as it's a requirement for your other PR's)

I'll try getting it reviewed, but the networking team is quite busy at the moment, so doing my best 😄

@thaJeztah Right, this needs to get merged first for the SCTP port mapping feature.
Thanks a lot for your help! I totally understand the networking team is quite busy 😅

ishidawataru · 2017-07-31T05:31:59Z

@aboch @mavenugo @sanimej @fcrisciani @aaronlehmann @thaJeztah Any chance you can take a look and review this PR?

thaJeztah · 2017-07-31T11:30:00Z

ping @mavenugo (who I think was going to look into this PR)

mavenugo · 2017-07-31T14:37:20Z

@ishidawataru @thaJeztah yes. I will take a look at it as soon as other higher priority activities are handled.

AkihiroSuda · 2017-08-31T03:30:52Z

@mavenugo ping 😃

AkihiroSuda · 2017-09-26T05:21:05Z

Can we move this forward?

ping libnetwork maintainers: @aboch @LK4D4 @icecrime @mavenugo @sanimej @chunchun @fcrisciani

cc those who made comments in moby/moby#9689 @rickhofstede @scottstamp @duglin @thaJeztah @razaborg

Anyone please take a look? 😄

orbsfoc · 2017-11-01T09:21:46Z

any chance of this getting merged in the near future?

rkbug · 2018-01-09T19:17:59Z

Any update on this? :)

Peter-eid · 2018-01-17T09:42:08Z

@aboch @mavenugo @sanimej @fcrisciani @aaronlehmann What is the current status of this feature ?

thaJeztah · 2018-01-17T12:28:33Z

ping @ddebroy PTAL

AkihiroSuda · 2018-02-02T08:49:26Z

rebased

codecov-io · 2018-02-02T09:07:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@20dd462). Click here to learn what that means.
The diff coverage is 18.91%.

@@            Coverage Diff            @@
##             master    #1825   +/-   ##
=========================================
  Coverage          ?   40.47%           
=========================================
  Files             ?      139           
  Lines             ?    22314           
  Branches          ?        0           
=========================================
  Hits              ?     9032           
  Misses            ?    11960           
  Partials          ?     1322

Impacted Files	Coverage Δ
types/types.go	`41.4% <0%> (ø)`
cmd/proxy/main.go	`0% <0%> (ø)`
portmapper/mapper.go	`65.24% <0%> (ø)`
portmapper/proxy.go	`33.33% <0%> (ø)`
service_linux.go	`3.23% <0%> (ø)`
agent.pb.go	`0.72% <0.69%> (ø)`
portallocator/portallocator.go	`81.08% <100%> (ø)`
drivers/bridge/port_mapping.go	`66.66% <100%> (ø)`
cmd/proxy/proxy.go	`80% <100%> (ø)`
cmd/proxy/sctp_proxy.go	`77.55% <77.55%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20dd462...1fced67. Read the comment docs.

ghost · 2018-02-02T20:13:03Z

cmd/proxy/sctp_proxy.go

+ to.Close()
+ wg.Done()
+ }
+


It doesn't appear that 'dir' is used here. Should it be removed and the invocations below adjusted?

Sorry, thought the placement would show more context. I mean the "dir" parameter of the broker function 7 lines up.

fcrisciani

Left some comments.
Also did not finish yet the review of the vendored files

fcrisciani · 2018-02-03T16:08:42Z

cmd/proxy/sctp_proxy.go

+ return nil, err
+ }
+ // If the port in frontendAddr was 0 then ListenSCTP will have a picked
+ // a port to listen on, hence the call to Addr to get that actual port:


do you mean that if the port is not specified is ephemeral?

yes. the comment is taken from tcp_proxy.go.

fcrisciani · 2018-02-03T16:11:49Z

cmd/proxy/sctp_proxy.go

+func (proxy *SCTPProxy) clientLoop(client *sctp.SCTPConn, quit chan bool) {
+ backend, err := sctp.DialSCTP("sctp", nil, proxy.backendAddr)
+ if err != nil {
+ log.Printf("Can't forward traffic to backend sctp/%v: %s\n", proxy.backendAddr, err)


please change this to use logrus package.
es: logrus.WithError(err).Errorf("[...]")

Currently, cmd/proxy package does not use logrus.
Not sure why, probably for reducing dependency.

fcrisciani · 2018-02-03T16:17:20Z

cmd/proxy/sctp_proxy.go

+ for {
+ client, err := proxy.listener.Accept()
+ if err != nil {
+ log.Printf("Stopping proxy on sctp/%v for sctp/%v (%s)", proxy.frontendAddr, proxy.backendAddr, err)


same here use logrus

As above, cmd/proxy looks intentionally avoiding using logrus.

fcrisciani · 2018-02-03T16:23:12Z

cmd/proxy/sctp_proxy.go

+ backendC := sctp.NewSCTPSndRcvInfoWrappedConn(backend)
+
+ var wg sync.WaitGroup
+ var broker = func(to, from net.Conn, dir bool) {


dir is unused, please remove it

fcrisciani · 2018-02-03T16:33:21Z

portmapper/mapper.go

+ }
+
+ if useProxy {
+ m.userlandProxy, err = newProxy(proto, hostIP, allocatedHostPort, container.(*sctp.SCTPAddr).IP[0], container.(*sctp.SCTPAddr).Port, pm.proxyPath)


before accessing the first element should you check the len?

fcrisciani · 2018-02-03T16:34:42Z

portmapper/proxy.go

+ addr := &sctp.SCTPAddr{IP: []net.IP{hostIP}, Port: hostPort}
+ return &dummyProxy{addr: addr}
+ default:
+ panic(fmt.Errorf("Unknown addr type: %s", proto))


also here better to use logrus

Also wonder if a panic is correct here?

fcrisciani · 2018-02-03T16:38:23Z

vendor/github.com/ishidawataru/sctp/sctp.go

+ return buf.Bytes()
+}
+
+func htons(h uint16) uint16 {


why do you need to redefine this? why not using the standard: binary.BigEndian.PutUint16? Network order is bigendian

As I chose to let binary.Write() to serialize each field of golang structures, we need htons() which returns big endian format of uint16 to put to the structures.
If we serialize each field of the structure by hand, we can use binary.BigEndian.PutUint16() as you suggest.

fcrisciani · 2018-02-03T16:44:10Z

vendor/github.com/ishidawataru/sctp/sctp.go

+ sndRcvInfoSize = unsafe.Sizeof(info)
+}
+
+func toBuf(v interface{}) []byte {


following the comment below, if you make sure that the network format is respected, this function will have to simply read bigEndian format and the machine itself will take care of storing it with the proper endianess allowing you to delete all the dependency with the endianess discovery and data manipulation

toBuf() is not used for endian handling but for serializing golang structure to []byte.
I chose to use binary.Write() instead of using unsafe or serializing each field by hand using PutUint8() etc.

fcrisciani · 2018-02-03T16:48:29Z

vendor/github.com/ishidawataru/sctp/sctp.go

+}
+
+func (a *SCTPAddr) ToRawSockAddrBuf() []byte {
+ buf := []byte{}


it's better here to have a defined length, not doing so will create a lot of following allocation for every append.
From the code looks like the worst case the size will be len(a.IP) * 16Bytes (ipv6 len). If you think that the IPv6 case is not so common, maybe you can allocate at least the space for the IPv4 so that the append to them won't require any extra allocation during the append

We (@ishidawataru and me) believe this overhead is negligible, as a.IP should not contain so many entries.
So we'd like to keep the current code for simplicity.

fcrisciani · 2018-02-03T16:57:26Z

vendor/github.com/ishidawataru/sctp/sctp.go

+
+ for n, i := range a.IP {
+ if a.IP[n].To4() != nil {
+ b.WriteRune('/')


from the documentation of net.Addr looks like the format that they are suggesting in the example is like: (for example, "192.0.2.1:25", "[2001:db8::1]:80") this does not seem to match the suggested format

https://golang.org/pkg/net/#Addr

The two methods Network and String conventionally return strings that can be passed as the arguments to Dial, but the exact form and meaning of the strings is up to the implementation.

I think this SCTP-specific form is ok.
@ishidawataru WDYT?

Maybe I missed something, but I think that this looks like the same form as suggested by net.Addr. The difference is that this is a list of addresses instead of a single address and so they get separated by '/'s. Until I saw the last line of the function I didn't realize however, that the leading '/' that gets put in the buffer gets omitted from the final string.

opened ishidawataru/sctp#10 for improving readability

fcrisciani · 2018-02-06T17:35:47Z

vendor/github.com/ishidawataru/sctp/sctp.go

+ return nil, fmt.Errorf("invalid input: %s", addrs)
+ }
+ ipaddrs := make([]net.IP, 0, len(elems))
+ for _, e := range elems[:len(elems)-1] {


shouldn't this start from 1? from above looks like the string starts with /

See above. The starting '/' built in the string gets trimmed.

fcrisciani · 2018-02-06T17:39:55Z

vendor/github.com/ishidawataru/sctp/sctp.go

+
+func (c *SCTPConn) SubscribeEvents(flags int) error {
+ var d, a, ad, sf, p, sh, pa, ada, au, se uint8
+ if flags&SCTP_EVENT_DATA_IO > 0 {


what if you remove all these if and you merge everything in the init down like:

param := EventSubscribe{ DataIO: uint8(flags&SCTP_EVENT_DATA_IO > 0), Association: uint8(flags&SCTP_EVENT_ASSOCIATION > 0),

Go doesn't allow conversion from bool to int by cast. From what I can see on the mailing lists it was requested around go 1.5, but declined. Pity...

ghost · 2018-02-06T16:45:35Z

cmd/proxy/main.go

@@ -48,6 +50,9 @@ func parseHostContainerAddrs() (host net.Addr, container net.Addr) {
 case "udp":
 host = &net.UDPAddr{IP: net.ParseIP(*hostIP), Port: *hostPort}
 container = &net.UDPAddr{IP: net.ParseIP(*containerIP), Port: *containerPort}


nit: should probably update the comment at the start of the function to include SCTP.

ghost · 2018-02-06T16:52:43Z

cmd/proxy/network_proxy_test.go

+ if err != nil {
+ t.Fatal(err)
+ }
+ server = &TCPEchoServer{listener: listener, testCtx: t}


nit: Should this structure be renamed now? It's no longer just a "TCP" echo server. Maybe "StreamEchoServer"...

ghost · 2018-02-06T21:48:07Z

vendor/github.com/ishidawataru/sctp/sctp.go

+
+ for n, i := range a.IP {
+ if a.IP[n].To4() != nil {
+ b.WriteRune('/')


Maybe I missed something, but I think that this looks like the same form as suggested by net.Addr. The difference is that this is a list of addresses instead of a single address and so they get separated by '/'s. Until I saw the last line of the function I didn't realize however, that the leading '/' that gets put in the buffer gets omitted from the final string.

ghost · 2018-02-06T21:50:16Z

vendor/github.com/ishidawataru/sctp/sctp.go

+ return nil, fmt.Errorf("invalid input: %s", addrs)
+ }
+ ipaddrs := make([]net.IP, 0, len(elems))
+ for _, e := range elems[:len(elems)-1] {


See above. The starting '/' built in the string gets trimmed.

ghost · 2018-02-06T22:13:57Z

vendor/github.com/ishidawataru/sctp/sctp.go

+
+func (c *SCTPConn) SubscribeEvents(flags int) error {
+ var d, a, ad, sf, p, sh, pa, ada, au, se uint8
+ if flags&SCTP_EVENT_DATA_IO > 0 {


Go doesn't allow conversion from bool to int by cast. From what I can see on the mailing lists it was requested around go 1.5, but declined. Pity...

ghost · 2018-02-06T22:18:19Z

vendor/github.com/ishidawataru/sctp/sctp.go

+}
+
+func resolveFromRawAddr(ptr unsafe.Pointer, n int) (*SCTPAddr, error) {
+ addr := &SCTPAddr{


Not checking the boundary of the underlying buffer makes me a bit nervous on principle, but I suppose we generally trust the kernel.

ghost · 2018-02-06T22:34:35Z

vendor/github.com/ishidawataru/sctp/sctp_unsupported.go

+ "net"
+)
+
+var ErrUnsupported = errors.New("operation unsupported")


This seems like a perfectly reasonable discipline for building cleanly on other platforms in general. But from the docker/libnetwork perspective, I'm concerned about two things:

This error message is way too generic -- if libnetwork just returns this and the caller prints the string there's no context to say what isn't supported. At the very least perhaps the string should be changed to "SCTP not supported on this platform" or some such.

This file is fairly deep in the call stack to be flagging lack of support as far as libnetwork goes. Ideally either libnetwork or moby itself should flag early that an SCTP operation isn't supported by "this version" on "this platform". It kind of seems wrong to, for example, try to create an SCTP proxy thinking this is perfectly valid up until we attempt to bind a listening socket. Maybe I'm just being overly cautious on this one though.

opened ishidawataru/sctp#11

Signed-off-by: Wataru Ishida <ishida.wataru@lab.ntt.co.jp> Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda · 2018-02-13T07:02:17Z

@ghost @fcrisciani @thaJeztah

Thank you a lot for reviewing this PR.

Addressed the comments. PTAL.

cc @ishidawataru

thaJeztah · 2018-02-13T09:25:55Z

Hm; @ghost was @ctelfer-docker ... @ctelfer did you remove that account?

ctelfer

They addressed all of my concerns completely: LGTM.

(note: sorry for the confusion, but the "ghost" comments on this PR were mine. Switched accounts mid-review.)

fcrisciani

LGTM

AkihiroSuda · 2018-02-16T02:10:00Z

Thanks for merging!

fcrisciani · 2018-02-16T02:10:57Z

@AkihiroSuda thanks for contributing and addressing the comments

AkihiroSuda · 2018-02-16T04:34:02Z

updated moby-side PR moby/moby#33922

verizonold · 2018-03-06T17:46:30Z

@mavenugo I am new to docker support for SCTP apps. Can you please point me to some documentation on how docker supports SCTP apps?

thaJeztah · 2018-03-06T17:48:03Z

@verizonold see moby/moby#9689 (comment) for links to all related PR's (including some docs)

Vishesh-GIT · 2018-03-23T12:43:42Z

I am very much looking for the SCTP support on docker. I am following this thread from long. can anybody give info on when it will be available with docker installation or if I clone this source code how can it be used in my system so that I can test SCTP support

Peter-eid · 2018-03-23T13:11:06Z

@Vishesh-GIT SCTP support is added to the latest Docker release 18.03.0-ce, just update and use it.

verizonold · 2018-03-23T14:17:40Z

so is this for port mapping? we can now map tcp/udp/sctp ports? Can you please provide an example?

…

On Fri, Mar 23, 2018 at 6:11 AM, Peter-eid ***@***.***> wrote: @Vishesh-GIT <https://github.com/vishesh-git> SCTP support is added to the latest Docker release 18.03.0-ce <https://docs.docker.com/release-notes/docker-ce/#stable-releases>, just update and use it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1825 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AjVVDY4HRgD914L07lYCTTWcJgWE7xc4ks5thPRxgaJpZM4OI5nn> .

ishidawataru force-pushed the sctp branch from e943873 to c04444a Compare June 29, 2017 07:57

ishidawataru mentioned this pull request Jul 3, 2017

Support SCTP port mapping (bump up API to v1.37) moby/moby#33922

Merged

4 tasks

ishidawataru mentioned this pull request Jul 11, 2017

Support SCTP port mapping docker/cli#278

Merged

AkihiroSuda mentioned this pull request Jul 11, 2017

Proposal: exposing SCTP ports for container moby/moby#9689

Closed

thaJeztah reviewed Jul 21, 2017

View reviewed changes

AkihiroSuda force-pushed the sctp branch 2 times, most recently from fff72eb to 1fced67 Compare February 2, 2018 08:49

fcrisciani requested review from fcrisciani, ddebroy and a user February 2, 2018 18:18

ghost suggested changes Feb 2, 2018

View reviewed changes

fcrisciani reviewed Feb 3, 2018

View reviewed changes

fcrisciani reviewed Feb 6, 2018

View reviewed changes

ghost suggested changes Feb 6, 2018

View reviewed changes

Support SCTP port mapping

5c67fbe

Signed-off-by: Wataru Ishida <ishida.wataru@lab.ntt.co.jp> Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the sctp branch from 1fced67 to 5c67fbe Compare February 13, 2018 07:01

fcrisciani requested a review from ctelfer February 13, 2018 16:22

ctelfer approved these changes Feb 13, 2018

View reviewed changes

fcrisciani approved these changes Feb 16, 2018

View reviewed changes

fcrisciani merged commit ed2130d into moby:master Feb 16, 2018

andrewhsu mentioned this pull request Feb 21, 2018

vndr libnetwork to 14db3c4 moby/swarmkit#2520

Closed

Support SCTP port mapping #1825

Support SCTP port mapping #1825

Conversation

ishidawataru commented Jun 29, 2017 • edited Loading

AkihiroSuda commented Jul 5, 2017

AkihiroSuda commented Jul 6, 2017

ishidawataru commented Jul 13, 2017

sanimej commented Jul 14, 2017

ishidawataru commented Jul 15, 2017

ishidawataru commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishidawataru commented Jul 31, 2017

thaJeztah commented Jul 31, 2017

mavenugo commented Jul 31, 2017

AkihiroSuda commented Aug 31, 2017

AkihiroSuda commented Sep 26, 2017

orbsfoc commented Nov 1, 2017 • edited Loading

rkbug commented Jan 9, 2018

Peter-eid commented Jan 17, 2018

thaJeztah commented Jan 17, 2018

AkihiroSuda commented Feb 2, 2018

codecov-io commented Feb 2, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcrisciani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Feb 13, 2018

thaJeztah commented Feb 13, 2018

ctelfer left a comment

Choose a reason for hiding this comment

fcrisciani left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Feb 16, 2018

fcrisciani commented Feb 16, 2018

AkihiroSuda commented Feb 16, 2018

verizonold commented Mar 6, 2018

thaJeztah commented Mar 6, 2018

Vishesh-GIT commented Mar 23, 2018

Peter-eid commented Mar 23, 2018 • edited Loading

verizonold commented Mar 23, 2018 via email

ishidawataru commented Jun 29, 2017 •

edited

Loading

orbsfoc commented Nov 1, 2017 •

edited

Loading

codecov-io commented Feb 2, 2018 •

edited

Loading

Peter-eid commented Mar 23, 2018 •

edited

Loading