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

ApiListener: Log connection attempts from an already connected client prominently #10207

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Oct 30, 2024

Something is definitely going wrong if a client tries to reconnect to this endpoint while it still has an active connection to that client. So we shouldn't hide this, but at least log it at info level. Apart from that, I've added some additional information about the currently active client, such as when the last message was sent and received.

Tests

Start two Icinga 2 endpoints and try to connect to one of them using either ones certificate.

echo -n '86:{"jsonrpc":"2.0","method":"icinga::Hello","params":{"capabilities":3,"version":21400}}\r\n\r\n' | openssl s_client -connect localhost:5665 -cert satellite.crt -key satellite.key

Logs on the master endpoint:

[2024-10-30 11:47:10 +0100] information/ApiListener: New client connection for identity 'satellite' from [::1]:58029
[2024-10-30 11:47:10 +0100] information/ApiListener: Ignoring JSON-RPC connection from [::1]:58029. We're already connected to Endpoint 'satellite' (last message sent: 2024-10-30 11:47:09, last message received: 2024-10-30 11:46:33).

Something is definitely going wrong if a client tries to reconnect to
this endpoint while it still has an active connection to that client. So
we shouldn't hide this, but at least log it at info level. Apart from
that, I've added some additional information about the currently active
client, such as when the last message was sent and received.
@cla-bot cla-bot bot added the cla/signed label Oct 30, 2024
@yhabteab yhabteab added enhancement New feature or request area/api REST API labels Oct 30, 2024
@yhabteab yhabteab changed the title ApiListener: Log connection attempts from an already connected client ApiListener: Log connection attempts from an already connected client prominently Oct 30, 2024
@Al2Klimov Al2Klimov added this to the 2.15.0 milestone Oct 30, 2024
@yhabteab
Copy link
Member Author

@julianbrost please also give your thoughts on this, especially on whether a log info level is enough or should be promoted to warning. I didn't want to log it as a warning because the end user is neither responsible for this nor can do anything to fix it.

@julianbrost
Copy link
Contributor

No strong opinion there on my side. Increasing it to info definitely makes sense. Currently, with the "New client connection for identity" at level info, there's otherwise little hint that this connection was closed immediately.

Going to warning, well yes, this message could be a hint that there's something funky. I presume you've created this PR because you're currently debugging a connection-related issue where you don't really know why this happens. So until we understand that issue, we couldn't provide a good answer to the question "what to do about this warning", thus, info sounds fine for the moment. If we better understand the issue and what to do about it, we can still bump it to warning (and ideally fix the underlying issue and/or provide more helpful information on what's actually wrong in the log message).

@yhabteab yhabteab added the consider backporting Should be considered for inclusion in a bugfix release label Oct 30, 2024
@yhabteab yhabteab merged commit 10775f4 into master Oct 30, 2024
27 checks passed
@yhabteab yhabteab deleted the log-connected-endpoint-connection-attempts branch October 30, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api REST API cla/signed consider backporting Should be considered for inclusion in a bugfix release enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants