Conversation
|
Added implementation, will add tests for the same. |
0feaf24 to
6e91346
Compare
6e91346 to
3d1e59d
Compare
|
The PR lacks test, will remove from drafts once I have added them. |
6633df4 to
933e165
Compare
4bb0de4 to
d0b881b
Compare
sabaini
left a comment
There was a problem hiding this comment.
Heya,
this looks very useful.
Some comments after a first quick look:
User interface: we talked about this a bit but jftr. -- think the microceph s3 [create|delete|...] ... might be a bit confusing as it's not clear that this is about creating or deleting users. Maybe could do another subcommand e.g. microceph s3 user [create|delete|...] ... or rename the subcommand to something like microceph s3-user [create|delete|...] ...
REST API: it's recommended to give every entity in a REST api their own URI, and have the REST verbs (GET/PUT/DELETE/...) act on those entities
For this API the entities would be users. It's a convention (though not required by REST) to have directories map to collections of entities.
A mapping to URIs could therefore look something like this, with an example user fooUser
GET /s3/users # list user collection
POST /s3/users # create a new user in collection with data from the req. body
GET /s3/users/fooUser # get details for fooUser (no data required)
PUT /s3/users/fooUser # update fooUser with data from req. body
DELETE /s3/users/fooUser # delete fooUser (no data required)
Note there's no POST /s3/users/fooUser as POST is for creating new entities.
Thanks!
3aa3465 to
88f45a5
Compare
51da731 to
e1374d6
Compare
|
@sabaini I have modified the APIs to be using ""services/rgw/user"". |
e1374d6 to
5496249
Compare
c46d992 to
a31fb6c
Compare
sabaini
left a comment
There was a problem hiding this comment.
Heya Utkarsh, thanks for the update, added a comment about REST API structure
| Get: rest.EndpointAction{Handler: cmdS3Get, ProxyTarget: true}, | ||
| Put: rest.EndpointAction{Handler: cmdS3Put, ProxyTarget: true}, | ||
| Delete: rest.EndpointAction{Handler: cmdS3Delete, ProxyTarget: true}, | ||
| } |
There was a problem hiding this comment.
For optimal RESTy-ness I'd like to suggest an API structure like this:
GET /services/rgw/user # returns a list of users
GET /services/rgw/user/{uid} # returns info about a specific user
POST /services/rgw/user # create a new user
PUT /services/rgw/user/{name} # update existing user
DELETE /services/rgw/user/{name} # delete user
Something like this can be used to specify a path with a {uid} var:
var s3Cmd = rest.Endpoint{
Path: "services/rgw/user/{name}",
Get: rest.EndpointAction{Handler: cmdS3GetSingle, ProxyTarget: true},
Put:
...
func cmdS3GetSingle(s *state.State, r *http.Request) response.Response {
var err error
var name string
name, err = url.PathUnescape(mux.Vars(r)["name"])
if err != nil {
return response.BadRequest(err)
}
// do something with user name
user = ceph.GetS3User(name)
...
microceph/client/s3.go
Outdated
| defer cancel() | ||
|
|
||
| ret := "" | ||
| err := c.Query(queryCtx, "GET", api.NewURL().Path("services", "rgw", "user"), user, &ret) |
There was a problem hiding this comment.
With the API structure change I've suggested above the URL would be built with something like
err = c.Query(queryCtx, "GET", api.NewURL().Path("services", "rgw", "user", strconv.FormatInt(user.name, 10)), nil, nil)
|
Thanks for the detailed review @sabaini 🙏🏼 . I will implement your RESTy suggestions in and make the PR ready. |
d693ae0 to
99ce437
Compare
Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>
Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>
99ce437 to
89fe07b
Compare
d14c3ba to
10cb50d
Compare
Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>
10cb50d to
a163bc8
Compare
sabaini
left a comment
There was a problem hiding this comment.
Hey @UtkarshBhatthere did a quick once over, added some suggestions mostly around API RESTfulness
|
|
||
| - name: Exercise RGW | ||
| run: ~/actionutils.sh headexec testrgw | ||
| run: ~/actionutils.sh headexec testrgw_old |
There was a problem hiding this comment.
Naming nit: maybe a more descriptive name instead of testrgw_old
| # boto3 for appS3 test script. | ||
| sudo python -m pip install --upgrade pip | ||
| sudo pip install boto3 | ||
| ~/actionutils.sh setup_lxd |
There was a problem hiding this comment.
I see these lines being repeated
Maybe could refactor the actionutils.sh a bit to have a setup_host function instead of setup_lxd that handles all deps
| Manage S3 users on MicroCeph | ||
| ============================= | ||
|
|
||
| MicroCeph provides an easy to use interface for creating, viewing and deleting s3 users for interfacing with the RGW endpoint. |
There was a problem hiding this comment.
Suggest: capitalize S3 here and below (proper noun)
| ============================= | ||
|
|
||
| MicroCeph provides an easy to use interface for creating, viewing and deleting s3 users for interfacing with the RGW endpoint. | ||
| This enables smooth and easy access to Object Storage. |
There was a problem hiding this comment.
Suggest: s/Object Storage/object storage/
| | 2 | testUser | | ||
| +---+-------------+ | ||
|
|
||
| 3. Get details of a an s3 user (optionally use --json flag to get complete details): |
| "github.com/canonical/microcluster/state" | ||
| ) | ||
|
|
||
| // /1.0/resources endpoint. |
| } | ||
| } | ||
|
|
||
| func cmdClientS3Put(s *state.State, r *http.Request) response.Response { |
There was a problem hiding this comment.
For maximum RESTfulness we should be using a POST to the /client/s3 url to create a user
| Delete: rest.EndpointAction{Handler: cmdClientS3Delete, ProxyTarget: true}, | ||
| } | ||
|
|
||
| func cmdClientS3Get(s *state.State, r *http.Request) response.Response { |
There was a problem hiding this comment.
RESTfulness:
- retrieving a list of users should be a GET to
/client/s3 - retrieving a single user should be a GET to
/client/s3/{userid}
See the /disks/{osdid} handler in microceph/api/disks.go for an example on how to implement a handler for a path variable i.e. {userid}
| return response.SyncResponse(true, output) | ||
| } | ||
|
|
||
| func cmdClientS3Delete(s *state.State, r *http.Request) response.Response { |
There was a problem hiding this comment.
RESTfulness: this should be using a DELETE against /client/s3/{userid} (also cf. above for getting users)
| @@ -0,0 +1 @@ | |||
| package main | |||
There was a problem hiding this comment.
I guess this is not actually used?
Added CLI to Create, Delete, List and Get S3 users for rgw