On 27 November 2015 at 05:35, Flavio Leitner <f...@sysclose.org> wrote:
> On Fri, Nov 20, 2015 at 03:33:20AM -0800, Andy Zhou wrote:
> > Make RHEL systemd distributions start OVS and OVN daemons under user
> > ovs. The 'ovs' user and group will be created at the openvswitch RPM
> > installtion time.
> >
>
Hi Flavio. I have taken over the development of this patch from Andy. See
my comments inline.
Also, while reviewing and trying to fix issues you pointed out in this
patch I have thought about another possible implementation. Basically, what
you say that user-switch would happen at run-time with something like:
# ovs-vsctl migrate-to-user <user>
This might have few benefits:
1. there wouldn't anymore be scattered and hardcoded chmod, chown
invocations from the packaging hooks and init scripts. I am afraid that if
chowns are invoked from various places it might become hard to maintain
project.
2. instead of hardcoding --user in each ovs/ovn init scripts the user could
be derived from the /etc/default/openvswitch file. We don't seem to use
this file on RHEL, but we do have it on Debian.
3. RHEL and Fedora are distributions that on upgrade require admin to
explicitly stop and start Openv vSwitch service. So it seems that natural
place where this user-switch should happen is between service openvswitch
stop and service openvswitch start commands that admin has to invoke.
Otherwise, If we don't follow this approach then there could be corner case
where ovs running as root reverts back some changes made by packaging hooks.
4. Admin could also specify arbitrary user if he does not want to use
"openvswitch".
However, the downsides of this approach are:
1. that Fedora spec file would have to become user agnostic (ie %files
section should have "-" everywhere and rpm -V would not be that useful for
audits).
2. since admin would have to invoke "ovs-ctl migrate-to-user <user>" before
"service openvswitch start", then there could be two openvswitch
environments out there - one where ovs runs under "root" and one where ovs
runs under "openvswitch" user. Though we could always automate this from
the service startup scripts.
I am still thinking about this approach, but I was curious if you see a
reason why this would or would not work?
> > Signed-off-by: Andy Zhou <az...@ovn.org>
> > Acked-by: Ben Pfaff <b...@ovn.org>
> > ---
> > rhel/openvswitch-fedora.spec.in | 18
> ++++++++----------
> > ...sr_lib_systemd_system_openvswitch-nonetwork.service | 4 ++--
> > .../usr_lib_systemd_system_ovn-controller-vtep.service | 2 +-
> > rhel/usr_lib_systemd_system_ovn-controller.service | 2 +-
> > rhel/usr_lib_systemd_system_ovn-northd.service | 2 +-
> > 5 files changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/
> openvswitch-fedora.spec.in
> > index be22e87..b91f1b3 100644
> > --- a/rhel/openvswitch-fedora.spec.in
> > +++ b/rhel/openvswitch-fedora.spec.in
> > @@ -13,10 +13,6 @@
> >
> > #%define kernel 2.6.40.4-5.fc15.x86_64
> >
> > -# If libcap-ng isn't available and there is no need for running OVS
> > -# as regular user, specify the '--without libcapng'
> > -%bcond_without libcapng
> > -
>
> People building small build roots with openvswitch could use that option,
> but I don't know for sure if anyone is actually doing that. OK, let's
> remove that and if anyone complains we can easily bring it back.
>
>
> > # Enable PIE, bz#955181
> > %global _hardened_build 1
> >
> > @@ -46,9 +42,7 @@ BuildRequires: desktop-file-utils
> > BuildRequires: groff graphviz
> > # make check dependencies
> > BuildRequires: procps-ng
> > -%if %{with libcapng}
> > BuildRequires: libcap-ng libcap-ng-devel
> > -%endif
> >
> > Requires: openssl iproute module-init-tools
> > #Upstream kernel commit 4f647e0a3c37b8d5086214128614a136064110c3
> > @@ -112,11 +106,7 @@ overlays and security groups.
> >
> > %build
> > %configure \
> > -%if %{with libcapng}
> > --enable-libcapng \
> > -%else
> > - --disable-libcapng \
> > -%endif
> > --enable-ssl \
> > --with-pkidir=%{_sharedstatedir}/openvswitch/pki
> >
> > @@ -162,6 +152,11 @@ install -d -m 0755
> $RPM_BUILD_ROOT/%{_sharedstatedir}/openvswitch
> > touch $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch/conf.db
> > touch $RPM_BUILD_ROOT%{_sysconfdir}/openvswitch/system-id.conf
> >
> > +%pre
> > +# Add the "ovs" user and group
> > +/usr/sbin/useradd -c "Openvswitch Daemons" -s /sbin/nologin -r \
> > + -d %{_rundir}/openvswitch ovs 2> /dev/null || :
> > +
>
> I suggest to rename the user and group to 'openvswitch'.
>
Ok. I renamed user and group to "openvswitch". Also for Debian.
>
> Redirecting all errors to /dev/null can hide tricky bugs during
> installations. I suggest to do something like:
>
> if ! getent passwd openvswitch >/dev/null; then
> useradd -c "Openvswitch Daemons" -s /sbin/nologin -r \
> -d %{_rundir}/openvswitch openvswitch
> fi
> exit 0
>
Ok
>
>
>
> > %check
> > %if %{with check}
> > if make check TESTSUITEFLAGS='%{_smp_mflags}' ||
> > @@ -204,6 +199,8 @@ rm -rf $RPM_BUILD_ROOT
> > %endif
> >
> > %post
> > +chown -R ovs:ovs /etc/openvswitch #OVS DB files
> > +chown -R ovs:ovs %{_rundir}/openvswitch
>
> This breaks rpm -V. You need to change file permissions in the
> %file sections otherwise the filesystem and rpmdb won't match.
>
I think this "chown <rundir> " invocation should be moved out from
packaging hooks because it might not exist in the first place on fresh
installs (another option would be to suppress errors).
> Also, %{_rundir}/openvswitch is marked as %ghost which means that
> directory isn't packaged. It is created by systemd when the service
> is initializing (RuntimeDirectory). Here we have a problem because
> systemd will set rundir ownership to User= and Group= specified in
> the service (which we don't specify, so root:root is assumed) and we
> can't package the directory because /run is a tmpfs.
>
> Since you fix the %{_rundir}/openvswitch in the script ovs-lib, it seems
> enough to just patch the line below:
>
> - %ghost %attr(755,root,root) %{_rundir}/openvswitch
> + %ghost %attr(755,openvswitch,openvswitch) %{_rundir}/openvswitch
To me it seems that rpm -V does not complain about %ghost directories.
[root@localhost x86_64]# chown root:root /var/run/openvswitch/
[root@localhost x86_64]# chown root:root /run/openvswitch/
[root@localhost x86_64]# chown root:root /etc/openvswitch/
[root@localhost x86_64]# rpm -V openvswitch-2.5.90-1.fc23.x86_64
.....UG.. /etc/openvswitch
[root@localhost x86_64]# chown openvswitch:openvswitch /etc/openvswitch/
[root@localhost x86_64]# chown openvswitch:openvswitch /run/openvswitch
[root@localhost x86_64]# chown openvswitch:openvswitch /var/run/openvswitch
[root@localhost x86_64]# rpm -V openvswitch-2.5.90-1.fc23.x86_64
[root@localhost x86_64]#
Just curious if I am doing something wrong or if this is an issue with
"rpm"?
> See:
> http://www.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html
>
>
> > %if 0%{?systemd_post:1}
> > %systemd_post %{name}.service
> > %else
> > @@ -214,6 +211,7 @@ rm -rf $RPM_BUILD_ROOT
> > %endif
> >
> > %post ovn
> > +chown -R ovs:ovs /var/lib/openvswitch #OVN DB files
>
> breaks rpmdb too.
>
True, fixed it.
>
>
> > %if 0%{?systemd_post:1}
> > %systemd_post ovn-controller.service
> > %systemd_post ovn-controller-vtep.service
> > diff --git a/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
> b/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
> > index e4c2a66..f32ba24 100644
> > --- a/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
> > +++ b/rhel/usr_lib_systemd_system_openvswitch-nonetwork.service
> > @@ -9,7 +9,7 @@ Type=oneshot
> > RemainAfterExit=yes
> > EnvironmentFile=-/etc/sysconfig/openvswitch
> > ExecStart=/usr/share/openvswitch/scripts/ovs-ctl start \
> > - --system-id=random $OPTIONS
> > + --system-id=random --user=ovs:ovs $OPTIONS
> > ExecStop=/usr/share/openvswitch/scripts/ovs-ctl stop
> > RuntimeDirectory=openvswitch
> > -RuntimeDirectoryMode=0755
> > +RuntimeDirectoryMode=0775
>
> You need to sync this with the %attr above and the
> ovs-lib (proposed 755)
>
ok
>
> Thanks,
> fbl
>
> > diff --git a/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
> b/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
> > index 867a906..994bd77 100644
> > --- a/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
> > +++ b/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
> > @@ -27,4 +27,4 @@ Environment=VTEP_DB=unix:%t/openvswitch/db.sock
> > ExecStart=/usr/bin/ovn-controller-vtep -vconsole:emer -vsyslog:err
> -vfile:info \
> > --log-file=/var/log/openvswitch/ovn-controller-vtep.log \
> > --no-chdir --pidfile=${OVS_RUNDIR}/ovn-controller-vtep.pid \
> > - --ovnsb-db=${OVN_DB} --vtep-db=${VTEP_DB}
> > + --user ovs:ovs --ovnsb-db=${OVN_DB} --vtep-db=${VTEP_DB}
> > diff --git a/rhel/usr_lib_systemd_system_ovn-controller.service
> b/rhel/usr_lib_systemd_system_ovn-controller.service
> > index 6b53ced..b01a804 100644
> > --- a/rhel/usr_lib_systemd_system_ovn-controller.service
> > +++ b/rhel/usr_lib_systemd_system_ovn-controller.service
> > @@ -18,5 +18,5 @@ Type=simple
> > Environment=OVS_RUNDIR=%t/openvswitch
> > Environment=OVS_DB=unix:%t/openvswitch/db.sock
> > ExecStart=/usr/bin/ovn-controller -vconsole:emer -vsyslog:err
> -vfile:info \
> > - --log-file=/var/log/openvswitch/ovn-controller.log \
> > + --log-file=/var/log/openvswitch/ovn-controller.log --user
> ovs:ovs \
> > --no-chdir --pidfile=${OVS_RUNDIR}/ovn-controller.pid
> ${OVS_DB}
> > diff --git a/rhel/usr_lib_systemd_system_ovn-northd.service
> b/rhel/usr_lib_systemd_system_ovn-northd.service
> > index 5b3b03a..1abb8b3 100644
> > --- a/rhel/usr_lib_systemd_system_ovn-northd.service
> > +++ b/rhel/usr_lib_systemd_system_ovn-northd.service
> > @@ -8,5 +8,5 @@ After=openvswitch.service
> > Type=oneshot
> > RemainAfterExit=yes
> > Environment=OVS_RUNDIR=%t/openvswitch OVS_DBDIR=/var/lib/openvswitch
> > -ExecStart=/usr/share/openvswitch/scripts/ovn-ctl start_northd
> > +ExecStart=/usr/share/openvswitch/scripts/ovn-ctl --user=ovs:ovs
> start_northd
> > ExecStop=/usr/share/openvswitch/scripts/ovn-ctl stop_northd
> > --
> > 1.8.3.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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
