Fix panic when upgrading configurations with patches #1971

corneliusweig · 2019-04-16T22:25:56Z

This PR fixes two problems:

the upgrade path of skaffold yaml. The problem here was that config upgrades rely on JSON serialization, but the yamlpatch.Node did not behave as desired.
validation visits all fields in the Skaffold config, while it should only visit exported fields.

Fix #1964

Signed-off-by: Cornelius Weig <[email protected]>

corneliusweig · 2019-04-16T23:29:37Z

Unexported field access is fixed, however schema upgrade of configs with profile patches still fails:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x8e57e3]

goroutine 1 [running]:
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.handleErr(0xc00051bc68)
        vendor/gopkg.in/yaml.v2/yaml.go:249 +0x9a
panic(0x1503440, 0x27101d0)
        /usr/lib64/go/src/runtime/panic.go:522 +0x1b5
github.com/GoogleContainerTools/skaffold/vendor/github.com/krishicks/yaml-patch.(*Node).MarshalYAML(0xc00001e3e0, 0x159e480, 0xc00001e3e0, 0x7f15b9e6b968, 0xc00001e3e0)
        vendor/github.com/krishicks/yaml-patch/node.go:26 +0x33
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x159e480, 0xc0001283b0, 0x196)
        vendor/gopkg.in/yaml.v2/encode.go:98 +0x82a
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).structv.func1()
        vendor/gopkg.in/yaml.v2/encode.go:198 +0x55a
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).mappingv(0xc00023ab00, 0x0, 0x0, 0xc00051ab80)
        vendor/gopkg.in/yaml.v2/encode.go:228 +0x147
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).structv(0xc00023ab00, 0x0, 0x0, 0x15de100, 0xc000128380, 0x199)
        vendor/gopkg.in/yaml.v2/encode.go:185 +0xf7
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x15de100, 0xc000128380, 0x199)
        vendor/gopkg.in/yaml.v2/encode.go:132 +0x726
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).slicev(0xc00023ab00, 0x0, 0x0, 0x143f460, 0xc0004e8b30, 0x197)
        vendor/gopkg.in/yaml.v2/encode.go:244 +0x1f8
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x143f460, 0xc0004e8b30, 0x197)
        vendor/gopkg.in/yaml.v2/encode.go:138 +0x412
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).structv.func1()
        vendor/gopkg.in/yaml.v2/encode.go:198 +0x55a
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).mappingv(0xc00023ab00, 0x0, 0x0, 0xc00051b108)
        vendor/gopkg.in/yaml.v2/encode.go:228 +0x147
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).structv(0xc00023ab00, 0x0, 0x0, 0x15de1c0, 0xc0004e8a80, 0x199)
        vendor/gopkg.in/yaml.v2/encode.go:185 +0xf7
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x15de1c0, 0xc0004e8a80, 0x199)
        vendor/gopkg.in/yaml.v2/encode.go:132 +0x726
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).slicev(0xc00023ab00, 0x0, 0x0, 0x143f4a0, 0xc000128520, 0x197)
        vendor/gopkg.in/yaml.v2/encode.go:244 +0x1f8
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x143f4a0, 0xc000128520, 0x197)
        vendor/gopkg.in/yaml.v2/encode.go:138 +0x412
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).structv.func1()
        vendor/gopkg.in/yaml.v2/encode.go:198 +0x55a
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).mappingv(0xc00023ab00, 0x0, 0x0, 0xc00051b690)
        vendor/gopkg.in/yaml.v2/encode.go:228 +0x147
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).structv(0xc00023ab00, 0x0, 0x0, 0x15de280, 0xc000128460, 0x199)
        vendor/gopkg.in/yaml.v2/encode.go:185 +0xf7
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x15de280, 0xc000128460, 0x199)
        vendor/gopkg.in/yaml.v2/encode.go:132 +0x726
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshal(0xc00023ab00, 0x0, 0x0, 0x150aba0, 0xc000128460, 0x16)
        vendor/gopkg.in/yaml.v2/encode.go:126 +0x59f
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.(*encoder).marshalDoc(0xc00023ab00, 0x0, 0x0, 0x150aba0, 0xc000128460, 0x16)
        vendor/gopkg.in/yaml.v2/encode.go:80 +0x10f
github.com/GoogleContainerTools/skaffold/vendor/gopkg.in/yaml%2ev2.Marshal(0x150aba0, 0xc000128460, 0x0, 0x0, 0x0, 0x0, 0x0)
        vendor/gopkg.in/yaml.v2/yaml.go:203 +0x329
github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd.runFix(0x191d520, 0xc0000cc008, 0x16f799e, 0xd, 0xc00051bc00, 0x0, 0x0)
        cmd/skaffold/app/cmd/fix.go:66 +0x144
github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd.NewCmdFix.func1(0xc00033c280, 0xc0002566e0, 0x0, 0x2, 0x0, 0x0)
        cmd/skaffold/app/cmd/fix.go:37 +0x56
github.com/GoogleContainerTools/skaffold/vendor/github.com/spf13/cobra.(*Command).execute(0xc00033c280, 0xc0002566a0, 0x2, 0x2, 0xc00033c280, 0xc0002566a0)
        vendor/github.com/spf13/cobra/command.go:762 +0x465
github.com/GoogleContainerTools/skaffold/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc00019b900, 0xc0000cc008, 0x191d520, 0xc0000cc010)
        vendor/github.com/spf13/cobra/command.go:852 +0x2ec
github.com/GoogleContainerTools/skaffold/vendor/github.com/spf13/cobra.(*Command).Execute(...)
        vendor/github.com/spf13/cobra/command.go:800
github.com/GoogleContainerTools/skaffold/cmd/skaffold/app.Run(0x191d520, 0xc0000cc008, 0x191d520, 0xc0000cc010, 0xc00051bf88, 0x40584f)
        cmd/skaffold/app/skaffold.go:35 +0xd3
main.main()
        cmd/skaffold/skaffold.go:30 +0x4e

The reason seems to be schema upgrade relies on json marshalling/unmarshalling. However, the Node struct (vendor/github.com/krishicks/yaml-patch/node.go:7) does not serialize well to json, so that its internal raw pointer becomes nil. This results in the above segfault.

yamlpatch.Node hides its implementation with unexported fields, which are not accessible for JSON serialization. Fix this serializing the struct to yaml and include that as an inline string in the json. Signed-off-by: Cornelius Weig <[email protected]>

Signed-off-by: Cornelius Weig <[email protected]>

codecov-io · 2019-04-17T10:18:55Z

Codecov Report

Merging #1971 into master will increase coverage by 0.1%.
The diff coverage is 87.09%.

@@            Coverage Diff            @@
##           master    #1971     +/-   ##
=========================================
+ Coverage   52.94%   53.04%   +0.1%     
=========================================
  Files         183      183             
  Lines        7920     7942     +22     
=========================================
+ Hits         4193     4213     +20     
- Misses       3327     3328      +1     
- Partials      400      401      +1

Impacted Files	Coverage Δ
pkg/skaffold/schema/v1beta8/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/schema/latest/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/schema/v1beta5/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/schema/v1beta6/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/schema/v1beta7/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/schema/profiles.go	`90.57% <100%> (+0.2%)`	⬆️
pkg/skaffold/schema/validation/validation.go	`100% <100%> (ø)`	⬆️
pkg/skaffold/schema/util/util.go	`81.81% <84%> (+6.81%)`	⬆️

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 782c111...0338c8b. Read the comment docs.

Fix panic when encountering unexported fields in validation

f426e15

Signed-off-by: Cornelius Weig <[email protected]>

googlebot added the cla: yes label Apr 16, 2019

corneliusweig mentioned this pull request Apr 16, 2019

[v0.27.0] Skaffold fix panic #1964

Closed

corneliusweig added 2 commits April 17, 2019 11:57

Make linter happy

0338c8b

Signed-off-by: Cornelius Weig <[email protected]>

corneliusweig marked this pull request as ready for review April 17, 2019 10:04

corneliusweig requested review from balopat, dgageot, nkubala, priyawadhwa and r2d4 as code owners April 17, 2019 10:04

corneliusweig changed the title ~~WIP Fix panic when encountering unexported fields in validation~~ Fix panic when upgrading configurations with patches Apr 17, 2019

balopat added the kokoro:run runs the kokoro jobs on a PR label Apr 17, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 17, 2019

dgageot approved these changes Apr 18, 2019

View reviewed changes

dgageot merged commit 73a4422 into GoogleContainerTools:master Apr 18, 2019

corneliusweig deleted the fix-panic branch April 18, 2019 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic when upgrading configurations with patches #1971

Fix panic when upgrading configurations with patches #1971

corneliusweig commented Apr 16, 2019 •

edited

Loading

corneliusweig commented Apr 16, 2019

codecov-io commented Apr 17, 2019

Fix panic when upgrading configurations with patches #1971

Fix panic when upgrading configurations with patches #1971

Conversation

corneliusweig commented Apr 16, 2019 • edited Loading

corneliusweig commented Apr 16, 2019

codecov-io commented Apr 17, 2019

Codecov Report

corneliusweig commented Apr 16, 2019 •

edited

Loading