Fix symlink resolution when mounting #3673

Sploder12 · 2024-09-12T20:41:42Z

fixes #1722

Symbolic links are now resolved when performing a mount.
Previously they were not resolved.

MULTI-1549

codecov · 2024-09-12T21:30:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.87%. Comparing base (21430a3) to head (d8bb07d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3673   +/-   ##
=======================================
  Coverage   88.87%   88.87%           
=======================================
  Files         256      256           
  Lines       14579    14581    +2     
=======================================
+ Hits        12957    12959    +2     
  Misses       1622     1622

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ricab

Thank you Trevor! Superficial review from my side, but it looks like it's headed in the right direction. I have a couple of comments inline.

ricab · 2024-09-16T11:07:14Z

src/daemon/daemon.cpp

-               ? std::make_unique<SSHFSMountHandler>(vm, config->ssh_key_provider.get(), target, mount)
-               : vm->make_native_mount_handler(target, mount);
+               ? std::make_unique<SSHFSMountHandler>(vm, config->ssh_key_provider.get(), target, std::move(mount))
+               : vm->make_native_mount_handler(target, std::move(mount));


This one doesn't help much unless you change make_native_mount_handler to take by value and move too. Otherwise you're asking for one extra temporary.

src/utils/vm_mount.cpp

ricab

Hey Trevor, this works well for what it does and the code looks good. However, currently it covers only paths ending in a symlink. I guess we should cover symlinks anywhere in the path, don't you think?

ricab@ricab-XPS-9320:~/tmp/asdf $ ln -s .. l4
ricab@ricab-XPS-9320:~/tmp/asdf $ cd
ricab@ricab-XPS-9320:~ $ mp mount tmp/asdf/l4/subl-mdpreview/ amazed-topi:l4
ricab@ricab-XPS-9320:~ $ mp info amazed-topi
Name:           amazed-topi
State:          Running
Snapshots:      2
IPv4:           10.251.95.73
Release:        Ubuntu 24.04.1 LTS
Image hash:     78547d336e4c (Ubuntu 24.04 LTS)
CPU(s):         2
Load:           0.00 0.00 0.00
Disk usage:     2.0GiB out of 4.8GiB
Memory usage:   342.3MiB out of 955.7MiB
Mounts:         /home/ricab/tmp/asdf/l4/subl-mdpreview => l4
                    UID map: 1000:default
                    GID map: 1000:default

I expected the source to become just /home/ricab/tmp/subl-mdpreview

ricab · 2024-10-11T19:43:23Z

src/utils/vm_mount.cpp

+
+        if (err)
+            throw std::runtime_error(
+                fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.",


This would end up being shown to the user, right? If so, I would try to make it more user friendly. Perhaps:

Suggested change

fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.",

fmt::format("Could not resolve symlinks in mount source path \"{}\": {}.",

src/utils/vm_mount.cpp

ricab · 2024-10-14T10:35:04Z

Hey Trevor, a quick experiment I did:

#include <filesystem>
#include <iostream>

using namespace std;
namespace fs = std::filesystem;

int main()
{
  // l4 -> ..
  // l2 -> ../subl-mdpreview
  auto src_path = "/home/ricab/tmp/asdf/l4/asdf/l4/asdf/l2/";

  error_code ec{};
  auto dst_path = fs::weakly_canonical(src_path, ec);

  if (!ec)
    cout << "\"" << src_path << "\" == " << dst_path << endl;

  // prints "/home/ricab/tmp/asdf/l4/asdf/l4/asdf/l2/" == "/home/ricab/tmp/subl-mdpreview"

  return bool(ec);
}

Sploder12 · 2024-10-21T16:47:07Z

I wonder if we should check elsewhere in the repo for usage of \"{}\" since a string like Hello\\ could cause issues (becomes "Hello\"). WDYT?

ricab

Thank you @Sploder12! I would like for someone to do a primary review on this, but I am approving on secondary.

src/utils/file_ops.cpp

ricab · 2024-11-15T14:30:55Z

I wonder if we should check elsewhere in the repo for usage of "{}" since a string like Hello\ could cause issues (becomes "Hello"). WDYT?

We ended up considering this moot, correct @Sploder12 ?

Sploder12 · 2024-11-15T14:34:07Z

We ended up considering this moot, correct @Sploder12 ?

There was one issue that came from it #3759, but no real impact.

georgeliao

@Sploder12 , thanks for the good work. It looks very good in general. I just have a few minor comments and questions. The functional testing went also fine.

tests/test_daemon_mount.cpp

georgeliao · 2024-11-21T10:11:47Z

tests/test_daemon_mount.cpp

+
+TEST_F(TestDaemonMount, symlinkSourceGetsResolved)
+{
+    const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac_addr, extra_interfaces));


filename field can be set to _, right?

_ is used later on line 277 so it'd cause a redefinition error. I have this one named over the other since the filename might be useful, but the mock guard shouldn't be touched.

In such case, I sometimes name it along the lines of ignored_filename.

georgeliao · 2024-11-21T10:30:09Z

tests/test_daemon_mount.cpp

+    mp::Daemon daemon{config_builder.build()};
+
+    mp::MountRequest request;
+    request.set_source_path(mount_dir.path().toStdString());


Not sure I fully get the purpose of this unit test, mount_dir.path() is a normal temp directory without dot, dot-dot, or symbolic, right? So we are not really testing resolving as far as I can tell.

Good point, this test isn't clear in what it's doing. The test does not test that resolution works, but rather that weakly_canonical is called when making a mount request. But it also doesn't verify that the value from weakly_canonical is actually used so it is a terrible test. Good catch!

georgeliao · 2024-11-21T11:27:02Z

tests/daemon_test_fixture.cpp

-                                                            "                \"source_path\": \"{}\",\n"
-                                                            "                \"target_path\": \"{}\",\n",
+                                                            "                \"source_path\": {:?},\n"
+                                                            "                \"target_path\": {:?},\n",


Two questions, here.

What is the point of using debug specifier here if we are just injecting two std::strings and there is no debug or normal display implementation?

I am not sure {:?} is supported in c++ fmt::format as well as it is in rust, do you know any official documentation where it introduces this feature.

Without the debug specifier the JSON on Windows will contain "C:\some\path\", since the \ is not escaped when we parse the JSON the " gets escaped and things break.

Yep, their syntax documentation describes the purpose of '?', which is "Debug format. The string is quoted and special characters escaped." So we get "C:\\some\\path\\" instead which gets parsed correctly.

I see, so the difference between {:?} and {} on a std::string is just the quotes and escapes. {:?} always gives the raw and quoted string, which is definitely more robust.

georgeliao · 2024-11-21T11:28:23Z

tests/test_base_snapshot.cpp

@@ -521,7 +521,7 @@ TEST_F(TestBaseSnapshot, adoptsMountsFromJson)
    const auto [snapshot_mnt_dst, snapshot_mount] = *snapshot_mounts.begin();

    EXPECT_EQ(snapshot_mnt_dst, dst_path);
-    EXPECT_EQ(snapshot_mount.get_source_path(), src_path);
+    EXPECT_EQ(snapshot_mount.get_source_path(), mp::fs::weakly_canonical(src_path));


src_path does not seem to have anything to resolve, is this for future proof meaning that if src_path becomes resolvable it can still match the snapshot_mount.get_source_path()?

I don't remember, but my assumption is that my past self did so to fix a test failing on Windows. But maybe not.

Huh, turns out it was for the test failing on MacOS.

src/utils/vm_mount.cpp

ricab

Approving on secondary again. Please don't wait for me to merge once the primary review is settled.

georgeliao · 2024-11-26T10:16:57Z

tests/test_daemon_mount.cpp

+    EXPECT_CALL(*mock_factory, create_virtual_machine).WillOnce(Return(std::move(mock_vm)));
+
+    // setup to make the daemon happy
+    MP_DELEGATE_MOCK_CALLS_ON_BASE(*mock_file_ops, mkpath, FileOps);


Nice macro usage, better than the boilerplate code before.

georgeliao · 2024-11-26T10:17:47Z

tests/test_daemon_mount.cpp

@@ -267,3 +268,51 @@ TEST_F(TestDaemonMount, performanceMountsNotImplementedHasErrorFails)
    EXPECT_EQ(status.error_code(), grpc::StatusCode::FAILED_PRECONDITION);
    EXPECT_THAT(status.error_message(), StrEq("The native mounts feature is not implemented on this backend."));
 }
+
+TEST_F(TestDaemonMount, mount_uses_resolved_source)
+{


The new unit test name is definitely more expressive.

georgeliao

It looks good to me now, thanks for addressing all the comments. Maybe do a rebase and we can get ready to merge.

ricab reviewed Sep 16, 2024

View reviewed changes

ricab requested changes Oct 11, 2024

View reviewed changes

Sploder12 force-pushed the fix-symlink-mounting branch 2 times, most recently from 9b71479 to ccac3af Compare October 18, 2024 19:30

sharder996 added this to the 1.15.0 milestone Oct 30, 2024

ricab previously approved these changes Nov 15, 2024

View reviewed changes

src/utils/file_ops.cpp Outdated Show resolved Hide resolved

georgeliao reviewed Nov 21, 2024

View reviewed changes

Sploder12 dismissed ricab’s stale review via e326a45 November 22, 2024 15:18

ricab previously approved these changes Nov 22, 2024

View reviewed changes

Sploder12 dismissed ricab’s stale review via cd722f5 November 22, 2024 17:14

georgeliao reviewed Nov 26, 2024

View reviewed changes

georgeliao previously approved these changes Nov 26, 2024

View reviewed changes

Sploder12 force-pushed the fix-symlink-mounting branch from cd722f5 to 48b8bf0 Compare November 26, 2024 15:05

Sploder12 added 10 commits November 26, 2024 11:38

[Daemon] fix symlinks not resolving at mount time

c0c1534

[daemon][unit test] Add mount symlink resolution tests

10ce1f4

Format changes

a6c1b71

[daemon][unit test] Fix tests expected path on Windows and MacOS

46d5754

[daemon] Move symlink resolution to VMMount ctor

6255378

[mounts] Resolve intermediate symlinks

7c50b8c

[unit test] Fix some failing tests

d6c2868

[unit tests] Escape mount source and target in tests

5306502

[mounts] Address review comments, tests and error_code changes

f226c0f

[tests] Fix use of move-from mock

d8bb07d

Sploder12 force-pushed the fix-symlink-mounting branch from 48b8bf0 to d8bb07d Compare November 26, 2024 16:38

[tests] Readd weakly canonical to fix MacOS tests

e4d1ea0

Sploder12 dismissed georgeliao’s stale review via e4d1ea0 November 26, 2024 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symlink resolution when mounting #3673

Fix symlink resolution when mounting #3673

Sploder12 commented Sep 12, 2024 •

edited by jibel

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

ricab left a comment •

edited

Loading

ricab Sep 16, 2024

ricab left a comment

ricab Oct 11, 2024

ricab commented Oct 14, 2024

Sploder12 commented Oct 21, 2024

ricab left a comment

ricab commented Nov 15, 2024

Sploder12 commented Nov 15, 2024

georgeliao left a comment

georgeliao Nov 21, 2024

Sploder12 Nov 21, 2024

ricab Nov 21, 2024

georgeliao Nov 21, 2024

Sploder12 Nov 21, 2024

georgeliao Nov 21, 2024

Sploder12 Nov 21, 2024

georgeliao Nov 26, 2024

georgeliao Nov 21, 2024

Sploder12 Nov 22, 2024

Sploder12 Nov 26, 2024

ricab left a comment

georgeliao Nov 26, 2024

georgeliao Nov 26, 2024 •

edited

Loading

georgeliao left a comment

	fmt::format("Mount symlink source path \"{}\" could not be made weakly canonical: {}.",
	fmt::format("Could not resolve symlinks in mount source path \"{}\": {}.",

Fix symlink resolution when mounting #3673

Are you sure you want to change the base?

Fix symlink resolution when mounting #3673

Conversation

Sploder12 commented Sep 12, 2024 • edited by jibel Loading

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

ricab left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricab commented Oct 14, 2024

Sploder12 commented Oct 21, 2024

ricab left a comment

Choose a reason for hiding this comment

ricab commented Nov 15, 2024

Sploder12 commented Nov 15, 2024

georgeliao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

georgeliao left a comment

Choose a reason for hiding this comment

Sploder12 commented Sep 12, 2024 •

edited by jibel

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

ricab left a comment •

edited

Loading

georgeliao Nov 26, 2024 •

edited

Loading