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. >> >>> >>> ---- >>> 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: 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. >> >>> ;; >>> >>> 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. > >> >> >>> $LISTEN $DAEMON_OPTS $SSL_OPTS >>> errcode=$? >>> else >>> diff --git a/debian/openvswitch-testcontroller.postinst >>> b/debian/openvswitch-testcontroller.postinst >>> index 7242b4a..e8584e2 100755 >>> --- a/debian/openvswitch-testcontroller.postinst >>> +++ b/debian/openvswitch-testcontroller.postinst >>> @@ -42,6 +42,8 @@ case "$1" in >>> chmod go+r cert.pem req.pem >>> umask $oldumask >>> fi >>> + >>> + chown ovs:ovs -R /etc/openvswitch-testcontroller >>> ;; >>> >>> abort-upgrade|abort-remove|abort-deconfigure) >>> diff --git a/debian/openvswitch-vtep.init b/debian/openvswitch-vtep.init >>> index ebf4e26..6fe02a1 100644 >>> --- a/debian/openvswitch-vtep.init >>> +++ b/debian/openvswitch-vtep.init >>> @@ -10,6 +10,8 @@ >>> # Description: Initializes the Open vSwitch VTEP emulator >>> ### END INIT INFO >>> >>> +OVS_USER=ovs >>> +OVS_GROUP=ovs >>> >>> # Include defaults if available >>> default=/etc/default/openvswitch-vtep >>> @@ -40,17 +42,21 @@ start () { >>> cd /etc/openvswitch && ovs-pki req ovsclient && ovs-pki self-sign >>> ovsclient >>> fi >>> >>> + chown -R "$OVS_USER":"$OVS_GROUP" /etc/openvswitch >>> + chown -R "$OVS_USER":"$OVS_GROUP" /var/run/openvswitch >>> + >>> ovsdb-server --pidfile --detach --log-file --remote \ >>> punix:/var/run/openvswitch/db.sock \ >>> --remote=db:hardware_vtep,Global,managers \ >>> --private-key=/etc/openvswitch/ovsclient-privkey.pem \ >>> --certificate=/etc/openvswitch/ovsclient-cert.pem \ >>> --bootstrap-ca-cert=/etc/openvswitch/vswitchd.cacert \ >>> + --user "$OVS_USER":"$OVS_GROUP" \ >>> /etc/openvswitch/conf.db /etc/openvswitch/vtep.db >>> >>> modprobe openvswitch >>> >>> - ovs-vswitchd --pidfile --detach --log-file \ >>> + ovs-vswitchd --pidfile --detach --log-file --user >>> "$OVS_USER":"$OVS_GROUP" \ >>> unix:/var/run/openvswitch/db.sock >>> } >>> >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
