cache: Add support for bind cache

[imnitishng]

Summary

Add bind cache support to pack

Output

Before

After

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #1474
Resolves #1473

[codecov]

Codecov Report

Merging #1497 (c0b32c3) into main (a2941f1) will decrease coverage by 0.13%.
The diff coverage is 66.67%.

❗ Current head c0b32c3 differs from pull request most recent head 32dabff. Consider uploading reports for the commit 32dabff to get more accurate results

@@            Coverage Diff             @@
##             main    #1497      +/-   ##
==========================================
- Coverage   80.25%   80.12%   -0.12%     
==========================================
  Files         154      155       +1     
  Lines       10021    10084      +63     
==========================================
+ Hits         8041     8079      +38     
- Misses       1499     1522      +23     
- Partials      481      483       +2     

Flag	Coverage Δ
os_linux	79.89% <66.67%> (-0.12%)	⬇️
os_macos	77.37% <66.67%> (-0.10%)	⬇️
os_windows	80.00% <66.67%> (-0.12%)	⬇️
unit	80.12% <66.67%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[imnitishng]

I have not allowed clear cache support for bind caches due to security reasons (since clearing the bind cache means removing the directory from host and we wouldn't want to allow users to delete files on host), let me know if this needs to be changed.

[jromero]

If the user provides a very specific directory to use as a cache, why would we want to make an exception in this case but not in a named volume? I think the unexpected difference in behavior is more problematic than deleting files within a targeted directory.

[imnitishng]

If the user provides a very specific directory to use as a cache, why would we want to make an exception in this case but not in a named volume?

I feel like if user does something like --cache="type=build;format=bind;name=." alongwith --clear-cache, then we'd be cleaning up the current dir. How do we tackle this?

[jromero]

That's an interesting callout. I would agree that it could be dangerous. What if we completely restricted the combination of options instead of bypassing the functionality? IOW, what do you think about failing early if the user provides any bind format with --clear-cache.

We can provide a helpful message like "'--clear-cache' is not supported when using 'bind' cache. You may instead delete the cache directory manually.".

The only drawback is that it would prevent --clear-cache when using a mix of any other cache format.

---

Alternatively, what if we had a nested directory with an explicit name for bind? That way at least it's less likely that we could delete unexpected content.

.
└── <user-defined>/
    └── <type>-cache/

For example, when the user provides:
--cache="type=build;format=bind;name=./my-cache/"

we create the parent directory (if missing), but additionally create a child directory <type>-cache (ie. build-cache) such as:

.
└── my-cache/
    └── build-cache/

then when the users provides the same flag --cache="type=build;format=bind;name=./my-cache/" plus --clear-cache we only delete the my-cache/build-cache directory.

WDYT?

[jromero]

Overall this PR is 💯. I'd like to discuss the behavior of ClearCache.

Maybe @dfreilich has some opinions here as well.

[jromero]

If the user provides a very specific directory to use as a cache, why would we want to make an exception in this case but not in a named volume? I think the unexpected difference in behavior is more problematic than deleting files within a targeted directory.

[jromero]

@imnitishng, It looks like I made a mistake in the original issues I created for this feature. As per the RFC, the "name" option of the "bind" format should have been "source". I went ahead and made the changes along with other minor polishing items. You can review the changes in this commit.

Overall, 💯 implementation. Thank you!

[imnitishng]

Hey I just noticed the same, thanks for the cleanup!