From 831904824cfb887699235007acf6dfbd70e8982b Mon Sep 17 00:00:00 2001 From: Chris Bednarski Date: Fri, 25 Nov 2016 10:47:05 -0800 Subject: [PATCH] Change parsing to return errors instead of silently truncating the hosts file; fixes #23 --- commands.go | 7 +++- hostfile.go | 35 ++++++++++++---- hostfile_test.go | 89 +++++++++++++++++++++++++++++------------ hostlist.go | 5 ++- hostlist_test.go | 60 +++++++++++++-------------- hostname.go | 19 +++++++-- hostname_test.go | 56 +++++++++++++++++--------- test-fixtures/hostfile2 | 2 + 8 files changed, 185 insertions(+), 88 deletions(-) create mode 100644 test-fixtures/hostfile2 diff --git a/commands.go b/commands.go index 1914783..112266c 100644 --- a/commands.go +++ b/commands.go @@ -10,6 +10,8 @@ import ( "github.com/codegangsta/cli" ) +// AnyBool checks whether a boolean CLI flag is set either globally or on this +// individual command. This provides more flexible flag parsing behavior. func AnyBool(c *cli.Context, key string) bool { return c.Bool(key) || c.GlobalBool(key) } @@ -97,7 +99,10 @@ func Add(c *cli.Context) { hostsfile := MaybeLoadHostFile(c) - hostname := NewHostname(c.Args()[0], c.Args()[1], true) + hostname, err := NewHostname(c.Args()[0], c.Args()[1], true) + if err != nil { + MaybeError(c, fmt.Sprintf("Failed to parse hosts entry: %s", err)) + } // If the command is aff instead of add then the entry should be disabled if c.Command.Name == "aff" { hostname.Enabled = false diff --git a/hostfile.go b/hostfile.go index 78aaf58..04ca657 100644 --- a/hostfile.go +++ b/hostfile.go @@ -1,6 +1,7 @@ package hostess import ( + "fmt" "io/ioutil" "os" "runtime" @@ -71,11 +72,11 @@ func TrimWS(s string) string { // (un)commented ip and one or more hostnames. For example // // 127.0.0.1 localhost mysite1 mysite2 -func ParseLine(line string) Hostlist { +func ParseLine(line string) (Hostlist, error) { var hostnames Hostlist if len(line) == 0 { - return hostnames + return hostnames, fmt.Errorf("line is blank") } // Parse leading # for disabled lines @@ -105,26 +106,44 @@ func ParseLine(line string) Hostlist { ip := words[0] domains := words[1:] - if LooksLikeIPv4(ip) || LooksLikeIPv6(ip) { - for _, v := range domains { - hostname := NewHostname(v, ip, enabled) - hostnames = append(hostnames, hostname) + // if LooksLikeIPv4(ip) || LooksLikeIPv6(ip) { + for _, v := range domains { + hostname, err := NewHostname(v, ip, enabled) + if err != nil { + return nil, err } + hostnames = append(hostnames, hostname) } + // } - return hostnames + return hostnames, nil +} + +// MustParseLine is like ParseLine but panics instead of errors. +func MustParseLine(line string) Hostlist { + hostlist, err := ParseLine(line) + if err != nil { + panic(err) + } + return hostlist } // Parse reads func (h *Hostfile) Parse() []error { var errs []error + var line = 1 for _, v := range strings.Split(string(h.data), "\n") { - for _, hostname := range ParseLine(v) { + hostnames, _ := ParseLine(v) + // if err != nil { + // log.Printf("Error parsing line %d: %s\n", line, err) + // } + for _, hostname := range hostnames { err := h.Hosts.Add(hostname) if err != nil { errs = append(errs, err) } } + line++ } return errs } diff --git a/hostfile_test.go b/hostfile_test.go index 81e2e06..8bbcb00 100644 --- a/hostfile_test.go +++ b/hostfile_test.go @@ -65,12 +65,12 @@ func TestFormatHostfile(t *testing.T) { hostfile := hostess.NewHostfile() hostfile.Path = "./hosts" - hostfile.Hosts.Add(hostess.NewHostname("localhost", "127.0.0.1", true)) - hostfile.Hosts.Add(hostess.NewHostname("ip-10-37-12-18", "127.0.1.1", true)) - hostfile.Hosts.Add(hostess.NewHostname("devsite", "127.0.0.1", true)) - hostfile.Hosts.Add(hostess.NewHostname("google.com", "8.8.8.8", false)) - hostfile.Hosts.Add(hostess.NewHostname("devsite.com", "10.37.12.18", true)) - hostfile.Hosts.Add(hostess.NewHostname("m.devsite.com", "10.37.12.18", true)) + hostfile.Hosts.Add(hostess.MustHostname("localhost", "127.0.0.1", true)) + hostfile.Hosts.Add(hostess.MustHostname("ip-10-37-12-18", "127.0.1.1", true)) + hostfile.Hosts.Add(hostess.MustHostname("devsite", "127.0.0.1", true)) + hostfile.Hosts.Add(hostess.MustHostname("google.com", "8.8.8.8", false)) + hostfile.Hosts.Add(hostess.MustHostname("devsite.com", "10.37.12.18", true)) + hostfile.Hosts.Add(hostess.MustHostname("m.devsite.com", "10.37.12.18", true)) f := string(hostfile.Format()) if f != expected { t.Errorf("Hostfile output is not formatted correctly: %s", Diff(expected, f)) @@ -87,59 +87,96 @@ func TestTrimWS(t *testing.T) { } } -func TestParseLine(t *testing.T) { - var hosts hostess.Hostlist - +func TestParseLineBlank(t *testing.T) { // Blank line - hosts = hostess.ParseLine("") + hosts, err := hostess.ParseLine("") + expected := "line is blank" + if err.Error() != expected { + t.Errorf("Expected error %q; found %q", expected, err.Error()) + } if len(hosts) > 0 { t.Error("Expected to find zero hostnames") } +} +func TestParseLineComment(t *testing.T) { // Comment - hosts = hostess.ParseLine("# The following lines are desirable for IPv6 capable hosts") + hosts, err := hostess.ParseLine("# The following lines are desirable for IPv6 capable hosts") + if err == nil { + t.Error(err) + } if len(hosts) > 0 { t.Error("Expected to find zero hostnames") } +} +func TestParseLineOneWordComment(t *testing.T) { // Single word comment - hosts = hostess.ParseLine("#blah") + hosts, err := hostess.ParseLine("#blah") + if err != nil { + t.Error(err) + } if len(hosts) > 0 { t.Error("Expected to find zero hostnames") } +} - hosts = hostess.ParseLine("#66.33.99.11 test.domain.com") - if !hosts.Contains(hostess.NewHostname("test.domain.com", "66.33.99.11", false)) || +func TestParseLineBasicHostnameComment(t *testing.T) { + hosts, err := hostess.ParseLine("#66.33.99.11 test.domain.com") + if err != nil { + t.Error(err) + } + if !hosts.Contains(hostess.MustHostname("test.domain.com", "66.33.99.11", false)) || len(hosts) != 1 { t.Error("Expected to find test.domain.com (disabled)") } +} - hosts = hostess.ParseLine("# 66.33.99.11 test.domain.com domain.com") - if !hosts.Contains(hostess.NewHostname("test.domain.com", "66.33.99.11", false)) || - !hosts.Contains(hostess.NewHostname("domain.com", "66.33.99.11", false)) || +func TestParseLineMultiHostnameComment(t *testing.T) { + hosts, err := hostess.ParseLine("# 66.33.99.11 test.domain.com domain.com") + if err != nil { + t.Error(err) + } + if !hosts.Contains(hostess.MustHostname("test.domain.com", "66.33.99.11", false)) || + !hosts.Contains(hostess.MustHostname("domain.com", "66.33.99.11", false)) || len(hosts) != 2 { t.Error("Expected to find domain.com and test.domain.com (disabled)") t.Errorf("Found %s", hosts) } +} +func TestParseLineMultiHostname(t *testing.T) { // Not Commented stuff - hosts = hostess.ParseLine("255.255.255.255 broadcasthost test.domain.com domain.com") - if !hosts.Contains(hostess.NewHostname("broadcasthost", "255.255.255.255", true)) || - !hosts.Contains(hostess.NewHostname("test.domain.com", "255.255.255.255", true)) || - !hosts.Contains(hostess.NewHostname("domain.com", "255.255.255.255", true)) || + hosts, err := hostess.ParseLine("255.255.255.255 broadcasthost test.domain.com domain.com") + if err != nil { + t.Error(err) + } + if !hosts.Contains(hostess.MustHostname("broadcasthost", "255.255.255.255", true)) || + !hosts.Contains(hostess.MustHostname("test.domain.com", "255.255.255.255", true)) || + !hosts.Contains(hostess.MustHostname("domain.com", "255.255.255.255", true)) || len(hosts) != 3 { t.Error("Expected to find broadcasthost, domain.com, and test.domain.com (enabled)") } +} +func TestParseLineIPv6A(t *testing.T) { // Ipv6 stuff - hosts = hostess.ParseLine("::1 localhost") - if !hosts.Contains(hostess.NewHostname("localhost", "::1", true)) || + hosts, err := hostess.ParseLine("::1 localhost") + if err != nil { + t.Error(err) + } + if !hosts.Contains(hostess.MustHostname("localhost", "::1", true)) || len(hosts) != 1 { t.Error("Expected to find localhost ipv6 (enabled)") } +} - hosts = hostess.ParseLine("ff02::1 ip6-allnodes") - if !hosts.Contains(hostess.NewHostname("ip6-allnodes", "ff02::1", true)) || +func TestParseLineIPv6B(t *testing.T) { + hosts, err := hostess.ParseLine("ff02::1 ip6-allnodes") + if err != nil { + t.Error(err) + } + if !hosts.Contains(hostess.MustHostname("ip6-allnodes", "ff02::1", true)) || len(hosts) != 1 { t.Error("Expected to find ip6-allnodes ipv6 (enabled)") } @@ -157,7 +194,7 @@ func TestLoadHostfile(t *testing.T) { if runtime.GOOS == "windows" { on = false } - hostname := hostess.NewHostname(domain, ip, on) + hostname := hostess.MustHostname(domain, ip, on) found := hostfile.Hosts.Contains(hostname) if !found { t.Errorf("Expected to find %#v", hostname) diff --git a/hostlist.go b/hostlist.go index c0e6396..2b0c145 100644 --- a/hostlist.go +++ b/hostlist.go @@ -179,7 +179,10 @@ func (h *Hostlist) ContainsIP(IP net.IP) bool { // Both duplicate and conflicts return errors so you are aware of them, but you // don't necessarily need to do anything about the error. func (h *Hostlist) Add(hostnamev *Hostname) error { - hostname := NewHostname(hostnamev.Domain, hostnamev.IP.String(), hostnamev.Enabled) + hostname, err := NewHostname(hostnamev.Domain, hostnamev.IP.String(), hostnamev.Enabled) + if err != nil { + return err + } for index, found := range *h { if found.Equal(hostname) { // If either hostname is enabled we will set the existing one to diff --git a/hostlist_test.go b/hostlist_test.go index 5645037..1e1a5cf 100644 --- a/hostlist_test.go +++ b/hostlist_test.go @@ -14,7 +14,7 @@ import ( func TestAddDuplicate(t *testing.T) { list := hostess.NewHostlist() - hostname := hostess.NewHostname("mysite", "1.2.3.4", false) + hostname := hostess.MustHostname("mysite", "1.2.3.4", false) err := list.Add(hostname) assert.Nil(t, err, "Expected no errors when adding a hostname for the first time") @@ -25,8 +25,8 @@ func TestAddDuplicate(t *testing.T) { } func TestAddConflict(t *testing.T) { - hostnameA := hostess.NewHostname("mysite", "1.2.3.4", true) - hostnameB := hostess.NewHostname("mysite", "5.2.3.4", false) + hostnameA := hostess.MustHostname("mysite", "1.2.3.4", true) + hostnameB := hostess.MustHostname("mysite", "5.2.3.4", false) list := hostess.NewHostlist() list.Add(hostnameA) @@ -58,8 +58,8 @@ func TestMakeSurrogateIP(t *testing.T) { func TestContainsDomainIp(t *testing.T) { hosts := hostess.NewHostlist() - hosts.Add(hostess.NewHostname(domain, ip, false)) - hosts.Add(hostess.NewHostname("google.com", "8.8.8.8", true)) + hosts.Add(hostess.MustHostname(domain, ip, false)) + hosts.Add(hostess.MustHostname("google.com", "8.8.8.8", true)) if !hosts.ContainsDomain(domain) { t.Errorf("Expected to find %s", domain) @@ -80,12 +80,12 @@ func TestContainsDomainIp(t *testing.T) { t.Errorf("Did not expect to find %s", extraneousIP) } - expectedHostname := hostess.NewHostname(domain, ip, true) + expectedHostname := hostess.MustHostname(domain, ip, true) if !hosts.Contains(expectedHostname) { t.Errorf("Expected to find %s", expectedHostname) } - extraneousHostname := hostess.NewHostname("yahoo.com", "4.3.2.1", false) + extraneousHostname := hostess.MustHostname("yahoo.com", "4.3.2.1", false) if hosts.Contains(extraneousHostname) { t.Errorf("Did not expect to find %s", extraneousHostname) } @@ -93,8 +93,8 @@ func TestContainsDomainIp(t *testing.T) { func TestFormat(t *testing.T) { hosts := hostess.NewHostlist() - hosts.Add(hostess.NewHostname(domain, ip, false)) - hosts.Add(hostess.NewHostname("google.com", "8.8.8.8", true)) + hosts.Add(hostess.MustHostname(domain, ip, false)) + hosts.Add(hostess.MustHostname("google.com", "8.8.8.8", true)) expected := `# 127.0.0.1 localhost 8.8.8.8 google.com @@ -106,8 +106,8 @@ func TestFormat(t *testing.T) { func TestRemove(t *testing.T) { hosts := hostess.NewHostlist() - hosts.Add(hostess.NewHostname(domain, ip, false)) - hosts.Add(hostess.NewHostname("google.com", "8.8.8.8", true)) + hosts.Add(hostess.MustHostname(domain, ip, false)) + hosts.Add(hostess.MustHostname("google.com", "8.8.8.8", true)) removed := hosts.Remove(1) if removed != 1 { @@ -120,7 +120,7 @@ func TestRemove(t *testing.T) { t.Errorf("Expected not to find google.com") } - hosts.Add(hostess.NewHostname(domain, "::1", enabled)) + hosts.Add(hostess.MustHostname(domain, "::1", enabled)) removed = hosts.RemoveDomain(domain) if removed != 2 { t.Error("Expected to remove 2 items") @@ -129,8 +129,8 @@ func TestRemove(t *testing.T) { func TestRemoveDomain(t *testing.T) { hosts := hostess.NewHostlist() - h1 := hostess.NewHostname("google.com", "127.0.0.1", false) - h2 := hostess.NewHostname("google.com", "::1", true) + h1 := hostess.MustHostname("google.com", "127.0.0.1", false) + h2 := hostess.MustHostname("google.com", "::1", true) hosts.Add(h1) hosts.Add(h2) @@ -159,16 +159,16 @@ func TestSort(t *testing.T) { // this is already too long. hosts := hostess.NewHostlist() - hosts.Add(hostess.NewHostname("google.com", "8.8.8.8", true)) - hosts.Add(hostess.NewHostname("google3.com", "::1", true)) - hosts.Add(hostess.NewHostname(domain, ip, false)) - hosts.Add(hostess.NewHostname("google2.com", "8.8.4.4", true)) - hosts.Add(hostess.NewHostname("blah2", "10.20.1.1", true)) - hosts.Add(hostess.NewHostname("blah3", "10.20.1.1", true)) - hosts.Add(hostess.NewHostname("blah33", "10.20.1.1", true)) - hosts.Add(hostess.NewHostname("blah", "10.20.1.1", true)) - hosts.Add(hostess.NewHostname("hostname", "127.0.1.1", true)) - hosts.Add(hostess.NewHostname("devsite", "127.0.0.1", true)) + hosts.Add(hostess.MustHostname("google.com", "8.8.8.8", true)) + hosts.Add(hostess.MustHostname("google3.com", "::1", true)) + hosts.Add(hostess.MustHostname(domain, ip, false)) + hosts.Add(hostess.MustHostname("google2.com", "8.8.4.4", true)) + hosts.Add(hostess.MustHostname("blah2", "10.20.1.1", true)) + hosts.Add(hostess.MustHostname("blah3", "10.20.1.1", true)) + hosts.Add(hostess.MustHostname("blah33", "10.20.1.1", true)) + hosts.Add(hostess.MustHostname("blah", "10.20.1.1", true)) + hosts.Add(hostess.MustHostname("hostname", "127.0.1.1", true)) + hosts.Add(hostess.MustHostname("devsite", "127.0.0.1", true)) hosts.Sort() @@ -186,8 +186,8 @@ func TestSort(t *testing.T) { func ExampleHostlist_1() { hosts := hostess.NewHostlist() - hosts.Add(hostess.NewHostname("google.com", "127.0.0.1", false)) - hosts.Add(hostess.NewHostname("google.com", "::1", true)) + hosts.Add(hostess.MustHostname("google.com", "127.0.0.1", false)) + hosts.Add(hostess.MustHostname("google.com", "::1", true)) fmt.Printf("%s\n", hosts.Format()) // Output: @@ -210,8 +210,8 @@ const hostsjson = `[ func TestDump(t *testing.T) { hosts := hostess.NewHostlist() - hosts.Add(hostess.NewHostname("google.com", "127.0.0.1", false)) - hosts.Add(hostess.NewHostname("google.com", "::1", true)) + hosts.Add(hostess.MustHostname("google.com", "127.0.0.1", false)) + hosts.Add(hostess.MustHostname("google.com", "::1", true)) expected := []byte(hostsjson) actual, _ := hosts.Dump() @@ -226,12 +226,12 @@ func TestApply(t *testing.T) { hosts := hostess.NewHostlist() hosts.Apply([]byte(hostsjson)) - hostnameA := hostess.NewHostname("google.com", "127.0.0.1", false) + hostnameA := hostess.MustHostname("google.com", "127.0.0.1", false) if !hosts.Contains(hostnameA) { t.Errorf("Expected to find %s", hostnameA.Format()) } - hostnameB := hostess.NewHostname("google.com", "::1", true) + hostnameB := hostess.MustHostname("google.com", "::1", true) if !hosts.Contains(hostnameB) { t.Errorf("Expected to find %s", hostnameB.Format()) } diff --git a/hostname.go b/hostname.go index b7529d2..687f3b2 100644 --- a/hostname.go +++ b/hostname.go @@ -8,7 +8,7 @@ import ( ) var ipv4Pattern = regexp.MustCompile(`^(?:[0-9]{1,3}\.){3}[0-9]{1,3}$`) -var ipv6Pattern = regexp.MustCompile(`^[a-z0-9:]+$`) +var ipv6Pattern = regexp.MustCompile(`^[(a-fA-F0-9){1-4}:]+$`) // LooksLikeIPv4 returns true if the IP looks like it's IPv4. This does not // validate whether the string is a valid IP address. @@ -41,10 +41,21 @@ type Hostname struct { // NewHostname creates a new Hostname struct and automatically sets the IPv6 // field based on the IP you pass in. -func NewHostname(domain, ip string, enabled bool) (hostname *Hostname) { +func NewHostname(domain, ip string, enabled bool) (*Hostname, error) { + if !LooksLikeIPv4(ip) && !LooksLikeIPv6(ip) { + return nil, fmt.Errorf("Unable to parse IP address %q", ip) + } IP := net.ParseIP(ip) - hostname = &Hostname{domain, IP, enabled, LooksLikeIPv6(ip)} - return + return &Hostname{domain, IP, enabled, LooksLikeIPv6(ip)}, nil +} + +// MustHostname calls NewHostname but panics if there is an error parsing it. +func MustHostname(domain, ip string, enabled bool) *Hostname { + hostname, err := NewHostname(domain, ip, enabled) + if err != nil { + panic(err) + } + return hostname } // Equal compares two Hostnames. Note that only the Domain and IP fields are diff --git a/hostname_test.go b/hostname_test.go index 6cd86f9..488dd5c 100644 --- a/hostname_test.go +++ b/hostname_test.go @@ -1,13 +1,14 @@ package hostess_test import ( - "github.com/cbednarski/hostess" "net" "testing" + + "github.com/cbednarski/hostess" ) func TestHostname(t *testing.T) { - h := hostess.NewHostname(domain, ip, enabled) + h := hostess.MustHostname(domain, ip, enabled) if h.Domain != domain { t.Errorf("Domain should be %s", domain) @@ -21,9 +22,9 @@ func TestHostname(t *testing.T) { } func TestEqual(t *testing.T) { - a := hostess.NewHostname("localhost", "127.0.0.1", true) - b := hostess.NewHostname("localhost", "127.0.0.1", false) - c := hostess.NewHostname("localhost", "127.0.1.1", false) + a := hostess.MustHostname("localhost", "127.0.0.1", true) + b := hostess.MustHostname("localhost", "127.0.0.1", false) + c := hostess.MustHostname("localhost", "127.0.1.1", false) if !a.Equal(b) { t.Errorf("%s and %s should be equal", a, b) @@ -34,8 +35,8 @@ func TestEqual(t *testing.T) { } func TestEqualIP(t *testing.T) { - a := hostess.NewHostname("localhost", "127.0.0.1", true) - c := hostess.NewHostname("localhost", "127.0.1.1", false) + a := hostess.MustHostname("localhost", "127.0.0.1", true) + c := hostess.MustHostname("localhost", "127.0.1.1", false) ip := net.ParseIP("127.0.0.1") if !a.EqualIP(ip) { @@ -47,23 +48,42 @@ func TestEqualIP(t *testing.T) { } func TestIsValid(t *testing.T) { - a := hostess.NewHostname("localhost", "127.0.0.1", true) - d := hostess.NewHostname("", "127.0.0.1", true) - e := hostess.NewHostname("localhost", "localhost", true) + hostname := &hostess.Hostname{ + Domain: "localhost", + IP: net.ParseIP("127.0.0.1"), + Enabled: true, + IPv6: true, + } + if !hostname.IsValid() { + t.Fatalf("%s should be a valid hostname", hostname) + } +} - if !a.IsValid() { - t.Errorf("%s should be a valid hostname", a) +func TestIsValidBlank(t *testing.T) { + hostname := &hostess.Hostname{ + Domain: "", + IP: net.ParseIP("127.0.0.1"), + Enabled: true, + IPv6: true, } - if d.IsValid() { - t.Errorf("%s should be invalid because the name is blank", d) + if hostname.IsValid() { + t.Errorf("%s should be invalid because the name is blank", hostname) + } +} +func TestIsValidBadIP(t *testing.T) { + hostname := &hostess.Hostname{ + Domain: "localhost", + IP: net.ParseIP("localhost"), + Enabled: true, + IPv6: true, } - if e.IsValid() { - t.Errorf("%s should be invalid because the ip is malformed", e) + if hostname.IsValid() { + t.Errorf("%s should be invalid because the ip is malformed", hostname) } } func TestFormatHostname(t *testing.T) { - hostname := hostess.NewHostname(domain, ip, enabled) + hostname := hostess.MustHostname(domain, ip, enabled) const exp_enabled = "127.0.0.1 localhost" if hostname.Format() != exp_enabled { @@ -78,7 +98,7 @@ func TestFormatHostname(t *testing.T) { } func TestFormatEnabled(t *testing.T) { - hostname := hostess.NewHostname(domain, ip, enabled) + hostname := hostess.MustHostname(domain, ip, enabled) const expectedOn = "(On)" if hostname.FormatEnabled() != expectedOn { t.Errorf("Expected hostname to be turned %s", expectedOn) diff --git a/test-fixtures/hostfile2 b/test-fixtures/hostfile2 new file mode 100644 index 0000000..eb20dbf --- /dev/null +++ b/test-fixtures/hostfile2 @@ -0,0 +1,2 @@ +# entries: 2361 +0.0.0.0 101com.com