noobaa / noobaa-core

High-performance S3 application gateway to any backend - file / s3-compatible / multi-clouds / caching / replication ...
https://www.noobaa.io
Apache License 2.0
272 stars 80 forks source link

NC | NSFS | Bucket Logging After Delete Bucket #8540

Open shirady opened 3 hours ago

shirady commented 3 hours ago

Environment info

Actual behavior

  1. When trying to send_bucket_op_logs on delete bucket operation, we would not have the bucket_info anymore in the cache; therefore we would see the printing of "Could not log bucket operation".

Expected behavior

  1. Needs to be discussed what is the optimal solution - but the basic expected is not to see those logs.

Steps to reproduce

  1. Deployment in NC NSFS
  2. Create an account and a bucket.
  3. Delete the bucket, and look for the printing of "Could not log bucket operation".

We can see it in the Ceph tests See this comment in the PR

More information - Screenshots / Logs / Other output

-

shirady commented 3 hours ago

Hi, The main question is to understand what should happen when a bucket is deleted - Should we create the bucket logging? should we have the bucket notifications?

https://github.com/noobaa/noobaa-core/blob/f924d96bc87993ca2d1615bfd69da460bb186093/src/endpoint/s3/s3_bucket_logging.js#L22 we don't have the config file anymore, like trying to access to a bucket that don't exist.

@aspandey @alphaprinz cc: @romayalon @jackyalbo

Note: In case I can add 'delete_bucket' to outer if it is a simple solution and I can do it in my PR.

    if (req.params && req.params.bucket &&
        !(req.op_name === 'put_bucket' ||
+         req.op_name === 'bucket_delete' ||
          req.op_name === 'put_bucket_notification' ||
          req.op_name === 'get_bucket_notification'
        )) {

But I must understand what you think, and better to have the GAP open than to create a wrong flow.

alphaprinz commented 2 hours ago

Regarding notifications, deleting a bucket is not an event that triggers a notification. (You can see the list of events here). So your suggested fix is ok for notifications.