From 55daf3b80ecdd288742012d55b4aceaa0d68e059 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 26 Jun 2015 12:23:21 +0200 Subject: [PATCH 1/2] Don't wrongly identify endpoint addresses only due to equal IP Before this patch the endpoint IP was used to identify endpoint addresses. This leads to wrong unification of endpoints of different pods having the same IP (e.g. non container IP in case of Mesos). This patch takes the EndpointAddress.targetRef.UID into consideration as well. --- pkg/api/endpoints/util.go | 63 +++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/pkg/api/endpoints/util.go b/pkg/api/endpoints/util.go index 0b440bb1bc4..ceeca51f6b0 100644 --- a/pkg/api/endpoints/util.go +++ b/pkg/api/endpoints/util.go @@ -24,6 +24,7 @@ import ( "sort" "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/types" "k8s.io/kubernetes/pkg/util" ) @@ -33,31 +34,35 @@ import ( // form for things like comparison. The result is a newly allocated slice. func RepackSubsets(subsets []api.EndpointSubset) []api.EndpointSubset { // First map each unique port definition to the sets of hosts that - // offer it. The sets of hosts must be de-duped, using IP as the key. - type ipString string - allAddrs := map[ipString]*api.EndpointAddress{} + // offer it. The sets of hosts must be de-duped, using IP+UID as the key. + type addressKey struct { + ip string + uid types.UID + } + allAddrs := map[addressKey]*api.EndpointAddress{} portsToAddrs := map[api.EndpointPort]addressSet{} for i := range subsets { for j := range subsets[i].Ports { epp := &subsets[i].Ports[j] for k := range subsets[i].Addresses { epa := &subsets[i].Addresses[k] - ipstr := ipString(epa.IP) - // Accumulate the most "complete" address we can. - if allAddrs[ipstr] == nil { + ak := addressKey{ip: epa.IP} + if epa.TargetRef != nil { + ak.uid = epa.TargetRef.UID + } + // Accumulate the address. + if allAddrs[ak] == nil { // Make a copy so we don't write to the // input args of this function. p := &api.EndpointAddress{} *p = *epa - allAddrs[ipstr] = p - } else if allAddrs[ipstr].TargetRef == nil { - allAddrs[ipstr].TargetRef = epa.TargetRef + allAddrs[ak] = p } // Remember that this port maps to this address. if _, found := portsToAddrs[*epp]; !found { portsToAddrs[*epp] = addressSet{} } - portsToAddrs[*epp].Insert(allAddrs[ipstr]) + portsToAddrs[*epp].Insert(allAddrs[ak]) } } } @@ -104,18 +109,32 @@ func hashAddresses(addrs addressSet) string { for k := range addrs { slice = append(slice, k) } - sort.Sort(addrPtrsByIP(slice)) + sort.Sort(addrPtrsByIpAndUID(slice)) hasher := md5.New() util.DeepHashObject(hasher, slice) return hex.EncodeToString(hasher.Sum(nil)[0:]) } -type addrPtrsByIP []*api.EndpointAddress +func LessEndpointAddress(a, b *api.EndpointAddress) bool { + ipComparison := bytes.Compare([]byte(a.IP), []byte(b.IP)) + if ipComparison != 0 { + return ipComparison < 0 + } + if (b.TargetRef == nil) { + return false + } + if (a.TargetRef == nil) { + return true + } + return a.TargetRef.UID < b.TargetRef.UID +} -func (sl addrPtrsByIP) Len() int { return len(sl) } -func (sl addrPtrsByIP) Swap(i, j int) { sl[i], sl[j] = sl[j], sl[i] } -func (sl addrPtrsByIP) Less(i, j int) bool { - return bytes.Compare([]byte(sl[i].IP), []byte(sl[j].IP)) < 0 +type addrPtrsByIpAndUID []*api.EndpointAddress + +func (sl addrPtrsByIpAndUID) Len() int { return len(sl) } +func (sl addrPtrsByIpAndUID) Swap(i, j int) { sl[i], sl[j] = sl[j], sl[i] } +func (sl addrPtrsByIpAndUID) Less(i, j int) bool { + return LessEndpointAddress(sl[i], sl[j]) } // SortSubsets sorts an array of EndpointSubset objects in place. For ease of @@ -123,7 +142,7 @@ func (sl addrPtrsByIP) Less(i, j int) bool { func SortSubsets(subsets []api.EndpointSubset) []api.EndpointSubset { for i := range subsets { ss := &subsets[i] - sort.Sort(addrsByIP(ss.Addresses)) + sort.Sort(addrsByIpAndUID(ss.Addresses)) sort.Sort(portsByHash(ss.Ports)) } sort.Sort(subsetsByHash(subsets)) @@ -146,12 +165,12 @@ func (sl subsetsByHash) Less(i, j int) bool { return bytes.Compare(h1, h2) < 0 } -type addrsByIP []api.EndpointAddress +type addrsByIpAndUID []api.EndpointAddress -func (sl addrsByIP) Len() int { return len(sl) } -func (sl addrsByIP) Swap(i, j int) { sl[i], sl[j] = sl[j], sl[i] } -func (sl addrsByIP) Less(i, j int) bool { - return bytes.Compare([]byte(sl[i].IP), []byte(sl[j].IP)) < 0 +func (sl addrsByIpAndUID) Len() int { return len(sl) } +func (sl addrsByIpAndUID) Swap(i, j int) { sl[i], sl[j] = sl[j], sl[i] } +func (sl addrsByIpAndUID) Less(i, j int) bool { + return LessEndpointAddress(&sl[i], &sl[j]) } type portsByHash []api.EndpointPort From 79e54c26793495be7f8b63888ce1192e9fe270b4 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Mon, 29 Jun 2015 12:13:40 +0200 Subject: [PATCH 2/2] Add unit tests for RepackSubsets to take pod UID into consideration --- pkg/api/endpoints/util.go | 6 +-- pkg/api/endpoints/util_test.go | 95 ++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/pkg/api/endpoints/util.go b/pkg/api/endpoints/util.go index ceeca51f6b0..58e661e95f2 100644 --- a/pkg/api/endpoints/util.go +++ b/pkg/api/endpoints/util.go @@ -36,7 +36,7 @@ func RepackSubsets(subsets []api.EndpointSubset) []api.EndpointSubset { // First map each unique port definition to the sets of hosts that // offer it. The sets of hosts must be de-duped, using IP+UID as the key. type addressKey struct { - ip string + ip string uid types.UID } allAddrs := map[addressKey]*api.EndpointAddress{} @@ -120,10 +120,10 @@ func LessEndpointAddress(a, b *api.EndpointAddress) bool { if ipComparison != 0 { return ipComparison < 0 } - if (b.TargetRef == nil) { + if b.TargetRef == nil { return false } - if (a.TargetRef == nil) { + if a.TargetRef == nil { return true } return a.TargetRef.UID < b.TargetRef.UID diff --git a/pkg/api/endpoints/util_test.go b/pkg/api/endpoints/util_test.go index 57dcaeb2e34..3f047db6da9 100644 --- a/pkg/api/endpoints/util_test.go +++ b/pkg/api/endpoints/util_test.go @@ -22,8 +22,14 @@ import ( "github.com/davecgh/go-spew/spew" "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/types" ) +func podRef(uid string) *api.ObjectReference { + ref := api.ObjectReference{UID: types.UID(uid)} + return &ref +} + func TestPackSubsets(t *testing.T) { // The downside of table-driven tests is that some things have to live outside the table. fooObjRef := api.ObjectReference{Name: "foo"} @@ -56,6 +62,26 @@ func TestPackSubsets(t *testing.T) { Addresses: []api.EndpointAddress{{IP: "1.2.3.4"}}, Ports: []api.EndpointPort{{Port: 111}}, }}, + }, { + name: "one set, one ip, one UID, one port", + given: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + expect: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + }, { + name: "one set, one ip, empty UID, one port", + given: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + expect: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, }, { name: "one set, two ips, one port", given: []api.EndpointSubset{{ @@ -135,6 +161,19 @@ func TestPackSubsets(t *testing.T) { Addresses: []api.EndpointAddress{{IP: "1.2.3.4"}}, Ports: []api.EndpointPort{{Port: 111}, {Port: 222}}, }}, + }, { + name: "two sets, dup ip, dup uids, two ports", + given: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }, { + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 222}}, + }}, + expect: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}, {Port: 222}}, + }}, }, { name: "two sets, two ips, dup port", given: []api.EndpointSubset{{ @@ -148,6 +187,62 @@ func TestPackSubsets(t *testing.T) { Addresses: []api.EndpointAddress{{IP: "1.2.3.4"}, {IP: "5.6.7.8"}}, Ports: []api.EndpointPort{{Port: 111}}, }}, + }, { + name: "two set, dup ip, two uids, dup ports", + given: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }, { + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-2")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + expect: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{ + {IP: "1.2.3.4", TargetRef: podRef("uid-1")}, + {IP: "1.2.3.4", TargetRef: podRef("uid-2")}, + }, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + }, { + name: "two set, dup ip, with and without uid, dup ports", + given: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "1.2.3.4"}}, + Ports: []api.EndpointPort{{Port: 111}}, + }, { + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-2")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + expect: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{ + {IP: "1.2.3.4"}, + {IP: "1.2.3.4", TargetRef: podRef("uid-2")}, + }, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + }, { + name: "two sets, two ips, two dup ip with uid, dup port, wrong order", + given: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{{IP: "5.6.7.8"}}, + Ports: []api.EndpointPort{{Port: 111}}, + }, { + Addresses: []api.EndpointAddress{{IP: "5.6.7.8", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }, { + Addresses: []api.EndpointAddress{{IP: "1.2.3.4", TargetRef: podRef("uid-1")}}, + Ports: []api.EndpointPort{{Port: 111}}, + }, { + Addresses: []api.EndpointAddress{{IP: "1.2.3.4"}}, + Ports: []api.EndpointPort{{Port: 111}}, + }}, + expect: []api.EndpointSubset{{ + Addresses: []api.EndpointAddress{ + {IP: "1.2.3.4"}, + {IP: "1.2.3.4", TargetRef: podRef("uid-1")}, + {IP: "5.6.7.8"}, + {IP: "5.6.7.8", TargetRef: podRef("uid-1")}, + }, + Ports: []api.EndpointPort{{Port: 111}}, + }}, }, { name: "two sets, two ips, two ports", given: []api.EndpointSubset{{