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
Bug 1956836: templates: Rework node-valid-hostname to fix SELinux denial #2618
Bug 1956836: templates: Rework node-valid-hostname to fix SELinux denial #2618
Conversation
@cgwalters: This pull request references Bugzilla bug 1956836, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@cgwalters: This pull request references Bugzilla bug 1956836, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold (I didn't really test this locally) |
4eacdf3
to
ad76e35
Compare
/cc @darkmuggle can you please review this. |
OK so the new code still works: AWS logs look good: But unfortunately because each |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1956836 Unfortunately right now, executing `bash` skips a domain transition (see fedora-selinux/selinux-policy#778) so the way we're sourcing the script means we stay in `initrc_t` and end up triggering a SELinux policy denial. (BTW this denial turns out to just delay the successful exit of the script, which will then end up just delaying kubelet start. it's otherwise harmless, but we also don't want SELinux policy denials in our product by default) Fix this in two ways: - First, just move the thing to `/usr/local/bin` to avoid issues with labeling of `/usr/local/sbin` that were fixed in openshift/os#551 - Second, rework it to be executed directly While we're here: - Clean the confusing+outdated comment about being a NM dispatcher - Drop the `logger` bit which was only necessary as a NM dispatcher; since we're *always* running under systemd, this makes `journalctl -u node-valid-hostname` actually show the script output. - Make it crystal clear that the "truncate hostname" is only run in GCP. - Fix various typos - Use the more precise term "non-localhost" in various places instead of the more ambiguous terms "real"/"valid"
ad76e35
to
783aa2c
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
The fix-ups are nice -- especially dropping things that are not longer needed after the hack of dispatcher scripts were dropped.
/hold cancel |
@sinnykumari this got a lgtm, mind adding an |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, darkmuggle, sinnykumari The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@cgwalters: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@cgwalters: All pull requests linked via external trackers have merged: Bugzilla bug 1956836 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1956836
Unfortunately right now, executing
bash
skips a domain transition(see fedora-selinux/selinux-policy#778)
so the way we're sourcing the script means we stay in
initrc_t
and end up triggering a SELinux policy denial.
(BTW this denial turns out to just delay the successful exit
of the script, which will then end up just delaying kubelet start.
it's otherwise harmless, but we also don't want SELinux policy denials
in our product by default)
Fix this in two ways:
/usr/local/bin
to avoid issueswith labeling of
/usr/local/sbin
that were fixed in BZ 1956836: overlay: Add rhcos-usrlocal-selinux-fixup.service os#551While we're here:
logger
bit which was only necessary as a NM dispatcher;since we're always running under systemd, this makes
journalctl -u node-valid-hostname
actually show the script output.
terms "real"/"valid"