Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DeepCopyJSON causes a panic #62769

Closed
ash2k opened this issue Apr 18, 2018 · 8 comments · Fixed by #62981
Closed

DeepCopyJSON causes a panic #62769

ash2k opened this issue Apr 18, 2018 · 8 comments · Fixed by #62981
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@ash2k
Copy link
Member

ash2k commented Apr 18, 2018

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

kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

Lines 449 to 468 in 3e34207

func DeepCopyJSONValue(x interface{}) interface{} {
switch x := x.(type) {
case map[string]interface{}:
clone := make(map[string]interface{}, len(x))
for k, v := range x {
clone[k] = DeepCopyJSONValue(v)
}
return clone
case []interface{}:
clone := make([]interface{}, len(x))
for i, v := range x {
clone[i] = DeepCopyJSONValue(v)
}
return clone
case string, int64, bool, float64, nil, encodingjson.Number:
return x
default:
panic(fmt.Errorf("cannot deep copy %T", x))
}
}

This function was written with assumption that only int64/float64 types are generated for numbers. This assumption comes from this code

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/json/json.go

Lines 111 to 118 in 5b27f8b

func convertNumber(n json.Number) (interface{}, error) {
// Attempt to convert to an int64 first
if i, err := n.Int64(); err == nil {
return i, nil
}
// Return a float64 (default json.Decode() behavior)
// An overflow will return an error
return n.Float64()

but the jsoniter unmarshaler that is used in the code to parse objects coming from server does something different:

kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

Lines 75 to 88 in 135d58b

case jsoniter.NumberValue:
var number json.Number
iter.ReadVal(&number)
u64, err := strconv.ParseUint(string(number), 10, 64)
if err == nil {
*(*interface{})(ptr) = u64
return
}
i64, err := strconv.ParseInt(string(number), 10, 64)
if err == nil {
*(*interface{})(ptr) = i64
return
}
f64, err := strconv.ParseFloat(string(number), 64)

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
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. labels Apr 18, 2018
@ash2k
Copy link
Member Author

ash2k commented Apr 18, 2018

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
}

@ash2k ash2k mentioned this issue Apr 18, 2018
Closed
@ash2k
Copy link
Member Author

ash2k commented Apr 18, 2018

See PR ^ for uint64 support.

@lavalamp
Copy link
Member

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 :)

@liggitt
Copy link
Member

liggitt commented Apr 19, 2018

What do we want to do? Support uint64 or only generate int64/float64 for numbers?

We should limit the decoder to int64/float64

@ash2k
Copy link
Member Author

ash2k commented Apr 20, 2018

Ok, I'm going to send another PR with the change to only use int64/float64.

@ash2k
Copy link
Member Author

ash2k commented Apr 20, 2018

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

@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018 •
edited
Loading

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.

Heh, but then it also says this:

All public integer fields MUST use the Go (u)int32 or Go (u)int64 types, not (u)int (which is ambiguous depending on target platform). Internal types may use (u)int.

Confusing.

@ash2k ash2k mentioned this issue Apr 23, 2018
Merged
@ash2k
Copy link
Member Author

ash2k commented Apr 23, 2018

Alternative PR #62981

dims pushed a commit to dims/kubernetes that referenced this issue May 7, 2018
Merge pull request kubernetes#62981 from atlassian/uint64-remove
0918155 
Automatic merge from submit-queue (batch tested with PRs 61154, 62981). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove incomplete uint64 support from JSON unmarshaling

**What this PR does / why we need it**:
Removes `uint64` support from JSON unmarshaling because:
- [unsigned types should not be used](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#primitive-types);
- big numbers (53+ bits) cannot be properly round tripped via JSON in most languages.

**Which issue(s) this PR fixes**
Fixes kubernetes#62769.

**Special notes for your reviewer**:
This is an alternative approach to kubernetes#62775.

**Release note**:
```release-note
NONE
```
/kind bug
/sig api-machinery
/cc @sttts
@hausdorff hausdorff mentioned this issue Aug 26, 2019
Closed
@travisn travisn mentioned this issue Oct 18, 2021
Merged
10 tasks
@GrahamDumpleton GrahamDumpleton mentioned this issue Jan 27, 2022
Open
@vasrem vasrem mentioned this issue Aug 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Milestone
No milestone
4 participants
@ash2k @lavalamp @liggitt @k8s-ci-robot