Merge pull request #466 from ChrisERo/master

fix: resolve issue #463

cmd/git-sync/main.go

@@ -462,8 +462,8 @@ func main() {
 		run:         cmdRunner,
 	}
 
-	// This context is used only for git credentials initialization. There are no long-running operations like
-	// `git clone`, so hopefully 30 seconds will be enough.
+	// This context is used only for git credentials initialization. There are
+	// no long-running operations like `git clone`, so hopefully 30 seconds will be enough.
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 
 	if *flUsername != "" {
@@ -544,6 +544,10 @@ func main() {
 	// Startup webhooks goroutine
 	var webhookRunner *hook.HookRunner
 	if *flWebhookURL != "" {
+		var webhookChannel chan bool
+		if *flOneTime {
+			webhookChannel = make(chan bool, 1)
+		}
 		webhook := hook.NewWebhook(
 			*flWebhookURL,
 			*flWebhookMethod,
@@ -556,13 +560,18 @@ func main() {
 			*flWebhookBackoff,
 			hook.NewHookData(),
 			log,
+			webhookChannel,
 		)
 		go webhookRunner.Run(context.Background())
 	}
 
 	// Startup exechooks goroutine
 	var exechookRunner *hook.HookRunner
 	if *flExechookCommand != "" {
+		var exechookChannel chan bool
+		if *flOneTime {
+			exechookChannel = make(chan bool, 1)
+		}
 		exechook := hook.NewExechook(
 			cmd.NewRunner(log),
 			*flExechookCommand,
@@ -576,6 +585,7 @@ func main() {
 			*flExechookBackoff,
 			hook.NewHookData(),
 			log,
+			exechookChannel,
 		)
 		go exechookRunner.Run(context.Background())
 	}
@@ -621,9 +631,27 @@ 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 {
+						log.Error(err, "exechook completed with error")
+						exitCode = 1
+					}
+				}
+				if webhookRunner != nil {
+					if err = webhookRunner.WaitForCompletion(); err != nil {
+						log.Error(err, "webhook completed with error")
+						exitCode = 1
+					}
+				}
 				log.DeleteErrorFile()
-				os.Exit(0)
+				os.Exit(exitCode)
 			}
 			if isHash, err := git.RevIsHash(ctx); err != nil {
 				log.Error(err, "can't tell if rev is a git hash, exiting", "rev", git.rev)

pkg/hook/hook.go

@@ -18,6 +18,8 @@ package hook
 
 import (
 	"context"
+	"fmt"
+	"runtime"
 	"sync"
 	"time"
 
@@ -82,8 +84,8 @@ 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, hasSucceededOnce chan bool) *HookRunner {
+	return &HookRunner{hook: hook, backoff: backoff, data: data, logger: log, hasCompletedOnce: hasSucceededOnce}
 }
 
 // HookRunner struct
@@ -96,6 +98,11 @@ type HookRunner struct {
 	data *hookData
 	// Logger
 	logger *logging.Logger
+	// hasCompletedOnce is used to send true if and only if first run executed
+	// successfully and false otherwise to some receiver.  Should be
+	// initialised to a buffered channel of size 1.
+	// Is only meant for use within sendCompletedOnceMessageIfApplicable.
+	hasCompletedOnce chan bool
 }
 
 // Send sends hash to hookdata
@@ -123,16 +130,51 @@ 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.sendCompletedOnceMessageIfApplicable(false)
 				time.Sleep(r.backoff)
 			} else {
 				updateHookRunCountMetric(r.hook.Name(), "success")
 				lastHash = hash
+				r.sendCompletedOnceMessageIfApplicable(true)
 				break
 			}
 		}
 	}
 }
 
+// If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller
+// provided success status (as a boolean) of HookRunner to receivers of
+// hasCompletedOnce, closes said chanel, and terminates this goroutine.
+// Using this function to write to hasCompletedOnce ensures it is only ever
+// written to once.
+func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) {
+	if r.hasCompletedOnce != nil {
+		r.hasCompletedOnce <- completedSuccessfully
+		close(r.hasCompletedOnce)
+		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.hasCompletedOnce is not nil, but if it is, returns an error
+func (r *HookRunner) WaitForCompletion() error {
+	// Make sure function should be called
+	if r.hasCompletedOnce == nil {
+		return fmt.Errorf("HookRunner.WaitForCompletion called on async runner")
+	}
+
+	// Perform wait on HookRunner
+	hookRunnerFinishedSuccessfully := <-r.hasCompletedOnce
+	if !hookRunnerFinishedSuccessfully {
+		return fmt.Errorf("hook completed with error")
+	}
+
+	return nil
+}
+
 func updateHookRunCountMetric(name, status string) {
 	hookRunCount.WithLabelValues(name, status).Inc()
 }

test_e2e.sh

@@ -143,6 +143,8 @@ 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 +163,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 \
@@ -1072,6 +1076,62 @@ 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 \
+        --period=100ms \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --root="$ROOT" \
+        --link="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 \
+            --period=100ms \
+            --one-time \
+            --repo="file://$REPO" \
+            --branch="$MAIN_BRANCH" \
+            --root="$ROOT" \
+            --link="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 least 1"
+        fi
+}
+
 ##############################################
 # Test webhook success
 ##############################################
@@ -1168,6 +1228,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 \
+        --period=100ms \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --root="$ROOT" \
+        --webhook-url="http://$IP" \
+        --webhook-success-status=200 \
+        --link="link" \
+        >> "$1" 2>&1
+
+    # check that basic call works
+    sleep 2
+    HITS=$(cat "$HITLOG" | wc -l)
+    if [[ "$HITS" < 1 ]]; then
+        fail "webhook 1 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 \
+        --period=100ms \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --root="$ROOT" \
+        --webhook-url="http://$IP" \
+        --webhook-success-status=200 \
+        --link="link" \
+        >> "$1" 2>&1
+
+    # Check that webhook was called
+    sleep 2
+    HITS=$(cat "$HITLOG" | wc -l)
+    if [[ "$HITS" < 1 ]]; then
+        fail "webhook 1 called $HITS times"
+    fi
+    docker_kill "$CTR"
+}
+
 ##############################################
 # Test webhook fire-and-forget
 ##############################################

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