From bdd5f82458585cfc92a0bd2297a08033d0dd14d8 Mon Sep 17 00:00:00 2001 From: Ed Santiago Date: Tue, 29 Nov 2022 10:26:48 -0700 Subject: [PATCH] CI setup: simplify environment passthrough code The passthrough_env function was unnecessarily complicated, hence fragile. Clean it up, and add regression tests. For future reference: CI broke horribly because of this. Rootless tests all failed with missing CI_DESIRED_NETWORK. Root cause was that CIRRUS_CHANGE_TITLE had a trailing space which, because of shell indirection, passthrough_env() wrote as trailing backslash (not backslash-space) in the /etc/ci_environment file, which then caused the next line in the file to get glommed onto CIRRUS_CHANGE_TITLE. Signed-off-by: Ed Santiago --- contrib/cirrus/lib.sh | 42 +++--- contrib/cirrus/lib.sh.t | 193 ++++++++++++++++++++++++++++ contrib/cirrus/prebuild.sh | 3 + contrib/cirrus/runner.sh | 4 +- contrib/cirrus/setup_environment.sh | 4 +- 5 files changed, 222 insertions(+), 24 deletions(-) create mode 100755 contrib/cirrus/lib.sh.t diff --git a/contrib/cirrus/lib.sh b/contrib/cirrus/lib.sh index 68b896335e..7a5364886b 100644 --- a/contrib/cirrus/lib.sh +++ b/contrib/cirrus/lib.sh @@ -91,9 +91,21 @@ EPOCH_TEST_COMMIT="$CIRRUS_BASE_SHA" # testing operations on all platforms and versions. This is necessary # to avoid needlessly passing through global/system values across # contexts, such as host->container or root->rootless user -PASSTHROUGH_ENV_RE='(^CI.*)|(^CIRRUS)|(^DISTRO_NV)|(^GOPATH)|(^GOCACHE)|(^GOSRC)|(^SCRIPT_BASE)|(CGROUP_MANAGER)|(OCI_RUNTIME)|(^TEST.*)|(^PODBIN_NAME)|(^PRIV_NAME)|(^ALT_NAME)|(^ROOTLESS_USER)|(SKIP_USERNS)|(.*_NAME)|(.*_FQIN)|(NETWORK_BACKEND)|(DEST_BRANCH)' +# +# List of envariables which must be EXACT matches +PASSTHROUGH_ENV_EXACT='CGROUP_MANAGER|DEST_BRANCH|DISTRO_NV|GOCACHE|GOPATH|GOSRC|NETWORK_BACKEND|OCI_RUNTIME|ROOTLESS_USER|SCRIPT_BASE|SKIP_USERNS' + +# List of envariable patterns which must match AT THE BEGINNING of the name. +PASSTHROUGH_ENV_ATSTART='CI|TEST' + +# List of envariable patterns which can match ANYWHERE in the name +PASSTHROUGH_ENV_ANYWHERE='_NAME|_FQIN' + +# Combine into one +PASSTHROUGH_ENV_RE="(^($PASSTHROUGH_ENV_EXACT)\$)|(^($PASSTHROUGH_ENV_ATSTART))|($PASSTHROUGH_ENV_ANYWHERE)" + # Unsafe env. vars for display -SECRET_ENV_RE='(ACCOUNT)|(GC[EP]..+)|(SSH)|(PASSWORD)|(TOKEN)' +SECRET_ENV_RE='ACCOUNT|GC[EP]..|SSH|PASSWORD|SECRET|TOKEN' # Type of filesystem used for cgroups CG_FS_TYPE="$(stat -f -c %T /sys/fs/cgroup)" @@ -107,28 +119,18 @@ set +a lilto() { err_retry 8 1000 "" "$@"; } # just over 4 minutes max bigto() { err_retry 7 5670 "" "$@"; } # 12 minutes max -# Print shell-escaped variable=value pairs, one per line, based on -# variable name matching a regex. This is intended to catch -# variables being passed down from higher layers, like Cirrus-CI. +# Return a list of environment variables that should be passed through +# to lower levels (tests in containers, or via ssh to rootless). +# We return the variable names only, not their values. It is up to our +# caller to reference values. passthrough_envars(){ - local xchars local envname - local envval - # Avoid values containing entirely punctuation|control|whitespace - xchars='[:punct:][:cntrl:][:space:]' warn "Will pass env. vars. matching the following regex: $PASSTHROUGH_ENV_RE" - for envname in $(awk 'BEGIN{for(v in ENVIRON) print v}' | \ - grep -Ev "SETUP_ENVIRONMENT" | \ - grep -Ev "$SECRET_ENV_RE" | \ - grep -E "$PASSTHROUGH_ENV_RE"); do - - envval="${!envname}" - [[ -n $(tr -d "$xchars" <<<"$envval") ]] || continue - - # Properly escape values to prevent injection - printf -- "$envname=%q\n" "$envval" - done + compgen -A variable | \ + grep -Ev "SETUP_ENVIRONMENT" | \ + grep -Ev "$SECRET_ENV_RE" | \ + grep -E "$PASSTHROUGH_ENV_RE" } setup_rootless() { diff --git a/contrib/cirrus/lib.sh.t b/contrib/cirrus/lib.sh.t new file mode 100755 index 0000000000..79d4a0082c --- /dev/null +++ b/contrib/cirrus/lib.sh.t @@ -0,0 +1,193 @@ +#!/bin/bash +# +# tests for lib.sh +# + +# To ensure consistent sorting +export LC_ALL=C + +############################################################################### +# BEGIN code to define a clean safe environment + +# Envariables which we should keep; anything else, we toss. +declare -a keep_env_list=(IFS HOME PATH SECRET_ENV_RE + PASSTHROUGH_ENV_EXACT + PASSTHROUGH_ENV_ATSTART + PASSTHROUGH_ENV_ANYWHERE + PASSTHROUGH_ENV_RE + TMPDIR tmpdir keep_env rc_file testnum_file) +declare -A keep_env +for i in "${keep_env_list[@]}"; do + keep_env[$i]=1 +done + +# END code to define a clean safe environment +############################################################################### +# BEGIN test scaffolding + +tmpdir=$(mktemp --tmpdir --directory lib-sh-tests.XXXXXXX) +# shellcheck disable=SC2154 +trap 'status=$?; rm -rf $tmpdir;exit $status' 0 + +# Needed by lib.sh, but we don't actually need anything in it +touch "$tmpdir"/common_lib.sh + +# Iterator and return code. Because some tests run in subshells (to avoid +# namespace pollution), variables aren't preserved. Use files to track them. +testnum_file=$tmpdir/testnum +rc_file=$tmpdir/rc + +echo 0 >"$testnum_file" +echo 0 >"$rc_file" + +# Helper function: runs passthrough_envars(), compares against expectations +function check_passthrough { + testnum=$(< "$testnum_file") + testnum=$((testnum + 1)) + echo $testnum > "$testnum_file" + + # shellcheck disable=SC2046,SC2005,SC2116 + actual="$(echo $(passthrough_envars))" + + if [[ "$actual" = "$1" ]]; then + # Multi-level echo flattens newlines, makes success messages readable + # shellcheck disable=SC2046,SC2005,SC2116 + echo $(echo "ok $testnum $2") + else + echo "not ok $testnum $2" + echo "# expected: $1" + echo "# actual: $actual" + echo 1 >| "$rc_file" + fi +} + +# END test scaffolding +############################################################################### + +# vars and a function needed by lib.sh +# shellcheck disable=SC2034 +{ + AUTOMATION_LIB_PATH=$tmpdir + CIRRUS_BASE_SHA=x + CIRRUS_TAG=x + function warn() { + : + } + # shellcheck disable=all + source $(dirname "$0")/lib.sh +} + +# Our environment is now super-polluted. Clean it up, preserving critical env. +while read -r v;do + if [[ -z "${keep_env[$v]}" ]]; then + unset "$v" 2>/dev/null + fi +done < <(compgen -A variable) + +# begin actual tests + +check_passthrough "" "with empty environment" + +# +# Now set all sorts of secrets, which should be excluded +# +# shellcheck disable=SC2034 +{ + ACCOUNT_ABC=1 + ABC_ACCOUNT=1 + ABC_ACCOUNT_DEF=1 + GCEFOO=1 + GCPBAR=1 + SSH123=1 + NOTSSH=1 + SSH=1 + PASSWORD=1 + MYSECRET=1 + SECRET2=1 + TOKEN=1 + check_passthrough "" "secrets are filtered" +} + +# These are passed through only when they match EXACTLY. +readarray -d '|' -t pt_exact <<<"$PASSTHROUGH_ENV_EXACT" +# shellcheck disable=SC2048 +for s in ${pt_exact[*]}; do + # Run inside a subshell, to avoid cluttering environment + ( + eval "${s}=1" # This is the only one that should be passed + eval "a${s}=1" + eval "${s}z=1" + eval "YYY_${s}_YYY=1" + eval "ZZZ_${s}=1" + eval "${s}_ZZZ=1" + + # Only the exact match should be passed + check_passthrough "$s" "exact match only: $s" + ) +done + +# These are passed through only when they match AT THE BEGINNING. +# +# Also, we run this _entire_ test inside a subshell, cluttering the +# environment, so we're testing that passthrough_envars can handle +# and return long lists of unrelated matches. Kind of a pointless +# test, there's not really any imaginable way that could fail. +( + # Inside the subshell. Start with null expectations. + expect= + + # WARNING! $PASSTHROUGH_ENV_ATSTART must be in alphabetical order, + # because passthrough_envars always returns a sorted list and (see + # subshell comments above) we're incrementally adding to our env. + readarray -d '|' -t pt_atstart <<<"$PASSTHROUGH_ENV_ATSTART" + # shellcheck disable=SC2048 + for s in ${pt_atstart[*]}; do + eval "${s}=1" + eval "${s}123=1" + eval "NOPE_${s}=1" + eval "NOR_${s}_EITHER=1" + + if [[ -n "$expect" ]]; then + expect+=" " + fi + expect+="$s ${s}123" + + check_passthrough "$expect" "at start only: $s" + done +) + +# These are passed through if they match ANYWHERE IN THE NAME +readarray -d '|' -t pt_anywhere <<<"$PASSTHROUGH_ENV_ANYWHERE" +# shellcheck disable=SC2048 +for s in ${pt_anywhere[*]}; do + ( + eval "${s}=1" + eval "${s}z=1" + eval "z${s}=1" + eval "z${s}z=1" + + check_passthrough "${s} ${s}z z${s} z${s}z" "anywhere: $s" + ) +done + +# And, to guard against null runs of the above loops, hardcoded tests of each: +# shellcheck disable=SC2034 +( + CI=1 + CI_FOO=1 + CIRRUS_BAR=1 + GOPATH=gopath + GOPATH_NOT=not + ROOTLESS_USER=rootless + ZZZ_NAME=1 + + check_passthrough "CI CIRRUS_BAR CI_FOO GOPATH ROOTLESS_USER ZZZ_NAME" \ + "final handcrafted sanity check" +) + +# Final check +check_passthrough "" "Environment remains unpolluted at end" + +# Done +# shellcheck disable=all +exit $(<"$rc_file") diff --git a/contrib/cirrus/prebuild.sh b/contrib/cirrus/prebuild.sh index 0fb9004ff0..0e43f19462 100755 --- a/contrib/cirrus/prebuild.sh +++ b/contrib/cirrus/prebuild.sh @@ -42,6 +42,9 @@ if [[ "${DISTRO_NV}" =~ fedora ]]; then ./.github/actions/check_cirrus_cron/* \ hack/get_ci_vm.sh + # Tests for lib.sh + showrun ${SCRIPT_BASE}/lib.sh.t + export PREBUILD=1 showrun bash ${CIRRUS_WORKING_DIR}/.github/actions/check_cirrus_cron/test.sh fi diff --git a/contrib/cirrus/runner.sh b/contrib/cirrus/runner.sh index 1c9d565fb6..f7f7eccc48 100755 --- a/contrib/cirrus/runner.sh +++ b/contrib/cirrus/runner.sh @@ -136,11 +136,11 @@ exec_container() { # Line-separated arguments which include shell-escaped special characters declare -a envargs - while read -r var_val; do + while read -r var; do # Pass "-e VAR" on the command line, not "-e VAR=value". Podman can # do a much better job of transmitting the value than we can, # especially when value includes spaces. - envargs+=("-e" "$(awk -F= '{print $1}' <<<$var_val)") + envargs+=("-e" "$var") done <<<"$(passthrough_envars)" # VM Images and Container images are built using (nearly) identical operations. diff --git a/contrib/cirrus/setup_environment.sh b/contrib/cirrus/setup_environment.sh index 44919a3490..01e248c694 100755 --- a/contrib/cirrus/setup_environment.sh +++ b/contrib/cirrus/setup_environment.sh @@ -51,8 +51,8 @@ git config --system --add safe.directory $GOSRC echo -e "\n# Begin single-use VM global variables (${BASH_SOURCE[0]})" \ > "/etc/ci_environment" ( - while read -r env_var_val; do - echo "$env_var_val" + while read -r env_var; do + printf -- "%s=%q\n" "${env_var}" "${!env_var}" done <<<"$(passthrough_envars)" ) >> "/etc/ci_environment"