Skip to content

Fix panic when upgrading configurations with patches#1971

Merged
dgageot merged 3 commits intoGoogleContainerTools:masterfrom
corneliusweig:fix-panic
Apr 18, 2019
Merged

Fix panic when upgrading configurations with patches#1971
dgageot merged 3 commits intoGoogleContainerTools:masterfrom
corneliusweig:fix-panic

Conversation

@corneliusweig
Copy link
Copy Markdown
Contributor

@corneliusweig corneliusweig commented Apr 16, 2019 •
edited
Loading

This PR fixes two problems:

  1. 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.
  2. validation visits all fields in the Skaffold config, while it should only visit exported fields.

Fix #1964

@corneliusweig
Fix panic when encountering unexported fields in validation
f426e15 
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig corneliusweig mentioned this pull request Apr 16, 2019
Closed
@corneliusweig
Copy link
Copy Markdown
Contributor Author

corneliusweig commented Apr 16, 2019

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.

@corneliusweig
Fix serialization of yamlpatch.Node to JSON
051aff9 
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 <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig
Make linter happy
0338c8b 
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig corneliusweig marked this pull request as ready for review April 17, 2019 10:04
@corneliusweig corneliusweig changed the title WIP Fix panic when encountering unexported fields in validation Fix panic when upgrading configurations with patches Apr 17, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 17, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Apr 17, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 17, 2019
@dgageot dgageot merged commit 73a4422 into GoogleContainerTools:master Apr 18, 2019
@corneliusweig corneliusweig deleted the fix-panic branch April 18, 2019 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@balopat balopat Awaiting requested review from balopat

@nkubala nkubala Awaiting requested review from nkubala

@priyawadhwa priyawadhwa Awaiting requested review from priyawadhwa

@r2d4 r2d4 Awaiting requested review from r2d4

1 more reviewer

@dgageot dgageot dgageot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[v0.27.0] Skaffold fix panic

6 participants

@corneliusweig @codecov-io @dgageot @balopat @googlebot @kokoro-team