Allow timeout to be configurable, use duration to define backoffs

client.go

@@ -96,16 +96,32 @@ func (c KeywhizHTTPClient) Logger() *logrus.Entry {
 
 // NewClient produces a ready-to-use client struct given client config and
 // CA file with the list of trusted certificate authorities.
-func NewClient(cfg *ClientConfig, caFile string, serverURL *url.URL, timeout time.Duration, logger *logrus.Entry, metricsHandle *sqmetrics.SquareMetrics) (client Client, err error) {
+func NewClient(cfg *ClientConfig, caFile string, serverURL *url.URL, logger *logrus.Entry, metricsHandle *sqmetrics.SquareMetrics) (client Client, err error) {
 	logger = logger.WithField("logger", "kwfs_client")
+
+	timeout, err := time.ParseDuration(cfg.Timeout)
+	if err != nil {
+		return &KeywhizHTTPClient{}, fmt.Errorf("bad timeout value '%s': %+v\n", cfg.Timeout, err)
+	}
+
+	minBackoff, err := time.ParseDuration(cfg.MinBackoff)
+	if err != nil {
+		return &KeywhizHTTPClient{}, fmt.Errorf("bad min backoff value '%s': %+v\n", cfg.MinBackoff, err)
+	}
+
+	maxBackoff, err := time.ParseDuration(cfg.MaxBackoff)
+	if err != nil {
+		return &KeywhizHTTPClient{}, fmt.Errorf("bad max backoff value '%s': %+v\n", cfg.MaxBackoff, err)
+	}
+
 	params := httpClientParams{
 		CertFile:   cfg.Cert,
 		KeyFile:    cfg.Key,
 		CaBundle:   caFile,
 		timeout:    timeout,
 		maxRetries: int(cfg.MaxRetries),
-		minBackoff: time.Duration(cfg.MinBackoff) * time.Millisecond,
-		maxBackoff: time.Duration(cfg.MaxBackoff) * time.Millisecond,
+		minBackoff: minBackoff,
+		maxBackoff: maxBackoff,
 	}
 
 	failCount := metrics.GetOrRegisterCounter("runtime.server.fails", metricsHandle.Registry)

client_test.go

@@ -21,7 +21,6 @@ import (
 	"net/url"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/square/go-sq-metrics"
@@ -40,6 +39,9 @@ func defaultClientConfig() *ClientConfig {
 		Key:        clientKey,
 		Cert:       clientCert,
 		MaxRetries: 1,
+		Timeout:    "1s",
+		MinBackoff: "1ms",
+		MaxBackoff: "10ms",
 	}
 }
 
@@ -50,7 +52,7 @@ func TestClientCallsServer(t *testing.T) {
 	defer server.Close()
 
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	secrets, err := client.SecretList()
@@ -76,7 +78,7 @@ func TestClientCallsServer(t *testing.T) {
 
 func TestClientRebuild(t *testing.T) {
 	serverURL, _ := url.Parse("http://dummy:8080")
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	http1 := client.(*KeywhizHTTPClient).httpClient
@@ -107,7 +109,7 @@ func TestClientCallsServerErrors(t *testing.T) {
 	defer server.Close()
 
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	secrets, err := client.SecretList()
@@ -166,7 +168,7 @@ func TestClientCallsServerIntermittentErrors(t *testing.T) {
 	serverURL, _ := url.Parse(server.URL)
 	cfg := defaultClientConfig()
 	cfg.MaxRetries = 2
-	client, err := NewClient(cfg, testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(cfg, testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	secrets, err := client.SecretList()
@@ -193,7 +195,7 @@ func TestClientCorruptedResponses(t *testing.T) {
 	defer server.Close()
 
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	_, err = client.SecretList()
@@ -212,7 +214,7 @@ func TestClientParsingError(t *testing.T) {
 	defer server.Close()
 
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	secrets, err := client.SecretList()
@@ -234,7 +236,7 @@ func TestClientServerStatusSuccess(t *testing.T) {
 	defer server.Close()
 
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	_, err = client.(*KeywhizHTTPClient).ServerStatus()
@@ -244,7 +246,7 @@ func TestClientServerStatusSuccess(t *testing.T) {
 func TestClientServerFailure(t *testing.T) {
 	server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	_, err = client.(*KeywhizHTTPClient).ServerStatus()
@@ -273,7 +275,7 @@ func TestNewClientFailure(t *testing.T) {
 	cfg := defaultClientConfig()
 	cfg.Cert = clientConfigs[clientName].Cert
 	cfg.Key = clientConfigs[clientName].Key
-	_, err = NewClient(cfg, config.CaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	_, err = NewClient(cfg, config.CaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	assert.NotNil(t, err)
 }
 
@@ -292,7 +294,7 @@ func TestDuplicateFilenames(t *testing.T) {
 	defer server.Close()
 
 	serverURL, _ := url.Parse(server.URL)
-	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, time.Second, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
+	client, err := NewClient(defaultClientConfig(), testCaFile, serverURL, logrus.NewEntry(logrus.New()), &sqmetrics.SquareMetrics{})
 	require.Nil(t, err)
 
 	_, err = client.SecretList()

config.go

@@ -26,37 +26,39 @@ import (
 
 // Config is the main yaml configuration file passed to the keysync binary
 type Config struct {
-	ClientsDir    string     `yaml:"client_directory"`      // A directory of configuration files
-	SecretsDir    string     `yaml:"secrets_directory"`     // The directory secrets will be written to
-	CaFile        string     `yaml:"ca_file"`               // The CA to trust (PEM)
-	YamlExt       string     `yaml:"yaml_ext"`              // The filename extension of the yaml config files
-	PollInterval  string     `yaml:"poll_interval"`         // If specified, poll at the given interval, otherwise, exit after syncing
-	MaxRetries    uint16     `yaml:"max_retries"`           // If specified, retry each HTTP call after non-200 response
-	MinBackoff    int        `yaml:"min_backoff_in_millis"` // If specified, wait time before first retry, otherwise, use default.
-	MaxBackoff    int        `yaml:"max_backoff_in_millis"` // If specified, max wait time before retries, otherwise, use default.
-	Server        string     `yaml:"server"`                // The server to connect to (host:port)
-	Debug         bool       `yaml:"debug"`                 // Enable debugging output
-	DefaultUser   string     `yaml:"default_user"`          // Default user to own files
-	DefaultGroup  string     `yaml:"default_group"`         // Default group to own files
-	PasswdFile    string     `yaml:"passwd_file"`           // /etc/passwd, for uid lookups
-	GroupFile     string     `yaml:"group_file"`            // /etc/groups, for gid lookups
-	APIPort       uint16     `yaml:"api_port"`              // Port for API to listen on
-	SentryDSN     string     `yaml:"sentry_dsn"`            // Sentry DSN
-	FsType        Filesystem `yaml:"filesystem_type"`       // Enforce writing this type of filesystem. Use value from statfs.
-	ChownFiles    bool       `yaml:"chown_files"`           // Do we chown files? Set to false when running without CAP_CHOWN.
-	MetricsPrefix string     `yaml:"metrics_prefix"`        // Prefix metric names with this
+	ClientsDir    string     `yaml:"client_directory"`  // A directory of configuration files
+	SecretsDir    string     `yaml:"secrets_directory"` // The directory secrets will be written to
+	CaFile        string     `yaml:"ca_file"`           // The CA to trust (PEM)
+	YamlExt       string     `yaml:"yaml_ext"`          // The filename extension of the yaml config files
+	PollInterval  string     `yaml:"poll_interval"`     // If specified, poll at the given interval, otherwise, exit after syncing
+	ClientTimeout string     `yaml:"client_timeout"`    // If specified, timeout client connections after specified duration, otherwise use default.
+	MinBackoff    string     `yaml:"min_backoff"`       // If specified, wait time before first retry, otherwise, use default.
+	MaxBackoff    string     `yaml:"max_backoff"`       // If specified, max wait time before retries, otherwise, use default.
+	MaxRetries    uint16     `yaml:"max_retries"`       // If specified, retry each HTTP call after non-200 response
+	Server        string     `yaml:"server"`            // The server to connect to (host:port)
+	Debug         bool       `yaml:"debug"`             // Enable debugging output
+	DefaultUser   string     `yaml:"default_user"`      // Default user to own files
+	DefaultGroup  string     `yaml:"default_group"`     // Default group to own files
+	PasswdFile    string     `yaml:"passwd_file"`       // /etc/passwd, for uid lookups
+	GroupFile     string     `yaml:"group_file"`        // /etc/groups, for gid lookups
+	APIPort       uint16     `yaml:"api_port"`          // Port for API to listen on
+	SentryDSN     string     `yaml:"sentry_dsn"`        // Sentry DSN
+	FsType        Filesystem `yaml:"filesystem_type"`   // Enforce writing this type of filesystem. Use value from statfs.
+	ChownFiles    bool       `yaml:"chown_files"`       // Do we chown files? Set to false when running without CAP_CHOWN.
+	MetricsPrefix string     `yaml:"metrics_prefix"`    // Prefix metric names with this
 }
 
 // The ClientConfig describes a single Keywhiz client.  There are typically many of these per keysync instance.
 type ClientConfig struct {
-	Key        string `yaml:"key"`                   // Mandatory: Path to PEM key to use
-	Cert       string `yaml:"cert"`                  // Optional: PEM Certificate (If cert isn't in key file)
-	User       string `yaml:"user"`                  // Optional: User and Group are defaults for files without metadata
-	DirName    string `yaml:"directory"`             // Optional: What directory under SecretsDir this client is in. Defaults to the client name.
-	Group      string `yaml:"group"`                 // Optional: If unspecified, the global defaults are used.
-	MaxRetries uint16 `yaml:"max_retries"`           // Optional: retry each HTTP call after non-200 response. Defaults to Keysync's config.
-	MinBackoff int    `yaml:"min_backoff_in_millis"` // Optional: wait time before first retry. Defaults to Keysync's config.
-	MaxBackoff int    `yaml:"max_backoff_in_millis"` // Optional: max wait time before retries. Defaults to Keysync's config.
+	Key        string `yaml:"key"`         // Mandatory: Path to PEM key to use
+	Cert       string `yaml:"cert"`        // Optional: PEM Certificate (If cert isn't in key file)
+	User       string `yaml:"user"`        // Optional: User and Group are defaults for files without metadata
+	DirName    string `yaml:"directory"`   // Optional: What directory under SecretsDir this client is in. Defaults to the client name.
+	Group      string `yaml:"group"`       // Optional: If unspecified, the global defaults are used.
+	MaxRetries uint16 `yaml:"max_retries"` // Optional: retry each HTTP call after non-200 response. Defaults to Keysync's config.
+	Timeout    string `yaml:"timeout"`     // Optional: timeout HTTP calls. Defaults to Keysync's config.
+	MinBackoff string `yaml:"min_backoff"` // Optional: wait time before first retry. Defaults to Keysync's config.
+	MaxBackoff string `yaml:"max_backoff"` // Optional: max wait time before retries. Defaults to Keysync's config.
 }
 
 // LoadConfig loads the "global" keysync configuration file.  This would generally be called on startup.
@@ -87,12 +89,16 @@ func LoadConfig(configFile string) (*Config, error) {
 		config.MaxRetries = 1
 	}
 
-	if config.MinBackoff < 1 {
-		config.MinBackoff = 100
+	if config.ClientTimeout == "" {
+		config.ClientTimeout = "60s"
 	}
 
-	if config.MaxBackoff < 1 {
-		config.MaxBackoff = 10000
+	if config.MinBackoff == "" {
+		config.MinBackoff = "100ms"
+	}
+
+	if config.MaxBackoff == "" {
+		config.MaxBackoff = "10000ms"
 	}
 
 	return &config, nil
@@ -140,17 +146,21 @@ func (config *Config) LoadClients() (map[string]ClientConfig, error) {
 }
 
 func (c *ClientConfig) setDefaults(cfg *Config) {
-	if c.MinBackoff < 1 {
+	if c.MinBackoff == "" {
 		c.MinBackoff = cfg.MinBackoff
 	}
 
-	if c.MaxBackoff < 1 {
+	if c.MaxBackoff == "" {
 		c.MaxBackoff = cfg.MaxBackoff
 	}
 
 	if c.MaxRetries < 1 {
 		c.MaxRetries = cfg.MaxRetries
 	}
+
+	if c.Timeout == "" {
+		c.Timeout = cfg.ClientTimeout
+	}
 }
 
 func (c *ClientConfig) validate() error {

config_test.go

@@ -90,14 +90,16 @@ func TestConfigLoadClientsSuccess(t *testing.T) {
 	newAssert.Equal("test-user", client.User)
 	newAssert.Equal("test-group", client.Group)
 	// defaults inherited from kesync's config
-	newAssert.Equal(23, client.MinBackoff)
-	newAssert.Equal(87, client.MaxBackoff)
+	newAssert.Equal("23ms", client.MinBackoff)
+	newAssert.Equal("87ms", client.MaxBackoff)
+	newAssert.Equal("60s", client.Timeout)
 	newAssert.Equal(uint16(1), client.MaxRetries)
 
 	client, ok = clients["withbackoff"]
 	newAssert.True(ok)
-	newAssert.Equal(123, client.MinBackoff)
-	newAssert.Equal(987, client.MaxBackoff)
+	newAssert.Equal("123ms", client.MinBackoff)
+	newAssert.Equal("987ms", client.MaxBackoff)
+	newAssert.Equal("20s", client.Timeout)
 	newAssert.Equal(uint16(4), client.MaxRetries)
 }
 

fixtures/clients/withbackoff.yaml

@@ -3,5 +3,6 @@ withbackoff:
     key: client1.key
     cert: client1.crt
     max_retries: 4
-    min_backoff_in_millis: 123
-    max_backoff_in_millis: 987
+    timeout: 20s
+    min_backoff: 123ms
+    max_backoff: 987ms

fixtures/configs/test-config.yaml

@@ -12,5 +12,5 @@ passwd_file : 'fixtures/ownership/passwd'
 group_file: 'fixtures/ownership/group'
 api_port: 31738
 poll_interval: 60s
-min_backoff_in_millis: 23
-max_backoff_in_millis: 87
+min_backoff: 23ms
+max_backoff: 87ms

syncer.go

@@ -215,7 +215,7 @@ func (s *Syncer) LoadClients() error {
 // buildClient collects the configuration and builds a client.  Most of this code should probably be refactored ito NewClient
 func (s *Syncer) buildClient(name string, clientConfig ClientConfig, metricsHandle *sqmetrics.SquareMetrics) (*syncerEntry, error) {
 	clientLogger := s.logger.WithField("client", name)
-	client, err := NewClient(&clientConfig, s.config.CaFile, s.server, time.Minute, clientLogger, metricsHandle)
+	client, err := NewClient(&clientConfig, s.config.CaFile, s.server, clientLogger, metricsHandle)
 	if err != nil {
 		return nil, err
 	}

syncer_test.go

@@ -71,14 +71,26 @@ func TestSyncerBuildClient(t *testing.T) {
 	assert.Equal(t, entry.ClientConfig, client1)
 
 	// Test misconfigured clients
-	entry, err = syncer.buildClient("missingkey", ClientConfig{DirName: "missingkey", Cert: "fixtures/clients/client4.crt"}, metricsForTest())
+	cfg := defaultClientConfig()
+	cfg.DirName = "missingkey"
+	cfg.Cert = "fixtures/clients/client4.crt"
+	cfg.Key = ""
+	entry, err = syncer.buildClient("missingkey", *cfg, metricsForTest())
 	require.NotNil(t, err)
 
-	entry, err = syncer.buildClient("missingcert", ClientConfig{DirName: "missingcert", Key: "fixtures/clients/client4.key"}, metricsForTest())
+	cfg = defaultClientConfig()
+	cfg.DirName = "missingcert"
+	cfg.Cert = ""
+	cfg.Key = "fixtures/clients/client4.key"
+	entry, err = syncer.buildClient("missingcert", *cfg, metricsForTest())
 	require.NotNil(t, err)
 
 	// The syncer currently handles clients configured with missing mountpoints
-	entry, err = syncer.buildClient("missingcert", ClientConfig{Key: "fixtures/clients/client4.key", Cert: "fixtures/clients/client4.crt"}, metricsForTest())
+	cfg = defaultClientConfig()
+	cfg.DirName = "valid"
+	cfg.Cert = "fixtures/clients/client4.crt"
+	cfg.Key = "fixtures/clients/client4.key"
+	entry, err = syncer.buildClient("missingcert", *cfg, metricsForTest())
 	require.Nil(t, err)
 }