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

JSON Patch in GameServer Controller is Lossy on Int64 #3636

Open
igooch opened this issue Feb 9, 2024 · 8 comments · May be fixed by #4090
Open

JSON Patch in GameServer Controller is Lossy on Int64 #3636

igooch opened this issue Feb 9, 2024 · 8 comments · May be fixed by #4090
Labels
awaiting-maintainer Block issues from being stale/obsolete/closed kind/bug These are bugs.

Comments

@igooch
Copy link
Collaborator

igooch commented Feb 9, 2024 •
edited
Loading

TL;DR:
JSON does not support Int64 well. The current recommendation is that anyone using the count or capacity values should avoid using extremely large numbers.

Issue with setting count or capacity for Counter to max(int64):

When the Counter capacity or count are set to max(int64) in the game server spec and running kubectl create -f fleet.yaml we get an error:
spec.template.spec.counters.sessions.capacity: Invalid value: "number": spec.template.spec.counters.sessions.capacity in body must be of type integer: "number"

When the Counter capacity or count in the game server spec are close to, but not at, max(int64) kubectl create does not error. However, when checking the details of the game server with kubectl describe the values are truncated:

spec:
  template:
    spec:
      counters:
        sessions:
          count: 8611686018427387904
          capacity: 8611686018427387904

becomes

Spec:
  Template:
    Spec:
      Counters:
        Sessions:
          Capacity:  8611686018427388000
          Count:     8611686018427388000

Debugging:

@zmerlynn root-caused the issue #3608, and found that the source issue is with jsonpatch translating values to scientific notation. A different repo ran into the same error and has a longer description of the issue evanphx/json-patch#189. We're unable to switch to this repo because the fixed method CreateMergePatch just gives back bytes, but in tests, etc. we use the jsonpatch structures. This will likely need to be an upstream fix in gomodules/jsonpatch or kubernetes-sigs/json. The takeaway is that JSON does not support int64 well, and if the controller is having this many issues there are likely other issues that would crop up elsewhere in the code, and other languages would have yet another set of issues.
@igooch igooch added the kind/bug These are bugs. label Feb 9, 2024
markmandel added a commit to markmandel/agones that referenced this issue Feb 9, 2024
@markmandel
Migrate to https://github.com/gomodules/jsonpatch
9bf03e0 
It's better we're on the same project that controller-runtime uses so we
can take advantage of any upstream security and bugfixes that may occur
- or have occurred already!

Work on googleforgames#3636 - since if that issue gets fixed, it will get fixed here.

Also, it ws a fun little fix to work on 😃
@markmandel markmandel mentioned this issue Feb 9, 2024
Merged
zmerlynn added a commit that referenced this issue Feb 10, 2024
@markmandel @zmerlynn
Migrate to https://github.com/gomodules/jsonpatch (#3639)
8b2c442 
It's better we're on the same project that controller-runtime uses so we
can take advantage of any upstream security and bugfixes that may occur
- or have occurred already!

Work on #3636 - since if that issue gets fixed, it will get fixed here.

Also, it ws a fun little fix to work on 😃

Co-authored-by: Zach Loafman <[email protected]>
@zmerlynn zmerlynn mentioned this issue Feb 13, 2024
Open
@zmerlynn
Copy link
Collaborator

Filed gomodules/jsonpatch#39

@github-actions GitHub Actions
Copy link

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

@github-actions github-actions bot added the stale Pending closure unless there is a strong objection. label Aug 15, 2024
@markmandel markmandel added awaiting-maintainer Block issues from being stale/obsolete/closed and removed stale Pending closure unless there is a strong objection. labels Aug 16, 2024
@markmandel
Copy link
Collaborator

Keeping this open to track the upstream work.

@lacroixthomas
Copy link
Collaborator

Hello,
I was looking into this issue, seems that the https://github.com/gomodules/jsonpatch is a fork of https://github.com/mattbaird/jsonpatch which is working fine with the int64 max values, the fork is currently outdated (13 commits behind)
Is there a specific reason agones is using the fork instead of the main repos ? (maybe for the go modules ?)

@markmandel
Copy link
Collaborator

https://github.com/mattbaird/jsonpatch which is working fine with the int64 max values

From memory, it was not working with int64 max values at the time we tested it. Looking at the commit history, I'm not seeing anything that indicates a fix - but please correct me if we're wrong.

the fork is currently outdated (13 commits behind)

...and 39 commits ahead. So they diverged in implementation.

It also made sense to align with controller-runtime. See implementing patch #3639 for more details.

@lacroixthomas
Copy link
Collaborator

My bad, I wasn't testing it properly 😄
Did some more test: https://go.dev/play/p/UanCz7xwaiB
Using the json.NewDecoder() with UseNumber() (https://pkg.go.dev/encoding/json#Decoder.UseNumber) seems to be enough

I will check the history of this issue to understand a bit more, will try to open a PR on the https://github.com/gomodules/jsonpatch

@markmandel
Copy link
Collaborator

I will check the history of this issue to understand a bit more, will try to open a PR on the https://github.com/gomodules/jsonpatch

Awesome. That'll be a good win for everyone!

@lacroixthomas
Copy link
Collaborator

Opened an MR on the jsonpatch repo: gomodules/jsonpatch#40
But just realised that the repo doesn't seems to be maintained anymore, last changes were a couple of years ago, not sure if it's going to be reviewed, will do some tests on agones with my fork though, shall see

@lacroixthomas lacroixthomas linked a pull request Jan 20, 2025 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
awaiting-maintainer Block issues from being stale/obsolete/closed kind/bug These are bugs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: bump version of jsonpatch for lossy max int64 lacroixthomas/agones
4 participants
@markmandel @zmerlynn @lacroixthomas @igooch