Bug 1160013

You should be able to work around this with a dispatcher script. Eg, copy this to a file:

    #!/bin/bash

    # most of this stolen from /etc/sysconfig/network-scripts/ifup-routes

    MATCH='^[[:space:]]*(\#.*)?$'

    handle_ip_file() {
        local f t type= file=$1 proto="-4"
        f=${file##*/}
        t=${f%%-*}
        type=${t%%6}
        if [ "$type" != "$t" ]; then
            proto="-6"
        fi

        { cat "$file" ; echo ; } | while read line; do
            if [[ ! "$line" =~ $MATCH ]]; then
                /sbin/ip $proto $type add $line
            fi
        done
    }

    case "$2" in
        up)
            FILES="/etc/sysconfig/network-scripts/rule-$1 /etc/sysconfig/network-scripts/rule6-$1"

            for file in $FILES; do
                if [ -f "$file" ]; then
                    handle_ip_file $file
                fi
            done
            ;;
    esac

Then save it as /etc/NetworkManager/dispatcher.d/50-rules.sh, owned by root, mode 755.

And make sure the "NetworkManager-dispatcher.service" service is enabled.

(In reply to Dan Winship from comment #6)
> One possibility would be to make ifcfg-rh ignore the route- file if there is
> also a rule- file, and then have a dispatcher pre-up script that runs
> ifup-routes on interfaces that have rule- files.

pushed danw/policy-based-routing-rh1160013, which implements this; it would also require spec file changes to install the example dispatcher script, which are not included there.

> Another problem is what to do if the user adds a route to the connection
> from nm-connection-editor. We don't want to overwrite the existing route-
> file in that case, since that would delete the other routes. I think we'd
> have to just have writer_update_connection() return an error in this case.

I implemented this as well, though it required making the check in two different places, because we want to prevent the daemon from even doing nm_settings_connection_replace_settings(), not just nm_settings_connection_commit_connection().

Hm... this means that if the connection has secrets, we won't be able to save them either though... maybe I should move the complex_routes check out of commit_connection() and just skip over writing the route files if we get asked to commit a connection with complex routes? (But still leave the check in replace_settings, to disallow ordinary connection editing.)

>> settings: add NMSettingsConnection:filename

How about drop the duplicate properties NMIfcfgConnectionPrivate.path and NMKeyfileConnectionPrivate.path and implement them based on NMSettingsConnection.filename.


>> dispatcher: pass CONNECTION_FILENAME to dispatcher scripts

I think we should prefix our environment variables with NM_*.
I think it's wrong that we didn't do that previously, but I would do it now for new variables.

>> ifcfg-rh: allow handling complex routing rules via dispatcher (rh #1160013)


-dir=`dirname $CONNECTION_FILENAME`
-profile=`echo $CONNECTION_FILENAME | sed -e 's/.*\/ifcfg-//'`
+dir="$(dirname "$CONNECTION_FILENAME")"
+if [ "$dir" != "/etc/sysconfig/network-scripts" ]; then
+    exit 0
+fi
+
+profile="$(printf '%s' "$CONNECTION_FILENAME" | sed -e 's/^.*\/ifcfg-\([^/\]\+\)$/\1/')"



pushed a minor change on your branch.

(In reply to Thomas Haller from comment #10)
> >> settings: add NMSettingsConnection:filename
> 
> How about drop the duplicate properties NMIfcfgConnectionPrivate.path and
> NMKeyfileConnectionPrivate.path and implement them based on
> NMSettingsConnection.filename.

done

> >> dispatcher: pass CONNECTION_FILENAME to dispatcher scripts
> 
> I think we should prefix our environment variables with NM_*.
> I think it's wrong that we didn't do that previously, but I would do it now
> for new variables.

We call the dispatcher script with a clean environment, so there's no chance of confusion. I think it's better to keep the variable naming consistent.

> -dir=`dirname $CONNECTION_FILENAME`
> +dir="$(dirname "$CONNECTION_FILENAME")"

I intentionally made it a /bin/sh script rather than bash, since it doesn't actually need any bash-isms, and so, post-shellshock that seems smarter.

> pushed a minor change on your branch.

This looks fine, but it's not really related to this branch, so it could just get committed independently. Although, I pushed a fixup on top of your patch...


(rebased and pushed)

(In reply to Dan Winship from comment #11)
> (In reply to Thomas Haller from comment #10)
> > >> settings: add NMSettingsConnection:filename
> > 
> > How about drop the duplicate properties NMIfcfgConnectionPrivate.path and
> > NMKeyfileConnectionPrivate.path and implement them based on
> > NMSettingsConnection.filename.
> 
> done
> 
> > >> dispatcher: pass CONNECTION_FILENAME to dispatcher scripts
> > 
> > I think we should prefix our environment variables with NM_*.
> > I think it's wrong that we didn't do that previously, but I would do it now
> > for new variables.
> 
> We call the dispatcher script with a clean environment, so there's no chance
> of confusion. I think it's better to keep the variable naming consistent.

ok

> > -dir=`dirname $CONNECTION_FILENAME`
> > +dir="$(dirname "$CONNECTION_FILENAME")"
> 
> I intentionally made it a /bin/sh script rather than bash, since it doesn't
> actually need any bash-isms, and so, post-shellshock that seems smarter.

My comment contained several changes. You dislike all of them?

The current script is for example wrong if you have two keyfiles
  /etc/NetworkManager/system-connections/ifcfg-em1
  /etc/NetworkManager/system-connections/rule-em1
and you activate the former.

Which part of my suggestion is a bashism? Neither $() nor printf is, is it?



> > pushed a minor change on your branch.
> 
> This looks fine, but it's not really related to this branch, so it could
> just get committed independently. Although, I pushed a fixup on top of your
> patch...

yes, I'd like to wanted piggyback the review on your bugzilla entry.



>> ifcfg-rh: allow handling complex routing rules via dispatcher (rh #1160013)

could you update contrib/rpm to package the dispatcher script too?

(In reply to Thomas Haller from comment #14)
> The current script is for example wrong if you have two keyfiles
>   /etc/NetworkManager/system-connections/ifcfg-em1
>   /etc/NetworkManager/system-connections/rule-em1
> and you activate the former.

oh, oops. I definitely had a check that the prefix was "/etc/sysconfig/network-scripts" in there at one point, but I seem to have accidentally removed it.

> Which part of my suggestion is a bashism? Neither $() nor printf is, is it?

Oh, huh. $() isn't old-school Bourne shell syntax, so I'd always assumed it was bash-specific. But it's actually a POSIX invention. OK.

repushed with three changes:

  - the second-to-last commit ("settings: make
    nm_settings_connection_replace_and_commit() virtual") is now making
    replace_and_commit() virtual, rather than making replace_settings()
    virtual as before, since there are other situations (such as
    "nmcli con reload") where replace_settings() needs to work

  - the dispatcher script now checks both the dirname and the basename
    for validity

  - the spec file now installs the dispatcher script

(In reply to Dan Winship from comment #16)

LGTM now

Pushed the commit documenting CONNECTION_FILENAME. Other than that the branch looks good to me.

committed upstream (6d5e62a3)

*** Bug 1163753 has been marked as a duplicate of this bug. ***

Created attachment 984814 [details]
jklimes's patch to install the script at all

Created attachment 984826 [details]
my patch to fix the "down" bug

attaching this as a patch to the build system so people can double-check my spec file modifications too.

I also pushed danw/policy-based-routing-rh1160013, because the build system patch doesn't actually show my entire commit, because I updated the spec file in contrib/ too, but contrib doesn't get disted, so that part of the change isn't in this patch

fixed. (note that the actual fixed build has the same version number as the scratch build from earlier today)

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-0311.html

I do not see how it should work now. For me NM still (NetworkManager-1.0.0-16.git20150121.b4ea599c.el7_1.x86_64) does not use config rule/route files. And I do not see there would be (unless user's own creation) a script to support dispatcher.

Hey lejeczek,

This should be completely working with NM 1.0.0-12.git20150121.b4ea599c and later, but it is provided by a separate package:

NetworkManager-config-routing-rules


The package provides dispatcher scripts:

$ rpm -ql NetworkManager-config-routing-rules
/etc/NetworkManager/dispatcher.d/10-ifcfg-rh-routes.sh
/etc/NetworkManager/dispatcher.d/pre-up.d/10-ifcfg-rh-routes.sh


Note that for these to be executed, the NetworkManager-dispatcher service must be enabled:

# systemctl enable NetworkManager-dispatcher.service



If this continues to not work as expected please open a case with Red Hat Technical Support.


Thank you,

Patrick

is it ok that this service does not show up in systemctl -a ?

Hey,

Yes, that is expected. I will admit to not being a systemd expert so I cannot immediately tell you how this works, but you'll see that the dispatcher is indeed fired every time you trigger a device state change:

# journalctl -u NetworkManager-dispatcher.service
-- Logs begin at Fri 2015-08-21 08:23:47 EDT, end at Fri 2015-08-21 17:28:25 EDT. --
Aug 21 08:24:12 patrick systemd[1]: Starting Network Manager Script Dispatcher Service...
Aug 21 08:24:12 patrick systemd[1]: Started Network Manager Script Dispatcher Service.
Aug 21 08:24:12 patrick nm-dispatcher[1530]: Dispatching action 'pre-up' for enp0s25
Aug 21 08:24:12 patrick nm-dispatcher[1530]: Dispatching action 'up' for enp0s25
Aug 21 08:24:14 patrick nm-dispatcher[1530]: Dispatching action 'pre-up' for virbr0
Aug 21 08:24:14 patrick nm-dispatcher[1530]: Dispatching action 'up' for virbr0-nic
Aug 21 08:24:14 patrick nm-dispatcher[1530]: Dispatching action 'up' for virbr0
Aug 21 14:02:22 patrick systemd[1]: Starting Network Manager Script Dispatcher Service...
Aug 21 14:02:22 patrick systemd[1]: Started Network Manager Script Dispatcher Service.
Aug 21 14:02:22 patrick nm-dispatcher[13743]: Dispatching action 'dhcp4-change' for enp0s25


If you need further assistance please open a case with Red Hat Technical Support or perhaps try the Customer Portal discussion groups for RHEL:

https://access.redhat.com/discussions?product=25&category=All&tags=All



Thank you,

Patrick

it's unfortunate, one can easily miss what one cannot see in usual(expected) places.
Maybe NetworkManager-config-routing-rules should become part of just NetworkManager?

It's November 20, 2020, and I'm still running into this BUG. It took me a while to find it here. It's a good idea to place NetworkManager-config-routing-rules in NetworkManager.

NetworkManager has had native support for applying routing policy "ip rule" and ip routes since v1.18.0:

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/master/NEWS#L269