Contributing User Policy tests #453

ketanarlulkar · 2022-05-19T08:12:12Z

Inline with the issue created at #448 , adding few put/get/list/delete User Policy Tests.

alimaredia · 2022-05-31T15:24:14Z

@ketanarlulkar @ravindrachoudhari @seagate-sarang-sawant could you squash all the commits down to one commit (or a lower number) so that this code is easier to cherry-pick once you're all set. Also if I were to test this on a Ceph cluster are there any Ceph config variables I would need to configure?

s3tests_boto3/functional/test_iam.py

seagate-sarang-sawant · 2022-06-01T04:11:28Z

@ketanarlulkar @ravindrachoudhari @seagate-sarang-sawant could you squash all the commits down to one commit (or a lower number) so that this code is easier to cherry-pick once you're all set. Also if I were to test this on a Ceph cluster are there any Ceph config variables I would need to configure?

Yes it was a mistake to not keep squash and merge as default in our fork. We have made 'squash and merge' default recently. Let us fix the PR.

s3tests_boto3/functional/test_iam.py

pritha-srivastava · 2022-06-01T04:53:42Z

@ketanarlulkar @ravindrachoudhari @seagate-sarang-sawant could you squash all the commits down to one commit (or a lower number) so that this code is easier to cherry-pick once you're all set. Also if I were to test this on a Ceph cluster are there any Ceph config variables I would need to configure?

I agree, squashing commits will make it easier to review.

Signed-off-by: Ketan Arlulkar <6256964+ketanarlulkar@users.noreply.github.com>

Fixed review comments on ceph#453 Signed-off-by: Ketan Arlulkar <6256964+ketanarlulkar@users.noreply.github.com>

s3tests_boto3/functional/test_iam.py

Signed-off-by: Ravindra Choudhari <ravindra.choudhari@seagate.com>

* Fixed review comments on ceph#453 Signed-off-by: Ravindra Choudhari <ravindra.choudhari@seagate.com>

pritha-srivastava · 2022-06-10T04:33:11Z

I ran the tests locally, and all tests passed. One thing that I had to do was to add --caps=user-policy=* to the iam user in vstart.sh @alimaredia : should this be added to vstart.sh? or someone running these tests is expected to manually add this caps to the iam_user in their config file? Also I think this change will be required in teuthology (adding user-policy caps to iam user). @ketanarlulkar @ravindrachoudhari : You said some tests were failing because of mismatch in the expected error code by AWS and that returned by RGW and that you modified the code. But all tests passed for me? Can you explain how is this possible? Readme needs to be updated for User Policy/IAM tests.

The following three are still unresolved:

The README hasn't been updated.
A clean teuthology run.
We still havent decided whether vstart.sh will have user-policy caps added for the iam user or is the user expected to do this manually. If the user is expected to do this manually, then that needs to go into the README too. @alimaredia can you please comment on this.

pritha-srivastava · 2022-06-13T05:45:03Z

@alimaredia: I plan to use https://github.com/ceph/ceph/pull/35986/files as a reference in order to integrate these iam/user-policy tests in teuthology. Can you please confirm if something else needs to be done? I think an extra needs to be added for each iam test here in order to filter the tests out when s3tests are executed?

ravindrachoudhari · 2022-06-13T08:48:40Z

@pritha-srivastava @alimaredia Added IAM policy tests section in the README file
Please review

README.rst

pritha-srivastava · 2022-06-14T05:08:38Z

Ceph changes for vstart and teuthology here: ceph/ceph#46660

pritha-srivastava · 2022-06-14T05:10:19Z

@ravindrachoudhari : can you please add this attr - 'test_of_iam' to each test, in order to filter these tests in teuthology?

ravindrachoudhari · 2022-06-14T05:12:13Z

@ravindrachoudhari : can you please add this attr - 'test_of_iam' to each test, in order to filter these tests in teuthology?

Sure, I will add @attr('test_of_iam') for all tests

ravindrachoudhari · 2022-06-14T05:18:01Z

@ravindrachoudhari : can you please add this attr - 'test_of_iam' to each test, in order to filter these tests in teuthology?

Sure, I will add @attr('test_of_iam') for all tests

@pritha-srivastava Should I remove @attr('user-policy') or should I keep both @attr('user-policy') and @attr('test_of_iam')

pritha-srivastava · 2022-06-14T05:18:59Z

@ravindrachoudhari : can you please add this attr - 'test_of_iam' to each test, in order to filter these tests in teuthology?

Sure, I will add @attr('test_of_iam') for all tests

@pritha-srivastava Should I remove @attr('user-policy') or should I keep both @attr('user-policy') and @attr('test_of_iam')

you can keep both

ravindrachoudhari · 2022-06-14T05:42:38Z

@ravindrachoudhari : can you please add this attr - 'test_of_iam' to each test, in order to filter these tests in teuthology?

Sure, I will add @attr('test_of_iam') for all tests

@pritha-srivastava Should I remove @attr('user-policy') or should I keep both @attr('user-policy') and @attr('test_of_iam')

you can keep both

Done

ravindrachoudhari · 2022-06-15T05:32:50Z

@pritha-srivastava did you get chance to take the teuthology run?

README.rst

alimaredia · 2022-06-15T14:21:12Z

@pritha-srivastava let's not make any changes to vstart.sh

@alimaredia: I plan to use https://github.com/ceph/ceph/pull/35986/files as a reference in order to integrate these iam/user-policy tests in teuthology. Can you please confirm if something else needs to be done? I think an extra needs to be added for each iam test here in order to filter the tests out when s3tests are executed?

@pritha-srivastava Yes an extra attr needs to be added, which you have seemed to discuss. Are you going to be adding in an iam directory in ceph/qa/suites/rgw that will run s3-tests to add the iam policy tests?

ravindrachoudhari · 2022-06-21T04:11:12Z

@pritha-srivastava @alimaredia can we merge this PR if everything is fine? or are we waiting for teuthology changes(ceph/ceph#46660) to get merged first?

pritha-srivastava · 2022-06-21T05:03:31Z

@pritha-srivastava @alimaredia can we merge this PR if everything is fine? or are we waiting for teuthology changes(ceph/ceph#46660) to get merged first?

@ravindrachoudhari : I remember we had discussed in the stand-up that ceph/ceph#46660 needs to get merged first, and that if you are following the PR, I am working on it.

alimaredia · 2022-06-24T20:54:12Z

@ketanarlulkar @ravindrachoudhari The Ceph PR ceph/ceph#46660 is ready to go. Could you squash your commits on this PR and then I'll merge this (and the Ceph PR) in conjunction.

c4d30d7 Ravindra Choudhari Mon, 27 Jun 2022 removing region name 4a13f58 Ravindra Choudhari Thu, 16 Jun 2022 Updating readme file (#15) 18bc152 Ravindra Choudhari Tue, 14 Jun 2022 Adding attr test_of_iam to all user policy tests (#13) 03f520a Ravindra Choudhari Tue, 14 Jun 2022 resolving review comments (#12) 7cf2823 Ravindra Choudhari Mon, 13 Jun 2022 added IAM policy test section in README.rst (#11) 563f3ea Ravindra Choudhari Fri, 10 Jun 2022 adding failing three tests back with attr @fails_on_rgw (#10) 696dd2e Ravindra Choudhari Mon, 6 Jun 2022 changes as per review comments 3d63dfd Ravindra Choudhari Mon, 6 Jun 2022 Fixed review comments (#8) 9492f69 Ravindra Choudhari Fri, 3 Jun 2022 Fixed review comments (#7) 74095dc Ketan Arlulkar Wed, 1 Jun 2022 Fixed review comments (#6) 942fb4f Ketan Arlulkar Wed, 1 Jun 2022 Added Tests for conflicting policies and IAM actions (#4) ad5b5ae Ravindra Choudhari Tue, 31 May 2022 IAM policies s3 actions (#5) 6515ec6 Ketan Arlulkar Fri, 27 May 2022 Corrected eq import 40a2841 Ravindra Choudhari Tue, 17 May 2022 resolving conflicts f53a5c1 Ravindra Choudhari Tue, 17 May 2022 added cleanup 747d563 Ketan Arlulkar Tue, 17 May 2022 Added cleanup/Delete Policy d1cc1d8 Ketan Arlulkar Mon, 16 May 2022 Fixed review comments 1ec43a2 Ravindra Choudhari Mon, 16 May 2022 delete user policy tests a01722e Ravindra Choudhari Mon, 16 May 2022 get user policy tests ff9d676 Ketan Arlulkar Fri, 13 May 2022 Removed TEST IDs d261400 Ketan Arlulkar Tue, 10 May 2022 Put User Policy & List User Policy Tests Signed-off-by: Ravindra Choudhari <ravindra.choudhari@seagate.com>

ravindrachoudhari · 2022-06-27T06:29:15Z

@ketanarlulkar @ravindrachoudhari The Ceph PR ceph/ceph#46660 is ready to go. Could you squash your commits on this PR and then I'll merge this (and the Ceph PR) in conjunction.

@alimaredia removed the region as suggested and done the commit's squash.

alimaredia · 2022-06-28T04:19:50Z

@ravindrachoudhari great! I will double check that both this PR and the Ceph are ready to merge with @cbodley in case he doesn't know about them and merge both ASAP.

ravindrachoudhari · 2022-06-28T04:23:35Z

@ravindrachoudhari great! I will double check that both this PR and the Ceph are ready to merge with @cbodley in case he doesn't know about them and merge both ASAP.

Thank you.

soumyakoduri · 2022-06-29T12:43:32Z

@pritha-srivastava @ravindrachoudhari @alimaredia ... the new tests added caused regression in dbstore test-suite - http://qa-proxy.ceph.com/teuthology/soumyakoduri-2022-06-29_09:47:08-rgw-wip-skoduri-dbstore-lc-distro-default-smithi/6905136/teuthology.log

Please include 'fails_on_dbstore' attr as well if you find any test cases failing against dbstore. Will submit new PR to fix this.

cbodley · 2022-06-29T12:59:01Z

thanks @soumyakoduri, i opened https://tracker.ceph.com/issues/56418 to track this

soumyakoduri · 2022-06-29T16:15:38Z

thanks @soumyakoduri, i opened https://tracker.ceph.com/issues/56418 to track this

Pushed #460 as an interim workaround. But I think the right fix is to skip these iam/sts tests if iam {} section is not added to s3tests.conf instead of throwing error . Have raised https://tracker.ceph.com/issues/56421 to fix the test cases.

amathuria requested a review from cbodley May 19, 2022 09:18

cbodley requested review from alimaredia and pritha-srivastava May 26, 2022 17:57