Do not panic on a manifest whose apiVersion has extra separators - #1136
Open
arpitjain099 wants to merge 1 commit into
Open
arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
GroupVersion split apiVersion by hand and panicked when the result had more than two pieces, with its own TODO on the line. apiVersion is copied straight from a user manifest, so "apiVersion: a/b/c" in a file passed to kapp deploy crashed rather than reporting anything useful. GroupKind, immediately above, already goes through the unstructured helper, which parses the same field and reports an empty group and version instead of failing. Use it here too. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Contributor
Author
|
The red That leg resolves the newest Happy to rebase once that leg is sorted, or to leave it if you would rather handle the kind config separately. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ResourceImpl.GroupVersionsplitsapiVersionby hand and panics when there are more than two pieces. The line carries its own// TODO panic:apiVersionis copied straight out of a user manifest, so a file containingapiVersion: a/b/cis enough to take kapp down instead of producing a message anyone can act on. It is reached on ordinary paths, includingAPIGroup, the permissions validators and the failing-api-services policy.GroupKind, immediately above it, already delegates to the unstructured helper. That helper parses the same field throughschema.ParseGroupVersionand falls back to an empty group and version when the value is malformed, so the two methods currently disagree about what a badapiVersionmeans. This makesGroupVersionuse the same parse, which removes the panic and leaves the well-formed cases untouched.Verification: a table test in
pkg/kapp/resourcesbuilds real resources from YAML. The core-group, named-group and empty rows pass either way, and thea/b/crow panics ondevelopand passes here.go test ./pkg/...output is otherwise unchanged againstdevelop, with no failures on either side.