[mounts] only allow one to one id mappings

[sharder996]

Fixes #3332

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 92.20779% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.85%. Comparing base (981b86c) to head (fd9e736). Report is 5 commits behind head on main.

Files	Patch %	Lines
src/daemon/daemon.cpp	14.28%	6 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3349      +/-   ##
==========================================
+ Coverage   88.81%   88.85%   +0.03%     
==========================================
  Files         253      253              
  Lines       14121    14168      +47     
==========================================
+ Hits        12542    12589      +47     
  Misses       1579     1579              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[andrei-toterman]

This is not a review, I just glanced over it out of curiosity. And I'm not sure I understand the whole BackInserter thing, so I would like to open up a discussion on this. Couldn't we use something like

auto unique_iter_end = std::unique(mappings.begin(), mappings.end(), [](auto& a, auto& b) {
    return a.first == b.first || a.second == b.second;
});

Then unique_iter_end != mappings.end() to check if there were duplicates or mappings.erase(unique_iter_end, mappings.end()) to remove the duplicates.

[sharder996]

Hey @andrei-toterman,

The idea of the BackInserter was so that insertions could be made into the returned vector from either an existing vector (as used in vm_mount) or from a repeated grpc object of type IdMap (as used in the daemon). Other motivations for changes in id_mappings was also to return a vector of duplicate mappings so that they could be reported back to the user. Also, templating the unique_id_mappings() function was done so that it could accept multiple types of containers.

Using std::unique() looks promising except for that it only finds groups of equivalent elements in consecutive groups.

[andrei-toterman]

Oops, you're right about the std::unique, I misread the documentation. We have to filter the duplicates manually.

Anyway, when trying to do a mount with repeated mappings, the errors are indeed reported, but the mount still occurs. Shouldn't we abort the operation entirely? For example, if I do this

mp mount dir instance:dir -u 1000:1000 -u 1000:2000

I get the following error message

mount failed: The following errors occurred:
cannot apply mapping 1000:2000 with duplicate uids

But the mount still happens, but only with the 1000:1000 mapping.

What about checking for duplicates only inside the MountHandler base class constructor and throwing if they occur. That way it would be impossible to create an invalid MountHandler. Or maybe do the check inside VMMount's constructor? Anyway, I think we should make invalid states unrepresentable. What do you think?

[sharder996]

Yes, I had considered that. For the some of the "errors" that can happen when creating mounts (eg. incorrect paths, incorrect instances name) we simply ignore those items and continue with establish as many of the mounts as possible. That is why I treated mappings the same way.

I like the idea of checking mapping inside of VMMount and not so much in the MountHandler since it need to dive inside the VMMount passed to it. Rather do that directly when the VMMount is created.

[townsend2010]

Hey @sharder996,

Any more thoughts on my comments in this? I've looked over the code and it otherwise seems reasonable to me, but I do think the error messages should be improved. Thanks!

[sharder996]

@andrei-toterman Thanks for the review! I think I've addressed all your comments.

[andrei-toterman]

Thanks, @sharder996. It's looking good. I just want your opinion on a last detail:

$ multipass mount -u 1000:1001 -u 1002:1001 -g 1000:1000 -g 1001:1000 ~ test:bla      
mount failed: Mount cannot apply mapping with duplicate uids: 1001: [1002:1001, 1000:1001]
Mount cannot apply mapping with duplicate gids: 1000: [1001:1000, 1000:1000]

As you can see, the first line reporting duplicate uids is prefixed by the mount failed: thing. How do you think this would look better? Maybe putting both messages on new lines? Or maybe somehow integrating them into one line? Or maybe something else?

[sharder996]

Hi @andrei-toterman, The reason I put it on the same line was to stay consistent with launch errors:

$ multipass launch docker
launch failed: Downloaded image hash does not match

For that reason, I don't really want to put both messages on new lines...

But, how about something like this:

$ multipass mount -u 1000:1001 -u 1002:1001 -g 1000:1000 -g 1001:1000 ~ test:bla      
mount failed: Mount cannot apply mapping with duplicate ids:
uids: 1001: [1002:1001, 1000:1001]
gids: 1000: [1001:1000, 1000:1000]

[andrei-toterman]

@sharder996, sure, I think that looks great. Thanks!

[andrei-toterman]

Alright, this should be ready to be merged. But it's failing to build on CI on mac. Seems like it's a missing include https://github.com/canonical/multipass-private/actions/runs/9411867391/job/25931752655#step:26:3601