From b08c28c03872a4548aa7b121be5461738db61245 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Mon, 1 Dec 2014 15:44:44 +0000 Subject: [PATCH] Cope with duplicate name->IP entries The zone DB no longer considers duplciates of (IP, name); it will reject duplicates of (IP, name, ident), but this ultimately doesn't matter since those are all the fields it records anyway. The HTTP interface treats these duplicates as trivial success (i.e., it acts as if it's idempotent, which it is). This changes the behaviour for removals: if (IP, name) is recorded for two different containers, removing one will leave the other extant. --- nameserver/http.go | 7 ++----- nameserver/http_test.go | 17 +++++++++++------ nameserver/testing_utils.go | 4 ++-- nameserver/zone.go | 17 +++++++++-------- nameserver/zone_test.go | 25 ++++++++++++++++--------- 5 files changed, 40 insertions(+), 30 deletions(-) diff --git a/nameserver/http.go b/nameserver/http.go index 7ab19550..615a00a2 100644 --- a/nameserver/http.go +++ b/nameserver/http.go @@ -61,15 +61,12 @@ func ListenHttp(domain string, db Zone, port int) { if dns.IsSubDomain(domain, name) { Info.Printf("Adding %s -> %s", name, ipStr) if err = db.AddRecord(ident, name, ip); err != nil { - if dup, ok := err.(DuplicateError); !ok { + if _, ok := err.(DuplicateError); !ok { httpErrorAndLog( Error, w, "Internal error", http.StatusInternalServerError, "Unexpected error from DB: %s", err) return - } else if dup.Ident != ident { - http.Error(w, err.Error(), http.StatusConflict) - return - } // else we are golden + } // oh, I already know this. whatever. } } else { Info.Printf("Ignoring name %s, not in %s", name, domain) diff --git a/nameserver/http_test.go b/nameserver/http_test.go index 71874e7d..fb8a6970 100644 --- a/nameserver/http_test.go +++ b/nameserver/http_test.go @@ -55,27 +55,32 @@ func TestHttp(t *testing.T) { assertNoErr(t, err) assertStatus(t, resp.StatusCode, http.StatusOK, "http success response for duplicate add") - // Now try adding the same address again with a different ident - should fail + // Now try adding the same address again with a different ident -- + // again should be fine otherUrl := fmt.Sprintf("http://localhost:%d/name/%s/%s", port, "other", addrParts[0]) resp, err = genForm("PUT", otherUrl, url.Values{"fqdn": {successTestName}, "local_ip": {dockerIP}, "routing_prefix": {addrParts[1]}}) assertNoErr(t, err) - assertStatus(t, resp.StatusCode, http.StatusConflict, "http response") + assertStatus(t, resp.StatusCode, http.StatusOK, "http response") // Delete the address resp, err = genForm("DELETE", addrUrl, nil) assertNoErr(t, err) assertStatus(t, resp.StatusCode, http.StatusOK, "http response") - // Check that the address is not there now. + // Check that the address is still resolvable. _, err = zone.LookupLocal(successTestName) - assertErrorType(t, err, (*LookupError)(nil), "nonexistent lookup") + assertNoErr(t, err) - // Delete the address again, it should accept this - resp, err = genForm("DELETE", addrUrl, nil) + // Delete the address record mentioning the other container + resp, err = genForm("DELETE", otherUrl, nil) assertNoErr(t, err) assertStatus(t, resp.StatusCode, http.StatusOK, "http response") + // Check that the address is gone + _, err = zone.LookupLocal(successTestName) + assertErrorType(t, err, (*LookupError)(nil), "fully-removed address") + // Would like to shut down the http server at the end of this test // but it's complicated. // See https://groups.google.com/forum/#!topic/golang-nuts/vLHWa5sHnCE diff --git a/nameserver/testing_utils.go b/nameserver/testing_utils.go index 6183d5f8..b018143b 100644 --- a/nameserver/testing_utils.go +++ b/nameserver/testing_utils.go @@ -19,7 +19,7 @@ func assertStatus(t *testing.T, got int, wanted int, desc string) { func assertErrorInterface(t *testing.T, got interface{}, wanted interface{}, desc string) { if got == nil { - t.Fatalf("Expected %s but got nil", reflect.TypeOf(wanted).Elem()) + t.Fatalf("Expected %s but got nil (%s)", reflect.TypeOf(wanted).Elem(), desc) } gotT, wantedT := reflect.TypeOf(got), reflect.TypeOf(wanted).Elem() if !gotT.Implements(wantedT) { @@ -29,7 +29,7 @@ func assertErrorInterface(t *testing.T, got interface{}, wanted interface{}, des func assertErrorType(t *testing.T, got interface{}, wanted interface{}, desc string) { if got == nil { - t.Fatalf("Expected %s but got nil", reflect.TypeOf(wanted).Elem()) + t.Fatalf("Expected %s but got nil (%s)", reflect.TypeOf(wanted).Elem(), desc) } gotT, wantedT := reflect.TypeOf(got), reflect.TypeOf(wanted).Elem() if gotT != wantedT { diff --git a/nameserver/zone.go b/nameserver/zone.go index db3ba489..44c194b9 100644 --- a/nameserver/zone.go +++ b/nameserver/zone.go @@ -34,13 +34,10 @@ func (ops LookupError) Error() string { } type DuplicateError struct { - Name string - IP net.IP - Ident string } -func (err DuplicateError) Error() string { - return "Duplicate " + err.Name + "," + err.IP.String() + " for identity " + err.Ident +func (dup DuplicateError) Error() string { + return "Tried to add a duplicate entry" } func (zone *ZoneDb) indexOf(match func(Record) bool) int { @@ -79,8 +76,10 @@ func (zone *ZoneDb) AddRecord(ident string, name string, ip net.IP) error { defer zone.mx.Unlock() fqdn := dns.Fqdn(name) if index := zone.indexOf( - func(r Record) bool { return r.Name == fqdn && r.IP.Equal(ip) }); index != -1 { - return DuplicateError{fqdn, ip, zone.recs[index].Ident} + func(r Record) bool { + return r.Name == fqdn && r.IP.Equal(ip) && r.Ident == ident + }); index != -1 { + return DuplicateError{} } zone.recs = append(zone.recs, Record{ident, fqdn, ip}) return nil @@ -90,7 +89,9 @@ func (zone *ZoneDb) DeleteRecord(ident string, ip net.IP) error { zone.mx.Lock() defer zone.mx.Unlock() if index := zone.indexOf( - func(r Record) bool { return r.Ident == ident && r.IP.Equal(ip) }); index == -1 { + func(r Record) bool { + return r.Ident == ident && r.IP.Equal(ip) + }); index == -1 { return LookupError(ident) } else { zone.recs = append(zone.recs[:index], zone.recs[index+1:]...) diff --git a/nameserver/zone_test.go b/nameserver/zone_test.go index 68b6aca1..3cb80f56 100644 --- a/nameserver/zone_test.go +++ b/nameserver/zone_test.go @@ -7,9 +7,10 @@ import ( func TestZone(t *testing.T) { var ( - containerID = "deadbeef" - successTestName = "test1.weave." - testAddr1 = "10.0.2.1/24" + containerID = "deadbeef" + otherContainerID = "cowjuice" + successTestName = "test1.weave." + testAddr1 = "10.0.2.1/24" ) var zone = new(ZoneDb) @@ -40,14 +41,20 @@ func TestZone(t *testing.T) { t.Fatal("Unexpected result for", ip, foundName) } - // Now try to add the same address again err = zone.AddRecord(containerID, successTestName, ip) assertErrorType(t, err, (*DuplicateError)(nil), "duplicate add") - // Now delete the record + err = zone.AddRecord(otherContainerID, successTestName, ip) + // Delete the record for the original container err = zone.DeleteRecord(containerID, ip) assertNoErr(t, err) + _, err = zone.LookupLocal(successTestName) + assertNoErr(t, err) + + err = zone.DeleteRecord(otherContainerID, ip) + assertNoErr(t, err) + // Check that the address is not there now. _, err = zone.LookupLocal(successTestName) assertErrorType(t, err, (*LookupError)(nil), "after deleting record") @@ -59,10 +66,10 @@ func TestZone(t *testing.T) { func TestDeleteFor(t *testing.T) { var ( - id = "foobar" - name = "foo.weave." - addr1 = "10.1.2.3/24" - addr2 = "10.2.7.8/24" + id = "foobar" + name = "foo.weave." + addr1 = "10.1.2.3/24" + addr2 = "10.2.7.8/24" ) zone := new(ZoneDb) for _, addr := range []string{addr1, addr2} {