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

[DNM] rgw: REST APIs for AWS Groups. #20383

Closed

Conversation

pritha-srivastava
Copy link
Contributor

Added code for REST APIs for manipulation of AWS Groups.
This PR is a continuation of PR 16077.

Currently admin users in rgw are allowed to create/ delete/ update groups. (User Policies for the same is not in place)

Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

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

This looks good so far.

Make sure to replace uses of ::encode/::decode with encode/decode so as not to break namespace stuff.

You may wish to rebase and compact things a bit since this goes over a relatively long timespan it would be easier to keep track of if older commits that were later changed were consolidated, but that's purely optional.

This is excellent so far.

One minor nit, why do we have the ARN stored explicitly as part of the group? That /should/ (I'd think) be derivable from the tenant and group name, unless I'm missing something.

src/rgw/rgw_group.h Outdated Show resolved Hide resolved
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
@stale
Copy link

stale bot commented Oct 18, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 18, 2018
@mattbenjamin
Copy link
Contributor

@pritha-srivastava @adamemerson is this still in holding pattern?

@stale stale bot removed the stale label Oct 18, 2018
@stale
Copy link

stale bot commented Dec 17, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Dec 17, 2018
@stale
Copy link

stale bot commented Apr 22, 2019

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@stale stale bot closed this Apr 22, 2019
@mattbenjamin
Copy link
Contributor

unstale for review

@mattbenjamin mattbenjamin reopened this Jan 19, 2022
@mattbenjamin
Copy link
Contributor

reopening for discussion

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@stale stale bot removed the stale label Jan 19, 2022
@adamemerson adamemerson self-requested a review January 21, 2022 21:12
Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

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

I approve of this PR!


An example of a Bucket Policy to list a bucket and its contents with Principal
set to a Group ARN is below::

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably note that this is NOT valid on AWS even if it is for us. Otherwise it might bite people doing multicloud.

(We /may/ want to consider having a 'compatibility' mode that rejects behaviors like this that we support that AWS doesn't, but I'm not sure how important that is.)

Limitations
===========

Currently, we do not support the REST APIs for Group operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer the case as of 7a8617c ?

@@ -474,6 +474,7 @@ void RGWUserInfo::dump(Formatter *f) const
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Things aren't going in rgw_json_enc.cc any more since 2414c75 . They should just go in the corresponding .cc files for their headers.

(I think this will result in linker errors if left unmodified.)


class RGWRestGroup : public RGWRESTOp {
protected:
string group_name;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have using namespace std in headers any more, so this should be std::

/**
* Add group name to user info
*/
extern bool rgw_add_group_to_user(RGWUserInfo& info, const string& group_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably stop sticking extern on function prototypes since it's the default.

@adamemerson
Copy link
Contributor

Does this have tests that can exercise it already in s3tests that we just have to enable? Or does it need tests?

@github-actions
Copy link

github-actions bot commented Jul 7, 2022

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Jul 7, 2022
@cbodley cbodley mentioned this pull request Jul 7, 2022
10 tasks
@djgalloway djgalloway changed the base branch from master to main July 10, 2022 00:00
@djgalloway djgalloway requested a review from a team as a code owner July 10, 2022 00:00
Copy link
Contributor

@anthonyeleven anthonyeleven left a comment

Choose a reason for hiding this comment

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

I left some formatting and wording requests, and Adam entered some unresolved notes as well.

Groups
===============

Ceph Object Gateway provides support for Amazon Groups. A group is a collection
Copy link
Contributor

Choose a reason for hiding this comment

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

The Ceph Object Gateway (RGW) provides support for Amazon S3 groups. A group is a collection of users.

Below, what does ARN mean?

Group Management
====================

A group can be created/ deleted/ updated and users can be added to /removed
Copy link
Contributor

Choose a reason for hiding this comment

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

A group may be created, deleted, or updated and users added or removed from a group with radosgw-admin commands.

Update a Group
--------------

To update the name/ path of a group, execute the following::
Copy link
Contributor

Choose a reason for hiding this comment

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

The name or path of a group

--path path to the group
--path-prefix path prefix for filtering groups
--new-group-name new name of an existing group
--new-path new path of an existing group
Copy link
Contributor

Choose a reason for hiding this comment

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

s/of/for/ ?

@github-actions github-actions bot removed the stale label Jul 15, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Sep 13, 2022
@mattbenjamin
Copy link
Contributor

un-stahl

@github-actions github-actions bot removed the stale label Sep 13, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Nov 12, 2022
@github-actions
Copy link

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@github-actions github-actions bot closed this Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants