-
Notifications
You must be signed in to change notification settings - Fork 37
chore(cni-plugin): make script stylistically more consistent #528
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
Changes from 52 commits
daf876e
e391969
70fa5ae
f19925b
967e463
f35a71c
0d24d29
f26afb3
af1282f
d0e67bb
3ce9c37
ca8381f
a9dbdda
28e335e
ee6ec4a
52daf12
555336d
6fbc0a9
798aab6
5f984e0
a97c205
f94d0e3
59b7fd8
1867a94
e61fd73
82b0910
fe8a1e4
2ec5177
7574345
49cd49a
8de3761
7aa3f83
8437fc2
c9e3218
cd9a99c
ca37702
6e200be
9119c32
d9a7077
10845da
36f44fd
a5af4b3
2615f0b
6908957
00db641
63ea7dc
6c19648
e5cba87
834ed72
0c4bb3a
6b3b0f3
3017863
97c421f
218a121
e43a87d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,18 +20,19 @@ | |
| # 2) https://github.com/istio/cni/blob/c63a509539b5ed165a6617548c31b686f13c2133/deployments/kubernetes/install/scripts/install-cni.sh | ||
|
|
||
| # Script to install Linkerd CNI on a Kubernetes host. | ||
| # - Expects the host CNI binary path to be mounted at /host/opt/cni/bin. | ||
| # - Expects the host CNI network config path to be mounted at /host/etc/cni/net.d. | ||
| # - Expects the desired CNI config in the CNI_NETWORK_CONFIG env variable. | ||
| # - Expects the host CNI binary path to be mounted at /host/opt/cni/bin | ||
| # - Expects the host CNI network config path to be mounted at /host/etc/cni/net.d | ||
| # - Expects the desired CNI config in the CNI_NETWORK_CONFIG env variable | ||
|
|
||
| # Ensure all variables are defined, and that the script fails when an error is hit. | ||
| set -u -e -o pipefail | ||
| # Ensure all variables are defined, and that the script fails when an error is | ||
| # hit. | ||
| set -u -e -o pipefail +o noclobber | ||
|
|
||
| # Helper function for raising errors | ||
| # Usage: | ||
| # some_command || exit_with_error "some_command_failed: maybe try..." | ||
| exit_with_error() { | ||
| log "${1}" | ||
| log "$1" | ||
| exit 1 | ||
| } | ||
|
|
||
|
|
@@ -54,7 +55,7 @@ CONTAINER_CNI_BIN_DIR=${CONTAINER_CNI_BIN_DIR:-/opt/cni/bin} | |
| # Directory path where CNI configuration should live on the host | ||
| HOST_CNI_NET="${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}" | ||
| # Location of legacy "interface mode" file, to be automatically deleted | ||
| DEFAULT_CNI_CONF_PATH="${HOST_CNI_NET}/01-linkerd-cni.conf" | ||
| DEFAULT_CNI_CONF_PATH="$HOST_CNI_NET/01-linkerd-cni.conf" | ||
| KUBECONFIG_FILE_NAME=${KUBECONFIG_FILE_NAME:-ZZZ-linkerd-cni-kubeconfig} | ||
| SERVICEACCOUNT_PATH=/var/run/secrets/kubernetes.io/serviceaccount | ||
|
|
||
|
|
@@ -66,7 +67,8 @@ SERVICEACCOUNT_PATH=/var/run/secrets/kubernetes.io/serviceaccount | |
| # *conflist files, then linkerd-cni configuration parameters will be removed | ||
| # from them. | ||
| cleanup() { | ||
| # First, kill both 'inotifywait' processes so we don't process any DELETE/CREATE events | ||
| # First, kill both 'inotifywait' processes so we don't process any | ||
| # DELETE/CREATE events. | ||
| pids=$(pgrep inotifywait) | ||
| if [ -n "$pids" ]; then | ||
| while read -r pid; do | ||
|
|
@@ -80,8 +82,8 @@ cleanup() { | |
| # Find all conflist files and print them out using a NULL separator instead of | ||
| # writing each file in a new line. We will subsequently read each string and | ||
| # attempt to rm linkerd config from it using jq helper. | ||
| local cni_data='' | ||
| find "${HOST_CNI_NET}" -maxdepth 1 -type f \( -iname '*conflist' \) -print0 | | ||
| local cni_data | ||
| find "$HOST_CNI_NET" -maxdepth 1 -type f \( -iname '*conflist' \) -print0 | | ||
cratelyn marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| while read -r -d $'\0' file; do | ||
| log "Removing linkerd-cni config from $file" | ||
| cni_data=$(jq 'del( .plugins[]? | select( .type == "linkerd-cni" ))' "$file") | ||
|
|
@@ -91,11 +93,11 @@ cleanup() { | |
| done | ||
|
|
||
| # Remove binary and kubeconfig file | ||
| if [ -e "${HOST_CNI_NET}/${KUBECONFIG_FILE_NAME}" ]; then | ||
| log "Removing linkerd-cni kubeconfig: ${HOST_CNI_NET}/${KUBECONFIG_FILE_NAME}" | ||
| rm -f "${HOST_CNI_NET}/${KUBECONFIG_FILE_NAME}" | ||
| if [ -e "$HOST_CNI_NET/$KUBECONFIG_FILE_NAME" ]; then | ||
| log "Removing linkerd-cni kubeconfig: $HOST_CNI_NET/$KUBECONFIG_FILE_NAME" | ||
| rm -f "$HOST_CNI_NET/$KUBECONFIG_FILE_NAME" | ||
| fi | ||
| if [ -e "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_BIN_DIR}"/linkerd-cni ]; then | ||
| if [ -e "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_BIN_DIR}/linkerd-cni" ]; then | ||
| log "Removing linkerd-cni binary: ${CONTAINER_MOUNT_PREFIX}${DEST_CNI_BIN_DIR}/linkerd-cni" | ||
| rm -f "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_BIN_DIR}/linkerd-cni" | ||
| fi | ||
|
|
@@ -113,54 +115,54 @@ trap 'log "ERROR caught, exiting..."; cleanup ' ERR | |
| install_cni_bin() { | ||
| # Place the new binaries if the mounted directory is writeable. | ||
| dir="${CONTAINER_MOUNT_PREFIX}${DEST_CNI_BIN_DIR}" | ||
| if [ ! -w "${dir}" ]; then | ||
| exit_with_error "${dir} is non-writeable, failure" | ||
| if [ ! -w "$dir" ]; then | ||
| exit_with_error "$dir is non-writeable, failure" | ||
| fi | ||
| for path in "${CONTAINER_CNI_BIN_DIR}"/*; do | ||
| cp "${path}" "${dir}"/ || exit_with_error "Failed to copy ${path} to ${dir}." | ||
| for path in "$CONTAINER_CNI_BIN_DIR"/*; do | ||
| cp "$path" "$dir/" || exit_with_error "Failed to copy $path to $dir." | ||
| done | ||
|
|
||
| log "Wrote linkerd CNI binaries to ${dir}" | ||
| log "Wrote linkerd CNI binaries to $dir" | ||
| } | ||
|
|
||
| create_kubeconfig() { | ||
| KUBE_CA_FILE=${KUBE_CA_FILE:-${SERVICEACCOUNT_PATH}/ca.crt} | ||
| KUBE_CA_FILE=${KUBE_CA_FILE:-$SERVICEACCOUNT_PATH/ca.crt} | ||
| SKIP_TLS_VERIFY=${SKIP_TLS_VERIFY:-false} | ||
| SERVICEACCOUNT_TOKEN=$(cat ${SERVICEACCOUNT_PATH}/token) | ||
| SERVICEACCOUNT_TOKEN=$(cat "$SERVICEACCOUNT_PATH/token") | ||
|
||
|
|
||
| # Check if we're not running as a k8s pod. | ||
| if [[ ! -f "${SERVICEACCOUNT_PATH}/token" ]]; then | ||
| if [[ ! -f "$SERVICEACCOUNT_PATH/token" ]]; then | ||
| return | ||
| fi | ||
|
|
||
| if [ -z "${KUBERNETES_SERVICE_HOST}" ]; then | ||
| if [ -z "$KUBERNETES_SERVICE_HOST" ]; then | ||
| log 'KUBERNETES_SERVICE_HOST not set'; exit 1; | ||
| fi | ||
| if [ -z "${KUBERNETES_SERVICE_PORT}" ]; then | ||
| if [ -z "$KUBERNETES_SERVICE_PORT" ]; then | ||
| log 'KUBERNETES_SERVICE_PORT not set'; exit 1; | ||
| fi | ||
|
|
||
| if [ "${SKIP_TLS_VERIFY}" = 'true' ]; then | ||
| if [ "$SKIP_TLS_VERIFY" = 'true' ]; then | ||
| TLS_CFG='insecure-skip-tls-verify: true' | ||
| elif [ -f "${KUBE_CA_FILE}" ]; then | ||
| TLS_CFG="certificate-authority-data: $(base64 "${KUBE_CA_FILE}" | tr -d '\n')" | ||
| elif [ -f "$KUBE_CA_FILE" ]; then | ||
| TLS_CFG="certificate-authority-data: $(base64 "$KUBE_CA_FILE" | tr -d '\n')" | ||
| fi | ||
|
|
||
| touch "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}/${KUBECONFIG_FILE_NAME}" | ||
| chmod "${KUBECONFIG_MODE:-600}" "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}/${KUBECONFIG_FILE_NAME}" | ||
| cat > "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}/${KUBECONFIG_FILE_NAME}" <<EOF | ||
| touch "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}/$KUBECONFIG_FILE_NAME" | ||
| chmod "${KUBECONFIG_MODE:-600}" "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}/$KUBECONFIG_FILE_NAME" | ||
| cat > "${CONTAINER_MOUNT_PREFIX}${DEST_CNI_NET_DIR}/$KUBECONFIG_FILE_NAME" <<EOF | ||
| # Kubeconfig file for linkerd CNI plugin. | ||
| apiVersion: v1 | ||
| kind: Config | ||
| clusters: | ||
| - name: local | ||
| cluster: | ||
| server: ${KUBERNETES_SERVICE_PROTOCOL:-https}://[${KUBERNETES_SERVICE_HOST}]:${KUBERNETES_SERVICE_PORT} | ||
| ${TLS_CFG} | ||
| server: ${KUBERNETES_SERVICE_PROTOCOL:-https}://[$KUBERNETES_SERVICE_HOST]:$KUBERNETES_SERVICE_PORT | ||
| $TLS_CFG | ||
| users: | ||
| - name: linkerd-cni | ||
| user: | ||
| token: ${SERVICEACCOUNT_TOKEN} | ||
| token: $SERVICEACCOUNT_TOKEN | ||
| contexts: | ||
| - name: linkerd-cni-context | ||
| context: | ||
|
|
@@ -179,20 +181,20 @@ create_cni_conf() { | |
| CNI_NETWORK_CONFIG="${CNI_NETWORK_CONFIG:-}" | ||
|
|
||
| # If the CNI Network Config has been overwritten, then use template from file | ||
| if [ -e "${CNI_NETWORK_CONFIG_FILE}" ]; then | ||
| log "Using CNI config template from ${CNI_NETWORK_CONFIG_FILE}." | ||
| cp "${CNI_NETWORK_CONFIG_FILE}" "${TMP_CONF}" | ||
| elif [ "${CNI_NETWORK_CONFIG}" ]; then | ||
| if [ -e "$CNI_NETWORK_CONFIG_FILE" ]; then | ||
| log "Using CNI config template from $CNI_NETWORK_CONFIG_FILE." | ||
| cp "$CNI_NETWORK_CONFIG_FILE" "$TMP_CONF" | ||
| elif [ "$CNI_NETWORK_CONFIG" ]; then | ||
| log 'Using CNI config template from CNI_NETWORK_CONFIG environment variable.' | ||
| cat >"${TMP_CONF}" <<EOF | ||
| ${CNI_NETWORK_CONFIG} | ||
| cat <<EOF > "$TMP_CONF" | ||
| $CNI_NETWORK_CONFIG | ||
| EOF | ||
| fi | ||
|
|
||
| # Use alternative command character "~", since these include a "/". | ||
| sed -i s~__KUBECONFIG_FILEPATH__~"${DEST_CNI_NET_DIR}/${KUBECONFIG_FILE_NAME}"~g ${TMP_CONF} | ||
| sed -i s~__KUBECONFIG_FILEPATH__~"$DEST_CNI_NET_DIR/$KUBECONFIG_FILE_NAME"~g "$TMP_CONF" | ||
|
|
||
| log "CNI config: $(cat ${TMP_CONF})" | ||
| log "CNI config: $(cat "$TMP_CONF")" | ||
|
||
| } | ||
|
|
||
| install_cni_conf() { | ||
|
|
@@ -209,15 +211,18 @@ install_cni_conf() { | |
|
|
||
| echo "$conf_data" > "$TMP_CONF" | ||
|
|
||
| # If the old config filename ends with .conf, rename it to .conflist, because it has changed to be a list | ||
| # If the old config filename ends with .conf, rename it to .conflist because | ||
| # it has changed to be a list. | ||
| local filename | ||
| local extension | ||
| filename=${cni_conf_path##*/} | ||
| extension=${filename##*.} | ||
| # When this variable has a file, we must delete it later. | ||
| old_file_path= | ||
| if [ "${filename}" != '01-linkerd-cni.conf' ] && [ "${extension}" = 'conf' ]; then | ||
| old_file_path=${cni_conf_path} | ||
| log "Renaming ${cni_conf_path} extension to .conflist" | ||
| cni_conf_path="${cni_conf_path}list" | ||
| if [ "$filename" != '01-linkerd-cni.conf' ] && [ "$extension" = 'conf' ]; then | ||
| old_file_path=$cni_conf_path | ||
| log "Renaming $cni_conf_path extension to .conflist" | ||
| cni_conf_path=${cni_conf_path}list | ||
| fi | ||
|
|
||
| # Store SHA of each patched file in global `CNI_CONF_SHA` variable. | ||
|
|
@@ -238,18 +243,19 @@ install_cni_conf() { | |
| CNI_CONF_SHA=$(jq -c --arg f "$cni_conf_path" --arg sha "$new_sha" '. * {$f: $sha}' <<< "$CNI_CONF_SHA") | ||
|
|
||
| # Move the temporary CNI config into place. | ||
| mv "${TMP_CONF}" "${cni_conf_path}" || exit_with_error 'Failed to mv files.' | ||
| [ -n "$old_file_path" ] && rm -f "${old_file_path}" && log "Removing unwanted .conf file" | ||
| mv "$TMP_CONF" "$cni_conf_path" || exit_with_error 'Failed to mv files.' | ||
| [ -n "$old_file_path" ] && rm -f "$old_file_path" && log "Removing unwanted .conf file" | ||
|
|
||
| log "Created CNI config ${cni_conf_path}" | ||
| log "Created CNI config $cni_conf_path" | ||
| } | ||
|
|
||
| # Sync() is responsible for reacting to file system changes. It is used in | ||
| # conjunction with inotify events; sync() is called with the event type (which | ||
| # can be either 'CREATE', 'MOVED_TO' or 'MODIFY'), and the name of the file that | ||
| # `sync()` is responsible for reacting to file system changes. It is used in | ||
| # conjunction with inotify events; `sync()` is called with the event type (which | ||
| # can be either 'CREATE', 'MOVED_TO', or 'MODIFY') and the name of the file that | ||
| # has changed. | ||
| # | ||
| # Based on the changed file, sync() might re-install the CNI configuration file. | ||
| # Based on the changed file, `sync()` might re-install the CNI configuration | ||
| # file. | ||
| sync() { | ||
| local ev=$1 | ||
| local file=${2//\/\//\/} # replace "//" with "/" | ||
|
|
@@ -286,7 +292,7 @@ sync() { | |
|
|
||
| # monitor_cni_config starts a watch on the host's CNI config directory | ||
| monitor_cni_config() { | ||
| inotifywait -m "${HOST_CNI_NET}" -e create,moved_to,modify | | ||
| inotifywait -m "$HOST_CNI_NET" -e create,moved_to,modify | | ||
| while read -r directory action filename; do | ||
| sync "$action" "$directory/$filename" | ||
| done | ||
|
|
@@ -302,18 +308,19 @@ monitor_cni_config() { | |
| # Indeed, as per atomic writer's Write function docs, in the final steps the | ||
| # ..data_tmp symlink points to a new timestamped directory containing the new | ||
| # files, which is then atomically renamed to ..data: | ||
| # > 8. A symlink to the new timestamped directory ..data_tmp is created that will | ||
| # > become the new data directory. | ||
| # > 9. The new data directory symlink is renamed to the data directory; rename is atomic. | ||
| # > 8. A symlink to the new timestamped directory ..data_tmp is created that | ||
| # > will become the new data directory. | ||
| # > 9. The new data directory symlink is renamed to the data directory; rename | ||
| # > is atomic. | ||
| # See https://github.com/kubernetes/kubernetes/blob/release-1.32/pkg/volume/util/atomic_writer.go | ||
| monitor_service_account_token() { | ||
| inotifywait -m "${SERVICEACCOUNT_PATH}" -e moved_to | | ||
| while read -r _ _ filename; do | ||
| if [[ "$filename" == "..data" ]]; then | ||
| inotifywait -m "$SERVICEACCOUNT_PATH" -e moved_to | | ||
| while read -r _ _ filename; do | ||
| if [[ "$filename" == "..data" ]]; then | ||
| log "Detected change in service account files; recreating kubeconfig file" | ||
| create_kubeconfig | ||
| fi | ||
| done | ||
| fi | ||
| done | ||
| } | ||
|
|
||
| log() { | ||
|
|
@@ -326,7 +333,7 @@ log() { | |
|
|
||
| # Delete old "interface mode" file, possibly left over from previous versions | ||
| # TODO(alpeb): remove this on stable-2.15 | ||
| rm -f "${DEFAULT_CNI_CONF_PATH}" | ||
| rm -f "$DEFAULT_CNI_CONF_PATH" | ||
|
|
||
| install_cni_bin | ||
|
|
||
|
|
@@ -339,24 +346,19 @@ CNI_CONF_SHA='{}' | |
| monitor_cni_config & | ||
|
|
||
| # Append our config to any existing config file (*.conflist or *.conf) | ||
| config_files=$(find "${HOST_CNI_NET}" -maxdepth 1 -type f \( -iname '*conflist' -o -iname '*conf' \)) | ||
| config_files=$(find "$HOST_CNI_NET" -maxdepth 1 -type f \( -iname '*conflist' -o -iname '*conf' \) | grep -v linkerd || true) | ||
|
||
| if [ -z "$config_files" ]; then | ||
| log "No active CNI configuration files found" | ||
| log "No active CNI configuration files found" | ||
| else | ||
| 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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. combine the two |
||
| find "${HOST_CNI_NET}" -maxdepth 1 -type f \( -iname '*conflist' -o -iname '*conf' \) -print0 | | ||
| while read -r -d $'\0' file; do | ||
| log "Trigger CNI config detection for $file" | ||
| tmp_file="$(mktemp -u /tmp/linkerd-cni.patch-candidate.XXXXXX)" | ||
| cp -fp "$file" "$tmp_file" | ||
| # The following will trigger the `sync()` function via filesystem event. | ||
| # This requires `monitor_cni_config()` to be up and running! | ||
| mv "$tmp_file" "$file" || exit_with_error 'Failed to mv files.' | ||
| done | ||
| fi | ||
| find "$HOST_CNI_NET" -maxdepth 1 -type f \( -iname '*conflist' -o -iname '*conf' \) -print0 | | ||
| while read -r -d $'\0' file; do | ||
| log "Trigger CNI config detection for $file" | ||
| tmp_file="$(mktemp -u /tmp/linkerd-cni.patch-candidate.XXXXXX)" | ||
| cp -fp "$file" "$tmp_file" | ||
| # The following will trigger the `sync()` function via filesystem event. | ||
| # This requires `monitor_cni_config()` to be up and running! | ||
| mv "$tmp_file" "$file" || exit_with_error 'Failed to mv files.' | ||
| done | ||
| fi | ||
|
|
||
| # Watch in bg so we can receive interrupt signals through 'trap'. From 'man | ||
|
|
@@ -368,5 +370,6 @@ fi | |
| # the wait builtin to return immediately with an exit status greater than 128, | ||
| # immediately after which the trap is executed." | ||
| monitor_service_account_token & | ||
| # uses -n so that we exit when the first background job exits (when there's an error) | ||
| # uses -n so that we exit when the first background job exits (when there's an | ||
| # error) | ||
| wait -n | ||
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.
may i ask, why are we opting to remove the surrounding braces throughout this script?
Uh oh!
There was an error while loading. Please reload this page.
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.
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:
${foo:-default_value}or${foo//a/b}$foobar!=${foo}barbut 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.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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:
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 have just pushed a commit that flips all variables to using braces.