Allow underscores at the start of directories in file backend. #3479
Conversation
| names[i] = name + "/" | ||
| } else { | ||
| if name[0] == '_' { |
There was a problem hiding this comment.
Why do we strip off underscores in this case?
There was a problem hiding this comment.
The way the file backend works right now is that it uses underscores to differentiate between a file and a directory since vault's storage can use the same name for both.
Which as I'm typing this makes me realize something, so stand by on the review until I fix something that this broke :-)
There was a problem hiding this comment.
Actually, I didn't break it, I just thought I might have. Added tests that show that it's working as intended.
Basically a file gets an underscore prepended to it so that the names on a filesystem don't collide between a same-named directory and file. (This does mean that this can still be defeated by e.g. creating a directory name with a double underscore matching another file that starts with a single underscore, but, c'est la vie). So during listing, because the user-facing paths shouldn't know about this, it gets stripped off. Since that underscore is only added to actual keys (files), and not to directories, we only strip off the underscore if what we're looking at is is a file. The earlier behavior would strip it off unilaterally, which was incorrect.
|
@calvn @vishalnayak Can you re-review in light of f177912 ? |
physical/couchdb/couchdb.go
Outdated
| @@ -227,6 +242,10 @@ func (m *CouchDBBackend) Delete(key string) error { | |||
| func (m *CouchDBBackend) List(prefix string) ([]string, error) { | |||
| defer metrics.MeasureSince([]string{"couchdb", "list"}, time.Now()) | |||
|
|
|||
| if m.prefixed { | |||
| prefix = "$" + prefix | |||
There was a problem hiding this comment.
Can we use another prefix other than "$"? If there's a tool that supports storage migration, for instance, some underlying storages might not support having symbols as part of the path (though I can't think any off the top of my head) and make migration/data manipulation non-trivial.
There was a problem hiding this comment.
Other storages wouldn't need this in the path. Note that the path is translated internally; the entry.Key is not updated. By the time the entry gets back from the underlying layer, the prefix is already stripped off.
There was a problem hiding this comment.
Ahh, I see what you mean. What's the ID used for in here: https://github.com/hashicorp/vault/pull/3479/files#diff-2bbba2fac19e5e53717543473f9a28efR318, is that something internal/specific to CouchDB?
There was a problem hiding this comment.
Yeah, it's a document store so I think that's the ID of the document.
Fixes #3476