[kubernetes/kubernetes] DeepCopyJSON causes a panic (#62769)
Mikhail Mazurskiy
What happened:
I have a Custom Resource with a map[string]interface{} field in the corresponding Go struct. I have generated most of the DeepCopy functions for it but for that field I have to make the deep copying myself (cannot be generated because of that interface{}). It calls runtime.DeepCopyJSON() function to do the copying.
When I run my integration tests against a cluster with KUBE_CACHE_MUTATION_DETECTOR=true I get the following panic:
goroutine 67 [running]:
vendor/k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:58 +0x16b
panic(0x53918e0, 0xc420462080)
GOROOT/src/runtime/panic.go:502 +0x24a
vendor/k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:58 +0x16b
panic(0x53918e0, 0xc420462080)
GOROOT/src/runtime/panic.go:502 +0x24a
vendor/k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:58 +0x16b
panic(0x53918e0, 0xc420462080)
GOROOT/src/runtime/panic.go:502 +0x24a
vendor/k8s.io/apimachinery/pkg/runtime.DeepCopyJSONValue(0x534f5c0, 0xc42027ac10, 0xc42027abe0, 0xc)
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:466 +0x684
vendor/k8s.io/apimachinery/pkg/runtime.DeepCopyJSONValue(0x53a8d00, 0xc42019c210, 0xc4204e9650, 0xc42002e150)
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:454 +0x1c8
vendor/k8s.io/apimachinery/pkg/runtime.DeepCopyJSONValue(0x53a8d00, 0xc42019c1e0, 0x53a9180, 0xc4200c9e18)
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:454 +0x1c8
vendor/k8s.io/apimachinery/pkg/runtime.DeepCopyJSON(0xc42019c1e0, 0xc4204e9830)
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:444 +0x49
my-type/v1.(*MyTypeConfigSet).DeepCopyInto(0xc4204e9750, 0xc4205bb738)
pkg/apis/composition/v1/types.go:64 +0x4e
my-type/v1.MyTypeConfigSet.DeepCopy(0xc42019c1e0, 0xc42019c1b0)
pkg/apis/composition/v1/zz_generated.deepcopy.go:47 +0x5a
my-type/v1.(*MyTypeSpec).DeepCopyInto(0xc4201b27e8, 0xc4203fa108)
pkg/apis/composition/v1/zz_generated.deepcopy.go:154 +0x45a
my-type/v1.(*MyType).DeepCopyInto(0xc4201b26e0, 0xc4203fa000)
pkg/apis/composition/v1/zz_generated.deepcopy.go:19 +0x12a
my-type/v1.(*MyType).DeepCopy(0xc4201b26e0, 0xc4201e8301)
pkg/apis/composition/v1/zz_generated.deepcopy.go:29 +0x5d
my-type/v1.(*MyType).DeepCopyObject(0xc4201b26e0, 0x54e63c0, 0xc4201b26e0)
pkg/apis/composition/v1/zz_generated.deepcopy.go:35 +0x39
vendor/k8s.io/client-go/tools/cache.(*defaultCacheMutationDetector).AddObject(0xc420336e00, 0x54e63c0, 0xc4201b26e0)
vendor/k8s.io/client-go/tools/cache/mutation_detector.go:99 +0xd3
vendor/k8s.io/client-go/tools/cache.(*sharedIndexInformer).HandleDeltas(0xc4201e82d0, 0x53cc3e0, 0xc420192400, 0x0, 0x0)
vendor/k8s.io/client-go/tools/cache/shared_informer.go:353 +0x1d1
vendor/k8s.io/client-go/tools/cache.(*sharedIndexInformer).HandleDeltas-fm(0x53cc3e0, 0xc420192400, 0x53cc3e0, 0xc420192400)
vendor/k8s.io/client-go/tools/cache/shared_informer.go:202 +0x56
vendor/k8s.io/client-go/tools/cache.(*DeltaFIFO).Pop(0xc4203f2000, 0xc420184020, 0x0, 0x0, 0x0, 0x0)
vendor/k8s.io/client-go/tools/cache/delta_fifo.go:444 +0x37d
vendor/k8s.io/client-go/tools/cache.(*controller).processLoop(0xc4201de080)
vendor/k8s.io/client-go/tools/cache/controller.go:150 +0x6e
...
or alternatively (why two ways to log this?):
E0418 15:02:00.239118 96944 runtime.go:66] Observed a panic: &errors.errorString{s:"cannot deep copy uint64"} (cannot deep copy uint64)
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:72
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:65
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:51
bazel-out/darwin-fastbuild/bin/external/io_bazel_rules_go/darwin_amd64_race_stripped/stdlib~/src/runtime/asm_amd64.s:573
GOROOT/src/runtime/panic.go:502
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:58
bazel-out/darwin-fastbuild/bin/external/io_bazel_rules_go/darwin_amd64_race_stripped/stdlib~/src/runtime/asm_amd64.s:573
GOROOT/src/runtime/panic.go:502
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:466
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:454
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:454
vendor/k8s.io/apimachinery/pkg/runtime/converter.go:444
mytype/v1/types.go:64
mytype/v1/zz_generated.deepcopy.go:47
mytype/v1/zz_generated.deepcopy.go:154
mytype/v1/zz_generated.deepcopy.go:19
mytype/v1/zz_generated.deepcopy.go:29
mytype/v1/zz_generated.deepcopy.go:35
vendor/k8s.io/client-go/tools/cache/mutation_detector.go:99
vendor/k8s.io/client-go/tools/cache/shared_informer.go:353
vendor/k8s.io/client-go/tools/cache/shared_informer.go:202
vendor/k8s.io/client-go/tools/cache/delta_fifo.go:444
vendor/k8s.io/client-go/tools/cache/controller.go:150
vendor/k8s.io/client-go/tools/cache/controller.go:124
vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133
vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134
vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88
vendor/k8s.io/client-go/tools/cache/controller.go:124
vendor/k8s.io/client-go/tools/cache/shared_informer.go:227
...
cannot deep copy uint64 comes from
https://github.com/kubernetes/kubernetes/blob/3e342077d597f7a972ceaff0f06379686bb040a3/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go#L449-L468
This function was written with assumption that only int64/float64 types are generated for numbers. This assumption comes from this code
but the jsoniter unmarshaler that is used in the code to parse objects coming from server does something different:
What do we want to do? Support uint64 or only generate int64/float64 for numbers? Would be great to only have one way to do things.
Environment:
Kubernetes 1.10.0, master has the same code.
/kind bug
/sig api-machinery
@kubernetes/sig-api-machinery-bugs
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Mikhail Mazurskiy
There is one more place that creates uint64:
https://github.com/kubernetes/kubernetes/blob/1fcd199cf7366a15f0e94f2892cb5b2cb5c27efa/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go#L741-L792
Mikhail Mazurskiy
See PR ^ for uint64 support.
Daniel Smith
I would advise not using a uint64 since it doesn't reliably go through json.
...we should probably have a better way of telling people that, though :)
Jordan Liggitt
What do we want to do? Support uint64 or only generate int64/float64 for numbers?
We should limit the decoder to int64/float64
Mikhail Mazurskiy
Ok, I'm going to send another PR with the change to only use int64/float64.
Mikhail Mazurskiy
@thockin Could you as the author of the change to add uint64 (here https://github.com/kubernetes/kubernetes/pull/48287/files#diff-f216f544515d2fd05d66d92c5f95a248) please confirm that you are ok with removing uint64 support/generation or it is needed for something? I don't want us to introduce a regression.
Mikhail Mazurskiy
https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#primitive-types explicitly says:
Do not use unsigned integers, due to inconsistent support across languages and libraries. Just validate that the integer is non-negative if that's the case.
Mikhail Mazurskiy
Alternative PR #62981
BOOK
book987 left a comment (kubernetes/kubernetes#62769)Still seeing cannot deep copy uint64 here.
There is one more place that creates
uint64:
kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go
Lines 741 to 792 in 1fcd199
func structToUnstructured(sv, dv reflect.Value) error {
st, dt := sv.Type(), dv.Type()
if dt.Kind() == reflect.Interface && dv.NumMethod() == 0 {
dv.Set(reflect.MakeMap(mapStringInterfaceType))
dv = dv.Elem()
dt = dv.Type()
}
if dt.Kind() != reflect.Map {
return fmt.Errorf("cannot convert struct to: %v", dt.Kind())
}
realMap := dv.Interface().(map[string]interface{})for i := 0; i < st.NumField(); i++ {
fieldInfo := fieldInfoFromField(st, i)
fv := sv.Field(i)if fieldInfo.name == "-" { // This field should be skipped. continue } if fieldInfo.omitempty && isZero(fv) { // omitempty fields should be ignored. continue } if len(fieldInfo.name) == 0 { // This field is inlined. if err := toUnstructured(fv, dv); err != nil { return err } continue } switch fv.Type().Kind() { case reflect.String: realMap[fieldInfo.name] = fv.String() case reflect.Bool: realMap[fieldInfo.name] = fv.Bool() case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: realMap[fieldInfo.name] = fv.Int() case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: realMap[fieldInfo.name] = fv.Uint() case reflect.Float32, reflect.Float64: realMap[fieldInfo.name] = fv.Float() default: subv := reflect.New(dt.Elem()).Elem() if err := toUnstructured(fv, subv); err != nil { return err } dv.SetMapIndex(fieldInfo.nameValue, subv) }}
return nil
}
As @ash2k mentioned above, there's another place generate uint64 in structToUnstructured.
I've create a PR fixing it #134235
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are on a team that was mentioned.