Graceful handling of misconfigure password for dyn (#470)

* Graceful handling of misconfigure password for dyn

If a bad password is given for provider "dyn" then the next
login attempt is at least 30minutes apart. This prevents an
account from being suspended.

Improve validation of flags for dyn provider. Add test for
ValidateConfig() and Config.String()

Also add --dyn-min-ttl option which sets the lower limit
of a record's TTL. Ignored if 0 (the default).

* docs: add graceful handling of misconfiguration to changelog

CHANGELOG.md

@@ -1,3 +1,4 @@
+  - Graceful handling of misconfigure password for dyn provider (#470) @jvassev
   - Don't log sensitive data on start (#463) @jvassev
   - Google: Improve logging to help trace misconfigurations (#388) @stealthybox
   - AWS: In addition to the one best public hosted zone, records will be added to all matching private hosted zones (#356) @coreypobrien

main.go

@@ -121,13 +121,14 @@ func main() {
 	case "dyn":
 		p, err = provider.NewDynProvider(
 			provider.DynConfig{
-				DomainFilter: domainFilter,
+				DomainFilter:  domainFilter,
-				ZoneIDFilter: zoneIDFilter,
+				ZoneIDFilter:  zoneIDFilter,
-				DryRun:       cfg.DryRun,
+				DryRun:        cfg.DryRun,
-				CustomerName: cfg.DynCustomerName,
+				CustomerName:  cfg.DynCustomerName,
-				Username:     cfg.DynUsername,
+				Username:      cfg.DynUsername,
-				Password:     cfg.DynPassword,
+				Password:      cfg.DynPassword,
-				AppVersion:   externaldns.Version,
+				MinTTLSeconds: cfg.DynMinTTLSeconds,
+				AppVersion:    externaldns.Version,
 			},
 		)
 	case "inmemory":

pkg/apis/externaldns/types.go

@@ -61,6 +61,7 @@ type Config struct {
 	DynCustomerName      string
 	DynUsername          string
 	DynPassword          string
+	DynMinTTLSeconds     int
 	InMemoryZones        []string
 	Policy               string
 	Registry             string
@@ -172,6 +173,8 @@ func (cfg *Config) ParseFlags(args []string) error {
 	app.Flag("dyn-customer-name", "When using the Dyn provider, specify the Customer Name").Default("").StringVar(&cfg.DynCustomerName)
 	app.Flag("dyn-username", "When using the Dyn provider, specify the Username").Default("").StringVar(&cfg.DynUsername)
 	app.Flag("dyn-password", "When using the Dyn provider, specify the pasword").Default("").StringVar(&cfg.DynPassword)
+	app.Flag("dyn-min-ttl", "Minimal TTL (in seconds) for records. This value will be used if the provided TTL for a service/ingress is lower than this.").IntVar(&cfg.DynMinTTLSeconds)
+
 	app.Flag("inmemory-zone", "Provide a list of pre-configured zones for the inmemory provider; specify multiple times for multiple zones (optional)").Default("").StringsVar(&cfg.InMemoryZones)
 
 	// Flags related to policies

pkg/apis/externaldns/types_test.go

@@ -18,6 +18,7 @@ package externaldns
 
 import (
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -222,3 +223,15 @@ func restoreEnv(t *testing.T, originalEnv map[string]string) {
 		require.NoError(t, os.Setenv(k, v))
 	}
 }
+
+func TestPasswordsNotLogged(t *testing.T) {
+	cfg := Config{
+		DynPassword:          "dyn-pass",
+		InfobloxWapiPassword: "infoblox-pass",
+	}
+
+	s := cfg.String()
+
+	assert.False(t, strings.Contains(s, "dyn-pass"))
+	assert.False(t, strings.Contains(s, "infoblox-pass"))
+}

pkg/apis/externaldns/validation/validation.go

@@ -52,5 +52,18 @@ func ValidateConfig(cfg *externaldns.Config) error {
 			return errors.New("no Infoblox WAPI password specified")
 		}
 	}
+
+	if cfg.Provider == "dyn" {
+		if cfg.DynUsername == "" {
+			return errors.New("no Dyn username specified")
+		}
+		if cfg.DynCustomerName == "" {
+			return errors.New("no Dyn customer name specified")
+		}
+
+		if cfg.DynMinTTLSeconds < 0 {
+			return errors.New("TTL specified for Dyn is negative")
+		}
+	}
 	return nil
 }

pkg/apis/externaldns/validation/validation_test.go

@@ -63,3 +63,56 @@ func newValidConfig(t *testing.T) *externaldns.Config {
 
 	return cfg
 }
+
+func addRequiredFieldsForDyn(cfg *externaldns.Config) {
+	cfg.LogFormat = "json"
+	cfg.Sources = []string{"ingress"}
+	cfg.Provider = "dyn"
+}
+
+func TestValidateBadDynConfig(t *testing.T) {
+	badConfigs := []*externaldns.Config{
+		{},
+		{
+			// only username
+			DynUsername: "test",
+		},
+		{
+			// only customer name
+			DynCustomerName: "test",
+		},
+		{
+			// negative timeout
+			DynUsername:      "test",
+			DynCustomerName:  "test",
+			DynMinTTLSeconds: -1,
+		},
+	}
+
+	for _, cfg := range badConfigs {
+		addRequiredFieldsForDyn(cfg)
+		err := ValidateConfig(cfg)
+		assert.NotNil(t, err, "Configuration %+v should NOT have passed validation", cfg)
+	}
+}
+
+func TestValidateGoodDynConfig(t *testing.T) {
+	goodConfigs := []*externaldns.Config{
+		{
+			DynUsername:      "test",
+			DynCustomerName:  "test",
+			DynMinTTLSeconds: 600,
+		},
+		{
+			DynUsername:      "test",
+			DynCustomerName:  "test",
+			DynMinTTLSeconds: 0,
+		},
+	}
+
+	for _, cfg := range goodConfigs {
+		addRequiredFieldsForDyn(cfg)
+		err := ValidateConfig(cfg)
+		assert.Nil(t, err, "Configuration should be valid, got this error instead", err)
+	}
+}

provider/dyn.go

@@ -19,6 +19,7 @@ package provider
 import (
 	"fmt"
 	"os"
+	"strconv"
 	"strings"
 	"time"
 
@@ -37,6 +38,12 @@ const (
 	// may be made configurable in the future but 20K records seems like enough for a few zones
 	cacheMaxSize = 20000
 
+	// two consecutive bad logins happen at least this many seconds appart
+	// While it is easy to get the username right, misconfiguring the password
+	// can get account blocked. Exit(1) is not a good solution
+	// as k8s will restart the pod and another login attempt will be made
+	badLoginMinIntervalSeconds = 30 * 60
+
 	// this prefix must be stripped from resource links before feeding them to dynect.Client.Do()
 	restAPIPrefix = "/REST/"
 )
@@ -60,17 +67,21 @@ func (c *cache) Put(link string, ep *endpoint.Endpoint) {
 
 	c.contents[link] = &entry{
 		ep:      ep,
-		expires: int64(time.Now().Unix()) + int64(ep.RecordTTL),
+		expires: unixNow() + int64(ep.RecordTTL),
 	}
 }
 
+func unixNow() int64 {
+	return int64(time.Now().Unix())
+}
+
 func (c *cache) Get(link string) *endpoint.Endpoint {
 	result, ok := c.contents[link]
 	if !ok {
 		return nil
 	}
 
-	now := int64(time.Now().Unix())
+	now := unixNow()
 
 	if result.expires < now {
 		delete(c.contents, link)
@@ -82,20 +93,22 @@ func (c *cache) Get(link string) *endpoint.Endpoint {
 
 // DynConfig hold connection parameters to dyn.com and interanl state
 type DynConfig struct {
-	DomainFilter DomainFilter
+	DomainFilter  DomainFilter
-	ZoneIDFilter ZoneIDFilter
+	ZoneIDFilter  ZoneIDFilter
-	DryRun       bool
+	DryRun        bool
-	CustomerName string
+	CustomerName  string
-	Username     string
+	Username      string
-	Password     string
+	Password      string
-	AppVersion   string
+	MinTTLSeconds int
-	DynVersion   string
+	AppVersion    string
+	DynVersion    string
 }
 
 // DynProvider is the actual interface impl.
 type dynProviderState struct {
 	DynConfig
-	Cache *cache
+	Cache              *cache
+	LastLoginErrorTime int64
 }
 
 // ZoneChange is missing from dynect: https://help.dyn.com/get-zone-changeset-api/
@@ -166,13 +179,17 @@ func filterAndFixLinks(links []string, filter DomainFilter) []string {
 	return result
 }
 
-func fixMissingTTL(ttl endpoint.TTL) string {
+func fixMissingTTL(ttl endpoint.TTL, minTTLSeconds int) string {
 	i := dynDefaultTTL
 	if ttl.IsConfigured() {
-		i = int(ttl)
+		if int(ttl) < minTTLSeconds {
+			i = minTTLSeconds
+		} else {
+			i = int(ttl)
+		}
 	}
 
-	return fmt.Sprintf("%d", i)
+	return strconv.Itoa(i)
 }
 
 // merge produces a singe list of records that can be used as a replacement.
@@ -320,7 +337,6 @@ func (d *dynProviderState) buildLinkToRecord(ep *endpoint.Endpoint) string {
 	}
 
 	if matchingZone == "" {
-		fmt.Printf("no zone")
 		// no matching zone, ignore
 		return ""
 	}
@@ -337,6 +353,12 @@ func (d *dynProviderState) buildLinkToRecord(ep *endpoint.Endpoint) string {
 // This method also stores the DynAPI version.
 // Don't user the dynect.Client.Login()
 func (d *dynProviderState) login() (*dynect.Client, error) {
+	if d.LastLoginErrorTime != 0 {
+		secondsSinceLastError := unixNow() - d.LastLoginErrorTime
+		if secondsSinceLastError < badLoginMinIntervalSeconds {
+			return nil, fmt.Errorf("will not attempt an API call as the last login failure occurred just %ds ago", secondsSinceLastError)
+		}
+	}
 	client := dynect.NewClient(d.CustomerName)
 
 	var req = dynect.LoginBlock{
@@ -348,9 +370,11 @@ func (d *dynProviderState) login() (*dynect.Client, error) {
 
 	err := client.Do("POST", "Session", req, &resp)
 	if err != nil {
+		d.LastLoginErrorTime = unixNow()
 		return nil, err
 	}
 
+	d.LastLoginErrorTime = 0
 	client.Token = resp.Data.Token
 
 	// this is the only change from the original
@@ -371,7 +395,7 @@ func (d *dynProviderState) buildRecordRequest(ep *endpoint.Endpoint) (string, *d
 	}
 
 	record := dynect.RecordRequest{
-		TTL:   fixMissingTTL(ep.RecordTTL),
+		TTL:   fixMissingTTL(ep.RecordTTL, d.MinTTLSeconds),
 		RData: *endpointToRecord(ep),
 	}
 	return link, &record

provider/dyn_test.go

@@ -255,10 +255,13 @@ func TestDyn_filterAndFixLinks(t *testing.T) {
 }
 
 func TestDyn_fixMissingTTL(t *testing.T) {
-	assert.Equal(t, fmt.Sprintf("%v", dynDefaultTTL), fixMissingTTL(endpoint.TTL(0)))
+	assert.Equal(t, fmt.Sprintf("%v", dynDefaultTTL), fixMissingTTL(endpoint.TTL(0), 0))
 
 	// nothing to fix
-	assert.Equal(t, "111", fixMissingTTL(endpoint.TTL(111)))
+	assert.Equal(t, "111", fixMissingTTL(endpoint.TTL(111), 25))
+
+	// apply min TTL
+	assert.Equal(t, "1992", fixMissingTTL(endpoint.TTL(111), 1992))
 }
 
 func TestDyn_cachePut(t *testing.T) {