-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix logrotate user in UBI images #6052
Conversation
@@ -32,7 +32,7 @@ RUN cd /tmp/openvswitch* && \ | |||
sed -e "s/@VERSION@/$OVS_VERSION/" rhel/openvswitch-fedora.spec.in > /tmp/ovs.spec && \ | |||
yum-builddep -y /tmp/ovs.spec && ./boot.sh && \ | |||
./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc && \ | |||
make rpm-fedora && mkdir -p /tmp/ovs-rpms && \ | |||
RPMBUILD_OPT="--without libcapng" make rpm-fedora && mkdir -p /tmp/ovs-rpms && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a public reference that documents how it fixes the issue? And better to add a comment as it can't be easily seen from the code itself.
Ideally the comment should also the other impacts of disabling libcapng if there are any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is described in https://github.com/openvswitch/ovs/blob/v2.17.9/rhel/openvswitch-fedora.spec.in#L26.
If libcap-ng isn't available and there is no need for running OVS as regular user, specify the '--without libcapng'
There should be no side effects as this option only skips adding user/group and changing folder owners for openvswitch.
Not linking with libcapng will cause the OVS daemon to fail when using the --user option, as explained in openvswitch/ovs@2ff63ae. If this is not ideal, we might need to sed the logrotate config afterwards to use the root user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't set --user
option when running OVS, right? If so, I think we could just set --without libcapng
. We should add comment for reference in the future, including the link, the purpose, and the side effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did not use --user
option ( --ovs-user
option of ovs-ctl in start_ovs
) in Antrea.
I will add a comment for this change.
faf09e5
to
2a2ff09
Compare
build/images/ovs/Dockerfile.ubi
Outdated
# Antrea will run OVS and logrotate as the root user. Disabling libcapng helps | ||
# to skip configuring the user and group for OVS and logrotate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change, OVS runs as openvswitch, which user logrotate uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change, OVS runs as root but logrorate will su to openvswitch when performing log rotation. We did not use the --ovs-user
option to specify the OVS user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand why the issue doesn't exist in ubuntu image, where I see OVS also runs as root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For OVS deb packages the logrotate configuration does not switch users before rotation by default.
https://github.com/openvswitch/ovs/blob/v2.17.9/debian/openvswitch-switch.logrotate#L2
But the configuration of rpm packages will switch to non-root users if libcap is linked.
The line https://github.com/openvswitch/ovs/blob/v2.17.9/rhel/etc_logrotate.d_openvswitch#L9 will be changed in https://github.com/openvswitch/ovs/blob/v2.17.9/rhel/openvswitch-fedora.spec.in#L325
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I feel we could rephrase it a little:
logrotate needs to run as the same user as OVS to get the proper permissions of log files.
As Antrea runs OVS as root, we disable libcapng to make logrotate also run as root.
See https://github.com/openvswitch/ovs/blob/v2.17.7/rhel/openvswitch-fedora.spec.in#L26-L27.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. Thanks!
2a2ff09
to
d388dfe
Compare
logrotate needs to run as the same user as OVS to get the proper permissions of log files. As Antrea runs OVS as root, we disable libcapng to make logrotate also run as root. Fixes: antrea-io#6046 Signed-off-by: Xu Liu <xliu2@vmware.com>
d388dfe
to
3927cb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's also check if the solution makes sense to @antoninbas
/skip-all |
@xliuxu could you backport it to 1.13-1.15? |
logrotate needs to run as the same user as OVS to get the proper permissions for log files. As Antrea runs OVS as root, we disable libcapng to make logrotate also run as root. Fixes: antrea-io#6046 Signed-off-by: Xu Liu <xliu2@vmware.com>
logrotate needs to run as the same user as OVS to get the proper permissions for log files. As Antrea runs OVS as root, we disable libcapng to make logrotate also run as root. Fixes: antrea-io#6046 Signed-off-by: Xu Liu <xliu2@vmware.com>
logrotate needs to run as the same user as OVS to get the proper permissions for log files. As Antrea runs OVS as root, we disable libcapng to make logrotate also run as root. Fixes: antrea-io#6046 Signed-off-by: Xu Liu <xliu2@vmware.com>
Done. Thanks! |
This change adds back the default RPMBUILD_OPT option to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in antrea-io#6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
This change adds back the default `RPMBUILD_OPT` to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in antrea-io#6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
This change adds back the default `RPMBUILD_OPT` to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in antrea-io#6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
This change adds back the default `RPMBUILD_OPT` to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in #6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
logrotate needs to run as the same user as OVS to get the proper permissions for log files. As Antrea runs OVS as root, we disable libcapng to make logrotate also run as root. Fixes: antrea-io#6046 Signed-off-by: Xu Liu <xliu2@vmware.com>
This change adds back the default `RPMBUILD_OPT` to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in antrea-io#6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
This change adds back the default `RPMBUILD_OPT` to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in antrea-io#6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
This change adds back the default `RPMBUILD_OPT` to disable unnecessary tests when building OVS rpm packages, which was previously overwritten in #6052. Signed-off-by: Xu Liu <xu.liu@broadcom.com>
Logrotate will run as the user openvswitch, but we start the OVS daemon as the root user. We can disable this behavior by specifying
--without libcapng
in RPM builds.Fixes: #6046