Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/skaffold/deploy/kubectl/kustomize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,16 @@ func TestDependenciesForKustomization(t *testing.T) {
- envs: [secret2.env, secret3.env]`},
expected: []string{"kustomization.yaml", "secret1.env", "secret1.file", "secret2.env", "secret2.file", "secret3.env", "secret3.file"},
},
{
description: "configMapGenerator & secretGenerator listing file sources with alternative keys",
kustomizations: map[string]string{"kustomization.yaml": `configMapGenerator:
- name: config-sample
files: [app.properties=app.local.properties]
secretGenerator:
- name: secret-sample
files: [secrets.json=secrets.local.json]`},
expected: []string{"app.local.properties", "kustomization.yaml", "secrets.local.json"},
},
{
description: "base exists locally",
kustomizations: map[string]string{"kustomization.yaml": `bases: [base]`},
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/render/renderer/kustomize/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (f TmpFSReal) GetPath(path string) (string, error) {
}

func (f TmpFSReal) validate(path string) error {
if path != f.root && !strings.HasPrefix(path, f.root+"/") {
if path != f.root && !strings.HasPrefix(path, f.root+string(filepath.Separator)) {
return fmt.Errorf("temporary file system operation is out of boundary, root: %s, trying to access %s", f.root, path)
}
return nil
Expand Down
85 changes: 70 additions & 15 deletions pkg/skaffold/render/renderer/kustomize/kustomize.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (k Kustomize) render(ctx context.Context, kustomizePath string, useKubectlK
defer fs.Cleanup()

if err := k.mirror(kustomizePath, fs); err == nil {
kustomizePath = filepath.Join(temp, kustomizePath)
kustomizePath = filepath.Join(temp, strings.TrimPrefix(kustomizePath, filepath.VolumeName(kustomizePath)))
} else {
return out, err
}
Expand Down Expand Up @@ -167,7 +167,9 @@ func (k Kustomize) mirror(kusDir string, fs TmpFS) error {
return err
}

if err := fs.WriteTo(kFile, bytes); err != nil {
pFile := strings.TrimPrefix(kFile, filepath.VolumeName(kFile))

if err := fs.WriteTo(pFile, bytes); err != nil {
return err
}

Expand Down Expand Up @@ -314,15 +316,18 @@ func (k Kustomize) mirrorFile(kusDir string, fs TmpFS, path string) error {
if sUtil.IsURL(path) {
return nil
}
pFile := filepath.Join(kusDir, path)
bytes, err := os.ReadFile(pFile)

sourceFile := filepath.Join(kusDir, path)
targetFile := strings.TrimPrefix(sourceFile, filepath.VolumeName(sourceFile))

bytes, err := os.ReadFile(sourceFile)
if err != nil {
return err
}
if err := fs.WriteTo(pFile, bytes); err != nil {
if err := fs.WriteTo(targetFile, bytes); err != nil {
return err
}
fsPath, err := fs.GetPath(pFile)
fsPath, err := fs.GetPath(targetFile)

if err != nil {
return err
Expand All @@ -336,7 +341,7 @@ func (k Kustomize) mirrorFile(kusDir string, fs TmpFS, path string) error {

err = k.applySetters.ApplyPath(fsPath)
if err != nil {
return fmt.Errorf("failed to apply setter to file %s, err: %v", pFile, err)
return fmt.Errorf("failed to apply setter to file %s, err: %v", sourceFile, err)
}
}
return nil
Expand Down Expand Up @@ -401,6 +406,18 @@ func (k Kustomize) mirrorSecretGenerators(kusDir string, fs TmpFS, args []types.
for _, arg := range args {
if arg.FileSources != nil {
for _, f := range arg.FileSources {
// Entries from FileSources can take the form: [{key}=]{path}
i := strings.IndexRune(f, '=')
if i > -1 {
f = f[i+1:]
}
if err := k.mirrorFile(kusDir, fs, f); err != nil {
return err
}
}
}
if arg.EnvSources != nil {
for _, f := range arg.EnvSources {
if err := k.mirrorFile(kusDir, fs, f); err != nil {
return err
}
Expand All @@ -414,6 +431,18 @@ func (k Kustomize) mirrorConfigMapGenerators(kusDir string, fs TmpFS, args []typ
for _, arg := range args {
if arg.FileSources != nil {
for _, f := range arg.FileSources {
// Entries from FileSources can take the form: [{key}=]{path}
i := strings.IndexRune(f, '=')
if i > -1 {
f = f[i+1:]
}
if err := k.mirrorFile(kusDir, fs, f); err != nil {
return err
}
}
}
if arg.EnvSources != nil {
for _, f := range arg.EnvSources {
if err := k.mirrorFile(kusDir, fs, f); err != nil {
return err
}
Expand Down Expand Up @@ -524,21 +553,47 @@ func DependenciesForKustomization(dir string) ([]string, error) {
}

for _, generator := range content.ConfigMapGenerator {
deps = append(deps, sUtil.AbsolutePaths(dir, generator.FileSources)...)
envs := generator.EnvSources
var sources []string

if generator.FileSources != nil {
for _, f := range generator.FileSources {
// Entries from FileSources can take the form: [{key}=]{path}
i := strings.IndexRune(f, '=')
if i > -1 {
f = f[i+1:]
}
sources = append(sources, f)
}
}

sources = append(sources, generator.EnvSources...)
if generator.EnvSource != "" {
envs = append(envs, generator.EnvSource)
sources = append(sources, generator.EnvSource)
}
deps = append(deps, sUtil.AbsolutePaths(dir, envs)...)

deps = append(deps, sUtil.AbsolutePaths(dir, sources)...)
}

for _, generator := range content.SecretGenerator {
deps = append(deps, sUtil.AbsolutePaths(dir, generator.FileSources)...)
envs := generator.EnvSources
var sources []string

if generator.FileSources != nil {
for _, f := range generator.FileSources {
// Entries from FileSources can take the form: [{key}=]{path}
i := strings.IndexRune(f, '=')
if i > -1 {
f = f[i+1:]
}
sources = append(sources, f)
}
}

sources = append(sources, generator.EnvSources...)
if generator.EnvSource != "" {
envs = append(envs, generator.EnvSource)
sources = append(sources, generator.EnvSource)
}
deps = append(deps, sUtil.AbsolutePaths(dir, envs)...)

deps = append(deps, sUtil.AbsolutePaths(dir, sources)...)
}
Comment on lines 555 to 597
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic to collect dependency sources from ConfigMapGenerator (lines 550-570) is identical to the logic for SecretGenerator (lines 572-592). To avoid this duplication and improve maintainability, you can extract this logic into a helper function.

Here's an example of what that helper function could look like:

func getGeneratorSources(fileSources, envSources []string, envSource string) []string {
	var sources []string
	for _, f := range fileSources {
		// Entries from FileSources can take the form: [{key}=]{path}
		if _, path, found := strings.Cut(f, "="); found {
			f = path
		}
		sources = append(sources, f)
	}

	sources = append(sources, envSources...)
	if envSource != "" {
		sources = append(sources, envSource)
	}
	return sources
}

Using this helper function would simplify both loops in DependenciesForKustomization:

for _, generator := range content.ConfigMapGenerator {
	sources := getGeneratorSources(generator.FileSources, generator.EnvSources, generator.EnvSource)
	deps = append(deps, sUtil.AbsolutePaths(dir, sources)...)
}

for _, generator := range content.SecretGenerator {
	sources := getGeneratorSources(generator.FileSources, generator.EnvSources, generator.EnvSource)
	deps = append(deps, sUtil.AbsolutePaths(dir, sources)...)
}

I also suggest using strings.Cut (available since Go 1.18) to make the path extraction from FileSources slightly cleaner.


return deps, nil
Expand Down
118 changes: 118 additions & 0 deletions pkg/skaffold/render/renderer/kustomize/kustomize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ limitations under the License.
package kustomize

import (
"path/filepath"
"slices"
"strings"
"testing"

"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/render"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/v2/testutil"
)

Expand Down Expand Up @@ -86,3 +91,116 @@ func TestBuildCommandArgs(t *testing.T) {
})
}
}

func TestMirror(t *testing.T) {
tests := []struct {
description string
shouldErr bool
createFiles map[string]string
touchFiles []string
}{
{
description: "mirror configmap and secret generators using env files, regular files, and regular files with keys",
shouldErr: false,
createFiles: map[string]string{
"kustomization.yaml": `configMapGenerator:
- name: app-env
envs:
- configmaps/app-env/app.env
- name: app-config
files:
- credentials.pub=configmaps/app-config/credentials.local.pub
- configmaps/app-config/setup.json

secretGenerator:
- name: app-env-secrets
envs:
- secrets/app-env-secrets/secrets.env
- name: app-config-secrets
files:
- credentials.key=secrets/app-config-secrets/credentials.local.key
- secrets/app-config-secrets/eyesonly.txt
`},
touchFiles: []string{
"configmaps/app-env/app.env",
"configmaps/app-config/credentials.local.pub",
"configmaps/app-config/setup.json",
"secrets/app-env-secrets/secrets.env",
"secrets/app-config-secrets/credentials.local.key",
"secrets/app-config-secrets/eyesonly.txt",
},
},
}

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
// Create the file structure required for the test
sourceDir := t.NewTempDir()

for path, contents := range test.createFiles {
sourceDir.Write(path, contents)
}

sourceDir.Touch(test.touchFiles...)

// Create the instance to test
mockCfg := render.MockConfig{
WorkingDir: sourceDir.Root(),
}

rc := latest.RenderConfig{
Generate: latest.Generate{
Kustomize: &latest.Kustomize{
Paths: []string{sourceDir.Root()},
},
},
}

k, err := New(mockCfg, rc, map[string]string{}, "default", "", nil, false)
t.CheckNoError(err)

// Test the mirror function on a temporary workspace
targetDir := t.NewTempDir()
fs := newTmpFS(targetDir.Root())
defer fs.Cleanup()

t_err := k.mirror(sourceDir.Root(), fs)

// Gather a list of the expected files
var expectedFiles []string

sourceFileList, err := sourceDir.List()
t.CheckNoError(err)

sourceVolName := filepath.VolumeName(sourceDir.Root())
if sourceVolName != "" {
for _, f := range sourceFileList {
expectedFiles = append(expectedFiles, strings.TrimPrefix(f, sourceVolName))
}
} else {
expectedFiles = sourceFileList
}

slices.Sort(expectedFiles)

// Gather a list of files that have been created by the mirror function
targetFileList, err := targetDir.List()
t.CheckNoError(err)

minPrefixLen := len(targetDir.Root()) + len(sourceDir.Root()) - len(sourceVolName)
prefixLen := len(targetDir.Root())

var mirroredFiles []string
for _, f := range targetFileList {
if len(f) >= minPrefixLen {
mirroredFiles = append(mirroredFiles, f[prefixLen:])
}
}

slices.Sort(mirroredFiles)

// Validate test results
t.CheckErrorAndDeepEqual(test.shouldErr, t_err, expectedFiles, mirroredFiles)
})
}
}
Loading