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

Change OpenStorageSchedule API parameter type to string #2454

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

twang-ps
Copy link
Contributor

@twang-ps twang-ps commented Jun 17, 2024

What this PR does / why we need it:
This PR is to resolve a compilation issue on release branch where it doesn't support enum type path parameter in protobuf REST API.

This fix is to cast Job.Type enum type to string type in request data structures.

Issue:
After I cherry pick PR #2426 onto release-9.8, it failed to build. It complains the following error, which means protoc-gen-grpc-gateway of this version doesn't support a enum type in path parameter.

service OpenStorageSchedule {
  // Inspect queries information of a schedule by type and ID
  rpc Inspect(SdkInspectScheduleRequest)
    returns (SdkInspectScheduleResponse) {
      option(google.api.http) = {
        get: "/v1/schedules/{type}/{id}"
      };
    }
message SdkInspectScheduleRequest {
  // ID of the schedule
  string id = 1;
  // Type is schedule type
  Job.Type type = 2;
}
root@ip-10-13-177-24:~/git/go/src/github.com/libopenstorage/openstorage# make docker-proto
docker pull quay.io/openstorage/osd-proto:pre-gomodules
pre-gomodules: Pulling from openstorage/osd-proto
Digest: sha256:76b09a1a0231f9cf78f22eecd243d7ddb2bcc464e0cea9493a2d08dfe534ef14
Status: Image is up to date for quay.io/openstorage/osd-proto:pre-gomodules
quay.io/openstorage/osd-proto:pre-gomodules
docker run \
        --privileged --rm \
        -v /root/git/go/src/github.com/libopenstorage/openstorage:/go/src/github.com/libopenstorage/openstorage \
        -e "GOPATH=/go" \
        -e "DOCKER_PROTO=yes" \
        -e "PATH=/bin:/usr/bin:/usr/local/bin:/go/bin:/usr/local/go/bin" \
        quay.io/openstorage/osd-proto:pre-gomodules \
                make proto mockgen
>>> Generating protobuf definitions from api/api.proto
protoc -I /go/src/github.com/libopenstorage/openstorage \
        -I /usr/local/include \
        -I /go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --go_out=plugins=grpc:. \
        /go/src/github.com/libopenstorage/openstorage/api/api.proto
protoc -I /go/src/github.com/libopenstorage/openstorage \
        -I /usr/local/include \
        -I /go/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --grpc-gateway_out=logtostderr=true:. \
        /go/src/github.com/libopenstorage/openstorage/api/api.proto
--grpc-gateway_out: template: client-rpc-request-func:30:53: executing "client-rpc-request-func" at <$param.ConvertFuncEx...>: error calling ConvertFuncExpr: unsupported field type TYPE_ENUM of parameter type in OpenStorageSchedule.Inspect
make: *** [Makefile:201: proto] Error 1
Makefile:184: recipe for target 'docker-proto' failed
make: *** [docker-proto] Error 2

Cause analysis
libopenstorage/openstorage is built inside a container. But the images used for master branch and release branch are different. Master branch uses quay.io/openstorage/osd-proto:lastest while release branch uses quay.io/openstorage/osd-proto:pro-gomodules
The version of protoc-gen-grpc-gateway is v1.16.0 on master branch while it's v1.4.1 on release branch
On master branch, it’s able to generate the following code to parse the enum type:

func request_OpenStorageSchedule_Inspect_0(ctx context.Context, marshaler runtime.Marshaler, client OpenStorageScheduleClient, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) {
	var protoReq SdkInspectScheduleRequest
	var metadata runtime.ServerMetadata

	var (
		val string
		e   int32
		ok  bool
		err error
		_   = err
	)

	val, ok = pathParams["type"]
	if !ok {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "type")
	}

	e, err = runtime.Enum(val, Job_Type_value)

	if err != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "type", err)
	}

	protoReq.Type = Job_Type(e)

On release branch, the code generated could not handle the parsing

func request_OpenStorageSchedule_Inspect_0(ctx context.Context, marshaler runtime.Marshaler, client OpenStorageScheduleClient, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) {
    var protoReq SdkInspectScheduleRequest
    var metadata runtime.ServerMetadata

    var (
        val string
        ok  bool
        err error
        _   = err
    )

    val, ok = pathParams["type"]
    if !ok {
        return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "type")
    }

    protoReq.Type, err = runtime.String(val)

    if err != nil {
        return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "type", err)
    }

Testing Notes
Add testing output or passing unit test output here.

Special notes for your reviewer:
Add any notes for the reviewer here.

@twang-ps twang-ps merged commit e4b050e into libopenstorage:master Jun 18, 2024
4 checks passed
twang-ps added a commit to twang-ps/openstorage that referenced this pull request Jul 9, 2024
twang-ps added a commit that referenced this pull request Jul 10, 2024
* Add a new type of Job: DefragJob (#2375)
(cherry picked from commit 759bca7)

* Add defrag status data structures (#2385)
(cherry picked from commit def904a)

* Add generic schedule structure; add fields for defrag schedule (#2408)
(cherry picked from commit 5aaf214)

* Add server and APIs for defrag and schedule services (#2426)
(cherry picked from commit e80fd7f)

* Change OpenStorageSchedule API parameter type to string (#2454)
(cherry picked from commit e4b050e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants