Added check for empty healthcheck post-body #2288

sankalp-r · 2021-10-04T12:58:52Z

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

Which issue(s) this PR fixes:

Closes #2256

sankalp-r · 2021-10-04T12:59:57Z

/assign @roberthbailey

agones-bot · 2021-10-04T13:49:22Z

Build Failed 😱

Build Id: 5c75da81-b9be-452e-a6af-35073e2f161b

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

roberthbailey · 2021-10-04T16:06:12Z

I've created #2289 to fix this failing test.

markmandel

Clever! I was wondering how we would solve this one, since it's all generated code!

agones-bot · 2021-10-04T18:28:51Z

Build Succeeded 👏

Build Id: 2085d9c8-d203-4204-8366-208dc90802bb

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

image: gcr.io/agones-images/agones-controller:1.18.0-03fb25d
image: gcr.io/agones-images/agones-ping:1.18.0-03fb25d
Linux C++ SDK (build): agonessdk-1.18.0-03fb25d-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.18.0-03fb25d.zip

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

https://03fb25d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2288/head:pr_2288 && git checkout pr_2288
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-03fb25d

markmandel · 2021-10-04T18:30:33Z

pkg/util/https/https.go

@@ -66,3 +66,15 @@ func LogRequest(logger *logrus.Entry, r *http.Request) *logrus.Entry {
 		WithField("headers", r.Header).
 		WithField("requestURI", r.RequestURI)
 }
+
+// RequestWrapper perform checks on request
+func RequestWrapper(h http.Handler) http.Handler {


I might have been a bit quick to approve - I think this could go into main.go / move into th sdk package - since it's not a generic https utility that's used across the project.

But still like the approach!

+1

What about just making this a function in cmd/sdk-server/main.go called something like func healthcheckWrapper(...) (not exported outside of that main pkg)?

+1

@roberthbailey I agree with you.
We can put the wrapper function in cmd/sdk-server/main.go

Sounds great. Let me know once it's ready for review, and we can try to get it merged before the release candidate tomorrow.

I have made the suggested changes, but build is failing.

agones-bot · 2021-10-05T04:43:40Z

Build Failed 😱

Build Id: 3fa132df-99c6-4371-8857-7144cc619a44

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

agones-bot · 2021-10-05T08:46:06Z

Build Failed 😱

Build Id: 0a32fd4d-6b34-4fc3-ae9c-809d3fe19efd

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

agones-bot · 2021-10-05T16:17:21Z

Build Failed 😱

Build Id: 99616cf7-e5aa-4785-a49c-c560e20e3a8e

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

agones-bot · 2021-10-05T17:21:08Z

Build Failed 😱

Build Id: 1c7cf5e0-4c77-4827-9fe5-1f92a2579a86

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

sankalp-r · 2021-10-06T07:33:53Z

@roberthbailey I was not able to figure out if build failed because of my changes?

roberthbailey · 2021-10-06T20:31:27Z

I was hoping to get this in before the release candidate yesterday so I re-ran the e2e tests a few times. Since they did not pass I was not able to merge it.

The tests failed with different reasons (some of which were unrelated to your change) but if I look at the last two failures it seems like it was likely caused by this change.

One had this failure:

--- FAIL: TestFleetRecreateGameServers (0.01s)
    --- FAIL: TestFleetRecreateGameServers/gameserver_shutdown (79.61s)
        fleet_test.go:1199: 
            	Error Trace:	fleet_test.go:1199
            	Error:      	"[{{GameServer agones.dev/v1} {simple-fleet-c7g95-99lrs-2qbjd simple-fleet-c7g95-99lrs- 1633450298  df5595e7-3d36-4fdd-b6d8-63d51a1caf07 46450332 %!s(int64=6) 2021-10-05 16:15:15 +0000 UTC <nil> %!s(*int64=<nil>) map[agones.dev/fleet:simple-fleet-c7g95 agones.dev/gameserverset:simple-fleet-c7g95-99lrs] map[agones.dev/ready-container-id:containerd://831f55609e4839e7150fb30b67bb91d1e54f7ec5791f297c734ea1d5aec79091 agones.dev/sdk-version:1.18.0-e59725a] [...]" should have 10 item(s), but has 11
            	Test:       	TestFleetRecreateGameServers/gameserver_shutdown

and I also saw this in a different run:

E1005 17:20:37.640284 524 runtime.go:78] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)

sankalp-r · 2021-10-08T08:59:01Z

I was hoping to get this in before the release candidate yesterday so I re-ran the e2e tests a few times. Since they did not pass I was not able to merge it.

The tests failed with different reasons (some of which were unrelated to your change) but if I look at the last two failures it seems like it was likely caused by this change.

One had this failure:
--- FAIL: TestFleetRecreateGameServers (0.01s)
    --- FAIL: TestFleetRecreateGameServers/gameserver_shutdown (79.61s)
        fleet_test.go:1199: 
            	Error Trace:	fleet_test.go:1199
            	Error:      	"[{{GameServer agones.dev/v1} {simple-fleet-c7g95-99lrs-2qbjd simple-fleet-c7g95-99lrs- 1633450298  df5595e7-3d36-4fdd-b6d8-63d51a1caf07 46450332 %!s(int64=6) 2021-10-05 16:15:15 +0000 UTC <nil> %!s(*int64=<nil>) map[agones.dev/fleet:simple-fleet-c7g95 agones.dev/gameserverset:simple-fleet-c7g95-99lrs] map[agones.dev/ready-container-id:containerd://831f55609e4839e7150fb30b67bb91d1e54f7ec5791f297c734ea1d5aec79091 agones.dev/sdk-version:1.18.0-e59725a] [...]" should have 10 item(s), but has 11
            	Test:       	TestFleetRecreateGameServers/gameserver_shutdown
and I also saw this in a different run:

E1005 17:20:37.640284 524 runtime.go:78] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)

Maybe you can trigger the build again to see if it passes.

roberthbailey · 2021-10-08T18:33:02Z

Maybe you can trigger the build again to see if it passes.

I'll update the branch which will give it another chance to run.

agones-bot · 2021-10-08T19:02:30Z

Build Failed 😱

Build Id: 9ca5715a-443c-4693-bbd5-2879e61f2aaa

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

agones-bot · 2021-10-08T20:23:13Z

Build Succeeded 👏

Build Id: 58316eb7-4561-42f0-8b75-bf3011ef4bdd

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

image: gcr.io/agones-images/agones-controller:1.18.0-4c9e667
image: gcr.io/agones-images/agones-ping:1.18.0-4c9e667
Linux C++ SDK (build): agonessdk-1.18.0-4c9e667-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.18.0-4c9e667.zip

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

https://4c9e667-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2288/head:pr_2288 && git checkout pr_2288
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.18.0-4c9e667

markmandel · 2021-10-12T17:26:29Z

cmd/sdk-server/main.go

@@ -318,3 +318,15 @@ type config struct {
 	GRPCPort    int
 	HTTPPort    int
 }
+
+// healthCheckWrapper perform checks on request


LGTM. Do we need some kind of unit test here though? I'm leaning towards yes, just to make sure it's always doing what we expect.

We have main_test.go already, which would be a good spot for the unit test.

@roberthbailey wdyt?

@markmandel I have added a test for healthCheckWrapper.
Please have a look.

Thank you for adding test coverage!

nit: What about "healthCheckWrapper ensures that an http 400 response is returned if the healthcheck receives a request with an empty post body."

agones-bot · 2021-10-13T17:26:01Z

Build Failed 😱

Build Id: efba6070-808e-4916-8515-353bc54134a9

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

roberthbailey · 2021-10-13T17:22:55Z

cmd/sdk-server/main.go

@@ -318,3 +318,15 @@ type config struct {
 	GRPCPort    int
 	HTTPPort    int
 }
+
+// healthCheckWrapper perform checks on request


nit: What about "healthCheckWrapper ensures that an http 400 response is returned if the healthcheck receives a request with an empty post body."

cmd/sdk-server/main_test.go

agones-bot · 2021-10-13T18:36:10Z

Build Failed 😱

Build Id: a623b346-7b16-4492-8965-b44cff7d6aee

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

Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>

agones-bot · 2021-10-13T19:10:58Z

Build Failed 😱

Build Id: f60cdbf5-132c-4733-99d9-2b971b7cf4dc

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

agones-bot · 2021-10-18T19:26:52Z

Build Succeeded 👏

Build Id: 6e871de0-a81c-43a1-a3a7-ff0510acc0f9

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

image: gcr.io/agones-images/agones-controller:1.19.0-3b20c1f
image: gcr.io/agones-images/agones-ping:1.19.0-3b20c1f
Linux C++ SDK (build): agonessdk-1.19.0-3b20c1f-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.19.0-3b20c1f.zip

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

https://3b20c1f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2288/head:pr_2288 && git checkout pr_2288
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.19.0-3b20c1f

roberthbailey

Any other comments @markmandel before I merge this?

markmandel

LGTM!

google-oss-robot · 2021-10-18T23:13:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, roberthbailey, sankalp-r

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markmandel,roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-cla bot added the cla: yes label Oct 4, 2021

google-oss-robot added the size/S label Oct 4, 2021

google-oss-robot requested review from cyriltovena and markmandel October 4, 2021 12:59

google-oss-robot assigned roberthbailey Oct 4, 2021

markmandel approved these changes Oct 4, 2021

View reviewed changes

google-oss-robot assigned markmandel Oct 4, 2021

google-oss-robot added lgtm approved labels Oct 4, 2021

markmandel requested changes Oct 4, 2021

View reviewed changes

google-oss-robot removed lgtm approved labels Oct 4, 2021

sankalp-r force-pushed the health-check-interceptor branch from 03fb25d to 01aed16 Compare October 5, 2021 04:30

sankalp-r requested a review from roberthbailey October 5, 2021 07:47

roberthbailey approved these changes Oct 5, 2021

View reviewed changes

google-oss-robot added lgtm approved labels Oct 5, 2021

zifter mentioned this pull request Oct 5, 2021

Prometheus metrics: Use ServiceMonitor instead of deprecated annotation mechanism #2290

Merged

roberthbailey added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 5, 2021

google-oss-robot removed the lgtm label Oct 8, 2021

markmandel reviewed Oct 12, 2021

View reviewed changes

roberthbailey removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 12, 2021

sankalp-r force-pushed the health-check-interceptor branch from 4c9e667 to 4e50adc Compare October 13, 2021 16:57

google-oss-robot added size/M and removed size/S labels Oct 13, 2021

roberthbailey reviewed Oct 13, 2021

View reviewed changes

sankalp-r force-pushed the health-check-interceptor branch from 4e50adc to bc22b59 Compare October 13, 2021 18:12

Added check for empty healthcheck post-body

fa948b3

Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>

sankalp-r force-pushed the health-check-interceptor branch from bc22b59 to fa948b3 Compare October 13, 2021 18:37

sankalp-r requested a review from roberthbailey October 18, 2021 10:28

Merge branch 'main' into health-check-interceptor

3b20c1f

roberthbailey approved these changes Oct 18, 2021

View reviewed changes

google-oss-robot added the lgtm label Oct 18, 2021

markmandel approved these changes Oct 18, 2021

View reviewed changes

markmandel merged commit 4968da9 into googleforgames:main Oct 18, 2021

markmandel added this to the 1.19.0 milestone Oct 18, 2021

markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added check for empty healthcheck post-body #2288

Added check for empty healthcheck post-body #2288

sankalp-r commented Oct 4, 2021

sankalp-r commented Oct 4, 2021

agones-bot commented Oct 4, 2021

roberthbailey commented Oct 4, 2021

markmandel left a comment

agones-bot commented Oct 4, 2021

markmandel Oct 4, 2021

roberthbailey Oct 4, 2021

sankalp-r Oct 4, 2021

roberthbailey Oct 4, 2021

sankalp-r Oct 5, 2021

agones-bot commented Oct 5, 2021

agones-bot commented Oct 5, 2021

agones-bot commented Oct 5, 2021

agones-bot commented Oct 5, 2021

sankalp-r commented Oct 6, 2021

roberthbailey commented Oct 6, 2021

sankalp-r commented Oct 8, 2021

roberthbailey commented Oct 8, 2021

agones-bot commented Oct 8, 2021

agones-bot commented Oct 8, 2021

markmandel Oct 12, 2021

sankalp-r Oct 13, 2021

roberthbailey Oct 13, 2021

roberthbailey Oct 13, 2021

agones-bot commented Oct 13, 2021

roberthbailey Oct 13, 2021

agones-bot commented Oct 13, 2021

agones-bot commented Oct 13, 2021

agones-bot commented Oct 18, 2021

roberthbailey left a comment

markmandel left a comment

google-oss-robot commented Oct 18, 2021

Added check for empty healthcheck post-body #2288

Added check for empty healthcheck post-body #2288

Conversation

sankalp-r commented Oct 4, 2021

sankalp-r commented Oct 4, 2021

agones-bot commented Oct 4, 2021

roberthbailey commented Oct 4, 2021

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Oct 5, 2021

agones-bot commented Oct 5, 2021

agones-bot commented Oct 5, 2021

agones-bot commented Oct 5, 2021

sankalp-r commented Oct 6, 2021

roberthbailey commented Oct 6, 2021

sankalp-r commented Oct 8, 2021

roberthbailey commented Oct 8, 2021

agones-bot commented Oct 8, 2021

agones-bot commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Oct 13, 2021

Choose a reason for hiding this comment

agones-bot commented Oct 13, 2021

agones-bot commented Oct 13, 2021

agones-bot commented Oct 18, 2021

roberthbailey left a comment

Choose a reason for hiding this comment

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Oct 18, 2021