agent: restart template runner on retry for unlimited retries (hashic…

…orp#11775) * agent: restart template runner on retry for unlimited retries * template: log error message early * template: delegate retries back to template if param is set to true * agent: add and use the new template config stanza * agent: fix panic, fix existing tests * changelog: add changelog entry * agent: add tests for exit_on_retry_failure * agent: properly check on agent exit cases, add separate tests for missing key vs missing secrets * agent: add note on difference between missing key vs missing secret * docs: add docs for template_config * Update website/content/docs/agent/template-config.mdx Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> * Update website/content/docs/agent/template-config.mdx Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> * Update website/content/docs/agent/template-config.mdx Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com> * Update website/content/docs/agent/template-config.mdx Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com> * Update website/content/docs/agent/template-config.mdx Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com> * docs: fix exit_on_retry_failure, fix Functionality section * docs: update interaction title * template: add internal note on behavior for persist case * docs: update agent, template, and template-config docs * docs: update agent docs on retry stanza * Apply suggestions from code review Co-authored-by: Jim Kalafut <jkalafut@hashicorp.com> Co-authored-by: Theron Voran <tvoran@users.noreply.github.com> * Update changelog/11775.txt Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com> * agent/test: rename expectExit to expectExitFromError * agent/test: add check on early exits on the happy path * Update website/content/docs/agent/template-config.mdx Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com> Co-authored-by: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com> Co-authored-by: Jim Kalafut <jkalafut@hashicorp.com> Co-authored-by: Theron Voran <tvoran@users.noreply.github.com> Co-authored-by: Brian Kassouf <briankassouf@users.noreply.github.com>
jartek · Sep 11, 2021 · 30f99cd · 30f99cd
1 parent 9b1c220
commit 30f99cd
Show file tree

Hide file tree

Showing 13 changed files with 571 additions and 23 deletions.
diff --git a/changelog/11775.txt b/changelog/11775.txt
@@ -0,0 +1,9 @@
+```release-note:change
+agent: Errors in the template engine will no longer cause agent to exit unless
+explicitly defined to do so. A new configuration parameter,
+`exit_on_retry_failure`, within the new top-level stanza, `template_config`, can
+be set to `true` in order to cause agent to exit. Note that for agent to exit if
+`template.error_on_missing_key` is set to `true`, `exit_on_retry_failure` must
+be also set to `true`. Otherwise, the template engine will log an error but then
+restart its internal runner.
+```
diff --git a/command/agent/cache/api_proxy.go b/command/agent/cache/api_proxy.go
@@ -119,6 +119,10 @@ func (ap *APIProxy) Send(ctx context.Context, req *SendRequest) (*SendResponse,
 	}
 	client.SetToken(req.Token)
 
+	// Derive and set a logger for the client
+	clientLogger := ap.logger.Named("client")
+	client.SetLogger(clientLogger)
+
 	// http.Transport will transparently request gzip and decompress the response, but only if
 	// the client doesn't manually set the header. Removing any Accept-Encoding header allows the
 	// transparent compression to occur.

diff --git a/command/agent/config/config.go b/command/agent/config/config.go
@@ -22,11 +22,12 @@ import (
 type Config struct {
 	*configutil.SharedConfig `hcl:"-"`
 
-	AutoAuth      *AutoAuth                  `hcl:"auto_auth"`
-	ExitAfterAuth bool                       `hcl:"exit_after_auth"`
-	Cache         *Cache                     `hcl:"cache"`
-	Vault         *Vault                     `hcl:"vault"`
-	Templates     []*ctconfig.TemplateConfig `hcl:"templates"`
+	AutoAuth       *AutoAuth                  `hcl:"auto_auth"`
+	ExitAfterAuth  bool                       `hcl:"exit_after_auth"`
+	Cache          *Cache                     `hcl:"cache"`
+	Vault          *Vault                     `hcl:"vault"`
+	TemplateConfig *TemplateConfig            `hcl:"template_config"`
+	Templates      []*ctconfig.TemplateConfig `hcl:"templates"`
 }
 
 func (c *Config) Prune() {
@@ -116,6 +117,11 @@ type Sink struct {
 	Config     map[string]interface{}
 }
 
+// TemplateConfig defines global behaviors around template
+type TemplateConfig struct {
+	ExitOnRetryFailure bool `hcl:"exit_on_retry_failure"`
+}
+
 func NewConfig() *Config {
 	return &Config{
 		SharedConfig: new(configutil.SharedConfig),
@@ -179,6 +185,10 @@ func LoadConfig(path string) (*Config, error) {
 		return nil, fmt.Errorf("error parsing 'cache':%w", err)
 	}
 
+	if err := parseTemplateConfig(result, list); err != nil {
+		return nil, fmt.Errorf("error parsing 'template_config': %w", err)
+	}
+
 	if err := parseTemplates(result, list); err != nil {
 		return nil, fmt.Errorf("error parsing 'template': %w", err)
 	}
@@ -553,6 +563,31 @@ func parseSinks(result *Config, list *ast.ObjectList) error {
 	return nil
 }
 
+func parseTemplateConfig(result *Config, list *ast.ObjectList) error {
+	name := "template_config"
+
+	templateConfigList := list.Filter(name)
+	if len(templateConfigList.Items) == 0 {
+		return nil
+	}
+
+	if len(templateConfigList.Items) > 1 {
+		return fmt.Errorf("at most one %q block is allowed", name)
+	}
+
+	// Get our item
+	item := templateConfigList.Items[0]
+
+	var cfg TemplateConfig
+	if err := hcl.DecodeObject(&cfg, item.Val); err != nil {
+		return err
+	}
+
+	result.TemplateConfig = &cfg
+
+	return nil
+}
+
 func parseTemplates(result *Config, list *ast.ObjectList) error {
 	name := "template"
 

diff --git a/command/agent/config/config_test.go b/command/agent/config/config_test.go
@@ -535,6 +535,59 @@ func TestLoadConfigFile_AgentCache_PersistMissingType(t *testing.T) {
 	}
 }
 
+func TestLoadConfigFile_TemplateConfig(t *testing.T) {
+
+	testCases := map[string]struct {
+		fixturePath            string
+		expectedTemplateConfig TemplateConfig
+	}{
+		"set-true": {
+			"./test-fixtures/config-template_config.hcl",
+			TemplateConfig{
+				ExitOnRetryFailure: true,
+			},
+		},
+		"empty": {
+			"./test-fixtures/config-template_config-empty.hcl",
+			TemplateConfig{
+				ExitOnRetryFailure: false,
+			},
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			config, err := LoadConfig(tc.fixturePath)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			expected := &Config{
+				SharedConfig: &configutil.SharedConfig{},
+				Vault: &Vault{
+					Address: "http://127.0.0.1:1111",
+					Retry: &Retry{
+						NumRetries: 5,
+					},
+				},
+				TemplateConfig: &tc.expectedTemplateConfig,
+				Templates: []*ctconfig.TemplateConfig{
+					{
+						Source:      pointerutil.StringPtr("/path/on/disk/to/template.ctmpl"),
+						Destination: pointerutil.StringPtr("/path/on/disk/where/template/will/render.txt"),
+					},
+				},
+			}
+
+			config.Prune()
+			if diff := deep.Equal(config, expected); diff != nil {
+				t.Fatal(diff)
+			}
+		})
+	}
+
+}
+
 // TestLoadConfigFile_Template tests template definitions in Vault Agent
 func TestLoadConfigFile_Template(t *testing.T) {
 	testCases := map[string]struct {

diff --git a/command/agent/config/test-fixtures/config-template_config-empty.hcl b/command/agent/config/test-fixtures/config-template_config-empty.hcl
@@ -0,0 +1,13 @@
+vault {
+  address = "http://127.0.0.1:1111"
+  retry {
+    num_retries = 5
+  }
+}
+
+template_config {}
+
+template {
+  source      = "/path/on/disk/to/template.ctmpl"
+  destination = "/path/on/disk/where/template/will/render.txt"
+}
diff --git a/command/agent/config/test-fixtures/config-template_config.hcl b/command/agent/config/test-fixtures/config-template_config.hcl
@@ -0,0 +1,15 @@
+vault {
+  address = "http://127.0.0.1:1111"
+  retry {
+    num_retries = 5
+  }
+}
+
+template_config {
+  exit_on_retry_failure = true
+}
+
+template {
+  source      = "/path/on/disk/to/template.ctmpl"
+  destination = "/path/on/disk/where/template/will/render.txt"
+}
diff --git a/command/agent/template/template.go b/command/agent/template/template.go
@@ -172,8 +172,20 @@ func (ts *Server) Run(ctx context.Context, incoming chan string, templates []*ct
 			}
 
 		case err := <-ts.runner.ErrCh:
+			ts.logger.Error("template server error", "error", err.Error())
 			ts.runner.StopImmediately()
-			return fmt.Errorf("template server: %w", err)
+
+			// Return after stopping the runner if exit on retry failure was
+			// specified
+			if ts.config.AgentConfig.TemplateConfig != nil && ts.config.AgentConfig.TemplateConfig.ExitOnRetryFailure {
+				return fmt.Errorf("template server: %w", err)
+			}
+
+			ts.runner, err = manager.NewRunner(runnerConfig, false)
+			if err != nil {
+				return fmt.Errorf("template server failed to create: %w", err)
+			}
+			go ts.runner.Start()
 
 		case <-ts.runner.TemplateRenderedCh():
 			// A template has been rendered, figure out what to do
@@ -253,6 +265,20 @@ func newRunnerConfig(sc *ServerConfig, templates ctconfig.TemplateConfigs) (*ctc
 	// that need to be fixed for 1.7x/1.8
 	if sc.AgentConfig.Cache != nil && sc.AgentConfig.Cache.Persist != nil && len(sc.AgentConfig.Listeners) != 0 {
 		attempts = 0
+
+		// If we don't want exit on template retry failure (i.e. unlimited
+		// retries), let consul-template handle retry and backoff logic.
+		//
+		// Note: This is a fixed value (12) that ends up being a multiplier to
+		// retry.num_retires (i.e. 12 * N total retries per runner restart).
+		// Since we are performing retries indefinitely this base number helps
+		// prevent agent from spamming Vault if retry.num_retries is set to a
+		// low value by forcing exponential backoff to be high towards the end
+		// of retries during the process.
+		if sc.AgentConfig.TemplateConfig != nil && !sc.AgentConfig.TemplateConfig.ExitOnRetryFailure {
+			attempts = ctconfig.DefaultRetryAttempts
+		}
+
 		scheme := "unix://"
 		if sc.AgentConfig.Listeners[0].Type == "tcp" {
 			scheme = "https://"

diff --git a/command/agent/template/template_test.go b/command/agent/template/template_test.go
@@ -352,8 +352,9 @@ func TestServerRun(t *testing.T) {
 	}
 
 	testCases := map[string]struct {
-		templateMap map[string]*templateTest
-		expectError bool
+		templateMap        map[string]*templateTest
+		expectError        bool
+		exitOnRetryFailure bool
 	}{
 		"simple": {
 			templateMap: map[string]*templateTest{
@@ -363,7 +364,8 @@ func TestServerRun(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
+			expectError:        false,
+			exitOnRetryFailure: false,
 		},
 		"multiple": {
 			templateMap: map[string]*templateTest{
@@ -403,7 +405,8 @@ func TestServerRun(t *testing.T) {
 					},
 				},
 			},
-			expectError: false,
+			expectError:        false,
+			exitOnRetryFailure: false,
 		},
 		"bad secret": {
 			templateMap: map[string]*templateTest{
@@ -413,7 +416,8 @@ func TestServerRun(t *testing.T) {
 					},
 				},
 			},
-			expectError: true,
+			expectError:        true,
+			exitOnRetryFailure: true,
 		},
 		"missing key": {
 			templateMap: map[string]*templateTest{
@@ -424,7 +428,8 @@ func TestServerRun(t *testing.T) {
 					},
 				},
 			},
-			expectError: true,
+			expectError:        true,
+			exitOnRetryFailure: true,
 		},
 		"permission denied": {
 			templateMap: map[string]*templateTest{
@@ -434,7 +439,8 @@ func TestServerRun(t *testing.T) {
 					},
 				},
 			},
-			expectError: true,
+			expectError:        true,
+			exitOnRetryFailure: true,
 		},
 	}
 
@@ -458,6 +464,9 @@ func TestServerRun(t *testing.T) {
 							NumRetries: 3,
 						},
 					},
+					TemplateConfig: &config.TemplateConfig{
+						ExitOnRetryFailure: tc.exitOnRetryFailure,
+					},
 				},
 				LogLevel:      hclog.Trace,
 				LogWriter:     hclog.DefaultOutput,