Fix automatic aliasing of muxed providers

[t0yv0]

Fixes #1937

This PR fixes the automatic aliasing of muxed providers by dispatching a ResourceMap Clone operation to the sub-map that owns the key being cloned and making PF resource maps mutable.

[t0yv0]

This needs unit tests on union.go and some high-level test for the feature, but wanted to get feedback first if approach makes sense? This would need a new release of muxer and bridge.

[codecov]

Codecov Report

Attention: Patch coverage is 82.30088% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 60.79%. Comparing base (98847bf) to head (0085315).
Report is 1 commits behind head on master.

Files	Patch %	Lines
pf/internal/muxer/union.go	92.68%	5 Missing and 1 partial ⚠️
pkg/tfshim/shim.go	54.54%	4 Missing and 1 partial ⚠️
pf/internal/muxer/muxer.go	0.00%	4 Missing ⚠️
pf/internal/schemashim/resource_map.go	50.00%	2 Missing and 1 partial ⚠️
pf/internal/schemashim/schemashim.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1938      +/-   ##
==========================================
+ Coverage   60.55%   60.79%   +0.23%     
==========================================
  Files         331      332       +1     
  Lines       44726    44700      -26     
==========================================
+ Hits        27084    27175      +91     
+ Misses      16120    16000     -120     
- Partials     1522     1525       +3     

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

[VenelinMartinov]

Looks sensible, is the main issue that previously the aliases were not added to the list of resources/data maps?

Also now that they are added, wouldn't Len return an invalid value? Do we need it to return the number of unique resources/data maps?

[t0yv0]

Hm, I can double check Len() impl but the intent was to keep it consistent with what you would expect.

The problem here is that we merge two maps and then we mutate the merged map, and further we try to find the owner of the resource via the said merged map. Prior to the change the _legacy resources couldn't be correlated to the owning provider.

Hm, perhaps instead of fixing it in this way we can just teach the dispatch table builder to recognize that foo_legacy is owned by the same provider that owns foo unless there is evidence of another owner.

[iwahbe]

I'm having trouble understanding why we need Clone (instead of just Set(key, Get(key))). It looks like Clone is different from get + set only when you need to copy a key in both baseline and extension to a different value. I'm struggling to come up with a scenario where that distinction can be witnessed, since m.Get, m.Range, m.GetOk will only ever return the result from m.baseline given a conflicting key.

This makes sense as a strategy. Clone tells the map that key should be copied into the same sub-map as it came from.

It would be really great if Clone had a doc comment explaining why it existed and the guarantee it held.

I'd be happy to merge once it's cleaned up and tests are added.

[t0yv0]

PTAL, added some tests around this.

[iwahbe]

Looks good to me. Lets get this merged.

I am at this point unsure what LGTM actually stands for, but anyway...