On Thu, Oct 8, 2015 at 4:01 PM, Ansis Atteka <aatt...@nicira.com> wrote: > On Wed, Oct 7, 2015 at 8:20 PM, Andy Zhou <az...@nicira.com> wrote: >> On Wed, Oct 7, 2015 at 6:49 PM, Ansis Atteka <aatt...@nicira.com> wrote: >>> On Mon, Oct 5, 2015 at 6:38 PM, Andy Zhou <az...@nicira.com> wrote: >>> >>> Thanks Andy for doing this! I will have another more careful look at >>> this patch tomorrow, because I think I somehow managed to get into a >>> state where after installing debian packages /etc/openvswitch still >>> belonged to root. >>> >> Is possible you have your selinux changes already applied? It worked >> in my set up. > > This is Debian so SElinux (and my RHEL patch) does not come into > picture here. Will spend a little more time to understand what exactly > happened, but to give a hint I tried to install packages *one by one* > (opposed to a single dpkg -i command) so perhaps this might have to do > something with the order if it wasn't my setup itself. > >>> >>>> Changes to Debian packaging scripts to create the ovs user and group. >>>> Fix the permissions of ovs created files and directories so that >>>> they are accessible by users belong to the ovs group. >>> >>> s/users belong/users that belong >> Thanks, will fix. >>> >>>> Start daemons as the ovs user. >>>> >>>> Signed-off-by: Andy Zhou <az...@nicira.com> >>> >>> This patch, I believe, breaks upgrades: >>> >>> Wed Oct 7 23:35:44 UTC 2015:stop >>> * ovs-vswitchd is not running >>> * ovsdb-server is not running >>> Wed Oct 7 23:35:44 UTC 2015:load-kmod >>> Wed Oct 7 23:35:44 UTC 2015:start --system-id=random --no-run-as-root >>> ovsdb-server: /var/run/openvswitch/ovsdb-server.pid.tmp: create failed >>> (Permission denied) >>> >>> I guess this was happening because that directory still belonged to >>> the root user after the upgrade. >>> >> The error mentioned above will cause this. > > The error I mentioned above was for /etc/openvswitch. This is for > /var/run/openvswitch/. And they happened independently.
I think I found the bug; it should be fixed in v2. > >>> >>>> >>>> ---- >>>> This patch does not include changes to the ipsec package. Ansis has >>>> other plans for updating it. >>> >>> Yeah, I will have to figure out how to do this from Python daemons. I >>> guess we have to synchronize our changes so that we don't break IPsec. >>> >>>> --- >>>> NEWS | 3 ++- >>>> debian/automake.mk | 1 + >>>> debian/openvswitch-common.postinst | 42 >>>> ++++++++++++++++++++++++++++++ >>>> debian/openvswitch-pki.postinst | 2 ++ >>>> debian/openvswitch-switch.init | 1 + >>>> debian/openvswitch-switch.logrotate | 2 +- >>>> debian/openvswitch-switch.postinst | 3 +++ >>>> debian/openvswitch-testcontroller.init | 3 ++- >>>> debian/openvswitch-testcontroller.postinst | 2 ++ >>>> debian/openvswitch-vtep.init | 8 +++++- >>>> 10 files changed, 63 insertions(+), 4 deletions(-) >>>> create mode 100755 debian/openvswitch-common.postinst >>>> >>>> diff --git a/NEWS b/NEWS >>>> index cdf2815..8f0e5b6 100644 >>>> --- a/NEWS >>>> +++ b/NEWS >>>> @@ -23,7 +23,8 @@ Post-v2.4.0 >>>> - Dropped support for GRE64 tunnel. >>>> - Mark --syslog-target argument as deprecated. It will be removed in >>>> the next OVS release. >>>> - - Added --user option to all daemons >>>> + - Added --user option to all daemons. >>>> + - Debain package starts daemons as the 'ovs' user. >>> s/Debain/Debian >>>> >>>> >>>> v2.4.0 - 20 Aug 2015 >>>> diff --git a/debian/automake.mk b/debian/automake.mk >>>> index c29a560..3092569 100644 >>>> --- a/debian/automake.mk >>>> +++ b/debian/automake.mk >>>> @@ -8,6 +8,7 @@ EXTRA_DIST += \ >>>> debian/dkms.conf.in \ >>>> debian/dirs \ >>>> debian/openvswitch-common.dirs \ >>>> + debian/openvswitch-common.postinst \ >>>> debian/openvswitch-common.docs \ >>>> debian/openvswitch-common.install \ >>>> debian/openvswitch-common.manpages \ >>>> diff --git a/debian/openvswitch-common.postinst >>>> b/debian/openvswitch-common.postinst >>>> new file mode 100755 >>>> index 0000000..c90ab5a >>>> --- /dev/null >>>> +++ b/debian/openvswitch-common.postinst >>>> @@ -0,0 +1,42 @@ >>>> +#!/bin/sh >>>> +# postinst script for openvswitch-switch >>> Copy-paste error: This is openvswitch-common and not >>> openvswitch-switch postinst script >>> >>>> +# >>>> +# see: dh_installdeb(1) >>>> + >>>> +set -e >>>> + >>>> +# summary of how this script can be called: >>>> +# * <postinst> `configure' <most-recently-configured-version> >>>> +# * <old-postinst> `abort-upgrade' <new version> >>>> +# * <conflictor's-postinst> `abort-remove' `in-favour' <package> >>>> +# <new-version> >>>> +# * <postinst> `abort-remove' >>>> +# * <deconfigured's-postinst> `abort-deconfigure' `in-favour' >>>> +# <failed-install-package> <version> `removing' >>>> +# <conflicting-package> <version> >>>> +# for details, see http://www.debian.org/doc/debian-policy/ or >>>> +# the debian-policy package >>>> + >>>> +case "$1" in >>>> + configure) >>>> + LOGDIR=/var/log/openvswitch >>>> + # Create the ovs user and group. >>>> + adduser --system --group --no-create-home --quiet ovs || true >>> There are useradd and adduser utilities. I tried *bare minimum* Debian >>> 8 installation and adduser was not installed by default. Should you >>> add adduser to dependencies in debian/control file? >>> >> Good catch. I will add adduser as dependency. >>> >>>> + >>>> + # Fix ownership and permissions. >>>> + chown -R ovs:ovs $LOGDIR >>>> + chmod -R 0770 $LOGDIR >>> You have probably thought more about this, but now "adm" group is >>> dropped for OVS logs. Do you see any issue with this? >>> >> This is an area I'd like to get some input. Should we add ovs to the >> adm group by default and >> set the ownership of those log files to ovs:adm? > > This is the best I could find: > http://serverfault.com/questions/485473/what-is-the-canonical-use-for-the-sys-and-adm-groups > > Basically after your patch I can't see > /var/log/openvswitch/ovs-vswitchd.log anymore with my regular Ubuntu > user. However, on Ubuntu there is already precedent with > speech-dispatcher that has the same behavior: Before my changes /var/log/openvswitch was set to allow read access by "others'. The v2 just posted keeps this setting. > > aatteka@aatteka-MacBookPro:/var/log$ cat speech-dispatcher/ > cat: speech-dispatcher/: Permission denied > > >>> >>>> + ;; >>>> + >>>> + abort-upgrade|abort-remove|abort-deconfigure) >>>> + ;; >>>> + >>>> + *) >>>> + echo "postinst called with unknown argument \`$1'" >&2 >>>> + exit 1 >>>> + ;; >>>> +esac >>>> + >>>> +#DEBHELPER# >>>> + >>>> +exit 0 >>>> diff --git a/debian/openvswitch-pki.postinst >>>> b/debian/openvswitch-pki.postinst >>>> index f4705e9..030180d 100755 >>>> --- a/debian/openvswitch-pki.postinst >>>> +++ b/debian/openvswitch-pki.postinst >>>> @@ -31,6 +31,8 @@ case "$1" in >>>> if test ! -e /var/lib/openvswitch/pki; then >>>> ovs-pki init >>>> fi >>>> + >>>> + chown ovs:ovs -R /var/lib/openvswitch/pki >>> Shouldn't changing user recursively for /var/lib/openvswitch be a >>> better approach? >> Probably. I see that this is only package that creates >> /var/lib/openvswitch, but I don't see any other directory >> being created in addition to pki. I could be wrong since I don't >> install this package often. > > I am a bit afraid that we have plenty of OVS packages creating > sub-directories in other package directories (either from debian.dirs > file, postinst script, init.d script or the daemon itself). I really > don't have a good alternative, but this is something, I am afraid, > could get easily out of hand and become hard to maintain, if we will > be calling mkdir and chown from various places. > >>>> ;; >>>> >>>> abort-upgrade|abort-remove|abort-deconfigure) >>>> diff --git a/debian/openvswitch-switch.init >>>> b/debian/openvswitch-switch.init >>>> index 8e156da..febf414 100755 >>>> --- a/debian/openvswitch-switch.init >>>> +++ b/debian/openvswitch-switch.init >>>> @@ -64,6 +64,7 @@ start () { >>>> if test X"$FORCE_COREFILES" != X; then >>>> set "$@" --force-corefiles="$FORCE_COREFILES" >>>> fi >>>> + set "$@" --no-run-as-root >>>> set "$@" $OVS_CTL_OPTS >>>> "$@" || exit $? >>>> if [ "$2" = "start" ] && [ "$READ_INTERFACES" != "no" ]; then >>>> diff --git a/debian/openvswitch-switch.logrotate >>>> b/debian/openvswitch-switch.logrotate >>>> index a7a71bd..be929b6 100644 >>>> --- a/debian/openvswitch-switch.logrotate >>>> +++ b/debian/openvswitch-switch.logrotate >>>> @@ -1,7 +1,7 @@ >>>> /var/log/openvswitch/*.log { >>>> daily >>>> compress >>>> - create 640 root adm >>>> + create 640 ovs ovs >>>> delaycompress >>>> missingok >>>> rotate 30 >>>> diff --git a/debian/openvswitch-switch.postinst >>>> b/debian/openvswitch-switch.postinst >>>> index 2464572..9183bdc 100755 >>>> --- a/debian/openvswitch-switch.postinst >>>> +++ b/debian/openvswitch-switch.postinst >>>> @@ -33,6 +33,9 @@ case "$1" in >>>> fi >>>> done >>>> fi >>>> + >>>> + # fix owner and permissions for /etc/openvswitch. >>>> + chown ovs:ovs -R /etc/openvswitch >>> >>> can you assume that this directory unconditionally exists (I believe >>> all debian scripts are run with set -e and you don't want them to >>> terminate prematurely)? >> It is listed in openvswitch-switch.dirs, not enough? > > I think yesterday offline we discussed something among the lines where > OVS should be able to handle gracefully corner case scenario where > unknowing user deletes /etc/openvswitch directory. I think we got into > this discussion because we tried to understand rationale on why OVS > init.d script creates /etc/openvswitch directory if it is not present > already. > > aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ sudo rm -rf /etc/openvswitch/ > aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ sudo dpkg-reconfigure > openvswitch-switch > chown: cannot access ‘/etc/openvswitch’: No such file or directory > > Overall, I think we should have a clear stance on what OVS should do > in such corner cases - either attempt to recover system or tell user > that if he deleted /etc/openvswitch directory then he should figure > out on his own how to recover setup. I haven't looked in Debian > Maintainerš book if it has any recommendations about this subject. I think SElinux may post a stricter restriction on what script can do. I think we should revisit this taking SElinux into consideration. v2 changes does not fully address those issues yet. > >>> >>>> ;; >>>> >>>> abort-upgrade|abort-remove|abort-deconfigure) >>>> diff --git a/debian/openvswitch-testcontroller.init >>>> b/debian/openvswitch-testcontroller.init >>>> index 67b7a99..352c95d 100755 >>>> --- a/debian/openvswitch-testcontroller.init >>>> +++ b/debian/openvswitch-testcontroller.init >>>> @@ -109,7 +109,7 @@ start_server() { >>>> fi >>>> >>>> if [ ! -d /var/run/openvswitch ]; then >>>> - install -d -m 755 -o root -g root /var/run/openvswitch >>>> + install -d -m 755 -o ovs -g ovs /var/run/openvswitch >>> if directory exists this will not change ownership, right? >> Yes, This is a bug. Thanks. >>>> fi >>>> >>>> SSL_OPTS= >>>> @@ -139,6 +139,7 @@ start_server() { >>>> if [ -z "$DAEMONUSER" ] ; then > > By they way DAEMONUSER and OVS_USER seem to replicate the same > use-cases. Do we really need both? > >>>> start-stop-daemon --start --pidfile $PIDFILE \ >>>> --exec $DAEMON -- --detach --pidfile=$PIDFILE \ >>>> + --user ovs:ovs \ >>> it seems inconsistent that in some places you use --user ovs:ovs but >>> in other --user "$OVS_USER":"$OVS_GROUP" >> I used it in openvswitch-vtep.init since there are multiple >> references. ovs:ovs is used when >> it is a single change. What's your preference? > > each time you hard code "ovs" (or "ovs:ovs") user you are repeating > yourself. The other place in this postinst script, I believe, is > "install" command: > > aatteka@aatteka-PowerEdge-T110:~/Git/ovs$ cat > debian/openvswitch-testcontroller.init | grep ovs > DAEMON=/usr/bin/ovs-testcontroller # Introduce the server's location here > NAME=ovs-testcontroller # Introduce the short server's name here > DESC=ovs-testcontroller # Introduce a short description here > install -d -m 755 -o ovs -g ovs /var/run/openvswitch > --user ovs:ovs \ > > Here are my preferences: > 1. define "ovs" user and group in one place and somehow pass it to all > postinst scripts. So that if one day you would need to change user > from "ovs" to something else a single line change would be sufficient. > 2. define "ovs" user and group at most once in each postinst script. > So that if one day you would need to change user from "ovs" to > something else a single line change in each postinst script would be > sufficient. > 3. define "ovs" user and group inline or sometimes in postinst script. I have adopted #2 for V2. I can't figure out a clean way to implement #1. Overall, thanks for testing out the changes and feedbacks. They are very helpful. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
