Fixes #5404: Skaffold configs downloaded from a url can define remote config dependencies #5405

gsquared94 · 2021-02-17T04:13:15Z

This PR makes two changes:

Fix the config parsing logic that was erroneously checking that a dependency config file exists on disk even if it's a URL.
Add missing check that multiple configs in the same file cannot have the same name.

codecov · 2021-02-17T04:23:34Z

Codecov Report

Merging #5405 (bbaf14f) into master (73d60cb) will decrease coverage by 0.14%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #5405      +/-   ##
==========================================
- Coverage   71.57%   71.43%   -0.15%     
==========================================
  Files         397      399       +2     
  Lines       14434    14516      +82     
==========================================
+ Hits        10331    10369      +38     
- Misses       3335     3380      +45     
+ Partials      768      767       -1

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/parse_config.go	`78.33% <75.00%> (+6.90%)`	⬆️
pkg/skaffold/schema/versions.go	`82.41% <0.00%> (ø)`
pkg/skaffold/schema/v2beta12/config.go	`20.58% <0.00%> (ø)`
pkg/skaffold/schema/v2beta12/upgrade.go	`100.00% <0.00%> (ø)`
pkg/skaffold/util/tar.go	`56.00% <0.00%> (+5.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d60cb...bbaf14f. Read the comment docs.

briandealwis · 2021-02-17T15:00:07Z

cmd/skaffold/app/cmd/parse_config.go

-			return nil, fmt.Errorf("skaffold config named %q found in multiple files: %q and %q", config.Metadata.Name, prevConfig, cfgOpts.file)
+		prev, found := r.configNameToFile[config.Metadata.Name]
+		if found {
+			sl := strings.SplitN(prev, "@", 2) // map value is always formatted as `file_name@config_index`


This overloading of configNameToFile seems error-prone (@ is a valid character in file names). It also seems unnecessary since it's entirely our code: we could just have the map take string → struct { name, index }. But more to the point, we don't seem to use this index anywhere else, so couldn't this search-for-duplicates just use a local variable to maintain its state?

And maybe this search-for-duplicates should be done separately?

don't know what I was doing here 🤦🏽‍♂️. Removed.

We can't effectively maintain all info in a local variable since we also want to print out the file names that cause the conflict.

briandealwis · 2021-02-17T15:02:13Z

cmd/skaffold/app/cmd/parse_config.go

@@ -108,11 +108,18 @@ func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, r *record) ([]*
 func processEachConfig(config *latest.SkaffoldConfig, cfgOpts configOpts, opts config.SkaffoldOptions, r *record, index int) ([]*latest.SkaffoldConfig, error) {


Would be helpful to describe what index represents here. (And it's → its)

fixed typo; added description

briandealwis · 2021-02-17T15:12:28Z

cmd/skaffold/app/cmd/parse_config.go

+			if prevConfig != cfgOpts.file {
+				return nil, fmt.Errorf("skaffold config named %q found in multiple files: %q and %q", config.Metadata.Name, prevConfig, cfgOpts.file)
+			}
+			if prevIndex != fmt.Sprint(index) {
+				return nil, fmt.Errorf("multiple skaffold configs named %q found in file %q", config.Metadata.Name, cfgOpts.file)
+			}


Can duplicate references not be handled by ParseConfigAndUpgrade()? So all we'd need is a set of seen config names?

It can't exactly be inside ParseConfigAndUpgrade() since that works with VersionedConfig which doesn't have any method like GetName. To make that change I'd have to modify every version of the skaffold config to satisfy that interface. But you're right in that I can do this check upfront. Moved.

briandealwis · 2021-02-17T15:14:53Z

cmd/skaffold/app/cmd/parse_config.go

+	if !util.IsURL(path) {
+		fi, err := os.Stat(path)
+		if err != nil {
+			if os.IsNotExist(errors.Unwrap(err)) {


The unwrap isn't correct: it's possible the ErrNoExist is actually a wrapped error. See the example at
https://golang.org/pkg/os/#IsNotExist

This is a weird one. It was existing code so I didn't mess around with it. It's calculating that the FileNotFound error from ReadConfiguration is wrapped exactly once in ParseConfig. So it unwraps exactly once to get the underlying error. 🤦🏽‍♂️

I'll convert this to a concrete error type.

The code for IsNotExist notes:

New code should use errors.Is(err, os.ErrNotExist)

errors.Is() will unwrap the error sequentially.

oh, got it. Fixed

briandealwis · 2021-02-17T15:25:06Z

cmd/skaffold/app/cmd/parse_config.go

-		return nil, fmt.Errorf("parsing dependencies for skaffold config %s: %w", cfgOpts.file, err)
-	}
-	if fi.IsDir() {
-		path = filepath.Join(path, "skaffold.yaml")
 	}
 	cfgOpts.isDependency = cfgOpts.isDependency || path != cfgOpts.file


This line is a bit puzzling and warrants a comment as I'm not sure how this situation can arise.

What's previous and current? Oh! Is configOpts from the config that required d as a dependency? That was not clear.

briandealwis · 2021-02-17T15:28:51Z

cmd/skaffold/app/cmd/parse_config_test.go

+			if test.err != "" {
+				t.CheckDeepEqual(errors.New(strings.ReplaceAll(test.err, "WORK_DIR", wd)), err, cmp.Comparer(errorsComparer))
+			} else {
+				t.CheckDeepEqual(test.expected, cfgs)
+			}


Might be easier to have an duplicateError type, and instead use a function to check the correctness of the error.

Better yet: create an error-code for this situation and test that error code. This should be reported upwards.

I will make this change in fixing #5412

The tests are failing as you're getting windows path separators. Maybe you need to replace / → \ too in the WORKDIR references.

briandealwis

LGTM once the tests pass on Windows.

briandealwis · 2021-02-18T22:39:01Z

cmd/skaffold/app/cmd/parse_config.go

+	// validate that config names are unique if specified
+	seen := make(map[string]bool)
+	for _, cfg := range parsed {
+		cfgName := cfg.(*latest.SkaffoldConfig).Metadata.Name


Maybe we should have a ConfigName() method on VersionedConfig?

briandealwis · 2021-02-18T22:41:40Z

cmd/skaffold/app/cmd/parse_config.go

-		return nil, fmt.Errorf("parsing dependencies for skaffold config %s: %w", cfgOpts.file, err)
-	}
-	if fi.IsDir() {
-		path = filepath.Join(path, "skaffold.yaml")
 	}
 	cfgOpts.isDependency = cfgOpts.isDependency || path != cfgOpts.file


What's previous and current? Oh! Is configOpts from the config that required d as a dependency? That was not clear.

briandealwis · 2021-02-18T22:44:01Z

cmd/skaffold/app/cmd/parse_config_test.go

+			if test.err != "" {
+				t.CheckDeepEqual(errors.New(strings.ReplaceAll(test.err, "WORK_DIR", wd)), err, cmp.Comparer(errorsComparer))
+			} else {
+				t.CheckDeepEqual(test.expected, cfgs)
+			}


The tests are failing as you're getting windows path separators. Maybe you need to replace / → \ too in the WORKDIR references.

briandealwis · 2021-02-18T22:46:38Z

cmd/skaffold/app/cmd/parse_config.go

+			continue
+		}
+		if seen[cfgName] {
+			return nil, fmt.Errorf("multiple skaffold configs named %q found in file %q", cfgName, cfgOpts.file)


Maybe seen should be a map of name → file? Then you could include at least two of the file nams.

fix: GoogleContainerTools#5404

7f7011c

gsquared94 requested a review from a team as a code owner February 17, 2021 04:13

pull-request-size bot added the size/M label Feb 17, 2021

google-cla bot added the cla: yes label Feb 17, 2021

gsquared94 changed the title ~~fix: #5404~~ Fixes #5404: Skaffold configs downloaded from a url can define remote configs Feb 17, 2021

gsquared94 requested a review from nkubala February 17, 2021 04:15

gsquared94 changed the title ~~Fixes #5404: Skaffold configs downloaded from a url can define remote configs~~ Fixes #5404: Skaffold configs downloaded from a url can define remote config dependencies Feb 17, 2021

briandealwis reviewed Feb 17, 2021

View reviewed changes

simplify duplicate configs check

6c32ef6

gsquared94 added this to the v1.21.0-rc milestone Feb 18, 2021

fix error unwrap

235da21

gsquared94 requested a review from briandealwis February 18, 2021 21:00

add comment about calculating isDependency

1484415

briandealwis approved these changes Feb 18, 2021

View reviewed changes

gsquared94 added 3 commits February 18, 2021 14:55

fix UT

0badb20

simplify test and add TODO

a769385

fix linter errors

bbaf14f

gsquared94 merged commit 5701c38 into GoogleContainerTools:master Feb 19, 2021

gsquared94 added the feature/multi-configs label Feb 20, 2021

gofish-bot mentioned this pull request Mar 19, 2021

skaffold 1.21.0 fishworks/fish-food#1297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5404: Skaffold configs downloaded from a url can define remote config dependencies #5405

Fixes #5404: Skaffold configs downloaded from a url can define remote config dependencies #5405

gsquared94 commented Feb 17, 2021

codecov bot commented Feb 17, 2021 •

edited

Loading

briandealwis Feb 17, 2021

gsquared94 Feb 17, 2021

gsquared94 Feb 17, 2021

briandealwis Feb 17, 2021

gsquared94 Feb 17, 2021

briandealwis Feb 17, 2021

gsquared94 Feb 17, 2021

briandealwis Feb 17, 2021

gsquared94 Feb 17, 2021

briandealwis Feb 18, 2021

gsquared94 Feb 18, 2021

briandealwis Feb 17, 2021

gsquared94 Feb 18, 2021

briandealwis Feb 18, 2021

briandealwis Feb 17, 2021

gsquared94 Feb 18, 2021

briandealwis Feb 18, 2021

briandealwis left a comment

briandealwis Feb 18, 2021

briandealwis Feb 18, 2021

briandealwis Feb 18, 2021

briandealwis Feb 18, 2021

		@@ -108,11 +108,18 @@ func getConfigs(cfgOpts configOpts, opts config.SkaffoldOptions, r record) ([]
		func processEachConfig(config latest.SkaffoldConfig, cfgOpts configOpts, opts config.SkaffoldOptions, r record, index int) ([]*latest.SkaffoldConfig, error) {

Fixes #5404: Skaffold configs downloaded from a url can define remote config dependencies #5405

Fixes #5404: Skaffold configs downloaded from a url can define remote config dependencies #5405

Conversation

gsquared94 commented Feb 17, 2021

codecov bot commented Feb 17, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 17, 2021 •

edited

Loading