fix signal handling for lifecycle sidecar

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## UNRELEASED
 
+BUG FIXES:
+* Connect: on termination of a connect injected pod the lifecycle-sidecar sometimes re-registered the application resulting in
+  stale service entries for applications which no longer existed. [[GH-409](https://github.com/hashicorp/consul-k8s/pull/409/)]
+
 BREAKING CHANGES:
 * Connect: the flags `-envoy-image` and `-consul-image` for command `inject-connect` are now required. [[GH-405](https://github.com/hashicorp/consul-k8s/pull/405)]
 

subcommand/lifecycle-sidecar/command.go

@@ -1,6 +1,7 @@
 package subcommand
 
 import (
+	"context"
 	"errors"
 	"flag"
 	"fmt"
@@ -90,6 +91,18 @@ func (c *Command) Run(args []string) int {
 	c.consulCommand = append(c.consulCommand, c.parseConsulFlags()...)
 	c.consulCommand = append(c.consulCommand, c.flagServiceConfig)
 
+	// ctx that we pass in to the main work loop, signal handling is handled in another thread
+	// due to the length of time it can take for the cmd to complete causing synchronization issues
+	// on shutdown. Also passing a context in so that it can interrupt the cmd and exit cleanly.
+	ctx, cancelFunc := context.WithCancel(context.Background())
+	go func() {
+		select {
+		case sig := <-c.sigCh:
+			logger.Info(fmt.Sprintf("%s received, shutting down", sig))
+			cancelFunc()
+			return
+		}
+	}()
 	// The main work loop. We continually re-register our service every
 	// syncPeriod. Consul is smart enough to know when the service hasn't changed
 	// and so won't update any indices. This means we won't be causing a lot
@@ -98,22 +111,21 @@ func (c *Command) Run(args []string) int {
 	//
 	// The loop will only exit when the Pod is shut down and we receive a SIGINT.
 	for {
-		cmd := exec.Command(c.flagConsulBinary, c.consulCommand...)
+		start := time.Now()
+		cmd := exec.CommandContext(ctx, c.flagConsulBinary, c.consulCommand...)
 
 		// Run the command and record the stdout and stderr output
 		output, err := cmd.CombinedOutput()
 		if err != nil {
-			logger.Error("failed to sync service", "output", strings.TrimSpace(string(output)), "err", err)
+			logger.Error("failed to sync service", "output", strings.TrimSpace(string(output)), "err", err, "duration", time.Since(start))
 		} else {
-			logger.Info("successfully synced service", "output", strings.TrimSpace(string(output)))
+			logger.Info("successfully synced service", "output", strings.TrimSpace(string(output)), "duration", time.Since(start))
 		}
-
-		// Re-loop after syncPeriod or exit if we receive interrupt or terminate signals.
 		select {
+		// Re-loop after syncPeriod or exit if we receive interrupt or terminate signals.
 		case <-time.After(c.flagSyncPeriod):
 			continue
-		case sig := <-c.sigCh:
-			logger.Info(fmt.Sprintf("%s received, shutting down", sig))
+		case <-ctx.Done():
 			return 0
 		}
 	}

subcommand/lifecycle-sidecar/command_test.go

@@ -36,13 +36,24 @@ func testRunSignalHandling(sig os.Signal) func(*testing.T) {
 		tmpDir, configFile := createServicesTmpFile(t, servicesRegistration)
 		defer os.RemoveAll(tmpDir)
 
+		a, err := testutil.NewTestServerConfigT(t, nil)
+		require.NoError(t, err)
+		defer a.Stop()
+
 		ui := cli.NewMockUi()
 		cmd := Command{
 			UI: ui,
 		}
+
+		client, err := api.NewClient(&api.Config{
+			Address: a.HTTPAddr,
+		})
+		require.NoError(t, err)
 		// Run async because we need to kill it when the test is over.
 		exitChan := runCommandAsynchronously(&cmd, []string{
 			"-service-config", configFile,
+			"-http-addr", a.HTTPAddr,
+			"-sync-period", "1s",
 		})
 		cmd.sendSignal(sig)
 
@@ -54,6 +65,11 @@ func testRunSignalHandling(sig os.Signal) func(*testing.T) {
 			// Fail if the signal was not caught.
 			require.Fail(t, "timeout waiting for command to exit")
 		}
+		// Assert that the services were not created because the cmd has exited.
+		_, _, err = client.Agent().Service("service-id", nil)
+		require.Error(t, err)
+		_, _, err = client.Agent().Service("service-id-sidecar-proxy", nil)
+		require.Error(t, err)
 	}
 }