[mounts] only allow one to one id mappings

[sharder996]

Fixes #3332

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (4b38e63) 83.92% compared to head (385dc25) 83.84%.

Files	Patch %	Lines
src/utils/vm_mount.cpp	26.66%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3349      +/-   ##
==========================================
- Coverage   83.92%   83.84%   -0.08%     
==========================================
  Files         251      251              
  Lines       13678    13690      +12     
==========================================
  Hits        11479    11479              
- Misses       2199     2211      +12     

☔ View full report in Codecov by Sentry.
📢 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!

Thanks for this! I'm not done reviewing, but I did want to go ahead and give you my feedback below.

[townsend2010]

I don't think the message is very clear here. It doesn't reference the first mapping found with the duplicate. For example, if I passed in -g 1000:1000 -g 1000:1001 -g 1000:1002, it will only print out the last two. I think it should print all of the duplicate mappings and let the user see what they are and decide how to fix it.

Also, I'm kind of on the fence on this, but I wonder if we should be a little more specific which gid is the duplicate meaning either the host gid or the instance gid that is the offender. Any opinions on this?

[sharder996]

I agree that the error message could be more descriptive, but it require the filtering algorithm to change quite a bit so that the offending id can be identified as well as it's associated duplicate id mappings. I haven't thought of a good way to do this yet and seeing as how other things are higher priority I haven't done anything yet.

[townsend2010]

Ok, makes sense. We can put this on the back burner for now until the other items you are working on are more complete.

[townsend2010]

Idem: Error message is a bit unclear as I pointed out above.

[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!