-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More Mount Conflict Detection #2919
Changes from 13 commits
ff650e5
af30967
d848929
cf07549
e8f330e
f3136f0
b0fbfe8
bf02212
0f36271
6836698
314a032
23aa611
c03b662
e2c0f63
5b60f6b
51c948f
d70bcd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ type Router struct { | |
mountUUIDCache *radix.Tree | ||
mountAccessorCache *radix.Tree | ||
tokenStoreSaltFunc func() (*salt.Salt, error) | ||
|
||
// storagePrefix maps the prefix used for storage (ala the BarrierView) | ||
// to the backend. This is used to map a key back into the backend that owns it. | ||
// For example, logical/uuid1/foobar -> secrets/ (kv backend) + foobar | ||
|
@@ -201,14 +200,46 @@ func (r *Router) MatchingMountByAccessor(mountAccessor string) *MountEntry { | |
// MatchingMount returns the mount prefix that would be used for a path | ||
func (r *Router) MatchingMount(path string) string { | ||
r.l.RLock() | ||
mount, _, ok := r.root.LongestPrefix(path) | ||
var mount = r.matchingMountInternal(path) | ||
r.l.RUnlock() | ||
return mount | ||
} | ||
|
||
func (r *Router) matchingMountInternal(path string) string { | ||
mount, _, ok := r.root.LongestPrefix(path) | ||
if !ok { | ||
return "" | ||
} | ||
return mount | ||
} | ||
|
||
// matchingPrefixInternal returns a mount prefix that a path may be a part of | ||
func (r *Router) matchingPrefixInternal(path string) string { | ||
var existing string = "" | ||
fn := func(existing_path string, _v interface{}) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this function executes at all wouldn't it mean that we've found a prefix? Is the |
||
if strings.HasPrefix(existing_path, path) { | ||
existing = existing_path | ||
return true | ||
} | ||
return false | ||
} | ||
r.root.WalkPrefix(path, fn) | ||
return existing | ||
} | ||
|
||
// MountConflict determines if there are potential path conflicts | ||
func (r *Router) MountConflict(path string) string { | ||
r.l.RLock() | ||
defer r.l.RUnlock() | ||
if exact_match := r.matchingMountInternal(path); exact_match != "" { | ||
return exact_match | ||
} | ||
if prefix_match := r.matchingPrefixInternal(path); prefix_match != "" { | ||
return prefix_match | ||
} | ||
return "" | ||
} | ||
|
||
// MatchingStorageView returns the storageView used for a path | ||
func (r *Router) MatchingStorageView(path string) *BarrierView { | ||
r.l.RLock() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,11 @@ func TestRouter_Mount(t *testing.T) { | |
t.Fatalf("err: %v", err) | ||
} | ||
|
||
meUUID, err = uuid.GenerateUUID() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this to be placed just above the definition of subMountEntry? |
||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
if path := r.MatchingMount("prod/aws/foo"); path != "prod/aws/" { | ||
t.Fatalf("bad: %s", path) | ||
} | ||
|
@@ -162,6 +167,24 @@ func TestRouter_Mount(t *testing.T) { | |
if len(n.Paths) != 1 || n.Paths[0] != "foo" { | ||
t.Fatalf("bad: %v", n.Paths) | ||
} | ||
|
||
subMountEntry := &MountEntry{ | ||
Path: "prod/", | ||
UUID: meUUID, | ||
Accessor: "prodaccessor", | ||
} | ||
|
||
if r.MountConflict("prod/aws/") == "" { | ||
t.Fatalf("bad: prod/aws/") | ||
} | ||
|
||
err = r.Mount(n, "prod/", subMountEntry, view) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a comment here as to why this should pass? |
||
if err != nil { | ||
t.Fatalf("err: %v", err) | ||
} | ||
if r.MountConflict("prod/test") == "" { | ||
t.Fatalf("bad: prod/test/") | ||
} | ||
} | ||
|
||
func TestRouter_MountCredential(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nitpick][Optional] We could do a
defer r.l.RUnlock()
andreturn r.matchingMountInternal(path)
and avoid the local varmount
.