add exists validation to kubectl manifests specified in skaffold config

Author: aaron-prindle

cmd/skaffold/app/cmd/fix.go

@@ -89,7 +89,7 @@ func fix(out io.Writer, configFile string, toVersion string, overwrite bool) err
 				SourceFile:     configFile,
 				IsRootConfig:   true})
 		}
-		if err := validation.Process(cfgs); err != nil {
+		if err := validation.Process(cfgs, validation.Config{CheckSourceFiles: false}); err != nil {
 			return fmt.Errorf("validating upgraded config: %w", err)
 		}
 	}

cmd/skaffold/app/cmd/runner.go

@@ -79,7 +79,7 @@ func runContext(out io.Writer, opts config.SkaffoldOptions) (*runcontext.RunCont
 	}
 	setDefaultDeployer(cfgSet)
 
-	if err := validation.Process(cfgSet); err != nil {
+	if err := validation.Process(cfgSet, validation.DefaultConfig); err != nil {
 		return nil, nil, fmt.Errorf("invalid skaffold config: %w", err)
 	}
 	var configs []*latestV1.SkaffoldConfig

cmd/skaffold/app/cmd/runner_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/validation"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/update"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
@@ -86,6 +87,7 @@ func TestCreateNewRunner(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.Override(&validation.DefaultConfig, validation.Config{CheckSourceFiles: false})
 			t.Override(&docker.NewAPIClient, func(docker.Config) (docker.LocalDaemon, error) {
 				return docker.NewLocalDaemon(&testutil.FakeAPIClient{
 					ErrVersion: true,

docs/content/en/api/skaffold.swagger.json

@@ -1019,6 +1019,7 @@ For Cancelled Error code, use range 800 to 850.<br>
 | CONFIG_UNKNOWN_API_VERSION_ERR | 1213 | Config API version not found |
 | CONFIG_UNKNOWN_VALIDATOR | 1214 | The validator is not allowed in skaffold-managed mode. |
 | CONFIG_UNKNOWN_TRANSFORMER | 1215 | The transformer is not allowed in skaffold-managed mode. |
+| CONFIG_MISSING_MANIFEST_FILE_ERR | 1216 | Manifest file not found |
 | INSPECT_UNKNOWN_ERR | 1301 | Catch-all `skaffold inspect` command error |
 | INSPECT_BUILD_ENV_ALREADY_EXISTS_ERR | 1302 | Trying to add new build environment that already exists |
 | INSPECT_BUILD_ENV_INCORRECT_TYPE_ERR | 1303 | Trying to modify build environment that doesn't exist |
@@ -1087,6 +1088,7 @@ Enum for Suggestion codes
 | CONFIG_FIX_API_VERSION | 707 | Fix config API version or upgrade the skaffold binary |
 | CONFIG_ALLOWLIST_VALIDATORS | 708 | Only the allow listed validators are acceptable in skaffold-managed mode. |
 | CONFIG_ALLOWLIST_transformers | 709 | Only the allow listed transformers are acceptable in skaffold-managed mode. |
+| CONFIG_FIX_MISSING_MANIFEST_FILE | 710 | Check mising manifest file section of config and fix as needed. |
 | INSPECT_USE_MODIFY_OR_NEW_PROFILE | 800 | Create new build env in a profile instead, or use the 'modify' command |
 | INSPECT_USE_ADD_BUILD_ENV | 801 | Check profile selection, or use the 'add' command instead |
 | INSPECT_CHECK_INPUT_PROFILE | 802 | Check profile flag value |

docs/content/en/docs/references/api/grpc.md

@@ -73,6 +73,16 @@ func GetAllConfigs(opts config.SkaffoldOptions) ([]*latestV1.SkaffoldConfig, err
 	return cfgs, nil
 }
 
+// GetBase returns the correct base address (work dir) for a given config file
+func GetBase(isDependency bool, cfgFile string) (string, error) {
+	if isDependency {
+		logrus.Tracef("found %s base dir for absolute path substitution within skaffold config %s", filepath.Dir(cfgFile), cfgFile)
+		return filepath.Dir(cfgFile), nil
+	}
+	logrus.Tracef("found cwd as base for absolute path substitution within skaffold config %s", cfgFile)
+	return util.RealWorkDir()
+}
+
 // GetConfigSet returns the list of all skaffold configurations parsed from the target config file in addition to all resolved dependency configs as a `SkaffoldConfigSet`.
 // This struct additionally contains the file location that each skaffold configuration is parsed from.
 func GetConfigSet(opts config.SkaffoldOptions) (SkaffoldConfigSet, error) {
@@ -129,6 +139,36 @@ func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, r *record) (Ska
 		seen[cfgName] = true
 	}
 
+	// // validate that manifest files referenced in config exist
+	// for _, cfg := range parsed {
+	// 	if cfg.(*latestV1.SkaffoldConfig).Deploy.KubectlDeploy == nil {
+	// 		break
+	// 	}
+	// 	manifests := cfg.(*latestV1.SkaffoldConfig).Deploy.KubectlDeploy.Manifests
+	// 	// TODO(6050) validate for other deploy types - helm, kpt, etc.
+	// 	for _, pattern := range manifests {
+	// 		if util.IsURL(pattern) {
+	// 			continue
+	// 		}
+	// 		// handle absolute manifest paths
+	// 		base := ""
+	// 		if !filepath.IsAbs(pattern) {
+	// 			base, err = getBase(cfgOpts)
+	// 			if err != nil {
+	// 				return nil, err
+	// 			}
+	// 		}
+
+	// 		expanded, err := filepath.Glob(filepath.Join(base, pattern))
+	// 		if err != nil {
+	// 			return nil, err
+	// 		}
+	// 		if len(expanded) == 0 {
+	// 			return nil, sErrors.ConfigHasMissingManifestFileErr(cfgOpts.file, pattern, base)
+	// 		}
+	// 	}
+	// }
+
 	var configs SkaffoldConfigSet
 	for i, cfg := range parsed {
 		config := cfg.(*latestV1.SkaffoldConfig)
@@ -176,7 +216,7 @@ func processEachConfig(config *latestV1.SkaffoldConfig, cfgOpts configOpts, opts
 	// if `opts.MakePathsAbsolute` is set, use that as condition to decide on making file paths absolute for all configs or none at all.
 	// This is used when the parsed config is marshalled out (for commands like `skaffold diagnose` or `skaffold inspect`), we want to retain the original relative paths in the output files.
 	if isMakePathsAbsoluteSet(opts) || (opts.MakePathsAbsolute == nil && cfgOpts.isDependency) {
-		base, err := getBase(cfgOpts)
+		base, err := GetBase(cfgOpts.isDependency, cfgOpts.file)
 		if err != nil {
 			return nil, sErrors.ConfigSetAbsFilePathsErr(config.Metadata.Name, cfgOpts.file, err)
 		}
@@ -359,15 +399,6 @@ func isMakePathsAbsoluteSet(opts config.SkaffoldOptions) bool {
 	return opts.MakePathsAbsolute != nil && (*opts.MakePathsAbsolute)
 }
 
-func getBase(cfgOpts configOpts) (string, error) {
-	if cfgOpts.isDependency {
-		logrus.Tracef("found %s base dir for absolute path substitution within skaffold config %s", filepath.Dir(cfgOpts.file), cfgOpts.file)
-		return filepath.Dir(cfgOpts.file), nil
-	}
-	logrus.Tracef("found cwd as base for absolute path substitution within skaffold config %s", cfgOpts.file)
-	return util.RealWorkDir()
-}
-
 func isRemoteConfig(file string, opts config.SkaffoldOptions) (bool, error) {
 	dir, err := git.GetRepoCacheDir(opts)
 	if err != nil {

integration/testdata/build/k8s/dummy.yaml

@@ -88,7 +88,7 @@ func checkSkaffoldConfig(t *testutil.T, yaml []byte) {
 		t.CheckNoError(err)
 		cfgs = append(cfgs, cfg)
 	}
-	err = Process(cfgs)
+	err = Process(cfgs, Config{CheckSourceFiles: false})
 	t.CheckNoError(err)
 }
 

integration/testdata/generate_pipeline/existing_oncluster/k8s-yaml/dummy.yaml

@@ -19,6 +19,7 @@ package validation
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"reflect"
 	"regexp"
 	"strings"
@@ -41,11 +42,16 @@ import (
 var (
 	// for testing
 	validateYamltags       = yamltags.ValidateStruct
+	DefaultConfig          = Config{CheckSourceFiles: true}
 	dependencyAliasPattern = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
 )
 
+type Config struct {
+	CheckSourceFiles bool
+}
+
 // Process checks if the Skaffold pipeline is valid and returns all encountered errors as a concatenated string
-func Process(configs parser.SkaffoldConfigSet) error {
+func Process(configs parser.SkaffoldConfigSet, validateConfig Config) error {
 	var errs = validateImageNames(configs)
 	for _, config := range configs {
 		var cfgErrs []error
@@ -63,6 +69,10 @@ func Process(configs parser.SkaffoldConfigSet) error {
 	}
 	errs = append(errs, validateArtifactDependencies(configs)...)
 	errs = append(errs, validateSingleKubeContext(configs)...)
+	if validateConfig.CheckSourceFiles {
+		// TODO(6050) validate for other deploy types - helm, kpt, etc.
+		errs = append(errs, validateKubectlManifests(configs)...)
+	}
 	if len(errs) == 0 {
 		return nil
 	}
@@ -581,3 +591,51 @@ func wrapWithContext(config *parser.SkaffoldConfigEntry, errs ...error) []error
 	}
 	return errs
 }
+
+// validateKubectlManifests
+// - validates that kubectl manifest files specified in the skaffold config exist
+func validateKubectlManifests(configs parser.SkaffoldConfigSet) (errs []error) {
+	for _, c := range configs {
+		if c.IsRemote {
+			continue
+		}
+		if c.Deploy.KubectlDeploy == nil {
+			continue
+		}
+		// validate that manifest files referenced in config exist
+		for _, pattern := range c.Deploy.KubectlDeploy.Manifests {
+			if util.IsURL(pattern) {
+				continue
+			}
+			// handle absolute manifest paths
+			base := ""
+			var err error
+			if !filepath.IsAbs(pattern) {
+				base, err = parser.GetBase(!c.IsRootConfig, c.SourceFile)
+				if err != nil {
+					errs = append(errs, err)
+				}
+			}
+
+			expanded, err := filepath.Glob(filepath.Join(base, pattern))
+			if err != nil {
+				errs = append(errs, err)
+			}
+			if len(expanded) == 0 {
+				msg := fmt.Sprintf("skaffold config named %q referenced file %q that could not be found", c.SourceFile, pattern)
+				errs = append(errs, sErrors.NewError(fmt.Errorf(msg),
+					proto.ActionableErr{
+						Message: msg,
+						ErrCode: proto.StatusCode_CONFIG_MISSING_MANIFEST_FILE_ERR,
+						Suggestions: []*proto.Suggestion{
+							{
+								SuggestionCode: proto.SuggestionCode_CONFIG_FIX_MISSING_MANIFEST_FILE,
+								Action:         fmt.Sprintf("Verify that file %q referenced in config %q exists at %q and the path and naming are correct", pattern, c.SourceFile, filepath.Join(base, pattern)),
+							},
+						},
+					}))
+			}
+		}
+	}
+	return errs
+}

integration/testdata/generate_pipeline/existing_other/k8s-yaml/dummy.yaml

@@ -20,6 +20,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/docker/docker/api/types"
@@ -90,7 +92,8 @@ func TestValidateSchema(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			err := Process(parser.SkaffoldConfigSet{&parser.SkaffoldConfigEntry{SkaffoldConfig: test.cfg}})
+			err := Process(parser.SkaffoldConfigSet{&parser.SkaffoldConfigEntry{SkaffoldConfig: test.cfg}},
+				Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -511,7 +514,7 @@ func TestValidateNetworkMode(t *testing.T) {
 							Artifacts: test.artifacts,
 						},
 					},
-				}}})
+				}}}, Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -812,7 +815,7 @@ func TestValidateSyncRules(t *testing.T) {
 							Artifacts: test.artifacts,
 						},
 					},
-				}}})
+				}}}, Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -970,7 +973,7 @@ func TestValidateImageNames(t *testing.T) {
 							},
 						},
 					},
-				})
+				}, Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -1074,7 +1077,7 @@ func TestValidateJibPluginType(t *testing.T) {
 						},
 					},
 				},
-			}})
+			}}, Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -1108,7 +1111,7 @@ func TestValidateLogsConfig(t *testing.T) {
 							},
 						},
 					},
-				}}})
+				}}}, Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -1441,7 +1444,7 @@ func TestValidateTaggingPolicy(t *testing.T) {
 						},
 					},
 				},
-			})
+			}, Config{CheckSourceFiles: false})
 
 			t.CheckError(test.shouldErr, err)
 		})
@@ -1507,3 +1510,71 @@ func TestValidateCustomTest(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateKubectlManifests(t *testing.T) {
+	tempDir := t.TempDir()
+	tests := []struct {
+		description string
+		configs     []*latestV1.SkaffoldConfig
+		files       []string
+		shouldErr   bool
+	}{
+		{
+			description: "specified manifest file exists",
+			configs: []*latestV1.SkaffoldConfig{
+				{
+					Pipeline: latestV1.Pipeline{
+						Deploy: latestV1.DeployConfig{
+							DeployType: latestV1.DeployType{
+								KubectlDeploy: &latestV1.KubectlDeploy{
+									Manifests: []string{filepath.Join(tempDir, "validation-test-exists.yaml")},
+								},
+							},
+						},
+					},
+				},
+			},
+			files: []string{"validation-test-exists.yaml"},
+		},
+		{
+			description: "specified manifest file does not exist",
+			configs: []*latestV1.SkaffoldConfig{
+				{
+					Pipeline: latestV1.Pipeline{
+						Deploy: latestV1.DeployConfig{
+							DeployType: latestV1.DeployType{
+								KubectlDeploy: &latestV1.KubectlDeploy{
+									Manifests: []string{filepath.Join(tempDir, "validation-test-missing.yaml")},
+								},
+							},
+						},
+					},
+				},
+			},
+			files:     []string{},
+			shouldErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			for _, file := range test.files {
+				_, err := os.Create(filepath.Join(tempDir, file))
+				if err != nil {
+					t.Errorf("error creating manifest file %s: %v", file, err)
+				}
+			}
+
+			set := parser.SkaffoldConfigSet{}
+			for _, c := range test.configs {
+				set = append(set, &parser.SkaffoldConfigEntry{SkaffoldConfig: c})
+			}
+			errs := validateKubectlManifests(set)
+			var err error
+			if len(errs) > 0 {
+				err = errs[0]
+			}
+			t.CheckError(test.shouldErr, err)
+		})
+	}
+}