Skip to content
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

Restore selinux context systemd unit file #8593

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

galal-hussein
Copy link
Contributor

Proposed Changes

Fix to restore the context of systemd unit file of k3s and its env file to the correct context container_unit_file_t

Types of Changes

Bug fix

Verification

Testing

Linked Issues

User-Facing Change


Further Comments

@galal-hussein galal-hussein requested a review from a team as a code owner October 11, 2023 22:12
@galal-hussein galal-hussein changed the title Fix systemd unit file Restore selinux context systemd unit file Oct 11, 2023
@@ -961,11 +961,16 @@ EOF

# --- write systemd or openrc service file ---
create_service_file() {
[ "${HAS_SYSTEMD}" = true ] && create_systemd_service_file
[ "${HAS_SYSTEMD}" = true ] && create_systemd_service_file && restore_systemd_service_file_context
Copy link
Member

@brandond brandond Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this perhaps fit better with the rest of the context changes? It doesn't feel right just blindly calling restorecon when creating the systemd unit.

Perhaps fit it in somewhere down here:

k3s/install.sh

Lines 560 to 569 in 7d38b4a

if ! $SUDO chcon -u system_u -r object_r -t container_runtime_exec_t ${BIN_DIR}/k3s >/dev/null 2>&1; then
if $SUDO grep '^\s*SELINUX=enforcing' /etc/selinux/config >/dev/null 2>&1; then
$policy_error "Failed to apply container_runtime_exec_t to ${BIN_DIR}/k3s, ${policy_hint}"
fi
elif [ ! -f /usr/share/selinux/packages/k3s.pp ]; then
if [ -x /usr/sbin/transactional-update ] || [ "${ID_LIKE:-}" = coreos ] || [ "${VARIANT_ID:-}" = coreos ]; then
warn "Please reboot your machine to activate the changes and avoid data loss."
else
$policy_error "Failed to find the k3s-selinux policy, ${policy_hint}"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately the systemd unit file deployment happens after this section in the create_service_file function, and a chicken-egg problem happens because the context is applied before the systemd unit file is deployed

setup_selinux
...    
create_env_file
create_service_file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that we're going to get a request to disable this behavior. Whenever we add something to the install script, someone always asks for an option to not do it.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7d38b4a) 47.05% compared to head (2707119) 49.74%.
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8593      +/-   ##
==========================================
+ Coverage   47.05%   49.74%   +2.68%     
==========================================
  Files         144      148       +4     
  Lines       14964    15919     +955     
==========================================
+ Hits         7042     7919     +877     
+ Misses       6809     6713      -96     
- Partials     1113     1287     +174     
Flag Coverage Δ
e2etests 46.72% <ø> (?)
inttests 42.87% <ø> (-1.56%) ⬇️
unittests 17.93% <ø> (-1.62%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 62 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@galal-hussein galal-hussein merged commit 112e133 into k3s-io:master Oct 31, 2023
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants