From 745ca5b11601edcd5d6b32898db252da8bc6cdf6 Mon Sep 17 00:00:00 2001 From: Ganesh Murthy Date: Mon, 23 Dec 2024 10:13:07 -0500 Subject: [PATCH] Fixes #1705: Remove ability to update or delete router links (#1706) --- .../skupper_router/management/skrouter.json | 6 - src/router_core/agent.c | 10 +- src/router_core/agent_link.c | 161 ++---------------- src/router_core/connections.c | 2 - src/router_core/router_core_private.h | 1 - tests/system_tests_management.py | 2 +- tests/system_tests_two_routers.py | 44 +++++ 7 files changed, 66 insertions(+), 160 deletions(-) diff --git a/python/skupper_router/management/skrouter.json b/python/skupper_router/management/skrouter.json index 39c0580e2..6540c8133 100644 --- a/python/skupper_router/management/skrouter.json +++ b/python/skupper_router/management/skrouter.json @@ -1392,13 +1392,7 @@ "router.link": { "description": "Link to another AMQP endpoint: router node, client or other AMQP process.", "extends": "operationalEntity", - "operations": ["UPDATE"], "attributes": { - "adminStatus": { - "type": ["enabled", "disabled"], - "default": "enabled", - "update": true - }, "operStatus": { "type": ["up", "down", "quiescing", "idle"] }, diff --git a/src/router_core/agent.c b/src/router_core/agent.c index a0f492da3..7540246b0 100644 --- a/src/router_core/agent.c +++ b/src/router_core/agent.c @@ -427,7 +427,7 @@ static void qdr_manage_create_CT(qdr_core_t *core, qdr_action_t *action, bool di case QD_ROUTER_CONFIG_AUTO_LINK: qdra_config_auto_link_create_CT(core, name, query, in_body); break; case QD_ROUTER_CONNECTION: break; case QD_ROUTER_ROUTER_METRICS: qdr_agent_forbidden(core, query, false); break; - case QD_ROUTER_LINK: break; + case QD_ROUTER_LINK: qdr_agent_forbidden(core, query, false); break; case QD_ROUTER_ADDRESS: break; case QD_ROUTER_FORBIDDEN: qdr_agent_forbidden(core, query, false); break; } @@ -451,7 +451,7 @@ static void qdr_manage_delete_CT(qdr_core_t *core, qdr_action_t *action, bool di case QD_ROUTER_CONFIG_AUTO_LINK: qdra_config_auto_link_delete_CT(core, query, name, identity); break; case QD_ROUTER_CONNECTION: qdr_agent_forbidden(core, query, false); break; case QD_ROUTER_ROUTER_METRICS: qdr_agent_forbidden(core, query, false); break; - case QD_ROUTER_LINK: break; + case QD_ROUTER_LINK: qdr_agent_forbidden(core, query, false); break; case QD_ROUTER_ADDRESS: break; case QD_ROUTER_FORBIDDEN: qdr_agent_forbidden(core, query, false); break; } @@ -469,15 +469,15 @@ static void qdr_manage_update_CT(qdr_core_t *core, qdr_action_t *action, bool di qd_parsed_field_t *in_body = action->args.agent.in_body; if (!discard) { - switch (query->entity_type) { + switch (query->entity_type) { case QD_ROUTER_CONFIG_ADDRESS: break; case QD_ROUTER_CONFIG_AUTO_LINK: break; case QD_ROUTER_CONNECTION: qdra_connection_update_CT(core, name, identity, query, in_body); break; - case QD_ROUTER_LINK: qdra_link_update_CT(core, name, identity, query, in_body); break; + case QD_ROUTER_LINK: qdr_agent_forbidden(core, query, false); break; case QD_ROUTER_ADDRESS: break; case QD_ROUTER_ROUTER_METRICS: qdr_agent_forbidden(core, query, false); break; case QD_ROUTER_FORBIDDEN: qdr_agent_forbidden(core, query, false); break; - } + } } qdr_field_free(action->args.agent.name); diff --git a/src/router_core/agent_link.c b/src/router_core/agent_link.c index 9bd4d6fa2..d2fb5b52f 100644 --- a/src/router_core/agent_link.c +++ b/src/router_core/agent_link.c @@ -34,23 +34,22 @@ #define QDR_LINK_UNSETTLED_COUNT 9 #define QDR_LINK_DELIVERY_COUNT 10 #define QDR_LINK_CONNECTION_ID 11 -#define QDR_LINK_ADMIN_STATE 12 -#define QDR_LINK_OPER_STATE 13 -#define QDR_LINK_PRESETTLED_COUNT 14 -#define QDR_LINK_DROPPED_PRESETTLED_COUNT 15 -#define QDR_LINK_ACCEPTED_COUNT 16 -#define QDR_LINK_REJECTED_COUNT 17 -#define QDR_LINK_RELEASED_COUNT 18 -#define QDR_LINK_MODIFIED_COUNT 19 -#define QDR_LINK_DELAYED_1SEC 20 -#define QDR_LINK_DELAYED_10SEC 21 -#define QDR_LINK_DELIVERIES_STUCK 22 -#define QDR_LINK_OPEN_MOVED_STREAMS 23 -#define QDR_LINK_INGRESS_HISTOGRAM 24 -#define QDR_LINK_PRIORITY 25 -#define QDR_LINK_SETTLE_RATE 26 -#define QDR_LINK_CREDIT_AVAILABLE 27 -#define QDR_LINK_ZERO_CREDIT_SECONDS 28 +#define QDR_LINK_OPER_STATE 12 +#define QDR_LINK_PRESETTLED_COUNT 13 +#define QDR_LINK_DROPPED_PRESETTLED_COUNT 14 +#define QDR_LINK_ACCEPTED_COUNT 15 +#define QDR_LINK_REJECTED_COUNT 16 +#define QDR_LINK_RELEASED_COUNT 17 +#define QDR_LINK_MODIFIED_COUNT 18 +#define QDR_LINK_DELAYED_1SEC 19 +#define QDR_LINK_DELAYED_10SEC 20 +#define QDR_LINK_DELIVERIES_STUCK 21 +#define QDR_LINK_OPEN_MOVED_STREAMS 22 +#define QDR_LINK_INGRESS_HISTOGRAM 23 +#define QDR_LINK_PRIORITY 24 +#define QDR_LINK_SETTLE_RATE 25 +#define QDR_LINK_CREDIT_AVAILABLE 26 +#define QDR_LINK_ZERO_CREDIT_SECONDS 27 const char *qdr_link_columns[] = {"name", @@ -65,7 +64,6 @@ const char *qdr_link_columns[] = "unsettledCount", "deliveryCount", "connectionId", // The connection id of the owner connection - "adminStatus", "operStatus", "presettledCount", "droppedPresettledCount", @@ -174,11 +172,6 @@ static void qdr_agent_write_column_CT(qdr_core_t *core, qd_composed_field_t *bod break; } - case QDR_LINK_ADMIN_STATE: - text = link->admin_enabled ? "enabled" : "disabled"; - qd_compose_insert_string(body, text); - break; - case QDR_LINK_OPER_STATE: switch (link->oper_status) { case QDR_LINK_OPER_UP: text = "up"; break; @@ -388,125 +381,3 @@ void qdra_link_get_next_CT(qdr_core_t *core, qdr_query_t *query) qdr_agent_enqueue_response_CT(core, query); } - -static void qdr_manage_write_response_map_CT(qdr_core_t *core, qd_composed_field_t *body, qdr_link_t *link) -{ - qd_compose_start_map(body); - - for(int i = 0; i < QDR_LINK_COLUMN_COUNT; i++) { - qd_compose_insert_string(body, qdr_link_columns[i]); - qdr_agent_write_column_CT(core, body, i, link); - } - - qd_compose_end_map(body); -} - - -static qdr_link_t *qdr_link_find_by_identity(qdr_core_t *core, qd_iterator_t *identity) -{ - if (!identity) - return 0; - - qdr_link_t *link = DEQ_HEAD(core->open_links); - - while(link) { - char id[100]; - if (link->identity) { - snprintf(id, 100, "%"PRId64, link->identity); - if (qd_iterator_equal(identity, (const unsigned char *)id)) - break; - } - link = DEQ_NEXT(link); - } - - return link; - -} - - -static qdr_link_t *qdr_link_find_by_name(qdr_core_t *core, qd_iterator_t *name) -{ - if(!name) - return 0; - - qdr_link_t *link = DEQ_HEAD(core->open_links); - - while(link) { - if (link->name && qd_iterator_equal(name, (const unsigned char *)link->name)) - break; - link = DEQ_NEXT(link); - } - - return link; -} - - -static void qdra_link_update_set_status(qdr_core_t *core, qdr_query_t *query, qdr_link_t *link) -{ - if (link) { - //link->admin_state = qd_iterator_copy(adm_state); - qdr_manage_write_response_map_CT(core, query->body, link); - query->status = QD_AMQP_OK; - } - else { - query->status = QD_AMQP_NOT_FOUND; - qd_compose_start_map(query->body); - qd_compose_end_map(query->body); - } -} - -static void qdra_link_set_bad_request(qdr_query_t *query) -{ - query->status = QD_AMQP_BAD_REQUEST; - qd_compose_start_map(query->body); - qd_compose_end_map(query->body); -} - -void qdra_link_update_CT(qdr_core_t *core, - qd_iterator_t *name, - qd_iterator_t *identity, - qdr_query_t *query, - qd_parsed_field_t *in_body) - -{ - // If the request was successful then the statusCode MUST contain 200 (OK) and the body of the message - // MUST contain a map containing the actual attributes of the entity updated. These MAY differ from those - // requested. - // A map containing attributes that are not applicable for the entity being created, or invalid values for a - // given attribute, MUST result in a failure response with a statusCode of 400 (Bad Request). - if (qd_parse_is_map(in_body)) { - // The absence of an attribute name implies that the entity should retain its existing value. - // If the map contains a key-value pair where the value is null then the updated entity should have no value - // for that attribute, removing any previous value. - - qd_parsed_field_t *admin_state = qd_parse_value_by_key(in_body, qdr_link_columns[QDR_LINK_ADMIN_STATE]); - if (admin_state) { //admin state is the only field that can be updated via the update management request - //qd_iterator_t *adm_state = qd_parse_raw(admin_state); - - if (identity) { - qdr_link_t *link = qdr_link_find_by_identity(core, identity); - // TODO - set the adm_state on the link - qdra_link_update_set_status(core, query, link); - } - else if (name) { - qdr_link_t *link = qdr_link_find_by_name(core, name); - // TODO - set the adm_state on the link - qdra_link_update_set_status(core, query, link); - } - else { - qdra_link_set_bad_request(query); - } - } - else - qdra_link_set_bad_request(query); - - } - else - query->status = QD_AMQP_BAD_REQUEST; - - // - // Enqueue the response. - // - qdr_agent_enqueue_response_CT(core, query); -} - diff --git a/src/router_core/connections.c b/src/router_core/connections.c index c7977dba6..ba4da6d4c 100644 --- a/src/router_core/connections.c +++ b/src/router_core/connections.c @@ -699,7 +699,6 @@ qdr_link_t *qdr_link_first_attach(qdr_connection_t *conn, link->link_direction = dir; link->capacity = conn->link_capacity; link->credit_pending = conn->link_capacity; - link->admin_enabled = true; link->oper_status = QDR_LINK_OPER_DOWN; link->core_ticks = qdr_core_uptime_ticks(conn->core); link->zero_credit_time = link->core_ticks; @@ -1219,7 +1218,6 @@ qdr_link_t *qdr_create_link_CT(qdr_core_t *core, link->disambiguated_name = 0; link->terminus_addr = 0; qdr_generate_link_name("qdlink", link->name, QD_DISCRIMINATOR_SIZE + 8); - link->admin_enabled = true; link->oper_status = QDR_LINK_OPER_DOWN; link->insert_prefix = 0; link->strip_prefix = 0; diff --git a/src/router_core/router_core_private.h b/src/router_core/router_core_private.h index df69d8a6d..e8734f6a9 100644 --- a/src/router_core/router_core_private.h +++ b/src/router_core/router_core_private.h @@ -462,7 +462,6 @@ struct qdr_link_t { int credit_reported; ///< Number of credits to expose to management uint32_t zero_credit_time; ///< Number of core ticks when credit last went to zero bool reported_as_blocked; ///< The fact that this link has been blocked with zero credit has been logged - bool admin_enabled; bool strip_annotations_in; bool strip_annotations_out; bool drain_mode; diff --git a/tests/system_tests_management.py b/tests/system_tests_management.py index e5975e3f5..d63d9fd63 100644 --- a/tests/system_tests_management.py +++ b/tests/system_tests_management.py @@ -434,7 +434,7 @@ def test_get_operations(self): result = self.node.get_operations() for type in AMQP_LISTENER_TYPE, ROUTER_LINK_TYPE: self.assertIn(type, result) - self.assertEqual(["UPDATE", "READ"], result[ROUTER_LINK_TYPE]) + self.assertEqual(["READ"], result[ROUTER_LINK_TYPE]) def test_get_attributes(self): result = self.node.get_attributes(type=DUMMY_TYPE) diff --git a/tests/system_tests_two_routers.py b/tests/system_tests_two_routers.py index 810bf42fc..2a0752e4f 100644 --- a/tests/system_tests_two_routers.py +++ b/tests/system_tests_two_routers.py @@ -335,6 +335,50 @@ def test_21_delete_connection_with_receiver(self): self.assertEqual(test.error, None) test.run() + def test_22_delete_update_inter_router_link(self): + """ + This test tries to delete an inter-router link but is + prevented from doing so. + """ + query_command = 'QUERY --type=link' + outputs = json.loads(self.run_skmanage(query_command)) + passed = False + + # Should fail trying to delete an inter-router link. + for output in outputs: + if "inter-router" == output['linkType']: + identity = output['identity'] + if identity: + update_command = 'DELETE --type=link --id=' + identity + try: + json.loads(self.run_skmanage(update_command)) + except Exception as e: + if "Forbidden" in str(e): + passed = True + break + + # The test has passed since we were forbidden from deleting + # inter-router links. + self.assertTrue(passed) + + passed = False + # Should fail trying to update the admin status an inter-router link. + for output in outputs: + if "inter-router" == output['linkType']: + identity = output['identity'] + if identity: + update_command = 'UPDATE --type=link linkName=helloWorld --id=' + identity + try: + json.loads(self.run_skmanage(update_command)) + except Exception as e: + if "Forbidden" in str(e): + passed = True + break + + # The test has passed since we were forbidden from updating + # inter-router links. + self.assertTrue(passed) + def test_30_huge_address(self): # try a link with an extremely long address # DISPATCH-1461