Skip to content

Commit

Permalink
add exists validation to kubectl manifests specified in skaffold config
Browse files Browse the repository at this point in the history
  • Loading branch information
aaron-prindle committed Jun 29, 2021
1 parent 9cd80d3 commit ebfd85d
Show file tree
Hide file tree
Showing 21 changed files with 354 additions and 228 deletions.
2 changes: 1 addition & 1 deletion cmd/skaffold/app/cmd/fix.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/skaffold/app/cmd/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions cmd/skaffold/app/cmd/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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,
Expand Down
45 changes: 30 additions & 15 deletions docs/content/en/api/skaffold.swagger.json

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions docs/content/en/docs/references/api/grpc.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
51 changes: 41 additions & 10 deletions pkg/skaffold/parser/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/schema/validation/samples_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
60 changes: 59 additions & 1 deletion pkg/skaffold/schema/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package validation
import (
"context"
"fmt"
"path/filepath"
"reflect"
"regexp"
"strings"
Expand All @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
15 changes: 8 additions & 7 deletions pkg/skaffold/schema/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,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)
})
Expand Down Expand Up @@ -511,7 +512,7 @@ func TestValidateNetworkMode(t *testing.T) {
Artifacts: test.artifacts,
},
},
}}})
}}}, Config{CheckSourceFiles: false})

t.CheckError(test.shouldErr, err)
})
Expand Down Expand Up @@ -812,7 +813,7 @@ func TestValidateSyncRules(t *testing.T) {
Artifacts: test.artifacts,
},
},
}}})
}}}, Config{CheckSourceFiles: false})

t.CheckError(test.shouldErr, err)
})
Expand Down Expand Up @@ -970,7 +971,7 @@ func TestValidateImageNames(t *testing.T) {
},
},
},
})
}, Config{CheckSourceFiles: false})

t.CheckError(test.shouldErr, err)
})
Expand Down Expand Up @@ -1074,7 +1075,7 @@ func TestValidateJibPluginType(t *testing.T) {
},
},
},
}})
}}, Config{CheckSourceFiles: false})

t.CheckError(test.shouldErr, err)
})
Expand Down Expand Up @@ -1108,7 +1109,7 @@ func TestValidateLogsConfig(t *testing.T) {
},
},
},
}}})
}}}, Config{CheckSourceFiles: false})

t.CheckError(test.shouldErr, err)
})
Expand Down Expand Up @@ -1441,7 +1442,7 @@ func TestValidateTaggingPolicy(t *testing.T) {
},
},
},
})
}, Config{CheckSourceFiles: false})

t.CheckError(test.shouldErr, err)
})
Expand Down
Loading

0 comments on commit ebfd85d

Please sign in to comment.