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.

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

kube-openapi tests fail at newer versions of go #501

Closed
jpbetz opened this issue Aug 12, 2024 · 7 comments · Fixed by #503
Closed

kube-openapi tests fail at newer versions of go #501

jpbetz opened this issue Aug 12, 2024 · 7 comments · Fixed by #503

Comments

@jpbetz
Copy link
Contributor

jpbetz commented Aug 12, 2024

Since we depend on kube-openapi from k8s/k8s and other projects using newer versions of go, we need to bump the go version of kube-openapi and address this test failure to ensure it is not going to cause problems downstream.

How to reproduce:

gvm use go1.22.2
go test -race ./cmd/... ./pkg/...


?   	k8s.io/kube-openapi/cmd/openapi-gen	[no test files]
?   	k8s.io/kube-openapi/cmd/openapi-gen/args	[no test files]
?   	k8s.io/kube-openapi/cmd/openapi2smd	[no test files]
?   	k8s.io/kube-openapi/pkg/builder3/util	[no test files]
ok  	k8s.io/kube-openapi/pkg/aggregator	(cached)
?   	k8s.io/kube-openapi/pkg/common	[no test files]
?   	k8s.io/kube-openapi/pkg/common/restfuladapter	[no test files]
ok  	k8s.io/kube-openapi/pkg/builder	(cached)
ok  	k8s.io/kube-openapi/pkg/builder3	(cached)
ok  	k8s.io/kube-openapi/pkg/cached	(cached)
?   	k8s.io/kube-openapi/pkg/util/jsontesting	[no test files]
?   	k8s.io/kube-openapi/pkg/util/proto/testing	[no test files]
?   	k8s.io/kube-openapi/pkg/util/sets	[no test files]
?   	k8s.io/kube-openapi/pkg/validation/strfmt/bson	[no test files]
ok  	k8s.io/kube-openapi/pkg/generators	7.610s
ok  	k8s.io/kube-openapi/pkg/generators/rules	(cached)
ok  	k8s.io/kube-openapi/pkg/handler	(cached)
ok  	k8s.io/kube-openapi/pkg/handler3	(cached)
ok  	k8s.io/kube-openapi/pkg/idl	(cached) [no tests to run]
ok  	k8s.io/kube-openapi/pkg/internal	(cached)
--- FAIL: TestMarshal (0.06s)
    --- FAIL: TestMarshal/Structs/OmitZero/NonZero (0.00s)
        arshal_test.go:3690: arshal_test.go:1346: Marshal output mismatch:
            got  {
            	"Bool": true,
            	"String": " ",
            	"Bytes": "",
            	"Int": 1,
            	"Uint": 1,
            	"Map": {},
            	"StructScalars": {
            		"Bool": false,
            		"String": "",
            		"Bytes": "",
            		"Int": 0,
            		"Uint": 0,
            		"Float": 0
            	},
            	"StructMaps": {
            		"MapBool": {},
            		"MapString": {},
            		"MapBytes": {},
            		"MapInt": {},
            		"MapUint": {},
            		"MapFloat": {}
            	},
            	"StructSlices": {
            		"SliceBool": [],
            		"SliceString": [],
            		"SliceBytes": [],
            		"SliceInt": [],
            		"SliceUint": [],
            		"SliceFloat": []
            	},
            	"Slice": [],
            	"Array": [
            		" "
            	],
            	"Pointer": {},
            	"Interface": null
            }
            want {
            	"Bool": true,
            	"String": " ",
            	"Bytes": "",
            	"Int": 1,
            	"Uint": 1,
            	"Float": -0,
            	"Map": {},
            	"StructScalars": {
            		"Bool": false,
            		"String": "",
            		"Bytes": "",
            		"Int": 0,
            		"Uint": 0,
            		"Float": 0
            	},
            	"StructMaps": {
            		"MapBool": {},
            		"MapString": {},
            		"MapBytes": {},
            		"MapInt": {},
            		"MapUint": {},
            		"MapFloat": {}
            	},
            	"StructSlices": {
            		"SliceBool": [],
            		"SliceString": [],
            		"SliceBytes": [],
            		"SliceInt": [],
            		"SliceUint": [],
            		"SliceFloat": []
            	},
            	"Slice": [],
            	"Array": [
            		" "
            	],
            	"Pointer": {},
            	"Interface": null
            }
    --- FAIL: TestMarshal/Structs/OmitEmpty/NonEmpty (0.00s)
        arshal_test.go:3690: arshal_test.go:1543: Marshal output mismatch:
            got  {
            	"Bool": true,
            	"PointerBool": true,
            	"String": "value",
            	"StringNonEmpty": "value",
            	"PointerString": "value",
            	"PointerStringNonEmpty": "value",
            	"Bytes": "dmFsdWU=",
            	"BytesNonEmpty": [
            		"value"
            	],
            	"PointerBytes": "dmFsdWU=",
            	"PointerBytesNonEmpty": [
            		"value"
            	],
            	"Float": -0,
            	"PointerFloat": -0,
            	"Map": {
            		"": ""
            	},
            	"MapNonEmpty": {
            		"key": "value"
            	},
            	"PointerMap": {
            		"": ""
            	},
            	"PointerMapNonEmpty": {
            		"key": "value"
            	},
            	"Slice": [
            		""
            	],
            	"SliceNonEmpty": [
            		"value"
            	],
            	"PointerSlice": [
            		""
            	],
            	"PointerSliceNonEmpty": [
            		"value"
            	],
            	"Interface": [
            		""
            	]
            }
            want {
            	"Bool": true,
            	"PointerBool": true,
            	"String": "value",
            	"StringNonEmpty": "value",
            	"PointerString": "value",
            	"PointerStringNonEmpty": "value",
            	"Bytes": "dmFsdWU=",
            	"BytesNonEmpty": [
            		"value"
            	],
            	"PointerBytes": "dmFsdWU=",
            	"PointerBytesNonEmpty": [
            		"value"
            	],
            	"Float": -0,
            	"PointerFloat": -0,
            	"Map": {
            		"": ""
            	},
            	"MapNonEmpty": {
            		"key": "value"
            	},
            	"PointerMap": {
            		"": ""
            	},
            	"PointerMapNonEmpty": {
            		"key": "value"
            	},
            	"Slice": [
            		""
            	],
            	"SliceNonEmpty": [
            		"value"
            	],
            	"PointerSlice": [
            		""
            	],
            	"PointerSliceNonEmpty": [
            		"value"
            	],
            	"Pointer": {
            		"Float": -0
            	},
            	"Interface": [
            		""
            	]
            }
FAIL
FAIL	k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json	29.633s
ok  	k8s.io/kube-openapi/pkg/openapiconv	(cached)
ok  	k8s.io/kube-openapi/pkg/schemaconv	(cached)

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 12, 2024

@Jefftree Do you have bandwidth to dig deeper into this?

@Jefftree
Copy link
Member

afaik the json library that we depend on is still experimental so strict compatibility isn't guaranteed. With that being said, I'm not aware of any breaking changes that would cause these test failures in later versions of go (cc @dsnet). (We're vendoring commit 540f014424240312547dcccddf11a8229ca0f463 which passes tests.

Upgrading go-json-experiment/json to HEAD does fixes the test failures. Fast forwarding the library would change the minimum go version that kube-openapi supports to 1.22 though. I remember we needed to stay on 1.20 for a while though #463 (cc @liggitt). Looks like the change will land in go1.23 golang/go#65573 which is still some time away.

@liggitt
Copy link
Member

liggitt commented Aug 13, 2024

"Float": -0, is the diff in the first one ... interesting

@liggitt
Copy link
Member

liggitt commented Aug 13, 2024

yeah, we should not bump to requiring go1.22 until the oldest kube branch we support is on go1.22 (or is building with go1.23 in ~January)

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 14, 2024

"Float": -0, is the diff in the first one ... interesting

@Jefftree can we confirm that this failure doesn't impact us? My main concern here is that we understand what is failing for downstream builds using go 1.22 to make sure we're not leaking a bug into production k8s.

@dsnet
Copy link

dsnet commented Aug 14, 2024

My apologies for the late reply, but I believe the commit that fixes the test is:
go-json-experiment/json@c2a874a

The cause of this breakage is because reflect.Value.IsZero was adjusted to report true for negative zero to match the existing language semantic of v == 0 reporting true even if v is -0. See golang/go#61827

I doubt there would be an real production impact by the change, but I'm often surprised.

@liggitt
Copy link
Member

liggitt commented Aug 15, 2024

ah, so it looks like a test-only "fix". we could pick just that change to our unit test copy while we're waiting to be able to pick up HEAD and go1.22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants