Change VolumeStore to use digests instead of names on the filesystem #3281
Conversation
85f664c to
d4c6dc8
Compare
| validateRegExp = regexp.MustCompile("^[a-zA-Z0-9_+:/-]{1,256}$") | ||
| ) | ||
|
|
||
| func validateName(name string) error { |
There was a problem hiding this comment.
Purely provisional ^ - to be replaced by whatever we deem right.
I do not quite care what this does exactly, as this PR technically should free us from any limitations on names.
| @@ -105,19 +123,63 @@ func (vs *volumeStore) Unlock() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (vs *volumeStore) legacyEnsureLink(name string) error { | |||
There was a problem hiding this comment.
If we have a legacy volume, calling this method will add the name inside volFilePath and create a symlink from legacyVolPath (with name) to volPath (with digest).
| func (vs *volumeStore) Get(name string, size bool) (*native.Volume, error) { | ||
| if err := identifiers.Validate(name); err != nil { | ||
| return nil, fmt.Errorf("malformed volume name %q: %w", name, err) | ||
| func (vs *volumeStore) Get(nameOrDigest string, size bool) (*native.Volume, error) { |
There was a problem hiding this comment.
Given how List is working, Get will now be able to retrieve from either name or digest (although this is not a feature).
|
From what i tested it doesnt seem docker is controlling it, rather the mkdir is failing for volume create for other identifier names like containers i am able to give a pretty long name. (> 255 chars) having a sha256 as identifier i feel needs to be a 2 way hash rather compression otherwise it would be difficult to list it out. Not sure if we need to complicate it. |
Our NameStore implementation also uses the filesystem, so, same thing here: we need to rewrite it (or get rid of it) if we want to open it up.
I do not see how we could lift our length limit with a two way hash. Wouldn't that make our length limit variable, dependent on how well we can compress? |
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
d4c6dc8 to
24e20fa
Compare
|
Thanks, but I feel this is overengineering and hard to test |
|
Something bad may happen when a legacy volume exists with hexadecimal name? |
It would fail The only case where we could possibly run into trouble is if someone has been using as a volume name a valid digest prefixed by sha256 (eg: If this is a concern, we could certainly adapt the new layout to be over 76 characters long. Eg: Since legacy identifiers cannot be over 76 chars, that would do it. |
Ok, that's fine. |
The motivation for this comes from #2942 - with some discussion in #3279
Restricting names to what the filesystem can do is very limiting.
Furthermore, user-controlled strings being used directly as paths components on the filesystem gives me the creeps.
This proposal here moves away from storing volumes inside
volume_root/volume_nametovolume_root/digest(volume_name).It is fully backward compatible: if a volume exists with the legacy layout, it will still be usable.
It is NOT entirely forward compatible though: although existing volumes will keep working even with older versions even after having been used by newer version, this is not true for entirely new volumes:
What do you folks think?
cc @AkihiroSuda and @Shubhranshu153
FWIW docker IS using names as path components, and their regexp is:
[a-zA-Z0-9][a-zA-Z0-9_.-]They do enforce a limitation at max 254 characters as far as I can tell.