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

Migrade from github.com/golang/protobuf to google.golang.org/protobuf #2644

Closed
wants to merge 1 commit into from
Closed

Migrade from github.com/golang/protobuf to google.golang.org/protobuf #2644

wants to merge 1 commit into from

Conversation

govargo
Copy link
Contributor

@govargo govargo commented Jun 25, 2022

What type of PR is this?

/kind breaking
/kind cleanup
/kind feature

What this PR does / Why we need it:

I found that the github.com/golang/protobuf will be deprecated in the future when I executed make gen-all-sdk-grpc.

+ protoc -I /go/src/agones.dev/agones/proto/googleapis -I /go/src/agones.dev/agones/proto/sdk sdk.proto --grpc-gateway_out=logtostderr=true:pkg/sdk
WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated.
        A future release of golang/protobuf will delete this package,
        which has long been excluded from the compatibility promise.

+ protoc -I /go/src/agones.dev/agones/proto/googleapis -I /go/src/agones.dev/agones/proto/sdk/alpha alpha.proto --grpc-gateway_out=logtostderr=true:pkg/sdk/alpha
WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated.
        A future release of golang/protobuf will delete this package,
        which has long been excluded from the compatibility promise.

+ protoc -I /go/src/agones.dev/agones/proto/googleapis -I /go/src/agones.dev/agones/proto/sdk/beta beta.proto --grpc-gateway_out=logtostderr=true:pkg/sdk/beta
beta.proto:20:1: warning: Import google/api/annotations.proto is unused.
WARNING: Package "github.com/golang/protobuf/protoc-gen-go/generator" is deprecated.
        A future release of golang/protobuf will delete this package,
        which has long been excluded from the compatibility promise.

This PR has many changes for migration to google.golang.org/protobuf.

Added

  1. Add proto files for proto/grpc-gateway
  2. Add auto-generated files for grpc(allocation_grpc.pb.go, alpha_grpc.pb.go, beta_grpc.pb.go, sdk_grpc.pb.go, protoc-gen-openapiv2/options/annotations.pb.h, protoc-gen-openapiv2/options/openapiv2.pb.h, protoc-gen-openapiv2/annotations.pb.cc, protoc-gen-openapiv2/openapiv2.pb.cc)
  3. Add absl installation for grpc build stability to sdk-base-image
  4. Add grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger to proto files in order to define swagger.json definitions
  5. Add tags to each swagger files. This is by protoc-gen-openapiv2
  6. Add protoc-gen-openapiv2/options file's installation to sdks/cpp/CMakeLists.txt
  7. Add cmake installation for protoc-gen-openapiv2 files to sdks/cpp/sources.cmake
  8. Add build path for proto/grpc-gateway to sdks/rust/build.rs
  9. Add description about make gen-allocation-grpc
  10. Add vendor files

Changed

  1. Migrade from github.com/golang/protobuf to google.golang.org/protobuf(v1.28)
  2. Migrade from github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway v1 to github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway(v2.10.3)
  3. protoc command options (e.g. --go_out=plugins=grpc is not supported. use --go-grpc_opt)
  4. Use github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2 instead of protoc-gen-swagger
  5. Use google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.2
  6. Upgrade google.golang.org/grpc v1.44.0 to v1.46.2
  7. Upgrade grpc_release_tag v1.27.1 to v1.46.3 for sdk-base-image
  8. Upgrade grpc_release_tag v1.27.1 to v1.46.3 for sdks/cpp/cmake/prerequisites.cmake
  9. Refactor gen.sh which has difference between each language
  10. Upgrade swagger-codegen-cli 2.4.10 to v3(3.0.34) for restapi test
  11. Upgrade grpc-tools 1.8.1 to 1.11.2 for nodejs sdk
  12. Upgrade @grpc/grpc-js 1.5.4 to 1.6.7 and google-protobuf 3.19.4 to 3.20.1 for nodejs sdk
  13. Change install way for protobuf from make to cmake
  14. Upgrade Google.Protobuf 3.11.4 to 3.19.4, Grpc Grpc.Core Grpc.Tools 2.28.1 to 2.45.0 in csharp sdk
  15. Update proto files under sdks/rust/proto which is copied for rust sdk
  16. Change Inline streamingDefinitions in sdks/swagger/sdk.swagger.json, which is changed by protoc_gen_openapiv2(x-stream-definitions is not already supported)
  17. Change test/sdk/restapi/http-api-test.go.nolint code because return code was changed
  18. Change test/sdk/websocket-watch/ws-watch-test.go code because the test was not passed with the old code
  19. Change tools.go dependencies
  20. Change auto-generated files(alpha.pb.go, alpha.pb.gw.go, beta.pb.go, beta.pb.gw.go, sdk.pb.go, sdk.pb.gw.go, sdk.grpc.pb.h, sdk.pb.h, google/api/annotations.pb.h, google/api/http.pb.h, sdk.grpc.pb.cc, sdk.pb.cc, google/annotations.pb.cc, google/http.pb.cc, Alpha.cs, AlphaGrpc.cs, Sdk.cs, SdkGrpc.cs, alpha_grpc_pb.js, alpha_pb.js, sdk_grpc_pb.js, sdk_pb.js)
  21. Change vendor files

Deleted

  1. Remove github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
  2. x-stream-definitions for swagger.json is already not support. So, delete model_xstreamdefinitionssdkgameserver.go.nolint file
  3. Delete vendor files

Workaround

  1. Add jq logic to sdks/swagger/sdk.swagger.json because protoc-gen-openapiv2 doesn't work well in Stream and doesn't generate nessesary definitions. It seems that the bug occurs if both stream definition and --openapiv2_opt=disable_default_errors=true exist.
  2. Remove protoc-gen-openapiv2 definitions from C# sdk image because C# package doesn't support grpc-gateway

Which issue(s) this PR fixes:

Closes #2462

Special notes for your reviewer:

This is large PR. I am very careful for this change, but if you have any suggestions, please let me know.
Thank you.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: govargo
To complete the pull request process, please assign dzlier-gcp after the PR has been reviewed.
You can assign the PR to them by writing /assign @dzlier-gcp in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@@ -64,6 +64,7 @@ Table of Contents
* [make gen-embedded-openapi](#make-gen-embedded-openapi)
* [make gen-crd-client](#make-gen-crd-client)
* [make gen-sdk-grpc](#make-gen-sdk-grpc)
* [make gen-allocation-grpc](#make-gen-allocation-grpc)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make gen-allocation-grpc is defined in Makefile, but this was not described in build/README.md

@@ -16,35 +16,48 @@

set -ex
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligned to build/build-sdk-images/go/gen.sh

Comment on lines +40 to +41
protoc -I ${googleapis} -I ${gatewaygrpc} -I ${sdk} --plugin=protoc-gen-grpc=`which grpc_cpp_plugin` --grpc_out=${protoc_intermediate} sdk.proto
protoc -I ${googleapis} -I ${gatewaygrpc} -I ${sdk} --cpp_out=dllexport_decl=AGONES_EXPORT:${protoc_intermediate} sdk.proto ${googleapis}/google/api/annotations.proto ${googleapis}/google/api/http.proto ${gatewaygrpc}/protoc-gen-openapiv2/options/annotations.proto ${gatewaygrpc}/protoc-gen-openapiv2/options/openapiv2.proto
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unified order of -I

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 555bb65d-3145-4857-838d-d96acb91a075

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

Comment on lines +32 to +51
# Remove protoc-gen-openapiv2 definitions because C# package doesn't support grpc-gateway
sed -i -e 's/import "protoc-gen-openapiv2\/options\/annotations.proto";//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/info: {//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/title: "sdk.proto";//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -z 's/version: "version not set";\n };//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/schemes: HTTP;//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/consumes: "application\/json";//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -z 's/produces: "application\/json";\n};//' ${protoc_intermediate}/sdk/sdk.proto
sed -i -e 's/bool disabled = 1.*/bool disabled = 1;/' ${protoc_intermediate}/sdk/sdk.proto

sed -i -e 's/import "protoc-gen-openapiv2\/options\/annotations.proto";//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/info: {//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/title: "alpha.proto";//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -z 's/version: "version not set";\n };//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/schemes: HTTP;//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/consumes: "application\/json";//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -z 's/produces: "application\/json";\n};//' ${protoc_intermediate}/sdk/alpha/alpha.proto
sed -i -e 's/bool bool = 1.*/bool bool = 1;/' ${protoc_intermediate}/sdk/alpha/alpha.proto
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is workaround. Remove protoc-gen-openapiv2 definitions from C# sdk image because C# package doesn't support grpc-gateway

Comment on lines -20 to -21
# swagger gen has a bug wherein it doesn't generate the file, so we're providing it by hand
cp ./model_xstreamdefinitionssdkgameserver.go.nolint ./swagger/model_xstreamdefinitionssdkgameserver.go
Copy link
Contributor Author

@govargo govargo Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed model_xstreamdefinitionssdkgameserver.go.nolint file because latest protoc-gen-swagger and protoc-gen-openapiv2 doesn't support x-stream-definitions.
Ref: grpc-ecosystem/grpc-gateway#1112

Comment on lines +33 to +38
cd third_party/protobuf/cmake && \
mkdir -p build && cd build && \
cmake -Dprotobuf_BUILD_TESTS=OFF \
-DBUILD_SHARED_LIBS=ON \
-DCMAKE_BUILD_TYPE=Release \
.. && \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use cmake instead of make because the latest protobuf doesn't suppor make installing.

#7 651.7
#7 651.7 Installing via 'make' is no longer supported. Use cmake or bazel instead.
#7 651.7
#7 651.7 Please consult BUILDING.md to get more information.
#7 651.7
#7 651.7 make: *** [Makefile:694: stop] Error 1

@@ -21,7 +21,7 @@ FROM debian:buster

RUN apt-get update && apt-get install -y \
build-essential autoconf libtool git pkg-config curl \
automake libtool curl make g++ unzip moreutils \
automake libz-dev curl make cmake g++ unzip moreutils \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add zlib package because the following error happened.

   #7 477.7 src/core/lib/compression/message_compress.cc:25:10: fatal error: zlib.h: No such file or directory
   #7 477.7  #include <zlib.h>
   #7 477.7           ^~~~~~~~
   #7 477.7 compilation terminated.
   #7 477.7 make: *** [Makefile:810: /var/local/git/grpc/objs/opt/src/core/lib/compression/message_compress.o] Error 1

Comment on lines +7 to +11
"tags": [
{
"name": "AllocationService"
}
],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tags is generated by protoc-gen-openapiv2. This is always printed and cannot be disabled.

Comment on lines +23 to +31
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger) = {
info: {
title: "proto/allocation/allocation.proto";
version: "version not set";
};
schemes: HTTPS;
consumes: "application/json";
produces: "application/json";
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc.gateway.protoc_gen_openapiv2.options.openapiv2_swagger definition is used in swagger.json definition

@@ -124,7 +135,7 @@ message Count {

// Store a boolean result
message Bool {
bool bool = 1;
bool bool = 1 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {format: "boolean"}];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {format: "boolean"}] is not defined, the output of swagger.json is missing format: "boolean".

@@ -4,7 +4,7 @@ fn main() {
.build_server(false)
.compile(
&["proto/sdk/alpha/alpha.proto", "proto/sdk/sdk.proto"],
&["proto/googleapis", "proto/sdk/alpha", "proto/sdk"],
&["proto/googleapis", "proto/grpc-gateway", "proto/sdk/alpha", "proto/sdk"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add build path, but rust sdk use tonic instread of protoc. So there are not many changes due to this PR
#2112

@@ -229,7 +234,16 @@
"200": {
"description": "A successful response.(streaming responses)",
"schema": {
"$ref": "#/x-stream-definitions/sdkGameServer"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#/x-stream-definitions is not supported by latest protoc-gen-swagger and protoc-gen-openapiv2. It moved to inline definitions.
https://github.com/grpc-ecosystem/grpc-gateway/pull/1112/files

Comment on lines +424 to +449
"googlerpcStatus": {
"type": "object",
"properties": {
"result": {
"$ref": "#/definitions/sdkGameServer"
"code": {
"type": "integer",
"format": "int32"
},
"message": {
"type": "string"
},
"error": {
"$ref": "#/definitions/runtimeStreamError"
"details": {
"type": "array",
"items": {
"$ref": "#/definitions/protobufAny"
}
}
}
},
"protobufAny": {
"type": "object",
"properties": {
"@type": {
"type": "string"
}
},
"title": "Stream result of sdkGameServer"
"additionalProperties": {}
Copy link
Contributor Author

@govargo govargo Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is workaround. Add jq logic to sdks/swagger/sdk.swagger.json because protoc-gen-openapiv2 doesn't work well in Stream and doesn't generate nessesary definitions.
It seems that the bug occurs if both stream definition and --openapiv2_opt=disable_default_errors=true exist.

If --openapiv2_opt=disable_default_errors=false(default behaviour), googlerpcStatus and protobufAny are outputed.

Comment on lines +60 to +62
# hard coding because protoc-gen-openapiv2 doesn't work well in Stream and doesn't generate 'googlerpcStatus' and 'protobufAny' definitions
cat sdks/swagger/sdk.swagger.json | jq '.definitions |= .+{"googlerpcStatus": {"type": "object", "properties": { "code": { "type": "integer", "format": "int32"}, "message": { "type":"string"}, "details": { "type": "array", "items": { "$ref": "#/definitions/protobufAny"}}}}}' | sponge sdks/swagger/sdk.swagger.json
cat sdks/swagger/sdk.swagger.json | jq '.definitions |= .+{"protobufAny": { "type": "object", "properties": { "@type": { "type": "string" }}, "additionalProperties": {}},}' | sponge sdks/swagger/sdk.swagger.json
Copy link
Contributor Author

@govargo govargo Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is workaround. Add jq logic to sdks/swagger/sdk.swagger.json because protoc-gen-openapiv2 doesn't work well in Stream and doesn't generate nessesary definitions.
It seems that the bug occurs if both stream definition and --openapiv2_opt=disable_default_errors=true exist.

If --openapiv2_opt=disable_default_errors=false(default behaviour), googlerpcStatus and protobufAny are outputed.

@@ -154,7 +154,7 @@ func main() {

if count, _, err := alphaCli.SDKApi.GetPlayerCount(ctx); err != nil {
log.Fatalf("Error retrieving player count: %s", err)
} else if count.Count != "" { // "" is 0, which is empty. This is what the generated client gives us.
} else if count.Count != "0" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed retCode

@@ -15,6 +15,7 @@
package main
Copy link
Contributor Author

@govargo govargo Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. setLabel API

When I tested this code of the old version, the test was not passed.

In this test, the setLabel API was called, however, the test mode of LocalSDKServer always reject unless specified label is.

{"message":"expected to receive '1655898932' as value for 'setlabel' request but received 'true'","sdkName":"restapi","severity":"error","source":"*sdkserver.LocalSDKServer","time":"2022-06-22T20:55:54.507127+09:00"}

https://github.com/googleforgames/agones/blob/main/pkg/sdkserver/localsdk.go#L197-L213

I changed to use reserved API instead of setLabel API.

Copy link
Contributor Author

@govargo govargo Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. go routine

This test used go routine, however, test result often changed because there is no block with paralell processing(the execution order is not guranteed).

The following test flow expected is watchGameserver → setLabel → receiveMessage, however, actual flow was wachGameserver → receiveMessage

+ cd /go/src/agones.dev/agones/test/sdk/websocket-watch
+ go run ws-watch-test.go
2022/06/22 12:28:09 Connecting to ws://localhost:9101/watch/gameserver
{"message":"Connected to watch GameServer...","sdkName":"restapi","severity":"info","source":"*sdkserver.LocalSDKServer","time":"2022-06-22T12:28:09.1496994Z"}
2022/06/22 12:28:09 Received message from websocket: {"result":{"object_meta":{"name":"local","namespace":"default","uid":"3133294039362009907","resource_version":"v1","generation":"1","creation_timestamp":"1655900878","annotations":{"agones.dev/sdk-UID":"3133294039362009907","annotation":"true"},"labels":{"agones.dev/sdk-creationTimestamp":"1655900878","islocal":"true"}},"spec":{"health":{"period_seconds":3,"failure_threshold":5,"initial_delay_seconds":10}},"status":{"state":"Shutdown","address":"127.0.0.1","ports":[{"name":"default","port":7777}],"players":{"capacity":"10"}}}}
2022/06/22 12:28:09 Could not find label 'agones.dev/sdk-testws' in message
time="2022-06-22T12:28:09Z" level=warning msg="error reading websocket message: websocket: close 1006 (abnormal closure): unexpected EOF"
exit status 1
make[2]: *** [run-sdk-command] Error 1

I rewrite this test due to the reason 1&2.

@@ -228,7 +229,7 @@ func runGrpc(grpcServer *grpc.Server, grpcEndpoint string) {

// runGateway runs the grpc-gateway
func runGateway(ctx context.Context, grpcEndpoint string, mux *gwruntime.ServeMux, httpServer *http.Server) {
conn, err := grpc.DialContext(ctx, grpcEndpoint, grpc.WithBlock(), grpc.WithInsecure())
conn, err := grpc.DialContext(ctx, grpcEndpoint, grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials()))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is added because make lint failed with the following error.

cmd/sdk-server/main.go:231:69: SA1019: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x. (staticcheck)
        conn, err := grpc.DialContext(ctx, grpcEndpoint, grpc.WithBlock(), grpc.WithInsecure())

@@ -57,7 +58,7 @@ func NewSDK() (*SDK, error) {
// Block for at least 30 seconds.
ctx, cancel := context.WithTimeout(s.ctx, 30*time.Second)
defer cancel()
conn, err := grpc.DialContext(ctx, addr, grpc.WithBlock(), grpc.WithInsecure())
conn, err := grpc.DialContext(ctx, addr, grpc.WithBlock(), grpc.WithTransportCredentials(insecure.NewCredentials()))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is added because make lint failed with the following error.

sdks/go/sdk.go:60:61: SA1019: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x. (staticcheck)
        conn, err := grpc.DialContext(ctx, addr, grpc.WithBlock(), grpc.WithInsecure())

Comment on lines +41 to +45
echo "--- installing absl for grpc build stability ---" && \
cd /var/local/git/grpc/third_party/abseil-cpp && \
mkdir -p build && cd build && \
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE .. && \
make -j4 && make install && make clean && ldconfig && \
Copy link
Contributor Author

@govargo govargo Jun 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Install absl because the log recommended when build

  Targets not yet defined:
  gRPC::absl_algorithm;gRPC::absl_atomic_hook;gRPC::absl_bad_optional_access;gRPC::absl_base;gRPC::absl_base_internal;gRPC::absl_bits;gRPC::absl_compressed_tuple;gRPC::absl_config;gRPC::absl_core_headers;gRPC::absl_dynamic_annotations;gRPC::absl_endian;gRPC::absl_inlined_vector;gRPC::absl_inlined_vector_internal;gRPC::absl_int128;gRPC::absl_log_severity;gRPC::absl_memory;gRPC::absl_optional;gRPC::absl_raw_logging_internal;gRPC::absl_span;gRPC::absl_spinlock_wait;gRPC::absl_strings;gRPC::absl_strings_internal;gRPC::absl_throw_delegate;gRPC::absl_type_traits;gRPC::absl_utility;gRPC::absl_meta;gRPC::grpc_cronet

Call Stack (most recent call first):
  .build/gRPC/lib/cmake/grpc/gRPCConfig.cmake:20 (include)
  CMakeLists.txt:77 (find_package)

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: ea535c33-bc61-4a3e-bbbe-28bbefbd59fa

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:

  • git fetch https://github.com/googleforgames/agones.git pull/2644/head:pr_2644 && git checkout pr_2644
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.tag=1.25.0-8f803e4-amd64

@govargo
Copy link
Contributor Author

govargo commented Jun 25, 2022

All CI passed

@markmandel
Copy link
Contributor

Wheee! Lots of work!

A couple of questions before I dig into in detail.

  1. Did you regenerate the grpc server and client code with make gen-sdk-grpc - I figured you had, since I see changes to various gen.sh files but just wanted to double check we were updating our tooling there as well.
  2. grpc-gateway has several breaking API changes going from v1 to v2 (this was always my biggest concern) -- are we fixing these issues in any way? (I had a quick check, I didn't see any WithMarshalerOption when setting up the grpcgateway to revert the change to CamelCase). Ideally we would have no breaking changes - especially around the Unity and Unreal SDKs.

@govargo
Copy link
Contributor Author

govargo commented Jun 28, 2022

  1. Did you regenerate the grpc server and client code with make gen-sdk-grpc - I figured you had, since I see changes to various gen.sh files but just wanted to double check we were updating our tooling there as well.

Yes. I regenerated by make gen-all-sdk-grpc and make gen-allocation-grpc

  1. grpc-gateway has several breaking API changes going from v1 to v2 (this was always my biggest concern) -- are we fixing these issues in any way? (I had a quick check, I didn't see any WithMarshalerOption when setting up the grpcgateway to revert the change to CamelCase).

I observed these breaking changes in protoc-gen-openapiv2 and latest protoc-gen-swagger.

1. Change OperationId to ServiceName_RpcName
example

- "operationId": "Allocate",
+ "operationId": "SDK_Allocate", 

ref: grpc-ecosystem/grpc-gateway#1193

This behaviour can change by --openapiv2_opt=simple_operation_ids=true.
This flag is already included in this PR.

2. Add the package name to the tags
example

+  "tags": [
+    {
+      "name": "SDK"
+    }
+  ],

ref: grpc-ecosystem/grpc-gateway#860

This behaviour cannot change. Any option is not provided, however, this doesn't affect API behaviour, because Tags is used for the uniqueness, I think.

3. Add default error response field
example

+          "default": {
+            "description": "An unexpected error response.",
+            "schema": {
+              "$ref": "#/definitions/rpcStatus"
+            }

ref: https://github.com/grpc-ecosystem/grpc-gateway/1141

This behaviour can change by --openapiv2_opt=disable_default_errors=true.
This flag is already included in this PR.

4. Remove boolean format
example

-        format: "boolean"

ref1: grpc-ecosystem/grpc-gateway#1598
ref2: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#dataTypes

This change is by OpenAPI specification(See ref2).
This behaviour can change by adding field [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {format: "boolean"}].
e.g. bool disabled = 1 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {format: "boolean"}];
This field is already included in this PR.

5. x-stream-definitions change to the inline definitions
x-stream-definition doesn't exist. It changed to the inline definition(type: object).

example

-              "$ref": "#/x-stream-definitions/sdkGameServer"
+              "type": "object",
+              "properties": {
+                "result": {
+                  "$ref": "#/definitions/sdkGameServer"
+                },
+                "error": {
+                  "$ref": "#/definitions/googlerpcStatus"
+                }
+              },
+              "title": "Stream result of sdkGameServer"

ref: grpc-ecosystem/grpc-gateway#1112

This behaviour cannot change. Any option is not provided. So we have to accept this change to upgrade to protoc-gen-openapiv2.

6. camelCase JSON names by default
example

-        "resource_version": {
+        "resourceVersion": {
...
-        "creation_timestamp": {
+        "creationTimestamp": {
...
-        "deletion_timestamp": {
+        "deletionTimestamp": {
...
-        "periodSeconds": {
+        "period_seconds": {
...
-        "failureThreshold": {
+        "failure_threshold": {
...
-        "initialDelaySeconds": {
+        "initial_delay_seconds": {

ref1: grpc-ecosystem/grpc-gateway#540
ref2: https://grpc-ecosystem.github.io/grpc-gateway/docs/development/grpc-gateway_v2_migration_guide/#we-now-use-the-camelcase-json-names-by-default

This behaviour can change by WithMarshalerOption in the code and --openapiv2_out=json_names_for_fields=false.

@markmandel
This flag is not included in this PR. Should I include this?

@govargo
Copy link
Contributor Author

govargo commented Jun 28, 2022

Ideally we would have no breaking changes - especially around the Unity and Unreal SDKs.

I tested make gen-all-sdk-grpc and make run-sdk-conformance-tests locally and also passed CI tests of CloudBuild.
But I think that make gen-all-sdk-grpc doesn't change any Unity and Unreal SDK's files.

Are these using grpc files? If so, I think I should fix these.

@markmandel
Copy link
Contributor

markmandel commented Jun 28, 2022

So as a general statement: We're post 1.0, so our API should be stable, and not break. If have to, we should have a deprecation period with plenty of time for migration.

So everything should ideally work between Agones releases -- this is definitely going to break this in its current form (this is what always scared me about upgrading grpc-gateway).

Personally I'm less concerned with the swagger output being different between versions somehow (as you pointed out, it was broken for streaming anyway), since that can't break active code, but very concerned that the underlying REST API is now different between versions.

Or to put it another way - if someone hand wrote a REST API to use against any of our APIS that go through grpc-gateway (SDK, Allocation, etc) it should still work with no modification between this release and the upcoming one.

Is that possible with this upgrade?

@govargo
Copy link
Contributor Author

govargo commented Jun 29, 2022

Thank you for your comment.

So as a general statement: We're post 1.0, so our API should be stable, and not break. If have to, we should have a deprecation period with plenty of time for migration.

So everything should ideally work between Agones releases -- this is definitely going to break this in its current form (this is what always scared me about upgrading grpc-gateway).

I see. I agree with that API should be stable.

Personally I'm less concerned with the swagger output being different between versions somehow (as you pointed out, it was broken for streaming anyway), since that can't break active code, but very concerned that the underlying REST API is now different between versions.

Or to put it another way - if someone hand wrote a REST API to use against any of our APIS that go through grpc-gateway (SDK, Allocation, etc) it should still work with no modification between this release and the upcoming one.

Is that possible with this upgrade?

I'm less concerned with that REST API behaviour changes if this PR merged.
This is because I changed rest api test only one line. The following line.

-		} else if count.Count != "" { // "" is 0, which is empty. This is what the generated client gives us.
+		} else if count.Count != "0" {

This change was created by golang json decode specification(by the following line), not grpc-gateway.
https://github.com/golang/go/blob/go1.17.11/src/encoding/json/decode.go#L180

I also changed test/sdk/websocket-watch/ws-watch-test.go. But this test failed with even though current active codebase.
The reason which I changed ws-watch-test.go is for test stability, not for grpc-gateway changes.

I recognize that I should include https://grpc-ecosystem.github.io/grpc-gateway/docs/development/grpc-gateway_v2_migration_guide/#we-now-use-the-camelcase-json-names-by-default in order to reduce difference with current codebase.

# sdk.swagger.json
-        "resource_version": {
+        "resourceVersion": {
...
-        "creation_timestamp": {
+        "creationTimestamp": {
...
-        "deletion_timestamp": {
+        "deletionTimestamp": {
...
-        "periodSeconds": {
+        "period_seconds": {
...
-        "failureThreshold": {
+        "failure_threshold": {
...
-        "initialDelaySeconds": {
+        "initial_delay_seconds": {

@markmandel
Copy link
Contributor

markmandel commented Jun 29, 2022

This is because I changed rest api test only one line. The following line.

From memory, the REST client generated from the swagger spec on each test:
https://github.com/googleforgames/agones/blob/main/build/build-sdk-images/restapi/build-sdk-test.sh

Because if it didn't, the conversion to camelCase in v2 would have broken the test.

This is kind what scares me 😬 (although it does make sense for the test - it checks both the swagger spec and the REST api is valid).

I wonder if we could do a simple smoke test manually - generate the client for the version on main put it somewhere safe, switch to this branch, and run it against this branch's SDK test, and see if everything works as expected. WDYT?

Even as a one time thing, I think it would go a long way to making me feel safer.

Thoughts?

@govargo
Copy link
Contributor Author

govargo commented Jun 30, 2022

From memory, the REST client generated from the swagger spec on each test:
https://github.com/googleforgames/agones/blob/main/build/build-sdk-images/restapi/build-sdk-test.sh

Because if it didn't, the conversion to camelCase in v2 would have broken the test.

restapi test generates the code from the swagger spec and test all REST method toward the LocalSDKServer.
I think there is a minimum guarantee for REST stability, because LocalSDK only emulates.

I wonder if we could do a simple smoke test manually - generate the client for the version on main put it somewhere safe, switch to this branch, and run it against this branch's SDK test, and see if everything works as expected. WDYT?

Even as a one time thing, I think it would go a long way to making me feel safer.

A smoke test looks good idea.
I thought about it, but maybe it would be nice to have an E2E test using the REST API, for example.
restapi test uses the LocalSDKServer, however, new E2E test builds/deploys the actual client using REST SDK, not LocalSDKServer.
https://agones.dev/site/docs/guides/client-sdks/rest/

How about this idea?
If this idea is good, I'll write test and verification codes.
My idea are...

  1. Create restapi-sample in Agones example folder and add it to the E2E test folder of the Agones this repository
  2. Create restapi test code outside the Agones project (=personal project)

Even as a one time thing, I think it would go a long way to making me feel safer.

It may take some time, but I'm willing to work on this to ensure safety.

@markmandel
Copy link
Contributor

The good news is - we have e2e tests for the allocation REST endpoints (although now I'm curious how they didn't fail 🤔 )

func TestRestAllocatorWithDeprecatedRequired(t *testing.T) {

func TestRestAllocatorWithSelectors(t *testing.T) {

So we know that those are (somehow?) the same between grpc-gateway versions.

It may take some time, but I'm willing to work on this to ensure safety.

  1. Thank you 😄
  2. Thank for taking on something I was always scared to touch! 😨

@govargo
Copy link
Contributor Author

govargo commented Jul 1, 2022

The good news is - we have e2e tests for the allocation REST endpoints

Thank you! It's good news.

although now I'm curious how they didn't fail 🤔

The difference between current code and this PR is the only following.
I think there is no change in allocation REST, even though grpc-gateway v2.
Camel case change is affected in only sdk.swagger.json

+  "tags": [
+    {
+      "name": "AllocationService"
+    }
+  ],

I'll separate and send the PR which run E2E test for the client using REST API.
I'll send the PR when I'm ready.

Thank for taking on something I was always scared to touch! 😨

Thank you always too for your review and comments😄

@SaitejaTamma SaitejaTamma added kind/feature New features for Agones kind/cleanup Refactoring code, fixing up documentation, etc kind/breaking Breaking change labels Jul 8, 2022
@SaitejaTamma SaitejaTamma added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Jul 26, 2022
@SaitejaTamma SaitejaTamma removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Aug 4, 2022
@markmandel
Copy link
Contributor

Just wanted to gently bump this, see where things had landed 😄

@markmandel
Copy link
Contributor

I just realised something for doing testing to make sure the SDK Rest API is the same 😄 that I think will actually be relatively straightforward d to test with.

We have curl commands for each of the SDK commands here:
https://agones.dev/site/docs/guides/client-sdks/rest/#reference

A way to test that everything is compliant is to build the sdkserver, run it in local mode, and then run each of the curl commands, and ensure it works exactly the same.

Sound like a good idea?

@govargo
Copy link
Contributor Author

govargo commented Aug 6, 2022

Just wanted to gently bump this, see where things had landed 😄

Currently, I'm working to the E2E test now(In private, I moved to new house this week, so there was no time to implement)
I'll resend the new divided PR next week.

A way to test that everything is compliant is to build the sdkserver, run it in local mode, and then run each of the curl commands, and ensure it works exactly the same.

Sound like a good idea?

I think of making restapi-simple by referring to nodejs-simple.
Such like...

	// Send request for Reserve API
	jsonStr := `{"seconds": "5"}`
	resp, err := sendHTTPRequest(ctx, http.MethodPost, fmt.Sprintf("%s/reserve", sdkURL), jsonStr)
	if err != nil {
		log.Fatalf("Reserve API failed: %v", err)
	}
	defer resp.Body.Close()
	// Check if the GameServer Status is Reserved
	body, err := getGameServer(ctx, sdkURL)
	g := string(body)
	if !strings.Contains(g, "Reserved") {
		log.Fatal("GameServer Status.State should be Reserved")
	}

Using Go's http library, I'll check the returned value and validation for restapi result.
I think a programming language would be more convenient for these check and validation.

@govargo
Copy link
Contributor Author

govargo commented Aug 18, 2022

Currently, I'm working to the E2E test now(In private, I moved to new house this week, so there was no time to implement)
I'll resend the new divided PR next week.

Sorry, still working

@markmandel
Copy link
Contributor

No worries - it's a big chunk of work, just wanted to touch base and see where things where at.

@SaitejaTamma SaitejaTamma added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Sep 6, 2022
@SaitejaTamma SaitejaTamma removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Sep 14, 2022
@markmandel
Copy link
Contributor

Just gently checking in 😄

@govargo
Copy link
Contributor Author

govargo commented Sep 30, 2022

Thank you for your paticience.
I haven't updated in a long time and stopped in preparation for test.

I sent separate PR #2757. This is for checking that REST API SDK works fine.
After #2757 is reviewed and merged, I will also rebase and update this PR.

Thank you.

@mangalpalli mangalpalli added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 18, 2022
@markmandel
Copy link
Contributor

markmandel commented Oct 19, 2022

Looking at all the conflicts, should we close this PR for now? Also since you have some new ones incoming?

@govargo
Copy link
Contributor Author

govargo commented Oct 21, 2022

It's OK to close this PR once.
I'll re-create new PR if #2757 is done.

@markmandel
Copy link
Contributor

It's OK to close this PR once. I'll re-create new PR if #2757 is done.

Sounds good! I dropped some comments on #2757 to clean some things up, and manage some licences - but definitely looks to me like it's going in the right direction 👍🏻

@markmandel markmandel closed this Oct 21, 2022
@markmandel markmandel removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/breaking Breaking change kind/cleanup Refactoring code, fixing up documentation, etc kind/feature New features for Agones size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update from golang/protobuf to google.golang.org/protobuf
5 participants