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

rgw: add a new error code for non-existed user. #16033

Merged

Conversation

Projects
None yet
4 participants
@zhaochao
Copy link
Contributor

zhaochao commented Jun 30, 2017

From http://docs.ceph.com/docs/master/radosgw/adminops/, There should be
a "NoSuchUser" response for user actions rather than "NoSuchKey".

Fixes: http://tracker.ceph.com/issues/20468

Signed-off-by: Zhao Chao zhaochao1984@gmail.com

@joscollin
Copy link
Member

joscollin left a comment

Please add the following in Commit Message too.

Fixes: http://tracker.ceph.com/issues/20468

@@ -2299,7 +2299,7 @@ int RGWUserAdminOp_User::info(RGWRados *store, RGWUserAdminOpState& op_state,
return ret;

if (!op_state.has_existing_user())
return -ENOENT;
return -ERR_NO_SUCH_USER;

This comment has been minimized.

Copy link
@joscollin

joscollin Jun 30, 2017

Member

What about if (!op_state.has_existing_subuser()) ? I think similar improvement can be implemented for this too.

This comment has been minimized.

Copy link
@joscollin

joscollin Jun 30, 2017

Member

There are other instances of if (!op_state.has_existing_user()) in L2031 and L2112. Please check.

This comment has been minimized.

Copy link
@zhaochao

zhaochao Jul 1, 2017

Author Contributor

I think it's not necessary to check subuser existence when operating on RGWUser.

Maybe we should return NoSuchUser for subuser/keys/caps operations too?

@@ -2394,6 +2397,8 @@ int RGWUserAdminOp_User::remove(RGWRados *store, RGWUserAdminOpState& op_state,

ret = user.remove(op_state, NULL);

if (ret == -ENOENT)
ret = -ERR_NO_SUCH_USER;

This comment has been minimized.

Copy link
@joscollin

joscollin Jun 30, 2017

Member

Similar change can be done for Sub User too.

This comment has been minimized.

Copy link
@zhaochao

zhaochao Jul 1, 2017

Author Contributor

Yes, it's better to apply the new error code for related operations.

if (ret < 0)
if (ret < 0) {
if (ret == -ENOENT)
ret = -ERR_NO_SUCH_USER;

This comment has been minimized.

Copy link
@joscollin

joscollin Jun 30, 2017

Member

Similar change can be done for Sub User too.

This comment has been minimized.

Copy link
@zhaochao

zhaochao Jul 1, 2017

Author Contributor

Ok, I see, SubUser/Key/Caps/Quota operations should use the new error code too.

@ceph ceph deleted a comment from zhaochao Jun 30, 2017

@zhaochao zhaochao force-pushed the zhaochao:rgw-add-a-new-errcode-for-non-existed-user branch 2 times, most recently from 4e46c11 to 1a9df57 Jul 4, 2017

@zhaochao

This comment has been minimized.

Copy link
Contributor Author

zhaochao commented Jul 4, 2017

@joscollin

  1. for subusers, maybe another new error code like 'NoSuchSubUser' is more suitable ?
  2. NoSuchUser now applies to all user related operations .
@joscollin

This comment has been minimized.

Copy link
Member

joscollin commented Jul 4, 2017

for subusers, maybe another new error code like 'NoSuchSubUser' is more suitable ?

@zhaochao Yes, That would be better to distinguish the error. You can even create a new PR and refer this PR in it for the subuser changes.

But I found two more instances in L2031 and L2112 for this PR.

@zhaochao

This comment has been minimized.

Copy link
Contributor Author

zhaochao commented Jul 4, 2017

@joscollin
L2031 and L2112 are in RGWUser::execute_remove and RGWUser::execute_modify, I think they are only for internal usage (called from RGWUserAdminOp_User_xxx or directly from radosgw-admin), so it's ok to leave them as before (return -ENOENT).

@joscollin

This comment has been minimized.

Copy link
Member

joscollin commented Jul 4, 2017

so it's ok to leave them as before (return -ENOENT).

@zhaochao Please see:

RGWUser::execute_add --> execute_modify() 
RGWUser::add() --> execute_add()
RGWUserAdminOp_User::create() --> user.add()

Here the checking is for if (ret == -EEXIST). But execute_modify() returns -ENOENT; in L2112.

@zhaochao

This comment has been minimized.

Copy link
Contributor Author

zhaochao commented Jul 4, 2017

@joscollin
It seems RGWUser::execute_add will only call RGWUser::execute_modify when the user exists, in this senario, RGWUser::execute_modify won't return -ENOENT.

@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Jul 6, 2017

Pls rebase @zhaochao @joscollin and tag "needs-qa"

[yuriw@smithi153 build]$ git pull https://github.com/zhaochao/ceph.git rgw-add-a-new-errcode-for-non-existed-user
remote: Counting objects: 8, done.
remote: Total 8 (delta 7), reused 7 (delta 7), pack-reused 1
Unpacking objects: 100% (8/8), done.
From https://github.com/zhaochao/ceph

  • branch rgw-add-a-new-errcode-for-non-existed-user -> FETCH_HEAD
    Auto-merging src/rgw/rgw_user.cc
    Auto-merging src/rgw/rgw_common.h
    CONFLICT (content): Merge conflict in src/rgw/rgw_common.h
    Auto-merging src/rgw/rgw_common.cc
    CONFLICT (content): Merge conflict in src/rgw/rgw_common.cc
    Automatic merge failed; fix conflicts and then commit the result.

@oritwas oritwas added the needs-qa label Jul 6, 2017

@zhaochao zhaochao force-pushed the zhaochao:rgw-add-a-new-errcode-for-non-existed-user branch from 1a9df57 to 8239ed5 Jul 7, 2017

@zhaochao

This comment has been minimized.

Copy link
Contributor Author

zhaochao commented Jul 7, 2017

@yuriw
rebased.

rgw: add a new error code for non-existed user.
From http://docs.ceph.com/docs/master/radosgw/adminops/, There should be
a "NoSuchUser" response for user actions rather than "NoSuchKey".

Fixes: http://tracker.ceph.com/issues/20468

Signed-off-by: Zhao Chao <zhaochao1984@gmail.com>

@zhaochao zhaochao force-pushed the zhaochao:rgw-add-a-new-errcode-for-non-existed-user branch from 8239ed5 to 29a802d Jul 7, 2017

@yuriw yuriw merged commit 0ccf2d7 into ceph:master Jul 10, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.