-
Notifications
You must be signed in to change notification settings - Fork 715
Add support for creating files and folders in a container via docker/podman cp
#8121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for creating files and folders inside a container via a new API that leverages a container file system tree instead of traditional bind mounts. Key changes include:
- Introduction of the ContainerCreateFileAnnotation and related file system item classes.
- Enhancements to conversion methods in the DCP model for mapping file system entries.
- Updates to tests and resource builder extensions (and PostgreSQL resource builder) to utilize the new file creation API.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting/ApplicationModel/ContainerCreateFileAnnotation.cs | Defines new abstractions for file system items and annotations for file/directory creation. |
| src/Aspire.Hosting/Dcp/Model/Container.cs | Adds conversion logic for transforming file system items into container file system entries and includes createFiles in container spec. |
| tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs | Adds a new test verifying that container create file functionality works as intended. |
| src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs | Introduces a new WithCreateFile extension method (with comprehensive XML docs) for container resource builders. |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Updates container creation logic to incorporate file creation annotations. |
| src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs | Revises PgAdmin setup to generate server configuration using the new API instead of temporary file bind mounts. |
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/ApplicationModel/ContainerCreateFileAnnotation.cs:57
- The summary for ContainerCreateFileAnnotation specifies file creation only, yet the annotation supports both files and directories. Consider updating the summary (or renaming the class) to better reflect that it supports creating file system entries.
/// Represents an annotation that indicates the creation of a file in a container.
src/Aspire.Hosting/ApplicationModel/ContainerCreateFileAnnotation.cs:59
- [nitpick] The DebuggerDisplay attribute contains an unusual format specifier 'nw' after GetType().Name. Verify that this is intentional or update it to the correct format (for example, removing 'nw' if not needed).
[DebuggerDisplay("Type = {GetType().Name,nw}")]
|
Its good that we are getting this capability. I have a few thoughts/questions:
|
| /// defaultMode: UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.GroupRead | UnixFileMode.GroupWrite); | ||
| /// </code> | ||
| /// </example> | ||
| public static IResourceBuilder<T> WithCreateFile<T>(this IResourceBuilder<T> builder, string destinationPath, List<ContainerFileSystemItem> entries, int defaultOwner = 0, int defaultGroup = 0, UnixFileMode defaultMode = UnixFileMode.None) where T : ContainerResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so I've been thinking about this some more. I think what we need to do with this API is make it take an async callback (maybe we can have some simple wrappers) which is evaluated just before the resource is created.
This would allow for very late configuration scenarios where you might want to grab some state from the app model to generate file content.
So I am thinking something like:
public static IResourceBuilder<T> WithContainerFiles(this IResourceBuilder<T> builder, Func<ContainerFilesContext, Task> callback);The usage might look something like this:
var c = builder.AddContainer(...)
.WithContainerFiles(async context => {
_ = context.Model; // Access to app model.
_ = context.ServiceProvider = // Access to DI container.
});This would allow us to do things like interrogate other resources that might have already started to download configurations from them and then inject them into another container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would end up with multiple container files annotations on a resource, and the callbacks would be invoked in order on the same context. The context itself would have methods that allow you to attach files/directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also have a simplified version of the method with the following signature:
public static IResourceBuilder<T> WithContainerFiles(this IResourceBuilder<T> builder, params (string, int) files);Usage would be as follows usage might be as follows:
var c = builder.AddContainer(...)
.WithContainerFiles([
("blah.txt", 777),
("foo.bin", 644)
]);I'm less certain about this API and we could live without it as I think this API in general is a more advanced scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should we have the file system types or just have annotations that capture all the details directly?
I thought about having nested annotations, but didn't see that we'd used that pattern anywhere else, so was a bit hesitant to introduce it here.
- What happens with file content in terms of clean up. Is it temporarily put on disk and then removed?
We build a tar file in memory and then stream that to stdin for the docker cp command, so there's no temporary files to cleanup (technically podman cp does create and cleanup a temporary file themselves, but I want to create a PR against them to remove that behavior since the temp file they create is never actually used).
- The WithCreateFile API seems like a weird name to me. I'd probably do something like WithContainerFile or something.
WithContainerFiles seems like a solid choice. The destination path may seem a bit odd, but it is technically necessary as you have to give a base path in the container you want to add or modify your files/folders and any folders you add/update will have their ownership and permissions updated if they already exist (whereas the destination path won't be modified). With a callback model, we'd likely need to support nested callbacks for directories and files:
.WithContainerFiles("/", async context =>
{
context.AddDirectory(".pgweb" d =>
{
d.AddDirectory("bookmarks", d =>
{
d.AddFile("somefile.toml", "contents", mode: FileMode644);
});
});
});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be able to pass streams if not a file path (even/too). Because files might be big enough that we don't want to load them in memory at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitchdenny I’ve implemented a callback version of the API and annotation, but need to update the functional tests based on the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests and API are updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with @mitchdenny offline; I'm updating the default permission behavior to match that of *nix systems (umask based default permissions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the API in the description the only APi? It’s super verbose. Where’s the easy mode APi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a non-callback version that's a little simplified, but most of the complexity comes from the fact that the API is letting us do direct file system manipulation in the container (and that manipulation has to happen at a specific existing path in the container). We're manipulating a file tree, so we ended up with a tree structure in the API.
If we did add a simpler API method, it'd be a bit weird in any slightly complicated scenario (here's an example based on the pgweb bookmark file creation):
// Creates new bookmark files under the newly created /.pgweb/bookmarks/
WithContainerFiles(
// We're creating folders and files relative to the root of the filesystem (/)
destinationPath: "/",
files: [
// We need to specify the full path for each file since the .pgweb/bookmarks/ portion of the path is newly created
(".pgweb/bookmarks/someserver.toml", "contents"),
(".pgweb/bookmarks/otherserver.toml", "contents"),
]);The AppHost would have to handle parsing and generating the tree structure that DCP expects from those file references.
| } | ||
| Name = "bookmarks", | ||
| Mode = FileMode644, | ||
| Entries = bookmarkFiles, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird to pass files and a folder. I would have expected to pass file with relative paths instead. And a directory entry to just be the entry and its permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a docker cp standpoint, it isn't really feasible to do relative paths as we need to apply permissions in the tar file on a folder by folder and file by file basis, otherwise base directory permissions get a bit weird. Basically if we want to have more granular control, we have to treat it as a full file system tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the key things I think this API brings is the direct manipulation of the container file system prior to startup, which opens up a lot of advanced scenarios, but necessitates the more complex tree focused design vs. a simple file/path model.
Description
Adds a new
WithContainerFilesAPI method that can be used to create files and folders in a container when it is run. This is intended as an alternative to existing usage ofWithBindMountto copy temporary files into a container for configuration purposes. This method offers several advantages overWithBindMountin that the ownership and permissions of individual files and folders can be controlled without having to resort to applying permissions to local temporary files.The design is intended to deal with locally ephemeral files, so does not read from the local file system. Due to the size constraints of resources registered for orchestration, it only makes sense for simple configuration files as the entire contents of the file have to be passed as part of resource creation. As this API is intended as somewhat of an advanced scenario, the design errs on the side of allowing granular control of individual file and folder permissions and uses a tree structure to represent the created (or updated) file system.
It's important to ensure the created file system (including file contents) is idempotent across runs as the specific configuration will contribute to the lifecycle hash used to identify changes in
ContainerLife.Persistentcontainers.A (simple) example of usage from the
WithPgAdminresource builder extension (this replaces the previous behavior that depended on creating and bind mounting temporary files):An example of the non-callback API:
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):