From fd1d20e61527093f669acdf6173eac5987e03564 Mon Sep 17 00:00:00 2001 From: "Jason E. Aten" Date: Sun, 11 Sep 2016 21:49:06 -0500 Subject: [PATCH] etcd/auth: fix range handling bugs. Test 15, counting from zero, in TestGetMergedPerms in etcd/auth/range_perm_cache_test.go, was trying incorrectly assert that [a, b) merged with [b, "") should be [a, b). Added a test specifically for this. This patch fixes the incorrect larger test and the bugs in the code that it was hiding. Fixes #6359 --- auth/range_perm_cache.go | 27 +++++++++++++++++++-------- auth/range_perm_cache_test.go | 8 ++++++-- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/auth/range_perm_cache.go b/auth/range_perm_cache.go index 108ee961238a..d20be1394f5d 100644 --- a/auth/range_perm_cache.go +++ b/auth/range_perm_cache.go @@ -22,19 +22,24 @@ import ( "github.com/coreos/etcd/mvcc/backend" ) -// isSubset returns true if a is a subset of b +// isSubset returns true if a is a subset of b. +// If a is a prefix of b, then a is a subset of b. +// Given intervals [a1,a2) and [b1,b2), is +// the a interval a subset of b? func isSubset(a, b *rangePerm) bool { switch { case len(a.end) == 0 && len(b.end) == 0: // a, b are both keys return bytes.Equal(a.begin, b.begin) case len(b.end) == 0: - // b is a key, a is a range + // b is a key, a is a range (even a prefix range has infinite membership) return false case len(a.end) == 0: - return 0 <= bytes.Compare(a.begin, b.begin) && bytes.Compare(a.begin, b.end) <= 0 + // a is a key, b is a range. need b1 <= a1 and a1 < b2 + return bytes.Compare(b.begin, a.begin) <= 0 && bytes.Compare(a.begin, b.end) < 0 default: - return 0 <= bytes.Compare(a.begin, b.begin) && bytes.Compare(a.end, b.end) <= 0 + // both are ranges. need b1 <= a1 and a2 <= b2 + return bytes.Compare(b.begin, a.begin) <= 0 && bytes.Compare(a.end, b.end) <= 0 } } @@ -88,12 +93,18 @@ func mergeRangePerms(perms []*rangePerm) []*rangePerm { i := 0 for i < len(perms) { begin, next := i, i - for next+1 < len(perms) && bytes.Compare(perms[next].end, perms[next+1].begin) != -1 { + for next+1 < len(perms) && bytes.Compare(perms[next].end, perms[next+1].begin) >= 0 { next++ } - - merged = append(merged, &rangePerm{begin: perms[begin].begin, end: perms[next].end}) - + // don't merge ["a", "b") with ["b", ""), because perms[next+1].end is empty. + if next != begin && len(perms[next].end) > 0 { + merged = append(merged, &rangePerm{begin: perms[begin].begin, end: perms[next].end}) + } else { + merged = append(merged, perms[begin]) + if next != begin { + merged = append(merged, perms[next]) + } + } i = next + 1 } diff --git a/auth/range_perm_cache_test.go b/auth/range_perm_cache_test.go index b5451efa3261..08b580b2aad9 100644 --- a/auth/range_perm_cache_test.go +++ b/auth/range_perm_cache_test.go @@ -46,6 +46,10 @@ func TestGetMergedPerms(t *testing.T) { []*rangePerm{{[]byte("a"), []byte("b")}}, []*rangePerm{{[]byte("a"), []byte("b")}}, }, + { + []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("")}}, + []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("")}}, + }, { []*rangePerm{{[]byte("a"), []byte("b")}, {[]byte("b"), []byte("c")}}, []*rangePerm{{[]byte("a"), []byte("c")}}, @@ -106,7 +110,7 @@ func TestGetMergedPerms(t *testing.T) { }, { []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("c")}, {[]byte("b"), []byte("")}, {[]byte("c"), []byte("")}, {[]byte("d"), []byte("")}}, - []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("c")}, {[]byte("d"), []byte("")}}, + []*rangePerm{{[]byte("a"), []byte("")}, {[]byte("b"), []byte("c")}, {[]byte("c"), []byte("")}, {[]byte("d"), []byte("")}}, }, // duplicate ranges { @@ -123,7 +127,7 @@ func TestGetMergedPerms(t *testing.T) { for i, tt := range tests { result := mergeRangePerms(tt.params) if !isPermsEqual(result, tt.want) { - t.Errorf("#%d: result=%q, want=%q", i, result, tt.want) + t.Fatalf("#%d: result=%q, want=%q", i, result, tt.want) } } }