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

Incorrect URIs for some lease related functions in JSON API #9430

Closed
Tracked by #94
skydoctor opened this issue Mar 13, 2018 · 7 comments · Fixed by #9450
Closed
Tracked by #94

Incorrect URIs for some lease related functions in JSON API #9430

skydoctor opened this issue Mar 13, 2018 · 7 comments · Fixed by #9450

Comments

@skydoctor
Copy link

Some Lease related URIs for the HTTP/JSON API appear to be incorrect in the generated Swagger API definitions at: https://github.com/coreos/etcd/blob/master/Documentation/dev-guide/apispec/swagger/rpc.swagger.json

Correct URIs:

  • "/v3/lease/grant"
  • "/v3/lease/keepalive"

Possibly incorrect URIs:

  • "/v3/kv/lease/leases"
  • "/v3/kv/lease/revoke"
  • "/v3/kv/lease/timetolive"

As per the API guide, I think these should not be under the "kv" service. Not sure if the problem is just with the Swagger JSON but also in the code.

@hexfusion
Copy link
Contributor

@akashbaid first of all the Swagger spec is correct technically, as it matches the code. Logically I have battled with this as well. I ended up managing the organization internally with my client to work around it. I am curious though if we could use additional_bindings from grpc-gateway to allow a non breaking depreciation.

So for example

  // LeaseRevoke revokes a lease. All keys attached to the lease will expire and be deleted.
  rpc LeaseRevoke(LeaseRevokeRequest) returns (LeaseRevokeResponse) {
      option (google.api.http) = {
        post: "/v3/lease/revoke"
        body: "*"
        additional_bindings {
		post: "/v3/kv/lease/revoke"
         }
}

We could then phase that out after a few minor iterations allowing folks to handle it gracefully.

/cc @gyuho

@hexfusion
Copy link
Contributor

I will do a bit of testing with this soon, if it works would you like to take it on @akashbaid ?

@skydoctor
Copy link
Author

@hexfusion Sure.

@hexfusion
Copy link
Contributor

hexfusion commented Mar 17, 2018

@akashbaid Well this is not going to be as easy as that. Leases are showing expired with timetolive, will try to dig in when I get some time but feel free to test and explore on your own.

scripts/genproto.sh will regenerate the swagger etc.

diff --git a/etcdserver/etcdserverpb/rpc.proto b/etcdserver/etcdserverpb/rpc.proto
index 0cfad92ab..826dff5fb 100644
--- a/etcdserver/etcdserverpb/rpc.proto
+++ b/etcdserver/etcdserverpb/rpc.proto
@@ -90,8 +90,12 @@ service Lease {
   // LeaseRevoke revokes a lease. All keys attached to the lease will expire and be deleted.
   rpc LeaseRevoke(LeaseRevokeRequest) returns (LeaseRevokeResponse) {
       option (google.api.http) = {
-        post: "/v3/kv/lease/revoke"
+        post: "/v3/lease/revoke"
         body: "*"
+
+        additional_bindings {
+            post: "/v3/kv/lease/revoke"
+        }
     };
   }

@@ -107,16 +111,24 @@ service Lease {
   // LeaseTimeToLive retrieves lease information.
   rpc LeaseTimeToLive(LeaseTimeToLiveRequest) returns (LeaseTimeToLiveResponse) {
       option (google.api.http) = {
-        post: "/v3/kv/lease/timetolive"
+        post: "/v3/lease/timetolive"
         body: "*"
+
+        additional_bindings {
+            post: "/v3/kv/lease/timetolive"
+        }
     };
   }

   // LeaseLeases lists all existing leases.
   rpc LeaseLeases(LeaseLeasesRequest) returns (LeaseLeasesResponse) {
       option (google.api.http) = {
-        post: "/v3/kv/lease/leases"
+        post: "/v3/lease/leases"
         body: "*"
+
+        additional_bindings {
+            post: "/v3/kv/lease/leases"
+        }
     };
   }
 }

@gyuho
Copy link
Contributor

gyuho commented Mar 17, 2018

Let's define additional_bindings for 3.4, and deprecate them all in 3.5.

@hexfusion
Copy link
Contributor

Let's define additional_bindings for 3.4, and deprecate them all in 3.5.

@gyuho sounds good I got this to work the newline seemed to cause the issue between body and the binding. Will issue PR.

@gyuho
Copy link
Contributor

gyuho commented Mar 17, 2018

@hexfusion I got it to work, by adding comma after body line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants