-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Changed command -v redirection for iptables bin check #7315
Conversation
install.sh
Outdated
then | ||
$SUDO iptables-save | grep -v KUBE- | grep -v CNI- | grep -iv flannel | $SUDO iptables-restore | ||
fi | ||
if command -v ip6tables-save &> /dev/null && command -v ip6tables-restore &> /dev/null | ||
if command -v ip6tables-save && command -v ip6tables-restore |
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.
Won't this cause the path to the various commands to be printed to the terminal whenever the install script is run? That's bad UX. I'd prefer to fix the shell syntax so that the redirection works properly.
[INFO] Finding release for channel stable
[INFO] Using v1.26.3+k3s1 as release
[INFO] Downloading hash https://github.com/k3s-io/k3s/releases/download/v1.26.3+k3s1/sha256sum-amd64.txt
[INFO] Downloading binary https://github.com/k3s-io/k3s/releases/download/v1.26.3+k3s1/k3s
[INFO] Verifying binary download
[INFO] Installing k3s to /usr/local/bin/k3s
[INFO] Skipping installation of SELinux RPM
[INFO] Creating /usr/local/bin/kubectl symlink to k3s
[INFO] Creating /usr/local/bin/crictl symlink to k3s
[INFO] Creating /usr/local/bin/ctr symlink to k3s
[INFO] Creating killall script /usr/local/bin/k3s-killall.sh
[INFO] Creating uninstall script /usr/local/bin/k3s-uninstall.sh
[INFO] env: Creating environment file /etc/systemd/system/k3s.service.env
[INFO] systemd: Creating service file /etc/systemd/system/k3s.service
[INFO] systemd: Enabling k3s unit
Created symlink /etc/systemd/system/multi-user.target.wants/k3s.service → /etc/systemd/system/k3s.service.
/usr/sbin/iptables-save
/usr/sbin/iptables-restore
/usr/sbin/ip6tables-save
/usr/sbin/ip6tables-restore
[INFO] systemd: Starting k3s
I'm honestly not convinced that the user doesn't have something wrong with their environment, I don't see why what we have now shouldn't work. It looked like underlying problem was the sudo shell couldn't find the iptables binaries, while the user's shell could.
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.
I think it's related only to ubuntu. With &>
redirect on other OS the output is rightly redirected but on Ubuntu it's not and the if condition is always true; I tried with 1>
and it seems to work also on ubuntu.
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.
Apparently use of &
to redirect both stderr and stdout is a bashism that I wasn't aware of. Posix shell syntax only allows redirection of specific numbered fds.
ubuntu@ip-172-31-5-176:~$ ls -la /usr/bin/sh
lrwxrwxrwx 1 root root 4 Mar 23 2022 /usr/bin/sh -> dash
ubuntu@ip-172-31-5-176:~$ bash -c 'command -v iptables-save &> /dev/null && echo foo'
ubuntu@ip-172-31-5-176:~$ sh -c 'command -v iptables-save &> /dev/null && echo foo'
foo
ubuntu@ip-172-31-5-176:~$ sh -c 'command -v iptables-save 1> /dev/null && echo foo'
ubuntu@ip-172-31-5-176:~$
On posix shells it's treated as
command -v iptables-save &
> /dev/null && echo foo
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.
My bad. I always tests shell script with bash.
Signed-off-by: Roberto Bonafiglia <roberto.bonafiglia@suse.com>
7225a06
to
eda0a72
Compare
We should probably add a posix compatibility shellcheck on this script as part of PR CI. |
May I merge this or is it better to wait the other PRs for the release? |
We're still in code freeze on master for the 1.27.1 release. In general we don't merge anything after the rc has been cut, unless it's to fix a bug that is going to require another rc. @dereknola is the release captain though, it's up to him. |
I'm fine with merging this |
Proposed Changes
Removed
command -v
redirection that wasn't working on ubuntu and always execute the content of the if even when it fails.Types of Changes
Verification
Testing
Linked Issues
#7291
User-Facing Change
Further Comments