chore(cni-plugin): make script stylistically more consistent

[sdickhoven]

this pr makes no functional changes but makes the cni-plugin/deployment/scripts/install-cni.sh script stylistically more consistent.

feel free to do with this pr what you will.

my goal was simply to do a bit of cleanup to make it more "enjoyable" to work on this script. 🧹

but feel free to close this pr if you don't mind the inconsistencies.

[sdickhoven]

"dangerous" not to quote command arguments. what if $SERVICEACCOUNT_PATH has a space char in it?

[sdickhoven]

"dangerous" not to quote command arguments. what if $TMP_CONF has a space char in it?

[sdickhoven]

...and, yes, bash understands how to correctly interpret "$(... "...")"

[sdickhoven]

i'm surprised shellcheck doesn't complain about this. it's a common rookie mistake. 🙂

$ file="a b"
$ touch "$file"
$ ls "$file"
a b
$ ls $file
ls: a: No such file or directory
ls: b: No such file or directory

[sdickhoven]

combine the two ifs into one.

[cratelyn]

i left a few questions below, noting that there are some semantic changes in this proposal.

i'm not sure about the utility of removing the braces from parameter expansion, personally. do we stand to benefit at all by using $FOO rather than ${FOO}?

i'll defer to @alpeb's judgement about that, but my inclination is that refraining from changing that would make the other more interesting changes related to e.g. conditional branches more reviewable.

[cratelyn]

may i ask, why are we opting to remove the surrounding braces throughout this script?

[sdickhoven]

yes. good question. i'm just going for consistency.

"$foo" == ${foo}

i'm happy going with either but the script is using both. i'm just trying to add consistency.

this is entirely a stylistic nit. 100% non-functional.

the braces are typically used for two reasons:

1. to do string manipulation - e.g. ${foo:-default_value} or ${foo//a/b}
2. to remove ambiguity - e.g. $foobar != ${foo}bar

but there's nothing wrong with always using braces.

however, the script currently uses them sometimes (when they are not actually required for any functional reasons) and sometimes not. that's all.

[cratelyn]

got it. my personal vote would be to use braces, rather than remove them. as you mention, these are often used to remove ambiguity or to otherwise avoid accidentally expanding a different variable than intended.

i'll cede to @alpeb's opinion on that, however.

[sdickhoven]

happy to update the pr to switch all variables to using braces.

i have no strong preference one way or the other. i just like consistency. that's all.

...but i realize that most people probably couldn't care less about the consistent use of braces here. 🙂

rest assured that it won't hurt my feelings if y'all decide to simply close this pr. 😌

...though i do think that we should (double)quote all variables that are used as command line arguments:

• https://github.com/linkerd/linkerd2-proxy-init/blob/97c421f/cni-plugin/deployment/scripts/install-cni.sh#L131
• https://github.com/linkerd/linkerd2-proxy-init/blob/97c421f/cni-plugin/deployment/scripts/install-cni.sh#L195
• https://github.com/linkerd/linkerd2-proxy-init/blob/97c421f/cni-plugin/deployment/scripts/install-cni.sh#L197

[sdickhoven]

my personal vote would be to use braces, rather than remove them.

i have just pushed a commit that flips all variables to using braces.

[cratelyn]

can you explain what this is doing? this seems like a change to the logic of this script, versus the cosmetic/stylistic changes happening elsewhere in this diff.

[sdickhoven]

this is a simplification.

right now, the script checks if there are any files in /host/etc/cni/net.d and if not it prints "No active CNI configuration files found":

config_files=$(find "${HOST_CNI_NET}" -maxdepth 1 -type f \( -iname '*conflist' -o -iname '*conf' \))
if [ -z "$config_files" ]; then
  log "No active CNI configuration files found"
else

in the else case, the script then removes any files from the $config_files variable that have the (sub)string linkerd in them and counts how many files are left (the sort is completely nonsensical here!).

and if there are no files left (i.e. wc -l == number of lines == 0) then it again prints "No active CNI configuration files found":

  config_file_count=$(echo "$config_files" | grep -v linkerd | sort | wc -l)
  if [ "$config_file_count" -eq 0 ]; then
    log "No active CNI configuration files found"
  else

here are a couple of examples:

---

/host/etc/cni/net.d has no files:

[ -z "" ] # true

-> first if statement is true .: "No active CNI configuration files found"

---

/host/etc/cni/net.d has the following file:

• something-linkerd-something

[ -z "something-linkerd-something" ] # false
echo "something-linkerd-something" | grep -v linkerd | wc -l # 0
[ "0" -eq 0 ] # true

-> first if statement is false, second if statement is true .: "No active CNI configuration files found"

---

/host/etc/cni/net.d has the following files:

• something-linkerd-something
• something-else

[ -z "something-linkerd-something
something-else" ] # false
echo "something-linkerd-something
something-else" | grep -v linkerd | wc -l # 1
[ "1" -eq 0 ] # false

-> find files and patch them

---

anyway, my logic just finds all files in /host/etc/cni/net.d and immediately removes any files that have the (sub)string linkerd in them and then checks whether any files are left:

config_files=$(find "$HOST_CNI_NET" -maxdepth 1 -type f \( -iname '*conflist' -o -iname '*conf' \) | grep -v linkerd || true)
if [ -z "$config_files" ]; then
...

this is much more straight forward and easier to understand:

1. find all files that don't have the (sub)string linkerd in them
2. if what was found is an empty string ... else ...

you may be wondering about the || true at the end.

this is due to the fact that the script configures bash with set -e (== exit immediately when a command exits with a non-zero exit status) and that grep foo or grep -v foo will exit with an exit status of 1 (which would then cause the entire script to exit) when no output is produced.

$ grep foo <<< foobarbaz
foobarbaz
$ echo $?
0

$ grep foo <<< noobarbaz
$ echo $?
1

$ grep -v foo <<< foobarbaz
$ echo $?
1

$ grep -v foo <<< noobarbaz
noobarbaz
$ echo $?
0

the || construct simply means "if exit status is non-zero do this instead" and the command true simply does nothing and exits with a zero exit status.

$ true
$ echo $?
0

$ false
$ echo $?
1

so the || true simply means: if the exit status is non-zero (which is entirely expected and totally fine), change it to zero so that the script doesn't exit (due to set -e).

by the way, the same thing can be accomplished with

config_files=$(find "$HOST_CNI_NET" -maxdepth 1 -type f ! -name '*linkerd*' \( -iname '*conflist' -o -iname '*conf' \))

actually... i like that better. i'll update... because then we don't need that somewhat mysterious || true.

[sdickhoven]

ok. so now we are trading

  config_file_count=$(echo "$config_files" | grep -v linkerd | sort | wc -l)
  if [ "$config_file_count" -eq 0 ]; then
    log "No active CNI configuration files found"
  else

for

! -name '*linkerd*' in the find command.

to get the same exact behavior.

by the way, it's a bit incongruent that files with linkerd in them are not excluded from the subsequent find that drives the actual patching.

i would personally get rid of that second find as well and just use $config_files but that would actually be a slightly different behavior and this pr is all about not making any functional changes.

[alpeb]

Would you please sync again with main so your fix is picked up?

[alpeb]

Thanks @sdickhoven for the polishments and @cratelyn for helping getting this through 👍
On its final state this looks good to me 👍