Add support for the subuser calls in the rgw admin interface #644

sebastianriese · 2022-02-11T13:54:37Z

These commits add support for the subuser calls in the rgw admin interface.

Closes #521.

Implementing the subuser commands required a restructuring of the way the objects are serialized to API parameters. The way this was done is up to review and I am open to changes.

This is a breaking change, since the User type changes (by adding detailed type information for the fields related to subusers to support type safe parsing from the json responses). The API calls on the wire may change as well (as go-ceph now only serializes those parameters that are actually supported by the Ceph RGW Adminops API, not all parameters that are set in the "mirror" object on the go-ceph side).

Checklist

Added tests for features and functional changes
Public functions and types are documented
Standard formatting is applied to Go code
Is this a new API? Is this new API marked PREVIEW?

sebastianriese · 2022-02-11T14:35:10Z

One problem that I already spotted with my approach is, that the documentation does not seem to be complete with respect to the supported query parameters, e.g. the feature from #600 (getting users by access key), is not documented in the api docs: https://docs.ceph.com/en/latest/radosgw/adminops/#get-user-info, there only the uid parameter is specified (and marked required).

phlogistonjohn · 2022-02-21T15:59:32Z

rgw/admin/utils.go

+// NOTE: we use linear search here, as none of the API endpoints
+// supports more than 10 parameters, in this case asymptotics don't
+// matter and we are likely faster this way (even when compared to a
+// map).


Great comment, BTW.

phlogistonjohn · 2022-02-21T16:05:20Z

I'd like to know more about what you think could cause backwards incompatibility issues. The go-ceph library is v0 and we don't guarantee no breaking changes but we do try to avoid them as much as possible as the library is used in various codebases.

Personally, nothing just jumped out at me as having a breaking API change but I may have overlooked something.

I'll also tag @leseb who helps maintain the rgw admin part of go-ceph and is more knoledgeable about rgw admin than I am.

sebastianriese · 2022-02-22T14:22:08Z

The potential problem I see, is that I restrict the parameters that are serialized and passed to the API. Although I do so based on the official documentation of the API, this means that there could be code out there, that passed in more parameters before the change and those parameters did something though they were not specified – I know the API documentation isn't fully reliable. This in turn would break real use cases.

I know of one example where I don't allow a parameter that isn't specified in the API documentation, but passing it does actually work when it's included in the API call, namely access-key in the GetUser call (compare #600). Given that I know this, I can fix it easily, the trouble is the undocumented parameters I don't know about.

Nevertheless, if this potential breakage is acceptable to you, it feels safer to me, to only send selected parameters the API (in the sense of "be conservative in what you send").
It would certainly be an option to note it in the change note, and have people complain if their real-world requests no longer work.

(The change will, of course, also break code that accessed the swift-related parts of the User structure – so far typed []interface{} – in any way).

I'll have a look at the RGW source code, perhaps the supported parameter sets can be extracted from there.

sebastianriese · 2022-02-22T14:30:21Z

Okay, that looks like the right place: https://github.com/ceph/ceph/blob/193895ffba4245787a2f50bbd5b80132f0e76e74/src/rgw/rgw_rest_user.cc#L72

I guess we can take the supported parameters from there. Then we can be sure that we don't break code, because any other parameters someone might send, will just be ignored by rgw.

sebastianriese · 2022-02-23T15:23:10Z

I have now updated the lists based on determining the supported parameters from the ceph source code and I have documented parameters supported by RGW that we do not yet support (as we don't have a field to specify them in our structs). This should make the change safe, in the sense that code using this library do to an API call will have the same effect before and after this change (although the API call on the wire will potentially change, as fields ignored by the RGW Adminops API will not be serialized).

In the process I've noted a few irregularities in the typing of the API parameters in this code base (but I guess that is not for this PR to address). E.g. int us used for parameters that are parsed as uint32_t or int32_t on the RGW side and at least in one case int is used as the type for a boolean flag (it works out as rgw allows to representations of booleans in the parameters: 0|1 and false|true). I've opened an issue to track that problem: #656.

sebastianriese · 2022-02-24T15:05:20Z

I've marked the new API methods as PREVIEW (as the development documentation advises).

Should the new types also be marked PREVIEW in some way, or are they not covered by the API stability tracking?

phlogistonjohn · 2022-02-24T15:55:57Z

I've marked the new API methods as PREVIEW (as the development documentation advises).

Great thanks.

Should the new types also be marked PREVIEW in some way, or are they not covered by the API stability tracking?

Types currently do not need to be marked PREVIEW. As you surmised they're not (directly) part of the API tracking.

We maintainers are currently discussing the possibility of dropping "PREVIEW" in the doc comment and relying entirely on the "ceph_preview" build tag. Aside from the annoyance of having to deal with another process change, we think this would be simpler to document and ask contributors to implement. However, the tooling will need to be updated to match.

sebastianriese · 2022-02-28T19:10:44Z

I've now improved the validation of API parameters (and corresponding errors) and expanded the tests.

Additionally, I've added some comments to the source that explain some irregularities in the RGW Admin OPS API (and how this impacts the API library/the choices made in the code because of this).

The tests could be expanded and have tighter matchers in places, but I'll un-draft this now, as I think it is mostly ready and those short-comings can be fixed together with any issues raised in further review.

leseb · 2022-03-10T13:24:10Z

rgw/admin/bucket.go

@@ -132,7 +134,7 @@ func (api *API) GetBucketPolicy(ctx context.Context, bucket Bucket) (Policy, err

 // RemoveBucket will remove a given token from the object store
 func (api *API) RemoveBucket(ctx context.Context, bucket Bucket) error {
-	_, err := api.call(ctx, http.MethodDelete, "/bucket", valueToURLParams(bucket))
+	_, err := api.call(ctx, http.MethodDelete, "/bucket", valueToURLParams(bucket, []string{"bucket", "purge-objects"}))


Is this purging all the objects from the bucket? If so, isn't that dangerous? Would it be good to have validation or something passed to the API by the user to do this?

I did not touch change the behaviour of this API call. I only added the filtering in valueToURLParams (so only those parameters that are actually supported by the API according to the ceph-rgw source code are serialized to the query string).

It is certainly a dangerous operation (even without purge objects, given that all obejcts that are only in this bucket will be deleted independently of the purge-objects setting), but the way it behaves does not change with this commit.

With purge-objects set to true all the objects in the bucket will be deleted, before deleting the bucket (I guess that means they will also be deleted from other buckets that may contain them).

For purge-objects to happen *bucket.PurgeObjects must be set to true, this will never be the case for Bucket objects returned from the API, so explicit action is necessary on the side of the user.

leseb · 2022-03-10T13:25:30Z

rgw/admin/usage.go

@@ -50,7 +50,8 @@ type Usage struct {

 // GetUsage request bandwidth usage information on the object store
 func (api *API) GetUsage(ctx context.Context, usage Usage) (Usage, error) {
-	body, err := api.call(ctx, http.MethodGet, "/usage", valueToURLParams(usage))
+	// unsupported parameters: category, bucket


Can we clarify? Unsupported by this API or by the RGW admin ops API?

I've clarified to // valid parameters not supported by go-ceph

If someone will implement support for the currently unsupported parameters in the structs, the parameter in question will also have to be added to the whitelists in the corresponding methods.

leseb · 2022-03-10T13:25:34Z

rgw/admin/usage.go

@@ -65,6 +66,7 @@ func (api *API) GetUsage(ctx context.Context, usage Usage) (Usage, error) {

 // TrimUsage removes bandwidth usage information. With no dates specified, removes all usage information.
 func (api *API) TrimUsage(ctx context.Context, usage Usage) error {
-	_, err := api.call(ctx, http.MethodDelete, "/usage", valueToURLParams(usage))
+	// unsupported parameters: bucket


All the comments have been updated.

mergify · 2022-03-14T20:10:43Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

The following changes have been done: * Up until now everything in the argument objects was serialized to the API calls. This was updated to restrict the serialization to the API call parameters that are parsed in the Ceph RGW source code. Parameters not yet supported by us are documented as comments. Note, that a superset of the documented parameters is supported. Documentation for the API: <https://docs.ceph.com/en/pacific/radosgw/adminops/> Link to the used source tree: <https://github.com/ceph/ceph/tree/193895ffba4245787a2f50bbd5b80132f0e76e74/src/rgw> The argument parsing happens in the rgw_rest_*.cc files. * The serialization code (valueToURLParams) has been updated to be more in line with other serialization methods: - A tag "-" causes the field to be ignored - Only the first item in a list of tag items is interpreted as name. - The handling of pointer and direct data types has been harmonized (the same rules for the names and value apply now). * There is still room for improvement to make things more consistent: A pointer to a non-elementary data type will emit unexpected request parameters. * Presence of required parameters is not validated by the library, this is left to the API. Signed-off-by: Sebastian Riese <[email protected]>

Only one of the two code paths of buildQueryPath was tested so far. This makes the test harness tight by testing the second one (where the path already contains a query parameter). Signed-off-by: Sebastian Riese <[email protected]>

THIS POTENTIALLY BREAKY DOWNSTREAM CODE (as it changes the types of exported fields in an existing, exported struct). The fields of the User structure representing the Subuser information have been specified to parse this data strictly and typesafe. The fields SwiftKeys and Subusers need the url:"-" annotation, because otherwise url-keys in that substructure would clash with those in the User structure. Signed-off-by: Sebastian Riese <[email protected]>

sebastianriese · 2022-03-16T10:07:31Z

Rebased to the newest master.

Pull request has been modified.

sebastianriese · 2022-03-17T09:44:57Z

Thanks for the reviews. I've slightly squashed the history and did some minor improvements to the tests – from my side it is ready to merge now.

phlogistonjohn

Hi there! Thanks a lot for working with @leseb on the functionality. Now that is squared away I do notice a few small book keeping related issues that need to be resolved.
I'm aware that these can be kind of annoying. I do wish I had noticed the file/ceph_preview build flag issue sooner, but I hope you wont find that too difficult to resolve.

phlogistonjohn · 2022-03-17T13:23:24Z

rgw/admin/user.go

+}
+
+// CreateSubuser - https://docs.ceph.com/en/latest/radosgw/adminops/#create-subuser
+//  PREVIEW


New functions that are in preview state must be located in files marked with the "ceph_preview" build tag. I thought the CI would have caught this but I guess not, sorry. I suggest moving the net new functions to "subuser.go"

Let me know if you have any questions about this process.

phlogistonjohn · 2022-03-17T13:25:38Z

docs/api-status.json

+      {
+        "name": "API.CreateSubuser",
+        "comment": "CreateSubuser - https://docs.ceph.com/en/latest/radosgw/adminops/#create-subuser\n PREVIEW\n",
+        "added_in_version": "v0.14.0",


Release v0.14 is already out. The added_in_version and expected_stable_version need to be incremented. You can do it by hand or by deleting the new entries and re-running the make api-update commands.
We are aware that this is annoying and time consuming and are looking into ways to avoid putting the burden on contributors... but we don't have a solution yet.

Ah, I tried to fix this manually, but obviously didn't catch all occurrences – I'll use the tools to regenerate it properly.

phlogistonjohn · 2022-03-17T13:25:55Z

docs/api-status.json

+      {
+        "name": "API.RemoveSubuser",
+        "comment": "RemoveSubuser - https://docs.ceph.com/en/latest/radosgw/adminops/#remove-subuser\n PREVIEW\n",
+        "added_in_version": "v0.14.0",


phlogistonjohn · 2022-03-17T13:26:03Z

docs/api-status.json

+      {
+        "name": "API.ModifySubuser",
+        "comment": "ModifySubuser - https://docs.ceph.com/en/latest/radosgw/adminops/#modify-subuser\n PREVIEW\n",
+        "added_in_version": "v0.14.0",


phlogistonjohn · 2022-03-17T13:26:54Z

docs/api-status.md

@@ -8,57 +8,59 @@

 ### Deprecated APIs

-Name | Deprecated in Version | Expected Removal Version | 


I suggest regenerating this file too.

Signed-off-by: Sebastian Riese <[email protected]>

Some unit tests for the validation and integration tests. Signed-off-by: Sebastian Riese <[email protected]>

Pull request has been modified.

sebastianriese · 2022-03-17T15:53:23Z

@phlogistonjohn The issues you raised should be fixed now. Thanks again for guiding me through the process!

phlogistonjohn

LGTM, thanks a bunch for working on this!

sebastianriese marked this pull request as draft February 11, 2022 13:54

sebastianriese changed the title ~~Add support for the subuser calls in the rgw admin interface~~ Draft: Add support for the subuser calls in the rgw admin interface Feb 11, 2022

ansiwen added the API This PR includes a change to the public API of a go-ceph package label Feb 15, 2022

phlogistonjohn reviewed Feb 21, 2022

View reviewed changes

sebastianriese force-pushed the feature/rgw-admin-subuser branch from b4eb471 to 5577cef Compare February 23, 2022 15:15

sebastianriese mentioned this pull request Feb 23, 2022

rgw: support more rgw admin API methods #653

Closed

sebastianriese force-pushed the feature/rgw-admin-subuser branch from 5577cef to 8cc6ff8 Compare February 24, 2022 14:51

sebastianriese force-pushed the feature/rgw-admin-subuser branch 3 times, most recently from 30a24e0 to 4eae29b Compare February 28, 2022 19:06

sebastianriese changed the title ~~Draft: Add support for the subuser calls in the rgw admin interface~~ Add support for the subuser calls in the rgw admin interface Feb 28, 2022

sebastianriese marked this pull request as ready for review March 1, 2022 10:46

sebastianriese force-pushed the feature/rgw-admin-subuser branch from 4eae29b to 382b5cf Compare March 1, 2022 11:06

leseb reviewed Mar 10, 2022

View reviewed changes

sebastianriese force-pushed the feature/rgw-admin-subuser branch from 382b5cf to c7368c9 Compare March 10, 2022 15:46

sebastianriese added 3 commits March 16, 2022 11:06

rgw/admin: Extend the testing of buildQueryPath

ba13594

Only one of the two code paths of buildQueryPath was tested so far. This makes the test harness tight by testing the second one (where the path already contains a query parameter). Signed-off-by: Sebastian Riese <[email protected]>

sebastianriese force-pushed the feature/rgw-admin-subuser branch from c7368c9 to 3be227a Compare March 16, 2022 10:07

leseb previously approved these changes Mar 17, 2022

View reviewed changes

sebastianriese force-pushed the feature/rgw-admin-subuser branch from 3be227a to 8f78854 Compare March 17, 2022 09:40

leseb previously approved these changes Mar 17, 2022

View reviewed changes

phlogistonjohn requested changes Mar 17, 2022

View reviewed changes

sebastianriese added 3 commits March 17, 2022 16:48

rgw/admin: Add the subuser API calls

f8dcafb

Signed-off-by: Sebastian Riese <[email protected]>

rgw/admin: Generate API documentation for the new methods

425a77b

rgw/admin: Tests for the subuser API calls

fac2574

Some unit tests for the validation and integration tests. Signed-off-by: Sebastian Riese <[email protected]>

sebastianriese force-pushed the feature/rgw-admin-subuser branch from 8f78854 to fac2574 Compare March 17, 2022 15:52

leseb approved these changes Mar 17, 2022

View reviewed changes

phlogistonjohn approved these changes Mar 17, 2022

View reviewed changes

mergify bot merged commit 6a4d7eb into ceph:master Mar 17, 2022

sebastianriese mentioned this pull request Mar 22, 2022

Rgw admin more actions #654

Merged

4 tasks

horazont mentioned this pull request Apr 19, 2022

Support Swift API and Keystone Integration rook/rook#9088

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the subuser calls in the rgw admin interface #644

Add support for the subuser calls in the rgw admin interface #644

sebastianriese commented Feb 11, 2022 •

edited

Loading

sebastianriese commented Feb 11, 2022

phlogistonjohn Feb 21, 2022

phlogistonjohn commented Feb 21, 2022

sebastianriese commented Feb 22, 2022 •

edited

Loading

sebastianriese commented Feb 22, 2022 •

edited

Loading

sebastianriese commented Feb 23, 2022 •

edited

Loading

sebastianriese commented Feb 24, 2022

phlogistonjohn commented Feb 24, 2022

sebastianriese commented Feb 28, 2022

leseb Mar 10, 2022

sebastianriese Mar 10, 2022

leseb Mar 10, 2022

sebastianriese Mar 10, 2022

leseb Mar 10, 2022

sebastianriese Mar 10, 2022

mergify bot commented Mar 14, 2022

sebastianriese commented Mar 16, 2022

sebastianriese commented Mar 17, 2022

phlogistonjohn left a comment

phlogistonjohn Mar 17, 2022

phlogistonjohn Mar 17, 2022

phlogistonjohn Mar 17, 2022

sebastianriese Mar 17, 2022

phlogistonjohn Mar 17, 2022

phlogistonjohn Mar 17, 2022

phlogistonjohn Mar 17, 2022

sebastianriese commented Mar 17, 2022

phlogistonjohn left a comment

		@@ -8,57 +8,59 @@

		### Deprecated APIs

		Name \| Deprecated in Version \| Expected Removal Version \|

Add support for the subuser calls in the rgw admin interface #644

Add support for the subuser calls in the rgw admin interface #644

Conversation

sebastianriese commented Feb 11, 2022 • edited Loading

Checklist

sebastianriese commented Feb 11, 2022

Choose a reason for hiding this comment

phlogistonjohn commented Feb 21, 2022

sebastianriese commented Feb 22, 2022 • edited Loading

sebastianriese commented Feb 22, 2022 • edited Loading

sebastianriese commented Feb 23, 2022 • edited Loading

sebastianriese commented Feb 24, 2022

phlogistonjohn commented Feb 24, 2022

sebastianriese commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 14, 2022

sebastianriese commented Mar 16, 2022

sebastianriese commented Mar 17, 2022

phlogistonjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianriese commented Mar 17, 2022

phlogistonjohn left a comment

Choose a reason for hiding this comment

sebastianriese commented Feb 11, 2022 •

edited

Loading

sebastianriese commented Feb 22, 2022 •

edited

Loading

sebastianriese commented Feb 22, 2022 •

edited

Loading

sebastianriese commented Feb 23, 2022 •

edited

Loading