Merge pull request #469 from thockin/release-3.x

Ensure web/exec hooks complete in --one-time

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)

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 {
+func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log *logging.Logger, oneTime bool) *HookRunner {
-	return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log}
+	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
+}

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.
+function print_tests() {
-if [[ "$#" == 1 && "$1" == "-?" ]]; then
     echo "available tests:"
     for t in "${tests[@]}"; do
         echo "    $t"
     done
+}
+
+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

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

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