-
Notifications
You must be signed in to change notification settings - Fork 819
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
Adding rest to allocation endpoint #1902
Adding rest to allocation endpoint #1902
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Build Failed 😱 Build Id: eab6e3e2-87b5-4921-aea0-338604d8d9aa To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 0d181954-aa13-4bdf-904c-7d58d86cd772 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you are getting caught on some linting issues - will come back again when you get this sorted.
But looks like a nice addition 🔥
Only other thought - would need to have some documentation here as well: https://agones.dev/site/docs/advanced/allocator-service/ (maybe also an example on multi-cluster as well).
But since this is draft, that may be your intent for when it's ready to go 👍
@@ -64,6 +63,18 @@ const ( | |||
sslPort = "8443" | |||
) | |||
|
|||
// grpcHandlerFunc returns an http.Handler that delegates to grpcServer on incoming gRPC | |||
// connections or otherHandler otherwise. Copied from https://github.com/philips/grpc-gateway-example. | |||
func grpcHandlerFunc(grpcServer http.Handler, otherHandler http.Handler) http.Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's clever!
@roberthbailey we probably should have done this on the sdk sidecar! oh well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option is apparently to use a dedicated muxer but since Agones doesn't have one pulled in atm I figured it is easier to use this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this backwards compatible? I'm wondering if we could take the existing grpc port on the sidecar and add a similar handler to handle both grpc and http.
If so, we could migrate the default ports to be the same for both and then deprecate the ability to select a separate http port. After a grace period we could remove the ability to select the http port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that it is backwards compatible in the sense that grpc clients will not be aware of the change when grpcHandlerFunc
is added. I double checked it by running the example allocator-client.
Build Failed 😱 Build Id: b3424705-b609-4ee9-a06f-df628c44bab2 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: c4be2e7c-8a92-464b-aaa8-9496a7f411ed 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): To install this version:
|
Thanks for the review!
I have added docs, but not sure if it is sufficient. |
Build Failed 😱 Build Id: 844490f9-6eaa-472a-bc56-4951d0b09461 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: f5865c7a-e709-4f92-8c41-fcccc84f0123 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: d3af07ac-61e8-4f9b-98b6-6813d518019f 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): To install this version:
|
Build Succeeded 👏 Build Id: fe7720e5-6abd-4c4a-a276-ae84575cda33 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): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome - can't wait to have it in. 🍰 Some comments and questions.
cmd/allocator/main.go
Outdated
err := grpcServer.Serve(listener) | ||
logger.WithError(err).Fatal("allocation service crashed") | ||
err = server.ListenAndServeTLS("", "") | ||
logger.WithError(err).Fatal("could not listen on REST") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this error message could be better somehow... 🤔
How about?
logger.WithError(err).Fatal("could not listen on REST") | |
logger.WithError(err).Fatal("Unable to start HTTPS listener") |
WDYT?
|
||
curl --key ${KEY_FILE} --cert ${CERT_FILE} --cacert ${TLS_CA_FILE} -H "Content-Type: application/json" --data '{"namespace":"'${NAMESPACE}'"}' https://${EXTERNAL_IP}/gameserverallocation -XPOST | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth providing a sample output? It might be nice to know the format of the JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you factor out the section for setting the env variable in a separate section before gRPC and REST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have modified both allocator-service.md and multi-cluster-allocation.md to have a factored out variable setting. Let me know if you think it looks ok now?
I think as a user I don't mind a bit of code duplication in the example scripts as it allows me to copy paste the script from documentation and have it run straight away, whereas with factored out env setting I need to do 2 steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually the code snippets will be turned into a script for running multiple times but having a clear documentation on the code snippets may make it easier for the first timer to understand the flow. @markmandel do you have any recommendation on this? I am not experienced in the user experience for documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - I get it 👍
If using REST use | ||
|
||
```bash | ||
go run examples/allocator-client/main.go --ip ${EXTERNAL_IP} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be curl
if using REST? (Also, we'll need to wrap it in the feature
shortcode like above please.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed
@@ -99,6 +103,68 @@ func TestAllocator(t *testing.T) { | |||
assert.NoError(t, err) | |||
} | |||
|
|||
func TestRestAllocator(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! E2E test!
Build Succeeded 👏 Build Id: 70d5b188-aba6-4e9a-abae-6c1ba34f7c2f 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): To install this version:
|
Build Succeeded 👏 Build Id: 22450df1-c25a-4fd3-a0d4-84872a0b16e2 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): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! Happy to approve once @pooneh-m has a chance to review. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change. I left some minor comments, but looks pretty good.
```bash | ||
#!/bin/bash | ||
|
||
NAMESPACE=default # replace with any namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note before the scripts says: Follow [agones-allocator]({{< relref "allocator-service.md#send-allocation-request">}}) to set the environment variables.
You can remove the section to add the environment variables for REST similar to gRPC sample or remove the note and add a script to set the env variables before the gRPC/REST calls.
|
||
curl --key ${KEY_FILE} --cert ${CERT_FILE} --cacert ${TLS_CA_FILE} -H "Content-Type: application/json" --data '{"namespace":"'${NAMESPACE}'"}' https://${EXTERNAL_IP}/gameserverallocation -XPOST | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you factor out the section for setting the env variable in a separate section before gRPC and REST?
cmd/allocator/main.go
Outdated
cfg.ClientAuth = tls.RequireAnyClientCert | ||
cfg.VerifyPeerCertificate = h.verifyClientCertificate | ||
} | ||
|
||
// Add options for creds and OpenCensus stats handler to enable stats and tracing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the documentation to remove options for creds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Do you mean that I should remove/change the comment that says:
// Add options for creds and OpenCensus stats handler to enable stats and tracing.
or that I need to document changes to options?
I don't think this change changes the command line options as h.tlsDisabled
and h.mTLSDisabled
are still used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the documentation says it adds options for creds and ... my comment was to remote certs
from the doc as you removed the grpc.Creds(credentials.NewTLS(cfg)),
. So change it to // Add an option for OpenCensus stats handler to enable stats and tracing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Fixed it now.
func grpcHandlerFunc(grpcServer http.Handler, otherHandler http.Handler) http.Handler { | ||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
// This is a partial recreation of gRPC's internal checks https://github.com/grpc/grpc-go/pull/514/files#diff-95e9a25b738459a2d3030e1e6fa2a718R61 | ||
if r.ProtoMajor == 2 && strings.Contains(r.Header.Get("Content-Type"), "application/grpc") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not clear why ProtoMajor == 2
and not 3 for the gRPC server.
Can you please add more documentations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProtoMajor
refers to the HTTP protocol major version part. We are checking if request is HTTP/1.1 or HTTP/2. Will add that as a comment to the code.
Build Succeeded 👏 Build Id: f502201d-f731-48f6-87ec-5f47a4d652d5 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): To install this version:
|
cmd/allocator/main.go
Outdated
cfg.ClientAuth = tls.RequireAnyClientCert | ||
cfg.VerifyPeerCertificate = h.verifyClientCertificate | ||
} | ||
|
||
// Add options for creds and OpenCensus stats handler to enable stats and tracing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the documentation says it adds options for creds and ... my comment was to remote certs
from the doc as you removed the grpc.Creds(credentials.NewTLS(cfg)),
. So change it to // Add an option for OpenCensus stats handler to enable stats and tracing.
# In case of MacOS replace "base64 -d" with "base64 -D" | ||
kubectl get secret allocator-client.default -n "${NAMESPACE}" -ojsonpath="{.data.tls\.crt}" | base64 -d > "${CERT_FILE}" | ||
kubectl get secret allocator-client.default -n "${NAMESPACE}" -ojsonpath="{.data.tls\.key}" | base64 -d > "${KEY_FILE}" | ||
kubectl get secret allocator-tls-ca -n agones-system -ojsonpath="{.data.tls-ca\.crt}" | base64 -d > "${TLS_CA_FILE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move L176-L178 to the section above as they are common between the two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the lines and renamed the section to be: Reuse following snippet for both grpc and rest examples:
Not sure whether this is good heading for it so open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like Set the environment variables and store the client secrets before allocating using gRPC or REST APIs
?
Also, please make the same change for the multi-cluster-allocation.md
file to keep them in sync.
|
||
curl --key ${KEY_FILE} --cert ${CERT_FILE} --cacert ${TLS_CA_FILE} -H "Content-Type: application/json" --data '{"namespace":"'${NAMESPACE}'"}' https://${EXTERNAL_IP}/gameserverallocation -XPOST | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually the code snippets will be turned into a script for running multiple times but having a clear documentation on the code snippets may make it easier for the first timer to understand the flow. @markmandel do you have any recommendation on this? I am not experienced in the user experience for documentation.
Build Succeeded 👏 Build Id: dbc84e75-56cf-422a-a31c-3148daf7ae30 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): To install this version:
|
Build Succeeded 👏 Build Id: e1bf2947-bd58-4ad2-8b8d-fc4ff5fa29dd 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): To install this version:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kdima, pooneh-m 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:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: b6556ddb-07da-4d5b-9743-e8406d0a0d2c 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): To install this version:
|
/kind feature
What this PR does / Why we need it:
This should solve #1495
Which issue(s) this PR fixes:
Closes #1495
Special notes for your reviewer:
We are running both grpc and rest on the same port so there is no need to update the services yaml.