-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
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). |
@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. |
I'm more about the actual gw behavior, logs are secondary to it. |
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". |
Fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
logger.error()
doesn't seem to be useful if we raise an error anyway.- 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]>
Done |
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%]