-
Notifications
You must be signed in to change notification settings - Fork 343
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
[local] Refuse to create lakectl locals on case-insensitive filesystems #7650
[local] Refuse to create lakectl locals on case-insensitive filesystems #7650
Conversation
Still need to add it to `lakectl local init`. To check this (on Linux): ```sh dd if=/dev/zero of=/tmp/my-loopback bs=1k count=10000 sudo losetup -f /tmp/my-loopback losetup --all mkfs -t ext4 -O casefold -E encoding_flag=strict /dev/loop11 mkdir /tmp/ifs; sudo mount /dev/loop11 /tmp/ifs/ mkdir /tmp/ifs/dir chattr +F /tmp/ifs/dir ``` Obviously this won't really work in a CI/CD container, where there are externalities that we do not control.
🎊 PR Preview 0a737f7 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-7650.surge.sh 🕐 Build time: 0.011s 🤖 By surge-preview |
How was this tested?Unit tests are easy, and included. But for something that depends on the OS interface they are not enough. A system check is also needed! I managed to perform a manual system check for acceptance. This is hard because developer machines should NOT have case-insensitive filesystems -- precisely for the reasons that we have this PR! Luckily ext4 (and, apparently, ext2 and ext3) can be configured to allow case-insensitive directories. To set it up on Linux: # Create a loopback file
dd if=/dev/zero of=/tmp/my-loopback bs=1k count=10000
# Create a loopback device
sudo losetup -f /tmp/my-loopback
# Get the name of the created device (I got /dev/loop11)
losetup --all
# Format an ext4 filesystem
sudo mkfs -t ext4 -O casefold -E encoding_flags=strict /dev/loop11
# Mount it
mkdir /tmp/ifs; sudo mount /dev/loop11 /tmp/ifs/
# (Give permissions...)
# Create a case-insensitive directory
mkdir /tmp/ifs/dir
chattr +F /tmp/ifs/dir
# Now use /tmp/ifs/dir for testing... So I did this and verified that I get an error message on a case-insensitive directory under This is an acceptance check not a test!Obviously this won't really work in a CI/CD container: there are
All of these issues can change between GitHub runner releases, and none of them can be fixed from inside a test container - they all manifest at the VM level. OTOH, this is an esoteric setup. |
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.
Thanks, I don't have a lot of comments on the code itself.
However, I feel we need to understand the implications of this change. Blocking since this will break users who are currently using lakectl local on FSs that are case insensitive. I agree that his is a bug that needs to be fixed but I believe we must first communicate it to all the relevant stakeholders
cmd/lakectl/cmd/local_clone.go
Outdated
@@ -107,5 +110,6 @@ var localCloneCmd = &cobra.Command{ | |||
func init() { | |||
withGitIgnoreFlag(localCloneCmd) | |||
withSyncFlags(localCloneCmd) | |||
withForceFlag(localCloneCmd, "Force clone even on case-insensitive filesystem") |
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.
We already have a "force" flag that does something differently.
Can we please rename it to something like --ignore-case-insensitive
or something of the sorts?
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.
Changed to --allow-case-insensitive
.
I disagree, asking @ozkatz for guidance please.
|
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.
Thanks!
Will wait for additional comments about desire UX here.
cmd/lakectl/cmd/local_clone.go
Outdated
@@ -107,5 +110,6 @@ var localCloneCmd = &cobra.Command{ | |||
func init() { | |||
withGitIgnoreFlag(localCloneCmd) | |||
withSyncFlags(localCloneCmd) | |||
withForceFlag(localCloneCmd, "Force clone even on case-insensitive filesystem") |
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.
Changed to --allow-case-insensitive
.
818b1f4
to
0a737f7
Compare
@arielshaqed Looking into the solution we chose, I’d like to suggest that we soften it a bit. Instead of dying by default on inits that involve dirs on case-insensitive FSs, print a warning and document this very well in our public docs and lakectl local best practices. |
- Don't break existing behaviour - Stay compatible with Git, which does even less - Don't tell off Windows and MacOS default filesystems
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.
Thanks,
I have one request regarding the IsCaseInsensitiveLocation
function
Warning(fmt.Sprintf(CaseInsensitiveWarningMessageFormat, path)) | ||
} | ||
if err != nil { | ||
Warning(fmt.Sprintf("Check whether directory '%s' is case-insensitive: %s", path, err)) |
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.
Warning(fmt.Sprintf("Check whether directory '%s' is case-insensitive: %s", path, err)) | |
Warning(fmt.Sprintf("Check whether directory '%s' is case-insensitive failed: %s", path, err)) |
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.
Not done. The word "failed" does nothing to help with a real error message. Compare
Check whether directory './foo' is case-insensitive: Permission denied.
with
Check whether directory './foo' is case-insensitive failed: Permission denied.
Bash does the same thing:
❯ echo foo > x
bash: x: Permission denied
So does every other UN*Xy program:
❯ echo foo | tee x
tee: x: Permission denied
cmd/lakectl/cmd/local.go
Outdated
@@ -116,6 +119,17 @@ Use "%s checkout..." to sync with the remote or run "lakectl local clone..." wit | |||
} | |||
} | |||
|
|||
func warnOnCaseInsensitiveDirectory(path string) { | |||
isCaseInsensitive, err := fileutil.IsCaseInsensitiveLocation(fileutil.OSFS{}, path) | |||
if isCaseInsensitive { |
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.
Need to switch the ifs. I know it doesn't really matter in this case but it still hurts my eyes :)
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.
Done.
@@ -39,6 +39,8 @@ var localCloneCmd = &cobra.Command{ | |||
DieFmt("directory '%s' exists and is not empty", localPath) | |||
} | |||
|
|||
warnOnCaseInsensitiveDirectory(localPath) |
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.
Maybe it's just better to add this check to the local root command?
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.
Not done.
Given that we plan to allow Windows and MacOS users to work like this, I will not warn them about something that they can do nothing about it and that we want to support.
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.
I'm not sure what you mean here. The issue is relevant to any local command we perform, so why add it only in 2 sub commands?
Also, not maintainable, the next command we add which might perform a sync we will most likely forget to add this check
pkg/fileutil/fs_case.go
Outdated
return false, fmt.Errorf("touch %s: %w", path, err) | ||
} | ||
defer func() { | ||
_ = fs.Remove(path) |
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.
I'd like to find a solution to the problem where remove fails as it affects the locally synced directory. The file will be synced to remote and the user is not aware of it.
I understand we don't want to use tmpfile here because the tmp folder might be on a different FS. But I think we need a solution for this.
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.
This is literally not possible to fix. OTOH it is also not possible to fail here unless a process with user privileges is actively trying to sabotage the user. I could return an error and panic
.
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.
Agreed f2f: print an warning message.
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.
Thanks!
Not sure which of the changes is requested; unfortunately I object to many of them, and am not sure what to do about one.
I suggest panicking when we cannot Remove the test file. Would that resolve your requested changes?
cmd/lakectl/cmd/local.go
Outdated
@@ -116,6 +119,17 @@ Use "%s checkout..." to sync with the remote or run "lakectl local clone..." wit | |||
} | |||
} | |||
|
|||
func warnOnCaseInsensitiveDirectory(path string) { | |||
isCaseInsensitive, err := fileutil.IsCaseInsensitiveLocation(fileutil.OSFS{}, path) | |||
if isCaseInsensitive { |
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.
Done.
Warning(fmt.Sprintf(CaseInsensitiveWarningMessageFormat, path)) | ||
} | ||
if err != nil { | ||
Warning(fmt.Sprintf("Check whether directory '%s' is case-insensitive: %s", path, err)) |
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.
Not done. The word "failed" does nothing to help with a real error message. Compare
Check whether directory './foo' is case-insensitive: Permission denied.
with
Check whether directory './foo' is case-insensitive failed: Permission denied.
Bash does the same thing:
❯ echo foo > x
bash: x: Permission denied
So does every other UN*Xy program:
❯ echo foo | tee x
tee: x: Permission denied
@@ -39,6 +39,8 @@ var localCloneCmd = &cobra.Command{ | |||
DieFmt("directory '%s' exists and is not empty", localPath) | |||
} | |||
|
|||
warnOnCaseInsensitiveDirectory(localPath) |
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.
Not done.
Given that we plan to allow Windows and MacOS users to work like this, I will not warn them about something that they can do nothing about it and that we want to support.
pkg/fileutil/fs_case.go
Outdated
return false, fmt.Errorf("touch %s: %w", path, err) | ||
} | ||
defer func() { | ||
_ = fs.Remove(path) |
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.
This is literally not possible to fix. OTOH it is also not possible to fail here unless a process with user privileges is actively trying to sabotage the user. I could return an error and panic
.
Warn first if we failed to detect. This will always produce the same output, and is easier for readers.
I think panicking is a bit drastic, but I do see this bug coming back to us sooner than later. Can we at least issue a an error or warning message in the defer function if the remove fails? |
2dd3bf2
to
f6fc38f
Compare
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.
Thanks; PTAL!
pkg/fileutil/fs_case.go
Outdated
return false, fmt.Errorf("touch %s: %w", path, err) | ||
} | ||
defer func() { | ||
_ = fs.Remove(path) |
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.
Agreed f2f: print an warning message.
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.
Thank you!!!
f6fc38f
to
d0c1821
Compare
Thanks! |
…ms (treeverse#7650) * Add case-insensitive filesystem checker * Add case-insensitive check to `lakectl local clone` Still need to add it to `lakectl local init`. To check this (on Linux): ```sh dd if=/dev/zero of=/tmp/my-loopback bs=1k count=10000 sudo losetup -f /tmp/my-loopback losetup --all mkfs -t ext4 -O casefold -E encoding_flag=strict /dev/loop11 mkdir /tmp/ifs; sudo mount /dev/loop11 /tmp/ifs/ mkdir /tmp/ifs/dir chattr +F /tmp/ifs/dir ``` Obviously this won't really work in a CI/CD container, where there are externalities that we do not control. * Mention that Git also fails on a case-insensitive filesystem * [lint] Drop unneeded "_" receiver * [CR] Rename --force to --allow-case-insensitive * [CR] *Only* warn on case-insensitive filesystem, never fail - Don't break existing behaviour - Stay compatible with Git, which does even less - Don't tell off Windows and MacOS default filesystems * [CR] Change order of warning messages for case insensitivity Warn first if we failed to detect. This will always produce the same output, and is easier for readers. * [CR] Warn when failed to file used to test case insensitivity
Of course, allow it anyway if
--force
d to.Fixes #7645.