Prevent Int64 Overflow by Kalaiselvi84 · Pull Request #3605 · agones-dev/agones

Kalaiselvi84 · 2024-01-23T00:13:38Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking

/kind bug

/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3604

Special notes for your reviewer:

agones-bot · 2024-01-23T00:22:42Z

Build Failed 😱

Build Id: b886e664-8de1-4115-bf7c-de28307cdbc5

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2024-01-23T20:11:01Z

Build Succeeded 👏

Build Id: ecdf5262-fa85-4f70-86a0-f56d37ff858c

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://8996c33-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3605/head:pr_3605 && git checkout pr_3605
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-8996c33-amd64

igooch

Could you also add in unit tests for the functionality? They can go in TestControllerUpdateFleetCounterStatus for the fleet controller and TestComputeStatus for the gameserverset controller.

igooch · 2024-01-23T21:05:27Z

 }

+// SafeAdd prevents overflow by limiting the sum to math.MaxInt64.
+func SafeAdd(x, y int64) int64 {


This function should probably go in pkg/apis/agones/v1/common.go

agones-bot · 2024-01-23T22:28:56Z

Build Failed 😱

Build Id: 2877393a-b061-4dc9-8f0d-ab82a8ec57d9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2024-01-23T23:45:00Z

Build Succeeded 👏

Build Id: a354c8fb-a61e-42d0-841b-928831cb43f8

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://cf2a0fb-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3605/head:pr_3605 && git checkout pr_3605
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-cf2a0fb-amd64

agones-bot · 2024-01-24T05:38:21Z

Build Succeeded 👏

Build Id: 9528b86c-8336-4724-8768-a1a9d4a3fef9

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://0c6a587-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3605/head:pr_3605 && git checkout pr_3605
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-0c6a587-amd64

igooch

For testing some edge cases you can change

	gsSet1.Status.Counters = map[string]agonesv1.AggregatedCounterStatus{
		"fullCounter": {
			AllocatedCount:    1000,
			AllocatedCapacity: 1000,
			Capacity:          1000,
			Count:             1000,
		},

To have maxInt64:

	gsSet1.Status.Counters = map[string]agonesv1.AggregatedCounterStatus{
		"fullCounter": {
			AllocatedCount:    9223372036854775807,
			AllocatedCapacity: 9223372036854775807,
			Capacity:          9223372036854775807,
			Count:             9223372036854775807,
		},

igooch · 2024-01-24T17:00:55Z

-			assert.Equal(t, int64(30), fleet.Status.Counters["thirdCounter"].AllocatedCapacity)
-			assert.Equal(t, int64(400), fleet.Status.Counters["thirdCounter"].Capacity)
-			assert.Equal(t, int64(21), fleet.Status.Counters["thirdCounter"].Count)
+			assert.Equal(t, agonesv1.SafeAdd(1000, 100), fleet.Status.Counters["fullCounter"].AllocatedCount)


Here we want to leave assert.Equal(t, int64(21), fleet.Status.Counters["thirdCounter"].Count). The err := c.updateFleetStatus(ctx, fleet) will call the SafeAdd as part of the method, so that's part of what we're testing.

igooch · 2024-01-24T17:03:57Z

-					AllocatedCapacity: 10,
-					Count:             50,
-					Capacity:          85,
+					AllocatedCount:    agonesv1.SafeAdd(5, 0),


For these we'll want to leave as plain int64.

You can add another Counter under multiple game servers similar to:

gs1.Status.Counters = map[string]agonesv1.CounterStatus{ "firstCounter": {Count: 5, Capacity: 10}, "secondCounter": {Count: 100, Capacity: 1000}, "fullCounter": {Count: 9223372036854775807, Capacity: 9223372036854775807}, }

And under expected add:

"fullCounter": { AllocatedCount: 9223372036854775807, AllocatedCapacity: 9223372036854775807, Count: 9223372036854775807, Capacity: 9223372036854775807, },

igooch · 2024-01-24T17:08:28Z

A general comment is that we noted there's an issue with jsonpatch here https://github.com/googleforgames/agones/blob/eb4035e1f78e7a8e0c147a013a5405f904ab9899/pkg/gameservers/controller.go#L260C21-L273 that's preventing the game server from taking an actual max(int64) value. There's a fix in a different json package evanphx/json-patch#194, but we need to incorporate that into our code.

All that is to say this code mostly looks good, but the game server won't currently take max(int64) due to a separate bug. I don't think this PR needs to wait on that code, we can solve that issue separately.

markmandel · 2024-01-24T17:26:24Z

All that is to say this code mostly looks good, but the game server won't currently take max(int64) due to a separate bug. I don't think this PR needs to wait on that code, we can solve that issue separately.

+1 on this.

markmandel

LGTM me too. Can come out of draft.

agones-bot · 2024-01-24T19:26:25Z

Build Succeeded 👏

Build Id: 278dca47-49ac-466d-b094-008344c3b93a

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://5750a60-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3605/head:pr_3605 && git checkout pr_3605
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-5750a60-amd64

Prevent Int64 Overflow

c8c1577

Kalaiselvi84 requested review from igooch and markmandel January 23, 2024 00:13

github-actions Bot added kind/bug These are bugs. size/S labels Jan 23, 2024

Kalaiselvi84 marked this pull request as draft January 23, 2024 00:59

Kalaiselvi84 added 2 commits January 23, 2024 18:58

Necessary changes

9cfc375

Modify Counter count

8996c33

igooch reviewed Jan 23, 2024

View reviewed changes

Review changes

a18ece4

update unit test

cf2a0fb

use big.Int

0c6a587

igooch reviewed Jan 24, 2024

View reviewed changes

Kalaiselvi84 added 2 commits January 24, 2024 17:43

Review changes

5675fd8

changes in controller_test.go

62ffce6

igooch approved these changes Jan 24, 2024

View reviewed changes

markmandel approved these changes Jan 24, 2024

View reviewed changes

Kalaiselvi84 marked this pull request as ready for review January 24, 2024 18:26

Merge branch 'main' into pr-3604

5750a60

markmandel merged commit 702141c into agones-dev:main Jan 24, 2024

Kalaiselvi84 deleted the pr-3604 branch March 15, 2024 01:02

Conversation

Kalaiselvi84 commented Jan 23, 2024

Uh oh!

agones-bot commented Jan 23, 2024

Uh oh!

agones-bot commented Jan 23, 2024

Uh oh!

igooch left a comment

Choose a reason for hiding this comment

Uh oh!

igooch Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

agones-bot commented Jan 23, 2024

Uh oh!

agones-bot commented Jan 23, 2024

Uh oh!

agones-bot commented Jan 24, 2024

Uh oh!

igooch left a comment

Choose a reason for hiding this comment

Uh oh!

igooch Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

igooch Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

igooch commented Jan 24, 2024

Uh oh!

markmandel commented Jan 24, 2024

Uh oh!

markmandel left a comment

Choose a reason for hiding this comment

Uh oh!

agones-bot commented Jan 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants