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

refactor(RHINENG-12780): Enhance delete events #2111

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/host.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ def delete_hosts_by_filter(


def _delete_host_list(host_id_list, rbac_filter):
request_host = flask.request.headers.get("Host", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the request_host value, if I make a REST API call from my laptop?
Also suggest using requesting_host to leave out any ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

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

This may be renamed to another thing? My understand is that this value would be the consoleDOT URL?

Suggested change
request_host = flask.request.headers.get("Host", "")
request_host_url = flask.request.headers.get("Host", "")

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jpramos123 I think it's just the hostname of the request, so maybe request_hostname?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kruai Yup, makes sense

current_identity = get_current_identity()
payload_tracker = get_payload_tracker(
account=current_identity.account_number, org_id=current_identity.org_id, request_id=threadctx.request_id
Expand All @@ -260,6 +261,7 @@ def _delete_host_list(host_id_list, rbac_filter):
inventory_config().host_delete_chunk_size,
identity=current_identity,
control_rule=get_control_rule(),
is_manual_delete=check_manual_deletion(request_host),
)

deleted_id_list = [str(r.host_row.id) for r in result_list]
Expand Down Expand Up @@ -312,6 +314,10 @@ def delete_host_by_id(host_id_list, rbac_filter=None):
return flask.Response(None, HTTPStatus.OK)


def check_manual_deletion(origin_host: str = ""):
return origin_host in ["console.stage.redhat.com", "console.redhat.com"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using origin_host.lower(), just in case CONSOLE.* or Console.* is used by the the request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check enough to tell that the request is specifically from the UI? It looks to me like this header gets set to the target hostname for all HTTP requests



@api_operation
@rbac(RbacResourceType.HOSTS, RbacPermission.READ)
@metrics.api_request_time.time()
Expand Down
5 changes: 4 additions & 1 deletion app/queue/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class HostDeleteEvent(Schema):
org_id = fields.Str()
insights_id = fields.Str()
request_id = fields.Str()
subscription_manager_id = fields.Str()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should start using fields.UUID() for all fields with value type UUID.

manual_delete = fields.Bool()
platform_metadata = fields.Dict()
metadata = fields.Nested(HostEventMetadataSchema())

Expand Down Expand Up @@ -113,14 +115,15 @@ def host_create_update_event(event_type, host, platform_metadata=None):
)


def host_delete_event(event_type, host, platform_metadata=None):
def host_delete_event(event_type, host, is_manual_delete=False, platform_metadata=None):
delete_event = {
"timestamp": datetime.now(timezone.utc),
"type": event_type.name,
"id": host.id,
**serialize_canonical_facts(host.canonical_facts),
"org_id": host.org_id,
"account": host.account,
"manual_delete": is_manual_delete,
"request_id": threadctx.request_id,
"platform_metadata": platform_metadata,
"metadata": {"request_id": threadctx.request_id},
Expand Down
9 changes: 7 additions & 2 deletions app/queue/host_mq.py
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,13 @@ def handle_message(message, notification_event_producer, message_operation=add_h
raise


def write_delete_event_message(event_producer: EventProducer, result: OperationResult):
event = build_event(EventType.delete, result.host_row, platform_metadata=result.platform_metadata)
def write_delete_event_message(event_producer: EventProducer, result: OperationResult, is_manual_delete: bool):
event = build_event(
EventType.delete,
result.host_row,
platform_metadata=result.platform_metadata,
is_manual_delete=is_manual_delete,
)
headers = message_headers(
EventType.delete,
result.host_row.canonical_facts.get("insights_id"),
Expand Down
12 changes: 9 additions & 3 deletions lib/host_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,15 @@ def _delete_host_db_records(select_query, chunk_size, identity, interrupt, contr


def _send_delete_messages_for_batch(
processed_rows: list[OperationResult], event_producer: EventProducer, notification_event_producer: EventProducer
processed_rows: list[OperationResult],
event_producer: EventProducer,
notification_event_producer: EventProducer,
is_manual_delete: bool,
):
for result in processed_rows:
if result is not None:
delete_host_count.inc()
write_delete_event_message(event_producer, result)
write_delete_event_message(event_producer, result, is_manual_delete)
send_notification(notification_event_producer, NotificationType.system_deleted, vars(result.host_row))


Expand All @@ -56,12 +59,15 @@ def delete_hosts(
interrupt=lambda: False,
identity=None,
control_rule=None,
is_manual_delete=False,
):
while select_query.count():
if kafka_available():
with session_guard(select_query.session):
batch_events = _delete_host_db_records(select_query, chunk_size, identity, interrupt, control_rule)
_send_delete_messages_for_batch(batch_events, event_producer, notification_event_producer)
_send_delete_messages_for_batch(
batch_events, event_producer, notification_event_producer, is_manual_delete
)

# yield the items in batch_events
yield from batch_events
Expand Down
15 changes: 14 additions & 1 deletion tests/helpers/mq_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,13 @@ def assert_mq_host_data(actual_id, actual_event, expected_results, host_keys_to_


def assert_delete_event_is_valid(
event_producer, host, timestamp, expected_request_id=None, expected_metadata=None, identity=USER_IDENTITY
event_producer,
host,
timestamp,
expected_request_id=None,
expected_metadata=None,
identity=USER_IDENTITY,
is_manual_delete=False,
):
event = json.loads(event_producer.event)

Expand All @@ -118,6 +124,8 @@ def assert_delete_event_is_valid(
"org_id",
"insights_id",
"request_id",
"subscription_manager_id",
"manual_delete",
"platform_metadata",
"metadata",
}
Expand All @@ -129,6 +137,11 @@ def assert_delete_event_is_valid(

assert host.canonical_facts.get("insights_id") == event["insights_id"]

assert event["manual_delete"] == is_manual_delete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert event["manual_delete"] == is_manual_delete
assert event["manual_delete"] is is_manual_delete


if is_manual_delete:
assert event["subscription_manager_id"] is not None

assert event_producer.key == str(host.id)
assert event_producer.headers == expected_headers(
"delete",
Expand Down
28 changes: 26 additions & 2 deletions tests/test_api_hosts_delete.py
Original file line number Diff line number Diff line change
Expand Up @@ -587,11 +587,32 @@ def test_postgres_delete_filtered_hosts_nomatch(
assert response_data["results"][0]["id"] == not_deleted_host_id


def test_delete_with_ui_host(
db_create_host, api_delete_host, event_datetime_mock, event_producer_mock, notification_event_producer_mock
):
host = db_create_host(extra_data={"canonical_facts": {"subscription_manager_id": generate_uuid()}})
headers = {"Host": "console.redhat.com"}

response_status, _ = api_delete_host(host.id, extra_headers=headers)

assert_response_status(response_status, expected_status=200)

assert_delete_event_is_valid(
event_producer=event_producer_mock, host=host, timestamp=event_datetime_mock, is_manual_delete=True
)


class DeleteHostsMock:
@classmethod
def create_mock(cls, hosts_ids_to_delete):
def create_mock(cls, hosts_ids_to_delete, is_manual_delete=False):
def _constructor(
select_query, event_producer, notification_event_producer, chunk_size, identity=None, control_rule=None
select_query,
event_producer,
notification_event_producer,
chunk_size,
identity=None,
control_rule=None,
is_manual_delete=is_manual_delete,
):
return cls(
hosts_ids_to_delete,
Expand All @@ -601,6 +622,7 @@ def _constructor(
chunk_size,
identity=identity,
control_rule=control_rule,
is_manual_delete=is_manual_delete,
)

return _constructor
Expand All @@ -614,6 +636,7 @@ def __init__(
chunk_size,
identity=None,
control_rule=None,
is_manual_delete=False,
):
self.host_ids_to_delete = host_ids_to_delete
self.original_query = delete_hosts(
Expand All @@ -623,6 +646,7 @@ def __init__(
chunk_size,
identity=identity,
control_rule=control_rule,
is_manual_delete=is_manual_delete,
)

def __getattr__(self, item):
Expand Down