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

Fix logging level in the assert_s3_acl #598

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

vvarg229
Copy link
Collaborator

Logging level changed from error to warning.
Now there will be no such messages in the logs:

-------------------------------- live log call ---------------------------------
Error: -14 10:10:37 [ERROR] FULL_CONTROL is given to All Users
PASSED [ 62%]

@roman-khimov
Copy link
Member

Are you sure it's a logger problem? It seems more like an S3 GW issue to me and a test issue as well (it only logs, but should fail).

@roman-khimov
Copy link
Member

@smallhive, can you check what's up there? This seems to be very suspicious to me.

Refs. nspcc-dev/neofs-s3-gw#801.

@smallhive
Copy link
Contributor

smallhive commented Aug 21, 2023

@smallhive, can you check what's up there? This seems to be very suspicious to me.

Refs. nspcc-dev/neofs-s3-gw#801.

actions-runner/_work/neofs-node/neofs-node/neofs-testcases/pytest_tests/helpers/s3_helper.py

def assert_s3_acl(acl_grants: list, permitted_users: str):
    ...

    if permitted_users == "CanonicalUser":
        for acl_grant in acl_grants:
            if acl_grant.get("Grantee", {}).get("Type") == "CanonicalUser":
                ...
            else:
                logger.error("FULL_CONTROL is given to All Users")

Inside the gate, we don't have logs with such patterns.

@roman-khimov
Copy link
Member

I'm more about the actual gw behavior, logs are secondary to it.

@smallhive
Copy link
Contributor

After deeper investigation and with @vvarg229 discussion we decided - it is a problem, but with test.

    def test_s3_copy_acl(self, bucket, simple_object_size):
        ...
        with allure.step("Copy object and check acl attribute"):
            copy_obj_path = s3_gate_object.copy_object_s3(
                self.s3_client, bucket, obj_key, ACL="public-read-write"
            )
            obj_acl = s3_gate_object.get_object_acl_s3(self.s3_client, bucket, copy_obj_path)
            assert_s3_acl(acl_grants=obj_acl, permitted_users="CanonicalUser")

We are copying objects but set ACL="public-read-write", which means Full control for all users. According to the gate code in this case, we give access to everyone.

To fix the test and remove FULL_CONTROL is given to All Users we need to set ACL="private".
I tend to think it was an error from a previous commit. It was feractor

@vvarg229 vvarg229 marked this pull request as draft September 2, 2023 14:22
@vvarg229 vvarg229 marked this pull request as ready for review September 2, 2023 17:08
@vvarg229
Copy link
Collaborator Author

vvarg229 commented Sep 2, 2023

After deeper investigation and with @vvarg229 discussion we decided - it is a problem, but with test.

    def test_s3_copy_acl(self, bucket, simple_object_size):
        ...
        with allure.step("Copy object and check acl attribute"):
            copy_obj_path = s3_gate_object.copy_object_s3(
                self.s3_client, bucket, obj_key, ACL="public-read-write"
            )
            obj_acl = s3_gate_object.get_object_acl_s3(self.s3_client, bucket, copy_obj_path)
            assert_s3_acl(acl_grants=obj_acl, permitted_users="CanonicalUser")

We are copying objects but set ACL="public-read-write", which means Full control for all users. According to the gate code in this case, we give access to everyone.

To fix the test and remove FULL_CONTROL is given to All Users we need to set ACL="private". I tend to think it was an error from a previous commit. It was feractor

Fixed

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

  1. I'd reorder these commits to fix the test first and then make it fail in case of wrong ACL, just to have it always pass.
  2. logger.error() doesn't seem to be useful if we raise an error anyway.
  3. These fixes should not sit in the queue for a long time, they're trivial.

Change ACL from public-read-write to private in the test_s3_copy_acl test.

Signed-off-by: Oleg Kulachenko <[email protected]>
Tests will fail with AssertionError, not just write the error to the log.

Signed-off-by: Oleg Kulachenko <[email protected]>
@vvarg229
Copy link
Collaborator Author

vvarg229 commented Sep 3, 2023

  1. I'd reorder these commits to fix the test first and then make it fail in case of wrong ACL, just to have it always pass.
  2. logger.error() doesn't seem to be useful if we raise an error anyway.
  3. These fixes should not sit in the queue for a long time, they're trivial.

Done

@roman-khimov roman-khimov merged commit 1b8a0b3 into nspcc-dev:master Sep 4, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants