Port validation and more testing

cmd/boulder-observer/main.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"errors"
 	"flag"
 	"io/ioutil"
 
@@ -12,7 +11,7 @@ import (
 
 func main() {
 	configPath := flag.String(
-		"config", "config.yaml", "Path to boulder-observer configuration file")
+		"config", "config.yml", "Path to boulder-observer configuration file")
 	flag.Parse()
 
 	configYAML, err := ioutil.ReadFile(*configPath)
@@ -22,11 +21,12 @@ func main() {
 	var config observer.ObsConf
 	err = yaml.Unmarshal(configYAML, &config)
 	if err != nil {
-		cmd.FailOnError(err, "failed to parse yaml config")
+		cmd.FailOnError(err, "failed to parse YAML config")
 	}
 
-	if config.DebugAddr == "" {
-		cmd.FailOnError(errors.New(""), "debugaddr is not defined")
+	err = config.ValidateDebugAddr()
+	if err != nil {
+		cmd.FailOnError(err, "config")
 	}
 
 	// start monitoring and logging

observer/monitor.go

@@ -32,11 +32,11 @@ func (m monitor) start() *time.Ticker {
 			case <-ticker.C:
 				result, dur := m.prober.Do(m.period)
 				statObservations.WithLabelValues(
-					m.prober.Name(), m.prober.Type(), strconv.FormatBool(result)).
+					m.prober.Name(), m.prober.Kind(), strconv.FormatBool(result)).
 					Observe(dur.Seconds())
 				m.logger.Infof(
-					"type=[%s] result=[%v] duration=[%f] name=[%s]",
-					m.prober.Type(), result, dur.Seconds(), m.prober.Name())
+					"kind=[%s] result=[%v] duration=[%f] name=[%s]",
+					m.prober.Kind(), result, dur.Seconds(), m.prober.Name())
 			}
 		}
 	}()

observer/obs_conf.go

@@ -3,6 +3,8 @@ package observer
 import (
 	"errors"
 	"fmt"
+	"regexp"
+	"strconv"
 
 	"github.com/letsencrypt/boulder/cmd"
 	blog "github.com/letsencrypt/boulder/log"
@@ -15,61 +17,78 @@ type ObsConf struct {
 	MonConfs  []*MonConf       `yaml:"monitors"`
 }
 
-// validateMonConfs calls the validate method for each of the received
-// `MonConf` objects. If an error is encountered, this is appended to a
-// slice of errors. If no `MonConf` remain, the list of errors is
-// returned along with `false`, indicating there are 0 valid `MonConf`.
-// Otherwise, the list of errors is returned to be presented to the
-// end-user, and true is returned to indicate that there is at least one
-// valid `MonConf`
+// validateMonConfs calls the validate method for each `MonConf`. If a
+// valiation error is encountered, this is appended to a slice of
+// errors. If no valid `MonConf` remain, the slice of errors is returned
+// along with `false`, indicating that observer should not start
 func (c *ObsConf) validateMonConfs() ([]error, bool) {
-	var validationErrs []error
+	var errs []error
 	for _, m := range c.MonConfs {
 		err := m.validate()
 		if err != nil {
-			validationErrs = append(validationErrs, err)
+			errs = append(errs, err)
 		}
 	}
+	if len(c.MonConfs) == len(errs) {
+		// all configured monitors are invalid, cannot continue
+		return errs, false
+	}
+	return errs, true
+}
 
-	// all configured monitors are invalid, cannot continue
-	if len(c.MonConfs) == len(validationErrs) {
-		return validationErrs, false
+// ValidateDebugAddr ensures the the `debugAddr` received by `ObsConf`
+// is properly formatted and a valid port
+func (c *ObsConf) ValidateDebugAddr() error {
+	addrExp := regexp.MustCompile("^:([[:digit:]]{1,5})$")
+	if !addrExp.MatchString(c.DebugAddr) {
+		return fmt.Errorf(
+			"invalid `debugaddr`, %q, not expected format", c.DebugAddr)
+	}
+	addrExpMatches := addrExp.FindAllStringSubmatch(c.DebugAddr, -1)
+	port, _ := strconv.Atoi(addrExpMatches[0][1])
+	if !(port > 0 && port < 65535) {
+		return fmt.Errorf(
+			"invalid `debugaddr`, %q, is not a valid port", port)
 	}
-	return validationErrs, true
+	return nil
 }
 
-// validate normalizes and validates the observer config as well as each
-// `MonConf`. If no valid `MonConf` remain, an error indicating that
-// Observer cannot be started is returned. In all instances the the
-// rationale for invalidating a 'MonConf' will logged to stderr
+// validate normalizes then validates the config received the `ObsConf`
+// and each of it's `MonConf`. If no valid `MonConf` remain, an error
+// indicating that Observer cannot be started is returned. In all
+// instances the rationale for invalidating a 'MonConf' will logged to
+// stderr
 func (c *ObsConf) validate(log blog.Logger) error {
 	if c == nil {
 		return errors.New("observer config is empty")
 	}
 
-	if len(c.MonConfs) == 0 {
-		return errors.New("observer config is invalid, 0 monitors configured")
+	// validate `debugaddr`
+	err := c.ValidateDebugAddr()
+	if err != nil {
+		return err
 	}
 
-	logErrs := func(errs []error, lenMons int) {
-		log.Errf("%d of %d monitors failed validation", len(errs), lenMons)
-		for _, err := range errs {
-			log.Errf("invalid monitor: %s", err)
-		}
+	// operator failed to provide any monitors
+	if len(c.MonConfs) == 0 {
+		return errors.New("observer config is invalid, no monitors provided")
 	}
 
 	errs, ok := c.validateMonConfs()
+	for mon, err := range errs {
+		log.Errf("monitor %q is invalid: %s", mon, err)
+	}
 
-	// if no valid `MonConf` remain, log validation errors, return error
-	if len(errs) != 0 && !ok {
-		logErrs(errs, len(c.MonConfs))
-		return fmt.Errorf("no valid mons, cannot continue")
+	if len(errs) != 0 {
+		log.Errf("%d of %d monitors failed validation", len(errs), len(c.MonConfs))
+	} else {
+		log.Info("all monitors passed validation")
 	}
 
-	// if at least one valid `MonConf` remains, only log validation
-	// errors
-	if len(errs) != 0 && ok {
-		logErrs(errs, len(c.MonConfs))
+	// if 0 `MonConfs` passed validation, return error
+	if !ok {
+		return fmt.Errorf("no valid mons, cannot continue")
 	}
+
 	return nil
 }

observer/obs_conf_test.go

@@ -0,0 +1,69 @@
+package observer
+
+import (
+	"testing"
+
+	"github.com/letsencrypt/boulder/cmd"
+	blog "github.com/letsencrypt/boulder/log"
+)
+
+func TestObsConf_ValidateDebugAddr(t *testing.T) {
+	type fields struct {
+		DebugAddr string
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		wantErr bool
+	}{
+		// valid
+		{"valid", fields{":8080"}, false},
+		// invalid
+		{"out of range high", fields{":65536"}, true},
+		{"out of range low", fields{":0"}, true},
+		{"not even a port", fields{":foo"}, true},
+		{"missing :", fields{"foo"}, true},
+		{"missing port", fields{"foo:"}, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := &ObsConf{
+				DebugAddr: tt.fields.DebugAddr,
+			}
+			if err := c.ValidateDebugAddr(); (err != nil) != tt.wantErr {
+				t.Errorf("ObsConf.ValidateDebugAddr() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestObsConf_validate(t *testing.T) {
+	type fields struct {
+		Syslog    cmd.SyslogConfig
+		DebugAddr string
+		MonConfs  []*MonConf
+	}
+	type args struct {
+		log blog.Logger
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		wantErr bool
+	}{
+		// TODO: Add test cases.
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := &ObsConf{
+				Syslog:    tt.fields.Syslog,
+				DebugAddr: tt.fields.DebugAddr,
+				MonConfs:  tt.fields.MonConfs,
+			}
+			if err := c.validate(tt.args.log); (err != nil) != tt.wantErr {
+				t.Errorf("ObsConf.validate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

observer/observer.go

@@ -18,15 +18,15 @@ var (
 			Name: "obs_monitors",
 			Help: "count of configured monitors",
 		},
-		[]string{"name", "type", "valid"},
+		[]string{"name", "kind", "valid"},
 	)
 	statObservations = prometheus.NewHistogramVec(
 		prometheus.HistogramOpts{
 			Name:    "obs_observations",
 			Help:    "time taken for a monitor to perform a request/query",
 			Buckets: metrics.InternetFacingBuckets,
 		},
-		[]string{"name", "type", "result"},
+		[]string{"name", "kind", "result"},
 	)
 )
 

observer/probes/dns/dns.go

@@ -22,8 +22,8 @@ func (p DNSProbe) Name() string {
 	return fmt.Sprintf("%s-%s-%s-%s", p.Proto, p.Server, p.QName, dns.TypeToString[p.QType])
 }
 
-// Type returns a name that uniquely identifies the monitor
-func (p DNSProbe) Type() string {
+// Kind returns a name that uniquely identifies the prober
+func (p DNSProbe) Kind() string {
 	return "DNS"
 }
 

observer/probes/dns/dns_conf.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net"
 	"regexp"
+	"strconv"
 	"strings"
 
 	p "github.com/letsencrypt/boulder/observer/probes"
@@ -49,25 +50,31 @@ func (c DNSConf) validateServer() error {
 	// ensure `server` does not contain scheme
 	if schemeExp.MatchString(c.Server) {
 		return fmt.Errorf(
-			"invalid server, %q, remove %q", c.Server,
+			"invalid `server`, %q, remove %q", c.Server,
 			strings.SplitAfter(c.Server, "://")[0])
 	}
 
-	serverExp := regexp.MustCompile("^(.*)+([[:alnum:]])+(:)([[:digit:]]{1,5})$")
+	servExp := regexp.MustCompile("^(.*)+([[:alnum:]])+(:)([[:digit:]]{1,5})$")
 	// ensure `server` contains a port
-	if !serverExp.MatchString(c.Server) {
+	if !servExp.MatchString(c.Server) {
 		return fmt.Errorf(
-			"invalid server, %q, is missing a port", c.Server)
+			"invalid `server`, %q, missing a port", c.Server)
 	}
-
-	// ensure `server` is a valid fqdn, ipv4, or ipv6 address
-	host := serverExp.FindAllStringSubmatch(c.Server, -1)[0][1]
+	// ensure the `server` port provided is a valid port
+	servExpMatches := servExp.FindAllStringSubmatch(c.Server, -1)
+	port, _ := strconv.Atoi(servExpMatches[0][4])
+	if !(port > 0 && port < 65535) {
+		return fmt.Errorf(
+			"invalid `server`, %q, is not a valid port", port)
+	}
+	// ensure `server` is a valid fqdn or ipv4/ipv6 address
+	host := servExpMatches[0][1]
 	ipv6 := net.ParseIP(host).To16()
 	ipv4 := net.ParseIP(host).To4()
 	fqdn := dns.IsFqdn(dns.Fqdn(host))
 	if ipv6 == nil && ipv4 == nil && fqdn != true {
 		return fmt.Errorf(
-			"invalid server, %q, is not an fqdn or ipv4/6 address", c.Server)
+			"invalid `server`, %q, is not an fqdn or ipv4/6 address", c.Server)
 	}
 	return nil
 }
@@ -79,7 +86,7 @@ func (c DNSConf) validateProto() error {
 		}
 	}
 	return fmt.Errorf(
-		"invalid protocol, got: %q, expected one in: %s", c.Proto, validProtos)
+		"invalid `protocol`, got: %q, expected one in: %s", c.Proto, validProtos)
 }
 
 func (c DNSConf) validateQType() error {
@@ -91,7 +98,7 @@ func (c DNSConf) validateQType() error {
 		}
 	}
 	return fmt.Errorf(
-		"invalid query_type, got: %q, expected one in %s", c.QType, q)
+		"invalid `query_type`, got: %q, expected one in %s", c.QType, q)
 }
 
 // Validate normalizes and validates the received `DNSConf`. If the
@@ -102,7 +109,7 @@ func (c DNSConf) Validate() error {
 
 	// validate `query_name`
 	if !dns.IsFqdn(dns.Fqdn(c.QName)) {
-		return fmt.Errorf("invalid query_name, %q is not an fqdn", c.QName)
+		return fmt.Errorf("invalid `query_name`, %q is not an fqdn", c.QName)
 	}
 
 	// validate `server`

observer/probes/http/http.go

@@ -18,8 +18,8 @@ func (p HTTPProbe) Name() string {
 	return fmt.Sprintf("%s-%d", p.URL, p.RCodes)
 }
 
-// Type returns the type of prober as a string
-func (p HTTPProbe) Type() string {
+// Kind returns a name that uniquely identifies the prober
+func (p HTTPProbe) Kind() string {
 	return "HTTP"
 }
 

observer/probes/prober.go

@@ -17,7 +17,7 @@ var (
 // Prober is the expected interface for Prober types
 type Prober interface {
 	Name() string
-	Type() string
+	Kind() string
 	Do(time.Duration) (bool, time.Duration)
 }