Skip to content

Do not panic on a manifest whose apiVersion has extra separators - #1136

Open
arpitjain099 wants to merge 1 commit into
carvel-dev:developfrom
arpitjain099:fix/group-version-malformed-apiversion
Open

arpitjain099 wants to merge 1 commit into
carvel-dev:developfrom
arpitjain099:fix/group-version-malformed-apiversion

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

ResourceImpl.GroupVersion splits apiVersion by hand and panics when there are more than two pieces. The line carries its own // TODO panic:

pieces := strings.Split(r.APIVersion(), "/")
if len(pieces) > 2 {
	panic(fmt.Errorf("Expected version to be of format group/version: was %s", r.APIVersion())) // TODO panic
}

apiVersion is copied straight out of a user manifest, so a file containing apiVersion: a/b/c is enough to take kapp down instead of producing a message anyone can act on. It is reached on ordinary paths, including APIGroup, 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 through schema.ParseGroupVersion and falls back to an empty group and version when the value is malformed, so the two methods currently disagree about what a bad apiVersion means. This makes GroupVersion use the same parse, which removes the panic and leaves the well-formed cases untouched.

Verification: a table test in pkg/kapp/resources builds real resources from YAML. The core-group, named-group and empty rows pass either way, and the a/b/c row panics on develop and passes here. go test ./pkg/... output is otherwise unchanged against develop, with no failures on either side.

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

Copy link
Copy Markdown
Contributor Author

The red test-all (latest) leg is the kind cluster failing to come up, not the change. It never reaches a test:

ERROR: failed to create cluster: failed to init node with kubeadm: ... kubeadm init ... exit status 1
error: your configuration file uses an old API spec: "kubeadm.k8s.io/v1beta3" (kind: "ClusterConfiguration").
Please use kubeadm v1.36 instead and run 'kubeadm config migrate' ...

That leg resolves the newest kindest/node image, and current Kubernetes no longer accepts kubeadm.k8s.io/v1beta3, so the pinned kind config is rejected before anything is installed. It failed after 21 seconds. The pinned test-all (v1.28.0) leg ran the real suite for 19 minutes and passed, as did lint.

Happy to rebase once that leg is sorted, or to leave it if you would rather handle the kind config separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants