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: swift: ability to update swift read and write acls separately. #14499

Merged
merged 1 commit into from Apr 26, 2017

Conversation

Projects
None yet
4 participants
@mdw-at-linuxbox
Contributor

mdw-at-linuxbox commented Apr 13, 2017

openstack swift stores read and write acls as separate attributes
which can be updated independently. Ceph stores acls in one merged
structure. To emulate swift behavior, when updating an acl, take
the old acl and merge the unmodified bits into the replacement acl.

// fixup: need to remove temp debugging stuff in src/rgw/rgw_common.cc &etc.

Fixes: http://tracker.ceph.com/issues/19289
Signed-off-by: Marcus Watts mwatts@redhat.com

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Apr 13, 2017

jenkins test this please (Agent went offline during the build)

@rzarzynski

@mdw-at-linuxbox: conceptually the changes look good. The comments are mostly about cosmetics.

if (perm == 0) perm = SWIFT_PERM_READ;
}
}
if (perm & rw_mask) {

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

As the rw_mask bases on general perm concept (SWIFT_PERM_READ, SWIFT_PERM_WRITE) and cooperates with the perm variable here, I believe it should be typed as uint32_t.

if (url_spec.empty()) {
continue;
}
if (perm == 0) perm = SWIFT_PERM_READ;

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

COSMETICS: we're mixing tab and spaces here.

if (url_spec.empty()) {
continue;
}
if (perm == 0) perm = SWIFT_PERM_READ;

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

COSMETICS: maybe something like that?

        if (perm == 0) {
          /* We need to carry also negative, HTTP referrer-based ACLs. */
          perm = SWIFT_PERM_READ;
        }
@@ -26,8 +26,10 @@ class RGWAccessControlPolicy_SWIFT : public RGWAccessControlPolicy
int create(RGWRados *store,
const rgw_user& id,
const std::string& name,
int& rw_mask,

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

COSMETICS: rw_mask is an output parameter, and thus it would be nice to have it as the very last one.

@@ -26,6 +26,8 @@
#include "include/str_list.h"
#include "auth/Crypto.h"
#include "rgw_acl_swift.h" // MDW TEMP debugging

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

ACK

rw_mask ^= (SWIFT_PERM_READ|SWIFT_PERM_WRITE);
multimap<string, ACLGrant> m = old->acl.get_grant_map();
multimap<string, ACLGrant>::iterator iter;
for (iter = m.begin(); iter != m.end(); ++iter) {

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

The range-based for loop would make this more readable.

multimap<string, ACLGrant> m = old->acl.get_grant_map();
multimap<string, ACLGrant>::iterator iter;
for (iter = m.begin(); iter != m.end(); ++iter) {
ACLGrant& grant = iter->second;

This comment has been minimized.

@rzarzynski

rzarzynski Apr 13, 2017

Contributor

It looks we don't mutate the grant. May we have it as const?

@rzarzynski rzarzynski added the bug fix label Apr 13, 2017

@rzarzynski rzarzynski self-assigned this Apr 13, 2017

@mdw-at-linuxbox

This comment has been minimized.

Contributor

mdw-at-linuxbox commented Apr 14, 2017

  1. for jenkins: looks like it worked: https://jenkins.ceph.com/job/ceph-pull-requests/22039/console
  2. range-based loop: did it. But I didn't change other pre-existing code to match so this isn't a total win.
  3. const ACLGrant. Can't do: add_grant() doesn't like const'ness.
  4. tab/spaces.... ok, sure.
  5. "carry also" - took your comment verbatim.
  6. uint32_t. did that.
  7. rw_mask: made it last.
  8. TEMP debugging. removed it.
    I've pushed the result of making 6 changes from the above (2,4-8).
@rzarzynski

@mdw-at-linuxbox: thanks for the update! :-) The patch looks good. The comments are solely about cosmetics. I'm starting the local testing.

@@ -179,11 +179,13 @@ int RGWAccessControlPolicy_SWIFT::create(RGWRados* const store,
const rgw_user& id,
const std::string& name,
const std::string& read_list,
const std::string& write_list)
const std::string& write_list,
int& rw_mask)

This comment has been minimized.

@rzarzynski

rzarzynski Apr 15, 2017

Contributor

May we have the uint32_t also here and in a couple other, related places?

@@ -27,7 +27,9 @@ class RGWAccessControlPolicy_SWIFT : public RGWAccessControlPolicy
const rgw_user& id,
const std::string& name,
const std::string& read_list,
const std::string& write_list);
const std::string& write_list,
int& rw_mask);

This comment has been minimized.

@rzarzynski

rzarzynski Apr 15, 2017

Contributor

uint32_t

@@ -940,14 +940,15 @@ class RGWPutMetadataBucket : public RGWOp {
map<string, buffer::list> attrs;
set<string> rmattr_names;
bool has_policy, has_cors;
int policy_rw_mask;

This comment has been minimized.

@rzarzynski

rzarzynski Apr 15, 2017

Contributor

uint32_t

@@ -502,6 +502,7 @@ static int get_swift_container_settings(req_state * const s,
RGWRados * const store,
RGWAccessControlPolicy * const policy,
bool * const has_policy,
int * rw_mask,

This comment has been minimized.

@rzarzynski

rzarzynski Apr 15, 2017

Contributor

uint32_t

@@ -622,8 +624,10 @@ static int get_swift_versioning_settings(
int RGWCreateBucket_ObjStore_SWIFT::get_params()
{
bool has_policy;
int policy_rw_mask = 0;

This comment has been minimized.

@rzarzynski

rzarzynski Apr 15, 2017

Contributor

uint32_t

rgw: swift: ability to update swift read and write acls separately.
openstack swift stores read and write acls as separate attributes
which can be updated independently.  Ceph stores acls in one merged
structure.  To emulate swift behavior, when updating an acl, take
the old acl and merge the unmodified bits into the replacement acl.

Fixes: http://tracker.ceph.com/issues/19289
Signed-off-by: Marcus Watts <mwatts@redhat.com>
@mdw-at-linuxbox

This comment has been minimized.

Contributor

mdw-at-linuxbox commented Apr 21, 2017

I've pushed a revised copy that uses uint32_t more places. No other changes.

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Apr 24, 2017

@mdw-at-linuxbox: thanks for the update! The patch looks very good and passed local verification -- s3tests nor Tempest haven't found any regression. Unfortunately, the Teuthology run on mira stuck in the queued state:

I'm going to spawn another one on smithi.

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Apr 24, 2017

OK, the run started progressing. Waiting for results. Meanwhile:

jenkins test this please

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Apr 26, 2017

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Apr 26, 2017

The runs look pretty good. Following unrelated failures were found:

  • one jobs failed because of network connectivity issues during the s3tests run on Apache:
2017-04-26T19:11:39.740 INFO:teuthology.orchestra.run.mira121.stderr:    raise ConnectionError(sockerr)
2017-04-26T19:11:39.740 INFO:teuthology.orchestra.run.mira121.stderr:ConnectionError: [Errno -2] Name or service not known
  • three jobs failed as Valgrind detected the well-known problems with md_config_t.

Merging.

@rzarzynski rzarzynski merged commit 362c106 into ceph:master Apr 26, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
arm build successfully built on arm
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment