Skip to content

[CORE-8020] tls: Watch for IN_MOVED_TO #150

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.

Sign up for GitHub

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

Open
BenPope wants to merge 1 commit into
base: v24.3.x
Choose a base branch
from
Open

[CORE-8020] tls: Watch for IN_MOVED_TO #150

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/net/tls-impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ class tls::reloadable_credentials_base {
}
}
}
future<fsnotifier::watch_token> add_watch(const sstring& filename, fsnotifier::flags flags = fsnotifier::flags::close_write|fsnotifier::flags::delete_self) {
future<fsnotifier::watch_token> add_watch(const sstring& filename, fsnotifier::flags flags = fsnotifier::flags::close_write|fsnotifier::flags::delete_self|fsnotifier::flags::move_to) {
Copy link

@pgellert pgellert Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. The man page made me think that move_to is generated for the containing directory rather than the file and that move_self is more appropriate.

What happens if someone does mv existing.crt existing.crt.bkp as a way of deleting certificates? I'm wondering if that would trigger a delete_self or if we should be watching move_from as well.

https://man7.org/linux/man-pages/man7/inotify.7.html

Copy link
Member

@oleiman oleiman Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, I don't think mv existing.crt existing.crt.bkp will reliably invalidate the inode corresponding to existing.crt.

Copy link

@IoannisRP IoannisRP Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When executing

mv existing.crt.bak existing.crt

existing.crt should trigger a delete_self, which is already watched for, and the parent dir should issue a move_to.

I do not understand why adding move_to to the file watcher fixes the issue. What am I missing here?

The add_dir_watch function seems to be there to cater for moving files around, but for some reason it is only applied on rebuild and not on init.

Copy link
Member

@oleiman oleiman Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this naively at the command line, it doesn't seem like this type of mv generates a delete event, or at least not guaranteed right away.

which makes sense I think because inotify monitors the state of the inodes pointed to by the directory entry, rather than the name->inode mapping itself?

Copy link
Member

@oleiman oleiman Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind, this is a file watch. Now I'm confused too!

Copy link

@IoannisRP IoannisRP Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, you are right. i should have been more careful in the description.
I was referring to the case where existing.crt already exists and we are trying to overwrite it with a move.

Copy link
Member

@oleiman oleiman Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, that type of move does not immediately notify delete on the directory watch, but it does notify delete_self on the file watch. Anyway, your original question is apt I think.

return _fsn.create_watch(filename, flags).then([this, filename = filename](fsnotifier::watch w) {
auto t = w.token();
// we might create multiple watches for same token in case of dirs, avoid deleting previously
Expand Down