From 89b0bd5448ffc054fd17d4ecb9f0f93a6ca9feec Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 19 Jan 2022 12:01:15 -0800 Subject: [PATCH] Ensure web/exec hooks complete in --one-time Also some small e2e cleanups. This is a port from master branch. --- cmd/git-sync/main.go | 20 ++- pkg/hook/hook.go | 49 ++++++- test_e2e.sh | 170 ++++++++++++++++++++--- test_exechook_command_fail_with_sleep.sh | 22 +++ test_exechook_command_with_sleep.sh | 22 +++ 5 files changed, 263 insertions(+), 20 deletions(-) create mode 100755 test_exechook_command_fail_with_sleep.sh create mode 100755 test_exechook_command_with_sleep.sh diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index 9ad8706..c2cd9c6 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -469,6 +469,7 @@ func main() { *flWebhookBackoff, hook.NewHookData(), log, + *flOneTime, ) go webhookRunner.Run(context.Background()) } @@ -489,6 +490,7 @@ func main() { *flExechookBackoff, hook.NewHookData(), log, + *flOneTime, ) go exechookRunner.Run(context.Background()) } @@ -525,9 +527,25 @@ func main() { } if initialSync { + // Determine if git-sync should terminate for one of several reasons if *flOneTime { + // Wait for hooks to complete at least once, if not nil, before + // checking whether to stop program. + // Assumes that if hook channels are not nil, they will have at + // least one value before getting closed + exitCode := 0 // is 0 if all hooks succeed, else is 1 + if exechookRunner != nil { + if err := exechookRunner.WaitForCompletion(); err != nil { + exitCode = 1 + } + } + if webhookRunner != nil { + if err := webhookRunner.WaitForCompletion(); err != nil { + exitCode = 1 + } + } log.DeleteErrorFile() - os.Exit(0) + os.Exit(exitCode) } if isHash, err := revIsHash(ctx, *flRev, *flRoot); err != nil { log.Error(err, "can't tell if rev is a git hash, exiting", "rev", *flRev) diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index d5b9857..9ce87a2 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -18,6 +18,8 @@ package hook import ( "context" + "fmt" + "runtime" "sync" "time" @@ -82,8 +84,12 @@ func (d *hookData) send(newHash string) { } // NewHookRunner returns a new HookRunner -func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger) *HookRunner { - return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log} +func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger, oneTime bool) *HookRunner { + hr := &HookRunner{hook: hook, backoff: backoff, data: data, logger: log} + if oneTime { + hr.oneTimeResult = make(chan bool, 1) + } + return hr } // HookRunner struct @@ -96,6 +102,9 @@ type HookRunner struct { data *hookData // Logger logger *logging.Logger + // Used to send a status result when running in one-time mode. + // Should be initialised to a buffered channel of size 1. + oneTimeResult chan bool } // Send sends hash to hookdata @@ -123,10 +132,14 @@ func (r *HookRunner) Run(ctx context.Context) { if err := r.hook.Do(ctx, hash); err != nil { r.logger.Error(err, "hook failed") updateHookRunCountMetric(r.hook.Name(), "error") + // don't want to sleep unnecessarily terminating anyways + r.sendOneTimeResultAndTerminate(false) + time.Sleep(r.backoff) } else { updateHookRunCountMetric(r.hook.Name(), "success") lastHash = hash + r.sendOneTimeResultAndTerminate(true) break } } @@ -136,3 +149,35 @@ func (r *HookRunner) Run(ctx context.Context) { func updateHookRunCountMetric(name, status string) { hookRunCount.WithLabelValues(name, status).Inc() } + +// If oneTimeResult is nil, does nothing. Otherwise, forwards the caller +// provided success status (as a boolean) of HookRunner to receivers of +// oneTimeResult, closes said chanel, and terminates this goroutine. +// Using this function to write to oneTimeResult ensures it is only ever +// written to once. +func (r *HookRunner) sendOneTimeResultAndTerminate(completedSuccessfully bool) { + if r.oneTimeResult != nil { + r.oneTimeResult <- completedSuccessfully + close(r.oneTimeResult) + runtime.Goexit() + } +} + +// WaitForCompletion waits for HookRunner to send completion message to +// calling thread and returns either true if HookRunner executed successfully +// and some error otherwise. +// Assumes that r.oneTimeResult is not nil, but if it is, returns an error +func (r *HookRunner) WaitForCompletion() error { + // Make sure function should be called + if r.oneTimeResult == nil { + return fmt.Errorf("HookRunner.WaitForCompletion called on async runner") + } + + // Perform wait on HookRunner + hookRunnerFinishedSuccessfully := <-r.oneTimeResult + if !hookRunnerFinishedSuccessfully { + return fmt.Errorf("hook completed with error") + } + + return nil +} diff --git a/test_e2e.sh b/test_e2e.sh index 6312bab..7f5e9c1 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -125,24 +125,13 @@ mkdir -p "$DOT_SSH" ssh-keygen -f "$DOT_SSH/id_test" -P "" >/dev/null cat "$DOT_SSH/id_test.pub" > "$DOT_SSH/authorized_keys" -function finish() { - r=$? - trap "" INT EXIT - if [[ $r != 0 ]]; then - echo - echo "the directory $DIR was not removed as it contains"\ - "log files useful for debugging" - fi - remove_containers - exit $r -} -trap finish INT EXIT - SLOW_GIT_CLONE=/slow_git_clone.sh SLOW_GIT_FETCH=/slow_git_fetch.sh ASKPASS_GIT=/askpass_git.sh EXECHOOK_COMMAND=/test_exechook_command.sh EXECHOOK_COMMAND_FAIL=/test_exechook_command_fail.sh +EXECHOOK_COMMAND_SLEEPY=/test_exechook_command_with_sleep.sh +EXECHOOK_COMMAND_FAIL_SLEEPY=/test_exechook_command_fail_with_sleep.sh RUNLOG="$DIR/runlog.exechook-fail-retry" rm -f $RUNLOG touch $RUNLOG @@ -161,6 +150,8 @@ function GIT_SYNC() { -v "$(pwd)/askpass_git.sh":"$ASKPASS_GIT":ro \ -v "$(pwd)/test_exechook_command.sh":"$EXECHOOK_COMMAND":ro \ -v "$(pwd)/test_exechook_command_fail.sh":"$EXECHOOK_COMMAND_FAIL":ro \ + -v "$(pwd)/test_exechook_command_with_sleep.sh":"$EXECHOOK_COMMAND_SLEEPY":ro \ + -v "$(pwd)/test_exechook_command_fail_with_sleep.sh":"$EXECHOOK_COMMAND_FAIL_SLEEPY":ro \ -v "$RUNLOG":/var/log/runs \ -v "$DOT_SSH/id_test":"/etc/git-secret/ssh":ro \ --env XDG_CONFIG_HOME=$DIR \ @@ -929,6 +920,63 @@ function e2e::exechook_fail_retry() { fi } +############################################## +# Test exechook-success with GIT_SYNC_ONE_TIME +############################################## +function e2e::exechook_success_once() { + # First sync + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" + + GIT_SYNC \ + --wait=0.1 \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --dest="link" \ + --exechook-command="$EXECHOOK_COMMAND_SLEEPY" \ + >> "$1" 2>&1 + + sleep 2 + assert_link_exists "$ROOT"/link + assert_file_exists "$ROOT"/link/file + assert_file_exists "$ROOT"/link/exechook + assert_file_exists "$ROOT"/link/link-exechook + assert_file_eq "$ROOT"/link/file "$FUNCNAME 1" + assert_file_eq "$ROOT"/link/exechook "$FUNCNAME 1" + assert_file_eq "$ROOT"/link/link-exechook "$FUNCNAME 1" +} + +############################################## +# Test exechook-fail with GIT_SYNC_ONE_TIME +############################################## +function e2e::exechook_fail_once() { + cat /dev/null > "$RUNLOG" + + # First sync - return a failure to ensure that we try again + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" + + GIT_SYNC \ + --wait=0.1 \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --dest="link" \ + --exechook-command="$EXECHOOK_COMMAND_FAIL_SLEEPY" \ + --exechook-backoff=1s \ + >> "$1" 2>&1 + + # Check that exechook was called + sleep 2 + RUNS=$(cat "$RUNLOG" | wc -l) + if [[ "$RUNS" != 1 ]]; then + fail "exechook called $RUNS times, it should be at exactly 1" + fi +} + ############################################## # Test webhook success ############################################## @@ -1025,6 +1073,79 @@ function e2e::webhook_fail_retry() { docker_kill "$CTR" } +############################################## +# Test webhook success with --one-time +############################################## +function e2e::webhook_success_once() { + HITLOG="$DIR/hitlog" + + # First sync + cat /dev/null > "$HITLOG" + CTR=$(docker_run \ + -v "$HITLOG":/var/log/hits \ + e2e/test/test-ncsvr \ + 80 'sleep 3 && echo -e "HTTP/1.1 200 OK\r\n"') + IP=$(docker_ip "$CTR") + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" + + GIT_SYNC \ + --wait=0.1 \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --webhook-url="http://$IP" \ + --webhook-success-status=200 \ + --dest="link" \ + >> "$1" 2>&1 + + # check that basic call works + sleep 2 + HITS=$(cat "$HITLOG" | wc -l) + if [[ "$HITS" != 1 ]]; then + fail "webhook called $HITS times" + fi + + docker_kill "$CTR" +} + +############################################## +# Test webhook fail with --one-time +############################################## +function e2e::webhook_fail_retry_once() { + HITLOG="$DIR/hitlog" + + # First sync - return a failure to ensure that we try again + cat /dev/null > "$HITLOG" + CTR=$(docker_run \ + -v "$HITLOG":/var/log/hits \ + e2e/test/test-ncsvr \ + 80 'sleep 3 && echo -e "HTTP/1.1 500 Internal Server Error\r\n"') + IP=$(docker_ip "$CTR") + echo "$FUNCNAME 1" > "$REPO"/file + git -C "$REPO" commit -qam "$FUNCNAME 1" + + GIT_SYNC \ + --wait=0.1 \ + --one-time \ + --repo="file://$REPO" \ + --branch="$MAIN_BRANCH" \ + --root="$ROOT" \ + --webhook-url="http://$IP" \ + --webhook-success-status=200 \ + --dest="link" \ + >> "$1" 2>&1 + + # Check that webhook was called + sleep 2 + HITS=$(cat "$HITLOG" | wc -l) + if [[ "$HITS" != 1 ]]; then + fail "webhook called $HITS times" + fi + docker_kill "$CTR" +} + ############################################## # Test webhook fire-and-forget ############################################## @@ -1521,14 +1642,29 @@ function list_tests() { # Figure out which, if any, tests to run. tests=($(list_tests)) -# Use -? to just list tests. -if [[ "$#" == 1 && "$1" == "-?" ]]; then +function print_tests() { echo "available tests:" for t in "${tests[@]}"; do echo " $t" done - exit 0 -fi +} + +for t; do + # Use -? to list known tests. + if [[ "${t}" == "-?" ]]; then + print_tests + exit 0 + fi + # Make sure we know this test. + if [[ " ${tests[*]} " =~ " ${t} " ]]; then + continue + fi + # Not a known test or flag. + echo "ERROR: unknown test or flag: '${t}'" + echo + print_tests + exit 1 +done # If no tests specified, run them all. if [[ "$#" == 0 ]]; then diff --git a/test_exechook_command_fail_with_sleep.sh b/test_exechook_command_fail_with_sleep.sh new file mode 100755 index 0000000..321f3d5 --- /dev/null +++ b/test_exechook_command_fail_with_sleep.sh @@ -0,0 +1,22 @@ +#!/bin/sh +# +# Copyright 2021 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Use for e2e test of --exechook-command. +# This option takes no command arguments, so requires a wrapper script. + +sleep 3 +date >> /var/log/runs +exit 1 diff --git a/test_exechook_command_with_sleep.sh b/test_exechook_command_with_sleep.sh new file mode 100755 index 0000000..78d633c --- /dev/null +++ b/test_exechook_command_with_sleep.sh @@ -0,0 +1,22 @@ +#!/bin/sh +# +# Copyright 2020 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Use for e2e test of --exechook-command. +# This option takes no command arguments, so requires a wrapper script. + +sleep 3 +cat file > exechook +cat ../link/file > link-exechook