Thanks Eelco, requested changes addressed here:
https://patchwork.ozlabs.org/project/openvswitch/patch/20230210160229.3689298-1-odiv...@gmail.com/ > On 10 Feb 2023, at 11:21, Eelco Chaudron <echau...@redhat.com> wrote: > > > > On 8 Feb 2023, at 16:22, Vladislav Odintsov wrote: > >> This patch adds new ovs-ctl options to pass umask configuration to allow >> OVS daemons set requested socket permissions on group. Previous >> behaviour (if using with systemd service unit) created sockets with 0750 >> permissions mask (group has no write permission). >> >> Write permission for group is reasonable in usecase, where ovs-vswitchd >> or ovsdb-server runs as a non-privileged user:group (say, >> openvswitch:openvswitch) and it is needed to access unix socket from >> process running as another non-privileged user. In this case >> administrator has to add that user to openvswitch group and can connect >> to OVS sockets from a process running under that user. >> >> Two new ovs-ctl options --ovsdb-server-umask and --ovs-vswitchd-umask >> were added to manage umask values for appropriate daemons. This is >> useful for systemd users: both ovs-vswitchd and ovsdb-server systemd >> units read options from single /etc/sysconfig/openvswitch configuration >> file. So, with separate options it is possible to set umask only for >> specific daemon. >> >> OPTIONS="--ovsdb-server-umask=0002" >> >> in /etc/openvswitch/sysconfig file will set umask to 0002 value before >> starting only ovsdb-server, while >> >> OPTIONS="--ovs-vswitchd-umask=0002" >> >> will set umask to ovs-vswitchd daemon. >> >> Previous behaviour (not setting umask) is left as default. > > Thanks for reworking the patch, small none technical nit below, the rest > looks good. > > //Eelco > > >> Reported-at: >> https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401501.html >> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >> >> --- >> v1 -> v2: >> - added item in NEWS file as Ilya's suggestion; >> - addressed Eelco's review comments; >> - moved umask call from ovs-ctl to ovs-lib; >> - added restoration of umask to effective value before the umask change; >> - previous version --ovs-umask option was split into two: >> --ovs-vswitchd-umask and --ovsdb-server-umask in order to make >> possible umask configuration for specific daemon when running with >> systemd. >> >> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >> --- >> NEWS | 7 +++++++ >> utilities/ovs-ctl.in | 16 ++++++++++++---- >> utilities/ovs-lib.in | 17 ++++++++++++++--- >> 3 files changed, 33 insertions(+), 7 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index fe6055a27..f7df598bd 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -4,6 +4,13 @@ Post-v3.1.0 >> * OVS now collects per-interface upcall statistics that can be obtained >> via 'ovs-appctl dpctl/show -s' or the interface's statistics column >> in OVSDB. Available with upstream kernel 6.2+. >> + - ovs-ctl: >> + * Added support to set umask value when starting OVS daemons. New >> options >> + --ovsdb-server-umask=MODE and --ovs-vswitchd-umask=MODE were added >> for >> + that. For instance, when write access on befalf of OVS group is >> needed >> + for ovsdb-server, pass --ovsdb-umask=0002. Use --vswitchd-umask to >> set >> + umask ovs-vswitchd daemon umask. This will allow ovsdb-server or >> + ovs-vswitchd to create sockets with access mode of 0770. >> >> >> v3.1.0 - xx xxx xxxx >> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in >> index d91552588..0b2820c36 100644 >> --- a/utilities/ovs-ctl.in >> +++ b/utilities/ovs-ctl.in >> @@ -156,8 +156,8 @@ do_start_ovsdb () { >> [ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER" >> [ "$OVSDB_SERVER_OPTIONS" != "" ] && set "$@" $OVSDB_SERVER_OPTIONS >> >> - start_daemon "$OVSDB_SERVER_PRIORITY" "$OVSDB_SERVER_WRAPPER" "$@" \ >> - || return 1 >> + start_daemon "$OVSDB_SERVER_PRIORITY" "$OVSDB_SERVER_WRAPPER" \ >> + "$OVSDB_SERVER_UMASK" "$@" || return 1 >> >> # Initialize database settings. >> ovs_vsctl -- init -- set Open_vSwitch . db-version="$schemaver" \ >> @@ -226,8 +226,8 @@ do_start_forwarding () { >> [ "$OVS_USER" != "" ] && set "$@" --user "$OVS_USER" >> [ "$OVS_VSWITCHD_OPTIONS" != "" ] &&set "$@" $OVS_VSWITCHD_OPTIONS >> >> - start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" "$@" >> || >> - return 1 >> + start_daemon "$OVS_VSWITCHD_PRIORITY" "$OVS_VSWITCHD_WRAPPER" \ >> + "$OVS_VSWITCHD_UMASK" "$@" || return 1 >> fi >> } >> >> @@ -348,6 +348,8 @@ set_defaults () { >> OVS_VSWITCHD_WRAPPER= >> OVSDB_SERVER_OPTIONS= >> OVS_VSWITCHD_OPTIONS= >> + OVSDB_SERVER_UMASK= >> + OVS_VSWITCHD_UMASK= >> >> DB_FILE=$dbdir/conf.db >> DB_SOCK=$rundir/db.sock >> @@ -421,6 +423,12 @@ Other important options for "start", "restart" and >> "force-reload-kmod": >> add given key-value pair to Open_vSwitch external-ids >> --delete-bridges delete all bridges just before starting ovs-vswitchd >> --ovs-user="user[:group]" pass the --user flag to ovs daemons >> + --ovsdb-server-umask=MODE Set umask prior to run ovsdb-server daemon. >> + This is useful to manage daemon's sockets >> permissions. >> + Default is not to change umask (inherited from >> shell). >> + --ovs-vswitchd-umask=MODE Set umask prior to run ovs-vswitchd daemon. >> + This is useful to manage daemon's sockets >> permissions. >> + Default is not to change umask (inherited from >> shell). >> >> Less important options for "start", "restart" and "force-reload-kmod": >> --daemon-cwd=DIR set working dir for OVS daemons (default: >> $DAEMON_CWD) >> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in >> index 13477a6a9..004de631c 100644 >> --- a/utilities/ovs-lib.in >> +++ b/utilities/ovs-lib.in >> @@ -165,9 +165,9 @@ install_dir () { >> } >> >> start_daemon () { >> - priority=$1 >> - wrapper=$2 >> - shift; shift >> + priority=$1 && shift >> + wrapper=$1 && shift >> + umask=$1 && shift >> daemon=$1 >> strace="" >> >> @@ -223,8 +223,19 @@ start_daemon () { >> set nice -n "$priority" "$@" >> fi >> >> + # set requested umask if any and turn previous value back > > Please use a capital S for Set and end with a dot. > >> + if [ -n "$umask" ]; then >> + previuos_umask_value=$(umask) >> + umask "$umask" >> + fi >> + >> action "Starting $daemon" "$@" || return 1 >> >> + # if umask was set, turn umask value to previous value > > Please use a capital I for If and end with a dot. > >> + if [ -n "$umask" ]; then >> + umask "$previuos_umask_value" >> + fi >> + >> if test X"$strace" != X; then >> # Strace doesn't have the -D option so we attach after the fact. >> setsid $strace -o "$logdir/$daemon.strace.log" \ >> -- >> 2.36.1 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org <mailto:d...@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > d...@openvswitch.org <mailto:d...@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
