Skip to content
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

fix containerized function mounts issue #4489

Merged
merged 5 commits into from Apr 18, 2022

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Feb 25, 2022

Fix #4290

Kustomize was not mounting files into containerized functions directly because the StorageMounts field was not being properly propagated to the function filter.

This PR allows users to use the StorageMounts fields, with the following requirements:

  • mount paths cannot be absolute paths
  • mount paths must be relative to the kustomization that refers to them
  • mount paths must be under the kustomization directory that refers to them

As a side note, if we are going to be promoting functions in kustomize we should probably build up a strong test suite for containerized functions and run those tests in CI. All tests needing docker are currently being skipped.

/cc @yuwenma
/cc @KnVerey

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 25, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2022
api/go.mod Outdated Show resolved Hide resolved
@yuwenma
Copy link
Contributor

yuwenma commented Feb 25, 2022

Some minor comments. Looks great!

@natasha41575 natasha41575 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 25, 2022
@k8s-ci-robot
Copy link
Contributor

@natasha41575: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 25, 2022
@KnVerey
Copy link
Contributor

KnVerey commented Feb 25, 2022

I poked around the history of the problem file, and it seems to me this never worked. Is that correct?

If so, if we were to release this PR, it would introduce a new security issue that (intended or not) we didn't have before.
Since container functions ARE enabled in kubectl, we should be very careful about this decision.

It looks like the feature was originally intended to have restrictions on the path, but that did not get implemented: #1902

volumes are relative to the Resource config
volumes must be in a directory under the config directory (no ../)

The way Plugin Graduation proposes to handle this is through Catalog, where the Catalog entry must specify allowed mounts. Inline config like what we're turning on here actually wouldn't be supported at all anymore:

If a containerized plugin needs network or disk access, its catalog entry MUST specify it. Redundant inline provider config cannot be used to grant it. When the catalog does specify it, no additional flags or config fields will be required to grant it the required access.

@natasha41575
Copy link
Contributor Author

I poked around the history of the problem file, and it seems to me this never worked. Is that correct?

It seems that this is the case, and I agree that you're right, it probably isn't great to allow an unrestricted path to be mounted in the container.

However, this lack of mounts would block usage of the render-helm-chart KRM function for local charts and I don't think it would be practical to wait for the Plugin Graduation and Catalog to be completed before asking users to migrate from the builtin Helm plugin.

WDYT about restricting mounted paths to the same load restrictions as all other paths (i.e. it must be in the current kustomization directory)? If we do that, would that alleviate your concerns regarding security?

@KnVerey
Copy link
Contributor

KnVerey commented Mar 22, 2022

WDYT about restricting mounted paths to the same load restrictions as all other paths (i.e. it must be in the current kustomization directory)? If we do that, would that alleviate your concerns regarding security?

Yes, this would work for me.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2022
@natasha41575
Copy link
Contributor Author

@KnVerey awesome, PR is updated, with a check to ensure that the mounted paths are in the current directory. I added a test TestFnContainerMountsLoadRestrictions for this restriction as well.

@natasha41575 natasha41575 force-pushed the FnMountIssue branch 2 times, most recently from 5f65d69 to 98ccc5e Compare April 4, 2022 01:23
@KnVerey
Copy link
Contributor

KnVerey commented Apr 5, 2022

As a side note, if we are going to be promoting functions in kustomize we should probably build up a strong test suite for containerized functions and run those tests in CI. All tests needing docker are currently being skipped.

As part of #4564 I noticed that it seems like some Docker-based tests do run in CI. But only the ones in cmd/config, only if they're gated with KUSTOMIZE_DOCKER_E2E specifically, and only on Linux: https://github.com/kubernetes-sigs/kustomize/blob/master/.github/workflows/go.yml#L61. Since we aren't running in verbose mode right now you can't see it in this PR's tests, but check out this test run from my PR: https://github.com/kubernetes-sigs/kustomize/runs/5826226816?check_suite_focus=true#step:4:846

@natasha41575 natasha41575 force-pushed the FnMountIssue branch 2 times, most recently from 56a5aab to 6f56f95 Compare April 11, 2022 22:44
@@ -49,7 +49,9 @@ func (l *Loader) Config() *types.PluginConfig {

// SetWorkDir sets the working directory for this loader's plugins
func (l *Loader) SetWorkDir(wd string) {
l.pc.FnpLoadingOptions.WorkingDir = wd
if wd != string(filepath.Separator) {
l.pc.FnpLoadingOptions.WorkingDir = wd
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change, TestFnContainerEnvVars fails with the error couldn't execute function: root working directory '/' not allowed. ldr.Root() returns "/" when run at the root-level, i.e. whenever we do th.Run(".") in our tests. If we don't set the working dir here, then the function filter uses the current working directory, which I believe is correct behavior in the cases that hit this problem.

Alternatively I think we could rewrite TestFnContainerEnvVars so that it does th.Run("dir") or something, and then we won't have to have this change here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but isn't this just bypassing a real restriction we have in our function code? In other words, won't it really enable the root directory to be used as valid plugin load location, effectively allowing plugins from anywhere on the disk?

If we don't set the working dir here, then the function filter uses the current working directory, which I believe is correct behavior in the cases that hit this problem.

Can you explain more why you believe this is correct? My inclination would be that it's easier to understand what's happening if the behavior is always the same (kustomization root, not invocation dir).

Alternatively I think we could rewrite TestFnContainerEnvVars so that it does th.Run("dir") or something, and then we won't have to have this change here.

Is it true that the tests in question are always using a virtual filesystem, and that's why it's not bad for the Kustomization dir to be the root? If so, that change will make the tests more realistic, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♀️ you're right on all points. I think my brain was in another location when I wrote this part, for some reason I was thinking of the working directory as the kustomization root, which is obviously not the case and is the whole reason we are doing this anyways.

I will update the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test updated! I had to change it to use a temporary directory in the real file system because it's shelling out to docker and needs the working directory to be a real one.

@natasha41575 natasha41575 force-pushed the FnMountIssue branch 2 times, most recently from 473d005 to 864b0e0 Compare April 11, 2022 23:36
@natasha41575
Copy link
Contributor Author

According to the logs, the docker tests are running in the linux job but not the mac one. Filed #4579

@@ -0,0 +1,27 @@
replicaCount: 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why two values files? And do we actually need the whole chart for the purposes of the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pared this down to a minimal example!

kyaml/fn/runtime/container/container_test.go Outdated Show resolved Hide resolved
assert.NoError(t, err)

testDir := filepath.Join(path, "testdata", "fnmounts")
command := exec.Command("kustomize", "build", testDir, "--enable-alpha-plugins")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test exec'ing Kustomize instead of using the test framework? How do we know the binary that will be found corresponds to the version of the code we think we're testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I was trying to copy the entire testdata/fnmounts directory into the temp directory where the test framework was running, which turned out to be a headache.

But with a pared down helm chart (per your other suggestion), this is much easier to do with the test framework. Updated!

api/krusty/fnplugin_test.go Show resolved Hide resolved
kyaml/runfn/runfn.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 15, 2022
@natasha41575 natasha41575 force-pushed the FnMountIssue branch 2 times, most recently from 5ac4df0 to 008ca9a Compare April 15, 2022 17:19
@KnVerey
Copy link
Contributor

KnVerey commented Apr 18, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9d5491c into kubernetes-sigs:master Apr 18, 2022
@natasha41575 natasha41575 deleted the FnMountIssue branch April 18, 2022 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Container KRM Mounts are not mounting via function parameters
4 participants