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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 14 additions & 1 deletion api/internal/plugins/loader/loader.go
Expand Up @@ -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.

}
}

func (l *Loader) LoadGenerators(
Expand Down Expand Up @@ -228,6 +230,17 @@ func (l *Loader) makeBuiltinPlugin(r resid.Gvk) (resmap.Configurable, error) {
func (l *Loader) loadPlugin(res *resource.Resource) (resmap.Configurable, error) {
spec := fnplugin.GetFunctionSpec(res)
if spec != nil {
// validation check that function mounts are under the current kustomization directory
for _, mount := range spec.Container.StorageMounts {
if filepath.IsAbs(mount.Src) {
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
return nil, errors.New(fmt.Sprintf("plugin %s with mount path '%s' is not permitted; "+
"mount paths must be relative to the current kustomization directory", res.OrgId(), mount.Src))
}
if strings.HasPrefix(filepath.Clean(mount.Src), "../") {
return nil, errors.New(fmt.Sprintf("plugin %s with mount path '%s' is not permitted; "+
"mount paths must be under the current kustomization directory", res.OrgId(), mount.Src))
}
}
return fnplugin.NewFnPlugin(&l.pc.FnpLoadingOptions), nil
}
return l.loadExecOrGoPlugin(res.OrgId())
Expand Down
227 changes: 227 additions & 0 deletions api/krusty/fnplugin_test.go
Expand Up @@ -7,9 +7,11 @@ import (
"os"
"os/exec"
"path/filepath"
"runtime"
"testing"

"github.com/stretchr/testify/assert"
. "sigs.k8s.io/kustomize/api/krusty"
kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
"sigs.k8s.io/kustomize/kyaml/filesys"
)
Expand Down Expand Up @@ -633,3 +635,228 @@ metadata:
name: env
`)
}

func TestFnContainerMounts(t *testing.T) {
skipIfNoDocker(t)

th := kusttest_test.MakeHarness(t)
o := th.MakeOptionsPluginsEnabled()
fSys := filesys.MakeFsOnDisk()
b := MakeKustomizer(&o)
tmpDir, err := filesys.NewTmpConfirmedDir()
assert.NoError(t, err)

path, err := os.Getwd()
assert.NoError(t, err)

src := filepath.Join(path, "testdata", "charts") + string(filepath.Separator)
dst := filepath.Join(tmpDir.String(), "testdata", "charts") + string(filepath.Separator)
assert.NoError(t, os.MkdirAll(dst, 0777))

switch runtime.GOOS {
case "darwin":
cmd := exec.Command("cp", "-r", src, dst)
assert.NoError(t, cmd.Run())
case "linux":
d := dst + "."
cmd := exec.Command("cp", "-r", src, d)
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
assert.NoError(t, cmd.Run())
default:
t.SkipNow()
}

chartPath, err := filepath.Rel(tmpDir.String(), dst)
assert.NoError(t, err)

assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
generators:
- |-
apiVersion: v1alpha1
kind: RenderHelmChart
metadata:
name: demo
annotations:
config.kubernetes.io/function: |
container:
image: gcr.io/kpt-fn/render-helm-chart:v0.1.0
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
mounts:
- type: "bind"
src: "`+chartPath+`"
dst: "/tmp/charts"
helmCharts:
- name: helloworld-chart
releaseName: test
valuesFile: /tmp/charts/helloworld-values/values.yaml
`)))
m, err := b.Run(
fSys,
tmpDir.String())
assert.NoError(t, err)
yml, err := m.AsYaml()
assert.NoError(t, err)
assert.Equal(t, `apiVersion: v1
kind: ServiceAccount
metadata:
labels:
app.kubernetes.io/instance: test
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: helloworld-chart
app.kubernetes.io/version: 1.16.0
helm.sh/chart: helloworld-chart-0.1.0
name: test-helloworld-chart
---
apiVersion: v1
kind: Service
metadata:
labels:
app.kubernetes.io/instance: test
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: helloworld-chart
app.kubernetes.io/version: 1.16.0
helm.sh/chart: helloworld-chart-0.1.0
name: test-helloworld-chart
spec:
ports:
- name: http
port: 80
protocol: TCP
targetPort: http
selector:
app.kubernetes.io/instance: test
app.kubernetes.io/name: helloworld-chart
type: ClusterIP
---
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app.kubernetes.io/instance: test
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: helloworld-chart
app.kubernetes.io/version: 1.16.0
helm.sh/chart: helloworld-chart-0.1.0
name: test-helloworld-chart
spec:
replicas: 5
selector:
matchLabels:
app.kubernetes.io/instance: test
app.kubernetes.io/name: helloworld-chart
template:
metadata:
labels:
app.kubernetes.io/instance: test
app.kubernetes.io/name: helloworld-chart
spec:
containers:
- image: nginx:1.16.0
imagePullPolicy: Always
livenessProbe:
httpGet:
path: /
port: http
name: helloworld-chart
ports:
- containerPort: 80
name: http
protocol: TCP
readinessProbe:
httpGet:
path: /
port: http
resources: {}
securityContext: {}
securityContext: {}
serviceAccountName: test-helloworld-chart
---
apiVersion: v1
kind: Pod
metadata:
annotations:
helm.sh/hook: test-success
labels:
app.kubernetes.io/instance: test
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: helloworld-chart
app.kubernetes.io/version: 1.16.0
helm.sh/chart: helloworld-chart-0.1.0
name: test-helloworld-chart-test-connection
spec:
containers:
- args:
- test-helloworld-chart:80
command:
- wget
image: busybox
name: wget
restartPolicy: Never
`, string(yml))

assert.NoError(t, fSys.RemoveAll(tmpDir.String()))
}

func TestFnContainerMountsLoadRestrictions_absolute(t *testing.T) {
skipIfNoDocker(t)
th := kusttest_test.MakeHarness(t)
o := th.MakeOptionsPluginsEnabled()
fSys := filesys.MakeFsOnDisk()
b := MakeKustomizer(&o)
tmpDir, err := filesys.NewTmpConfirmedDir()
assert.NoError(t, err)
assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
generators:
- |-
apiVersion: v1alpha1
kind: RenderHelmChart
metadata:
name: demo
annotations:
config.kubernetes.io/function: |
container:
image: gcr.io/kpt-fn/render-helm-chart:v0.1.0
mounts:
- type: "bind"
src: "/tmp/dir"
dst: "/tmp/charts"
`)))
_, err = b.Run(
fSys,
tmpDir.String())
assert.Error(t, err)
assert.Contains(t, err.Error(), "loading generator plugins: plugin RenderHelmChart."+
"v1alpha1.[noGrp]/demo.[noNs] with mount path '/tmp/dir' is not permitted; mount paths must"+
" be relative to the current kustomization directory")
}

func TestFnContainerMountsLoadRestrictions_outsideCurrentDir(t *testing.T) {
KnVerey marked this conversation as resolved.
Show resolved Hide resolved
skipIfNoDocker(t)
th := kusttest_test.MakeHarness(t)
o := th.MakeOptionsPluginsEnabled()
fSys := filesys.MakeFsOnDisk()
b := MakeKustomizer(&o)
tmpDir, err := filesys.NewTmpConfirmedDir()
assert.NoError(t, err)
assert.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(`
generators:
- |-
apiVersion: v1alpha1
kind: RenderHelmChart
metadata:
name: demo
annotations:
config.kubernetes.io/function: |
container:
image: gcr.io/kpt-fn/render-helm-chart:v0.1.0
mounts:
- type: "bind"
src: "./tmp/../../dir"
dst: "/tmp/charts"
`)))
_, err = b.Run(
fSys,
tmpDir.String())
assert.Error(t, err)
assert.Contains(t, err.Error(), "loading generator plugins: plugin RenderHelmChart."+
"v1alpha1.[noGrp]/demo.[noNs] with mount path './tmp/../../dir' is not permitted; mount paths must "+
"be under the current kustomization directory")
}
6 changes: 6 additions & 0 deletions api/krusty/testdata/charts/helloworld-chart/Chart.yaml
@@ -0,0 +1,6 @@
apiVersion: v2
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
name: helloworld-chart
description: A Helm chart for Kubernetes
type: application
version: 0.1.0
appVersion: 1.16.0
21 changes: 21 additions & 0 deletions api/krusty/testdata/charts/helloworld-chart/templates/NOTES.txt
@@ -0,0 +1,21 @@
1. Get the application URL by running these commands:
{{- if .Values.ingress.enabled }}
{{- range $host := .Values.ingress.hosts }}
{{- range .paths }}
http{{ if $.Values.ingress.tls }}s{{ end }}://{{ $host.host }}{{ . }}
{{- end }}
{{- end }}
{{- else if contains "NodePort" .Values.service.type }}
export NODE_PORT=$(kubectl get --namespace {{ .Release.Namespace }} -o jsonpath="{.spec.ports[0].nodePort}" services {{ include "helloworld-chart.fullname" . }})
export NODE_IP=$(kubectl get nodes --namespace {{ .Release.Namespace }} -o jsonpath="{.items[0].status.addresses[0].address}")
echo http://$NODE_IP:$NODE_PORT
{{- else if contains "LoadBalancer" .Values.service.type }}
NOTE: It may take a few minutes for the LoadBalancer IP to be available.
You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "helloworld-chart.fullname" . }}'
export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "helloworld-chart.fullname" . }} --template "{{"{{ range (index .status.loadBalancer.ingress 0) }}{{.}}{{ end }}"}}")
echo http://$SERVICE_IP:{{ .Values.service.port }}
{{- else if contains "ClusterIP" .Values.service.type }}
export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "helloworld-chart.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}")
echo "Visit http://127.0.0.1:8080 to use your application"
kubectl --namespace {{ .Release.Namespace }} port-forward $POD_NAME 8080:80
{{- end }}
63 changes: 63 additions & 0 deletions api/krusty/testdata/charts/helloworld-chart/templates/_helpers.tpl
@@ -0,0 +1,63 @@
{{/* vim: set filetype=mustache: */}}
{{/*
Expand the name of the chart.
*/}}
{{- define "helloworld-chart.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- end -}}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "helloworld-chart.fullname" -}}
{{- if .Values.fullnameOverride -}}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- $name := default .Chart.Name .Values.nameOverride -}}
{{- if contains $name .Release.Name -}}
{{- .Release.Name | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
{{- end -}}
{{- end -}}
{{- end -}}

{{/*
Create chart name and version as used by the chart label.
*/}}
{{- define "helloworld-chart.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}}
{{- end -}}

{{/*
Common labels
*/}}
{{- define "helloworld-chart.labels" -}}
helm.sh/chart: {{ include "helloworld-chart.chart" . }}
{{ include "helloworld-chart.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end -}}

{{/*
Selector labels
*/}}
{{- define "helloworld-chart.selectorLabels" -}}
app.kubernetes.io/name: {{ include "helloworld-chart.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end -}}

{{/*
Create the name of the service account to use
*/}}
{{- define "helloworld-chart.serviceAccountName" -}}
{{- if .Values.serviceAccount.create -}}
{{ default (include "helloworld-chart.fullname" .) .Values.serviceAccount.name }}
{{- else -}}
{{ default "default" .Values.serviceAccount.name }}
{{- end -}}
{{- end -}}