From c2cb7b8a76d725076712c7df8f805ac5f9f1728f Mon Sep 17 00:00:00 2001 From: Yuval Lifshitz Date: Mon, 30 Nov 2020 16:02:40 +0200 Subject: [PATCH] rgw/notification: automatically delete topics associated with a bucket when a bucket is deleted fixes: https://tracker.ceph.com/issues/46128 Signed-off-by: Yuval Lifshitz --- doc/radosgw/notifications.rst | 10 +++++-- src/rgw/rgw_pubsub.cc | 29 ++++++++++++++++++ src/rgw/rgw_pubsub.h | 3 ++ src/rgw/rgw_sal_rados.cc | 11 +++++++ src/test/rgw/rgw_multi/tests_ps.py | 48 +++++++++++++++++++++++------- 5 files changed, 87 insertions(+), 14 deletions(-) diff --git a/doc/radosgw/notifications.rst b/doc/radosgw/notifications.rst index 8e81fd47898b5..bda5e0e973129 100644 --- a/doc/radosgw/notifications.rst +++ b/doc/radosgw/notifications.rst @@ -84,7 +84,7 @@ The same counters are shared between the pubsub sync module and the bucket notif .. note:: ``pubsub_event_triggered`` and ``pubsub_event_lost`` are incremented per event, while: - ``pubsub_push_ok``, ``pubsub_push_fail``, are incremented per push action on each notification. + ``pubsub_push_ok``, ``pubsub_push_fail``, are incremented per push action on each notification Bucket Notification REST API ---------------------------- @@ -303,7 +303,12 @@ Delete Topic Action=DeleteTopic &TopicArn= -Delete the specified topic. Note that deleting a deleted topic should result with no-op and not a failure. +Delete the specified topic. + +.. note:: + + - Deleting an unknown notification (e.g. double delete) is not considered an error + - Deleting a topic does not automatically delete all notifications associated with it The response will have the following format: @@ -363,7 +368,6 @@ Detailed under: `Bucket Operations`_. - "Abort Multipart Upload" request does not emit a notification - Both "Initiate Multipart Upload" and "POST Object" requests will emit an ``s3:ObjectCreated:Post`` notification - Events ~~~~~~ diff --git a/src/rgw/rgw_pubsub.cc b/src/rgw/rgw_pubsub.cc index a4c83c4077320..66de492e9414d 100644 --- a/src/rgw/rgw_pubsub.cc +++ b/src/rgw/rgw_pubsub.cc @@ -612,6 +612,35 @@ int RGWPubSub::Bucket::remove_notification(const string& topic_name, optional_yi return 0; } +int RGWPubSub::Bucket::remove_notifications(optional_yield y) +{ + // get all topics on a bucket + rgw_pubsub_bucket_topics bucket_topics; + auto ret = get_topics(&bucket_topics); + if (ret < 0 && ret != -ENOENT) { + ldout(ps->store->ctx(), 1) << "ERROR: failed to get list of topics from bucket '" << bucket.name << "', ret=" << ret << dendl; + return ret ; + } + + // remove all auto-genrated topics + for (const auto& topic : bucket_topics.topics) { + const auto& topic_name = topic.first; + ret = ps->remove_topic(topic_name, y); + if (ret < 0 && ret != -ENOENT) { + ldout(ps->store->ctx(), 5) << "WARNING: failed to remove auto-generated topic '" << topic_name << "', ret=" << ret << dendl; + } + } + + // delete all notification of on a bucket + ret = ps->remove(bucket_meta_obj, nullptr, y); + if (ret < 0 && ret != -ENOENT) { + ldout(ps->store->ctx(), 1) << "ERROR: failed to remove bucket topics: ret=" << ret << dendl; + return ret; + } + + return 0; +} + int RGWPubSub::create_topic(const string& name, optional_yield y) { return create_topic(name, rgw_pubsub_sub_dest(), "", "", y); } diff --git a/src/rgw/rgw_pubsub.h b/src/rgw/rgw_pubsub.h index d5331197fe504..1b50d95c3f03b 100644 --- a/src/rgw/rgw_pubsub.h +++ b/src/rgw/rgw_pubsub.h @@ -663,6 +663,9 @@ public: // return -ENOENT if the topic does not exists // return 0 on success, error code otherwise int remove_notification(const string& topic_name, optional_yield y); + // remove all notifications (and autogenerated topics) associated with the bucket + // return 0 on success or if no topic was associated with the bucket, error code otherwise + int remove_notifications(optional_yield y); }; // base class for subscription diff --git a/src/rgw/rgw_sal_rados.cc b/src/rgw/rgw_sal_rados.cc index 043a9837515f9..d893016669777 100644 --- a/src/rgw/rgw_sal_rados.cc +++ b/src/rgw/rgw_sal_rados.cc @@ -35,6 +35,8 @@ #include "services/svc_tier_rados.h" #include "cls/rgw/cls_rgw_client.h" +#include "rgw_pubsub.h" + #define dout_subsys ceph_subsys_rgw namespace rgw::sal { @@ -147,6 +149,15 @@ int RGWRadosBucket::remove_bucket(bool delete_children, std::string prefix, std: return ret; } + // if bucket has notification definitions associated with it + // they should be removed (note that any pending notifications on the bucket are still going to be sent) + RGWPubSub ps(store, info.owner.tenant); + RGWPubSub::Bucket ps_bucket(&ps, info.bucket); + const auto ps_ret = ps_bucket.remove_notifications(y); + if (ps_ret < 0 && ps_ret != -ENOENT) { + lderr(store->ctx()) << "ERROR: unable to remove notifications from bucket. ret=" << ps_ret << dendl; + } + ret = store->ctl()->bucket->unlink_bucket(info.owner, info.bucket, y, false); if (ret < 0) { lderr(store->ctx()) << "ERROR: unable to remove user bucket information" << dendl; diff --git a/src/test/rgw/rgw_multi/tests_ps.py b/src/test/rgw/rgw_multi/tests_ps.py index d074644a5ea20..841ee77dc5228 100644 --- a/src/test/rgw/rgw_multi/tests_ps.py +++ b/src/test/rgw/rgw_multi/tests_ps.py @@ -972,20 +972,22 @@ def test_ps_s3_notification_on_master(): # create s3 topic endpoint_address = 'amqp://127.0.0.1:7001' endpoint_args = 'push-endpoint='+endpoint_address+'&amqp-exchange=amqp.direct&amqp-ack-level=none' - topic_conf = PSTopicS3(master_zone.conn, topic_name, zonegroup.name, endpoint_args=endpoint_args) - topic_arn = topic_conf.set_config() + topic_conf1 = PSTopicS3(master_zone.conn, topic_name+'_1', zonegroup.name, endpoint_args=endpoint_args) + topic_arn1 = topic_conf1.set_config() + topic_conf2 = PSTopicS3(master_zone.conn, topic_name+'_2', zonegroup.name, endpoint_args=endpoint_args) + topic_arn2 = topic_conf2.set_config() # create s3 notification notification_name = bucket_name + NOTIFICATION_SUFFIX topic_conf_list = [{'Id': notification_name+'_1', - 'TopicArn': topic_arn, + 'TopicArn': topic_arn1, 'Events': ['s3:ObjectCreated:*'] }, {'Id': notification_name+'_2', - 'TopicArn': topic_arn, + 'TopicArn': topic_arn1, 'Events': ['s3:ObjectRemoved:*'] }, {'Id': notification_name+'_3', - 'TopicArn': topic_arn, + 'TopicArn': topic_arn1, 'Events': [] }] s3_notification_conf = PSNotificationS3(master_zone.conn, bucket_name, topic_conf_list) @@ -995,7 +997,7 @@ def test_ps_s3_notification_on_master(): # get notifications on a bucket response, status = s3_notification_conf.get_config(notification=notification_name+'_1') assert_equal(status/100, 2) - assert_equal(response['NotificationConfiguration']['TopicConfiguration']['Topic'], topic_arn) + assert_equal(response['NotificationConfiguration']['TopicConfiguration']['Topic'], topic_arn1) # delete specific notifications _, status = s3_notification_conf.del_config(notification=notification_name+'_1') @@ -1005,20 +1007,44 @@ def test_ps_s3_notification_on_master(): response, status = s3_notification_conf.get_config() assert_equal(status/100, 2) assert_equal(len(response['TopicConfigurations']), 2) - assert_equal(response['TopicConfigurations'][0]['TopicArn'], topic_arn) - assert_equal(response['TopicConfigurations'][1]['TopicArn'], topic_arn) + assert_equal(response['TopicConfigurations'][0]['TopicArn'], topic_arn1) + assert_equal(response['TopicConfigurations'][1]['TopicArn'], topic_arn1) # delete remaining notifications _, status = s3_notification_conf.del_config() assert_equal(status/100, 2) # make sure that the notifications are now deleted - _, status = s3_notification_conf.get_config() + response, status = s3_notification_conf.get_config() + try: + dummy = response['TopicConfigurations'] + except: + print('"TopicConfigurations" is not in response') + else: + assert False, '"TopicConfigurations" should not be in response' - # cleanup - topic_conf.del_config() + # create another s3 notification + topic_conf_list = [{'Id': notification_name+'_1', + 'TopicArn': topic_arn1, + 'Events': ['s3:ObjectCreated:*'] + }] + _, status = s3_notification_conf.set_config() + assert_equal(status/100, 2) + + # make sure the notification and auto-genrated topic are deleted + response, status = topic_conf1.get_list() + topics = response['ListTopicsResponse']['ListTopicsResult']['Topics']['member'] + before_delete = len(topics) # delete the bucket master_zone.delete_bucket(bucket_name) + response, status = topic_conf2.get_list() + topics = response['ListTopicsResponse']['ListTopicsResult']['Topics']['member'] + after_delete = len(topics) + assert_equal(before_delete - after_delete, 3) + + # cleanup + topic_conf1.del_config() + topic_conf2.del_config() def ps_s3_notification_filter(on_master): -- 2.39.5