Skip to content

feat: Allow updating logging verbosity dynamically at runtime #1829

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sadhansood
Copy link
Contributor

@sadhansood sadhansood commented Mar 27, 2025

Description

This PR adds code for updating log directives to control logging verbosity dynamically at runtime through a service endpoint. The endpoint is only accessible from the localhost or from the same ip the server is listening on which prevents malicious access.

Test plan

Tested it on one of the private testnet node and confirmed to be working:

Basic Usage

// Set log level as info
curl --http0.9 -k -X POST "https://<endpoint>:9185/v1/log/directive?directive=info"

Granular Control

# Debug logs for walrus, info for everything else
curl --http0.9 -k -X POST "https://<endpoint>:9185/v1/log/directive?directive=info%2Cwalrus%3Ddebug"

@sadhansood sadhansood requested review from halfprice and mlegner March 27, 2025 00:55
@sadhansood sadhansood force-pushed the sadhan/update_log_directive branch from 3a94b07 to 309cf32 Compare March 27, 2025 01:03
@mlegner
Copy link
Collaborator

mlegner commented Mar 27, 2025

@sadhansood Thanks for adding this feature. I think we need to be pretty careful with this though: AFAICT from skimming the PR, this endpoint can currently be called by anyone without authentication. If we want to have this or other control endpoints, we should expose them on a separate port that is not exposed to the public.

@sadhansood sadhansood force-pushed the sadhan/update_log_directive branch 3 times, most recently from 907f6ae to e24cd5b Compare March 27, 2025 06:23
@sadhansood
Copy link
Contributor Author

sadhansood commented Mar 27, 2025

@sadhansood Thanks for adding this feature. I think we need to be pretty careful with this though: AFAICT from skimming the PR, this endpoint can currently be called by anyone without authentication. If we want to have this or other control endpoints, we should expose them on a separate port that is not exposed to the public.

Thanks for the security feedback @mlegner ! You're absolutely right to be cautious. I've actually implemented a security check in the set_log_directive function that restricts access to server IP only and tested it from the node and my local machine to verify it works.The code at line 766-771 verifies the request IP matches the server's bound address:

// Check if the request is from the server's bound IP
if addr.ip() != state.rest_api_address().ip() {
    return Err(OrRejection::Err(LogDirectiveError::Unauthorized(
        "This endpoint can only be accessed from localhost or the server's own IP".to_string(),
    )));
}

This ensures only requests from the same machine can modify log levels. I've also added proper error handling with detailed error messages for any unauthorized attempts.
That said, I completely agree that a dedicated admin port would be a more robust long-term solution. I am more than happy to implement your suggested approach as a long term solution but i think this IP-based restriction sufficient for now given this is primarily used for debugging/operational purposes and would be pretty useful for debugging node issues?

@sadhansood sadhansood force-pushed the sadhan/update_log_directive branch from e24cd5b to c4458b5 Compare March 27, 2025 06:51
Copy link
Collaborator

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

@sadhansood Sorry again for my original unqualified review. As already mentioned at #2020, we should ideally have a unified interface through which to perform run-time operations, either through a UNIX socket or through a separate port that is only available locally.

cc @liquid-helium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants