From d03b071d5b2270df05bf4a6a0794c16fd77fd975 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 2 Feb 2018 10:26:09 +0200 Subject: [PATCH 1/6] pkg/util/verify-util-pkg.sh: fix shell return value comparison. In shell scripts inside [[ .. ]] blocks, ">" is a string comparison operator. The return value check using it appears to work mostly by accident, because the only values are "0" and "1". Change to -gt operator. --- pkg/util/verify-util-pkg.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/verify-util-pkg.sh b/pkg/util/verify-util-pkg.sh index 755924a099a..2b8d628ebc0 100755 --- a/pkg/util/verify-util-pkg.sh +++ b/pkg/util/verify-util-pkg.sh @@ -41,7 +41,7 @@ pushd "${BASH_DIR}" > /dev/null done popd > /dev/null -if [[ ${ret} > 0 ]]; then +if [[ ${ret} -gt 0 ]]; then exit ${ret} fi From 2226b1de09daaa1a2ea3bbcff702f34dc2683e4f Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 2 Feb 2018 11:17:01 +0200 Subject: [PATCH 2/6] cluster/gce: fix shell return value comparison. In shell scripts inside [[ .. ]] blocks, ">" is a string comparison operator. The "attempt" number comparison works (most likely by accident) because the max number of attempts is below 10. Change to -gt operator. --- cluster/gce/list-resources.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cluster/gce/list-resources.sh b/cluster/gce/list-resources.sh index a63be2c24f9..a95f9c50f76 100755 --- a/cluster/gce/list-resources.sh +++ b/cluster/gce/list-resources.sh @@ -57,7 +57,7 @@ function gcloud-compute-list() { fi echo -e "Attempt ${attempt} failed to list ${resource}. Retrying." >&2 attempt=$(($attempt+1)) - if [[ ${attempt} > 5 ]]; then + if [[ ${attempt} -gt 5 ]]; then echo -e "List ${resource} failed!" >&2 exit 2 fi From 6372bb2f282ea935e3bd26bf25e645576efff309 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 2 Feb 2018 11:26:22 +0200 Subject: [PATCH 3/6] cluster/gce: fix checks for empty strings. In order to use -n, the value needs either be quoted or [[ .. ]] block has to be used. Fix the comparisons that way. To verify, consider this (analogous) script: #!/bin/bash subnetwork_url="" if [ -n ${subnetwork_url} ]; then echo "foo" fi if [[ -n ${subnetwork_url} ]]; then echo "bar" fi Here "foo" is echoed by the script, even though the variable subnetwork_url has a zero-length value. --- cluster/gce/gci/flexvolume_node_setup.sh | 4 ++-- cluster/gce/upgrade-aliases.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cluster/gce/gci/flexvolume_node_setup.sh b/cluster/gce/gci/flexvolume_node_setup.sh index cad4b48e80f..ce4d995e95d 100755 --- a/cluster/gce/gci/flexvolume_node_setup.sh +++ b/cluster/gce/gci/flexvolume_node_setup.sh @@ -81,10 +81,10 @@ flex_clean() { umount_silent ${MOUNTER_PATH} rm -rf ${MOUNTER_PATH} - if [ -n ${IMAGE_URL:-} ]; then + if [[ -n ${IMAGE_URL:-} ]]; then docker rmi -f ${IMAGE_URL} &> /dev/null || /bin/true fi - if [ -n ${MOUNTER_DEFAULT_NAME:-} ]; then + if [[ -n ${MOUNTER_DEFAULT_NAME:-} ]]; then docker rm -f ${MOUNTER_DEFAULT_NAME} &> /dev/null || /bin/true fi } diff --git a/cluster/gce/upgrade-aliases.sh b/cluster/gce/upgrade-aliases.sh index 29dbf0232ed..410cd639d88 100755 --- a/cluster/gce/upgrade-aliases.sh +++ b/cluster/gce/upgrade-aliases.sh @@ -53,7 +53,7 @@ function detect-k8s-subnetwork() { local subnetwork_url=$(gcloud compute instances describe \ ${KUBE_MASTER} --project=${PROJECT} --zone=${ZONE} \ --format='value(networkInterfaces[0].subnetwork)') - if [ -n ${subnetwork_url} ]; then + if [[ -n ${subnetwork_url} ]]; then IP_ALIAS_SUBNETWORK=$(echo ${subnetwork_url##*/}) fi } From 03b674bc86af298becb34ee45e48797ade4a9a45 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 2 Feb 2018 11:53:17 +0200 Subject: [PATCH 4/6] verify-godeps: change redirection order. In code "popd 2>&1 > /dev/null" the stderr output goes to the terminal instead of /dev/null, which is clearly the intention. Change the order to fix this in four places. Also, while at it, change one incorrect comparison to use "-gt" instead of ">" in the same file. --- hack/verify-godeps.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hack/verify-godeps.sh b/hack/verify-godeps.sh index b03826e7449..057da6b1536 100755 --- a/hack/verify-godeps.sh +++ b/hack/verify-godeps.sh @@ -65,7 +65,7 @@ _kubetmp="${_kubetmp}/kubernetes" # Do all our work in the new GOPATH export GOPATH="${_tmpdir}" -pushd "${_kubetmp}" 2>&1 > /dev/null +pushd "${_kubetmp}" > /dev/null 2>&1 # Restore the Godeps into our temp directory hack/godep-restore.sh @@ -78,11 +78,11 @@ pushd "${_kubetmp}" 2>&1 > /dev/null # Recreate the Godeps using the nice clean set we just downloaded hack/godep-save.sh -popd 2>&1 > /dev/null +popd > /dev/null 2>&1 ret=0 -pushd "${KUBE_ROOT}" 2>&1 > /dev/null +pushd "${KUBE_ROOT}" > /dev/null 2>&1 # Test for diffs if ! _out="$(diff -Naupr --ignore-matching-lines='^\s*\"GoVersion\":' --ignore-matching-line='^\s*\"GodepVersion\":' --ignore-matching-lines='^\s*\"Comment\":' Godeps/Godeps.json ${_kubetmp}/Godeps/Godeps.json)"; then echo "Your Godeps.json is different:" >&2 @@ -117,9 +117,9 @@ pushd "${KUBE_ROOT}" 2>&1 > /dev/null fi ret=1 fi -popd 2>&1 > /dev/null +popd > /dev/null 2>&1 -if [[ ${ret} > 0 ]]; then +if [[ ${ret} -gt 0 ]]; then exit ${ret} fi From 07d62835211ea31145952c7fee3e8fec55e165e3 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Thu, 8 Feb 2018 12:12:55 +0200 Subject: [PATCH 5/6] verify-cli-conventions.sh: use $(..) instead of `..`. Using $(..) is the recommended way of running subshells. It also is the convention used almost every place in kubernetes scripts. --- hack/verify-cli-conventions.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/verify-cli-conventions.sh b/hack/verify-cli-conventions.sh index d768eb0a560..935d5dc0d71 100755 --- a/hack/verify-cli-conventions.sh +++ b/hack/verify-cli-conventions.sh @@ -30,7 +30,7 @@ make -C "${KUBE_ROOT}" WHAT="${BINS[*]}" clicheck=$(kube::util::find-binary "clicheck") -if ! output=`$clicheck 2>&1` +if ! output=$($clicheck 2>&1) then echo "$output" echo From 261a33dfa76e019e82d904cee8d561150a16b128 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Thu, 8 Feb 2018 15:52:26 +0200 Subject: [PATCH 6/6] update-godep-licenses.sh: various fixes and cleanups. The file was analyzed with shellcheck, and various issues fixed. Most of the problems were just cleanups, but also potential bugs were fixed. Many variables were quoted with double quotes to prevent globbing. The local_files array expansion was quoted so that any file names with potential spaces in the filename would not be re-split. The empty default value was removed from the list processing. POSIX standard "grep -E" was used instead of egrep. --- hack/update-godep-licenses.sh | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/hack/update-godep-licenses.sh b/hack/update-godep-licenses.sh index 34e064672e9..bd9679968b4 100755 --- a/hack/update-godep-licenses.sh +++ b/hack/update-godep-licenses.sh @@ -74,10 +74,10 @@ process_content () { # Start search at package root case ${package} in github.com/*|golang.org/*|bitbucket.org/*) - package_root=$(echo ${package} |awk -F/ '{ print $1"/"$2"/"$3 }') + package_root=$(echo "${package}" |awk -F/ '{ print $1"/"$2"/"$3 }') ;; go4.org/*) - package_root=$(echo ${package} |awk -F/ '{ print $1 }') + package_root=$(echo "${package}" |awk -F/ '{ print $1 }') ;; gopkg.in/*) # Root of gopkg.in package always ends with '.v(number)' and my contain @@ -85,10 +85,10 @@ process_content () { # - gopkg.in/yaml.v2 # - gopkg.in/inf.v0 # - gopkg.in/square/go-jose.v2 - package_root=$(echo ${package} |grep -oh '.*\.v[0-9]') + package_root=$(echo "${package}" |grep -oh '.*\.v[0-9]') ;; *) - package_root=$(echo ${package} |awk -F/ '{ print $1"/"$2 }') + package_root=$(echo "${package}" |awk -F/ '{ print $1"/"$2 }') ;; esac @@ -98,7 +98,7 @@ process_content () { [[ -d ${DEPS_DIR}/${dir_root} ]] || continue # One (set) of these is fine - find ${DEPS_DIR}/${dir_root} \ + find "${DEPS_DIR}/${dir_root}" \ -xdev -follow -maxdepth ${find_maxdepth} \ -type f "${find_names[@]}" done | sort -u)) @@ -107,9 +107,14 @@ process_content () { local f index="${package}-${type}" if [[ -z "${CONTENT[${index}]-}" ]]; then - for f in ${local_files[@]-}; do + for f in "${local_files[@]-}"; do + if [[ -z "$f" ]]; then + # Set the default value and then check it to prevent + # accessing potentially empty array + continue + fi # Find some copyright info in any file and break - if egrep -i -wq "${ensure_pattern}" "${f}"; then + if grep -E -i -wq "${ensure_pattern}" "${f}"; then CONTENT[${index}]="${f}" break fi @@ -151,19 +156,17 @@ declare -Ag CONTENT echo "================================================================================" echo "= Kubernetes licensed under: =" echo -cat ${LICENSE_ROOT}/LICENSE +cat "${LICENSE_ROOT}/LICENSE" echo -echo "= LICENSE $(cat ${LICENSE_ROOT}/LICENSE | md5sum | awk '{print $1}')" +echo "= LICENSE $(cat "${LICENSE_ROOT}/LICENSE" | md5sum | awk '{print $1}')" echo "================================================================================" ) > ${TMP_LICENSE_FILE} # Loop through every package in Godeps.json -for PACKAGE in $(cat Godeps/Godeps.json | \ - jq -r ".Deps[].ImportPath" | \ - sort -f); do - process_content ${PACKAGE} LICENSE - process_content ${PACKAGE} COPYRIGHT - process_content ${PACKAGE} COPYING +for PACKAGE in $(jq -r ".Deps[].ImportPath" < Godeps/Godeps.json | sort -f); do + process_content "${PACKAGE}" LICENSE + process_content "${PACKAGE}" COPYRIGHT + process_content "${PACKAGE}" COPYING # display content echo @@ -195,7 +198,7 @@ __EOF__ cat "${file}" echo - echo "= ${file} $(cat ${file} | md5sum | awk '{print $1}')" + echo "= ${file} $(cat "${file}" | md5sum | awk '{print $1}')" echo "================================================================================" echo done >> ${TMP_LICENSE_FILE}