Bug #2647: VTY local-ip cmd in "listen m3ua <PORT>" node not applied. - libosmo-sccp + libosmo-sigtran - Open Source Mobile Communications

Custom queries

all-by-project

Actions

Copy link

Bug #2647

closed

VTY local-ip cmd in "listen m3ua <PORT>" node not applied.

Added by pespin over 6 years ago. Updated over 6 years ago.

Status:

Resolved

Priority:

Normal

Assignee:

pespin

Target version:

Start date:

11/17/2017

Due date:

% Done:

100%

Spec Reference:

Description

In osmo-stp, cmd "local-ip" inside node "listen m3ua 2905" is actually not applied. Because the server is created + bound at "listen" command time using NULL as IP, and at "local-ip" time the IP is changed but the server is not re-bound using the new IP, so it keeps listening at 0.0.0.0.

One possibility is adding code to re-bind the socket after setting the IP in "local-ip". With this way, however, e we cannot identify 2 servers which use different IP but same port. Furthermore, the code first binds against 0.0.0.0 and then it binds against the specific IP, which can create more problems. For instance, if 2 osmo-stp instances are started almost at the same time, one may bind to 0.0.0.0 and then the 2nd one may do the same before the 1st once changes the socket bound, which means the 2nd instance will fail to bind to 0.0.0.0.

I think we should have the IP in the "listen" cmd, but this means breaking backwards compatibility in VTY: listen m3ua A.B.C.D 2905 <0-65534>

History
Notes
Associated revisions

Actions

Copy link

Updated by pespin over 6 years ago

Status changed from New to Feedback
Assignee set to pespin

I submitted a patch to re-bind the socket once local-ip is run, but I still think the correct fix would be to have IP parameter in the listen node. I'm not sure if we are allowed to break the VTY config compatibility though.

https://gerrit.osmocom.org/#/c/4893/

Actions

Copy link

Updated by laforge over 6 years ago

It's sort-of a hard call. I don't want to introduce ITP syntax incompatibility without a good reason and stay as close as possible.

On the other hand, binding first to INADDR_ANY and then changing the bind on "local-ip" has a very high risk of failing in weird ways on start-up, if other sockets already are bound to some IPs of that port.

Moving the bind() call to the "local-ip" will have the strictest compatibility with ITP, but will make local-ip mandatory, which so far it was not. We have to figure out a way to warn the users if they use config files that rely on the current implicit "local-ip 0.0.0.0".

Actions

Copy link

Updated by neels over 6 years ago

As a general landscape that comes to mind here:

The ss7 VTY code has some actions in the go_parent_cb() which starts up the asp once the VTY node exits. See osmo_ss7_asp_restart(asp) call in osmo_ss7_vty_go_parent(). I guess we can shift that stuff around and/or do something similar for the listen node?

Related: I sometimes see 'Restart is not implemented' messages in the log, so the restart on go_parent is a nice try, but can't work when we haven't gotten shutdown/startup implemented yet.

Though it may seem a bit unintuitive at first, the 'no shutdown' way from OsmoGGSN's VTY is also a solution for stuff like this. Might make sense to add an alias as 'startup' .. the point is, deciding explicitly with a VTY command when things start up can be nice. Makes most sense when we also have a proper 'shutdown' and be able to alternate between the two...

Actions

Copy link

Updated by pespin over 6 years ago

neels wrote:

As a general landscape that comes to mind here:

The ss7 VTY code has some actions in the go_parent_cb() which starts up the asp once the VTY node exits. See osmo_ss7_asp_restart(asp) call in osmo_ss7_vty_go_parent(). I guess we can shift that stuff around and/or do something similar for the listen node?

I tried doing the same (deferring bind() until get get out of the listen node), but it seems the vty_go_parent() is not called during .cfg file parsing. I suspect it's only called when in vty manually willing to go a level back.

Related: I sometimes see 'Restart is not implemented' messages in the log, so the restart on go_parent is a nice try, but can't work when we haven't gotten shutdown/startup implemented yet.

Though it may seem a bit unintuitive at first, the 'no shutdown' way from OsmoGGSN's VTY is also a solution for stuff like this. Might make sense to add an alias as 'startup' .. the point is, deciding explicitly with a VTY command when things start up can be nice. Makes most sense when we also have a proper 'shutdown' and be able to alternate between the two...

For now the current patch delays the bind/listening until "local-ip" cmd is run. Harald pushed a commit to a branch to be able to register a callback to be called when a specific node is left behind (finished parsing), but it's not merged yet afaik, and for now the local-ip delay is enough.

However, after the patch people require to update their config to set "local-ip 0.0.0.0" if they still have no local-ip set up. I'll send an e-mail once the patch is merged.

Actions

Copy link

Updated by laforge over 6 years ago

On Mon, Nov 20, 2017 at 10:45:03AM +0000, pespin [REDMINE] wrote:

For now the current patch delays the bind/listening until "local-ip" cmd is run.

Harald pushed a commit to a branch to be able to register a callback
to be called when a specific node is left behind (finished parsing),
but it's not merged yet afaik,

I don't want us to introduce yet another new ABI / LIBVERSION just for
that.

However, after the patch people require to update their config to set
"local-ip 0.0.0.0" if they still have no local-ip set up. I'll send an
e-mail once the patch is merged.

This is what I'm mostly worried about. We keep doing something like
this every week or every few weeks, and it's bad to have our users
suffer through things that (from far) appear like they should be
resolvable in an automatic way.

At the very least we could (after config file parsing) iterate over the
list of XUA servers and print WARNING messages on that fact, like
"M3UA server for port XYZ: Not bound to any IP address. This is probably
not what you want. Consider adding local-ip 0.0.0.0"

Actions

Copy link

Updated by neels over 6 years ago

If taking actions while parsing the VTY doesn't work, IMHO the most elegant solution is still to read in the config file first, store the values, and then after config file parsing is done call the startup, bind, etc actions. Then at least we firmly don't support changing the running config instead of getting entangled in strings of half crazy automation attempts...

I tried doing the same (deferring bind() until get get out of the listen node), but it seems the vty_go_parent() is not called during .cfg file parsing. I suspect it's only called when in vty manually willing to go a level back.

This can't be the case. Before I started tweaking around in it, the go_parent_cb was the only way to move up to a parent node, so it's guaranteed that this is always called. (I have a patch that makes it optional to have a go_parent_cb at all, now that we remember parents explicitly, but it isn't merged nor pushed yet, so the go_parents should still be doing what they were always doing)

Instead I suspect that the go_parent_cb is not always called because the program doesn't take care to call ss7's go_parent_cb in the proper ways. Remember that there is only one go_parent_cb per client program and that they need to call each other in a sensible way, which we don't always get right. That's one of the reasons to rely less on the go_parent_cb.

Actions

Copy link