search

kubernetes-sigs / structured-merge-diff

Test cases and implementation for "server-side apply"
Apache License 2.0
105 stars 62 forks source link

Panic in TypeReflectCacheEntry.ToUnstructured if a pointer-typed field without `omitempty` specifier is attempted to be converted #229

Closed ulucinar closed 1 year ago

ulucinar commented 2 years ago

Hello, We have observed in Crossplane project's CNCF fuzzing tests that runtime.DefaultUnstructuredConverter.ToUnstructured (from k8s.io/apimachinery@v0.24.0) consuming sigs.k8s.io/structured-merge-diff/v4@v4.2.1 panics with the following sample program:

package main

import (
    metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
    "k8s.io/apimachinery/pkg/runtime"
)

type Test struct {
    Time *metav1.Time
}

func main() {
    if _, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&Test{}); err != nil {
        panic(err)
    }
}

, producing the following stacktrace:

panic: value method k8s.io/apimachinery/pkg/apis/meta/v1.Time.ToUnstructured called using nil *Time pointer

goroutine 1 [running]:
k8s.io/apimachinery/pkg/apis/meta/v1.(*Time).ToUnstructured(0x100000001?)
        <autogenerated>:1 +0x48
sigs.k8s.io/structured-merge-diff/v4/value.TypeReflectCacheEntry.ToUnstructured({0x1, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...)
        /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/v4@v4.2.1/value/reflectcache.go:188 +0x6b0
k8s.io/apimachinery/pkg/runtime.toUnstructured({0x100f7cba0?, 0x140001aa3d0?, 0x0?}, {0x100efef60?, 0x14000193f10?, 0x0?})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:655 +0x8a0
k8s.io/apimachinery/pkg/runtime.structToUnstructured({0x100f1f260?, 0x140001aa3d0?, 0x100f7f5e0?}, {0x100f0a3c0?, 0x140001aa3d8?, 0x100a61548?})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:843 +0x7ac
k8s.io/apimachinery/pkg/runtime.toUnstructured({0x100f1f260?, 0x140001aa3d0?, 0x140001aa3d0?}, {0x100f0a3c0?, 0x140001aa3d8?, 0x140000021a0?})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:692 +0x7f4
k8s.io/apimachinery/pkg/runtime.(*unstructuredConverter).ToUnstructured(0x101277530, {0x100edca80?, 0x140001aa3d0})
        /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:586 +0x364
main.main()
        /Users/alper/data/workspaces/github.com/ulucinar/crossplane/crossplane/cmd/test/main.go:13 +0x44

The same behavior is observed also with k8s.io/apimachinery@v0.25.3, which depends on sigs.k8s.io/structured-merge-diff/v4@v4.2.3.

As a workaround, if a json tag is added with the omitempty specifier, then no panic is observed:

package main

import (
    metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
    "k8s.io/apimachinery/pkg/runtime"
)

type Test struct {
    Time *metav1.Time `json:"time,omitempty"`
}

func main() {
    if _, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&Test{}); err != nil {
        panic(err)
    }
}

As the stacktrace points, it looks like we are calling converter.ToUnstructured with a nil converter here.

Would adding a nil check for the converter variable there be appropriate (and maybe returning a nil, nil so that we do not attempt to convert a nil value, or return an error)?

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

k8s-ci-robot commented 1 year ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/structured-merge-diff/issues/229#issuecomment-1556005136): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.