feat: enable volume mounts for local func run#3821
Conversation
- Updated newContainer and newHostConfig functions to accept an output writer for logging. - Introduced volumeMounts and toMount functions to handle Docker volume specifications from function definitions. - Added support for Secrets, ConfigMaps, EmptyDir, and PVCs in volume mounts, improving flexibility for container configurations. - Implemented warnings for missing paths and errors during volume mapping, enhancing user feedback during execution.
- Introduced comprehensive unit tests for the volume mount functionality, covering various volume types including Secrets, ConfigMaps, EmptyDir, and PVCs. - Verified correct behavior for scenarios such as missing paths, read-only mounts, and unrecognized volume types. - Enhanced error handling and user feedback through warnings for skipped volumes and detailed error messages. - Ensured that the tests validate the expected mount configurations and behaviors, improving overall test coverage for the Docker runner.
- Adjusted comment formatting in the TestVolumeMounts_SkipsNilPath test case for improved readability. - Ensured consistency in comment style for better understanding of test scenarios.
- Introduced unit tests to ensure that secret and configMap names containing path traversal sequences are rejected, enhancing security by preventing directory traversal attacks. - Implemented a new function, safeLocalName, to validate names against path separators and traversal patterns, improving error handling in volume mounting. - Updated the toMount function to utilize the new validation, ensuring robust handling of volume specifications.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
Bot
added
size/L
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Docker runner support for func.yaml volume definitions by translating them into Docker mount specs, along with unit tests covering mapping rules and error/skip behavior.
Changes:
- Add volume-to-mount translation (
toMount) and mount list construction (volumeMounts) during container creation. - Add validation to prevent path traversal for Secret/ConfigMap-backed local bind mounts.
- Introduce unit tests for volume mapping behavior and warning/skip semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/docker/runner.go | Implements volume→mount conversion and wires mounts into container host config. |
| pkg/docker/runner_volumes_test.go | Adds tests for mount mapping, validation failures, and warning/skip behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil | ||
| } | ||
|
|
||
| func toMount(root string, vol fn.Volume) (mount.Mount, error) { |
| // safeLocalName rejects names that contain path separators or traversal | ||
| // sequences. Secret and ConfigMap names are used as file-path components; an | ||
| // adversarial name such as "../../etc" would otherwise escape the .func tree. | ||
| func safeLocalName(name string) error { | ||
| if strings.ContainsAny(name, "/\\") || strings.Contains(name, "..") { | ||
| return fmt.Errorf("name %q must not contain path separators or \"..\"", name) | ||
| } |
| func volumeMounts(root string, volumes []fn.Volume, out io.Writer) []mount.Mount { | ||
| var mounts []mount.Mount | ||
| for _, vol := range volumes { | ||
| if vol.Path == nil { | ||
| fmt.Fprintf(out, "warning: skipping volume %s: missing path\n", vol.String()) | ||
| continue | ||
| } |
| if err := os.MkdirAll(src, 0700); err != nil { | ||
| return mount.Mount{}, fmt.Errorf("cannot create local secret dir %q: %w", src, err) | ||
| } | ||
| return mount.Mount{Type: mount.TypeBind, Source: src, Target: target}, nil |
| if err := os.MkdirAll(src, 0750); err != nil { | ||
| return mount.Mount{}, fmt.Errorf("cannot create local configmap dir %q: %w", src, err) | ||
| } | ||
| return mount.Mount{Type: mount.TypeBind, Source: src, Target: target}, nil |
| want := filepath.Join(root, fn.RunDataDir, "run", "configmaps", "app-config") | ||
| if m.Source != want { | ||
| t.Errorf("source: got %q, want %q", m.Source, want) | ||
| } |
Fixes #2940
Summary
pkg/docker/runner.go->newHostConfig()to configure volume mounts in addition to port bindings when running functions locally withfunc runvolumeMountsandtoMounthelpers to convertfunc.yamlvolume definitions into Docker mount specsVolume mapping
All bind-mount sources are rooted inside
.func/, satisfying the security constraint in the issue (no paths outside.funcare ever mounted).Notes
mount.Mount) instead of the legacyBinds []string+Tmpfsfields, and resolves local paths fromf.Rootrather than the process working directory, which fixes a subtle bug whenfunc runis invoked from outside the function root.