Dqlite: Fix TLS handshake errors - #470
Merged
Merged
Conversation
roosterfish
force-pushed
the
dqlite_fixes
branch
from
August 28, 2025 09:00
3bb2bc2 to
9944c68
Compare
Setting both the timeout (from the outer context's deadline) and the context for the dial is redundant. Instead only use the outer context for the TLS dial. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
We use lowercase dqlite in all other messages. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Add the response body to the reverter. Also closing the body should succeed. If not it can be considered an error. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
Member
|
If deadline is zero, does WithDeadline never cancel or immediately cancel? |
roosterfish
force-pushed
the
dqlite_fixes
branch
from
August 28, 2025 13:00
9944c68 to
c44bce0
Compare
Contributor
Author
I got rid of this part in the changes. But I guess it will never cancel as deadline is zero. So it's the same as setting no deadline. |
Contributor
Author
|
@markylaing let's talk this through during our 1:1 next week. |
roosterfish
marked this pull request as ready for review
August 28, 2025 15:22
markylaing
reviewed
Sep 1, 2025
roosterfish
force-pushed
the
dqlite_fixes
branch
from
September 1, 2025 14:59
c44bce0 to
5c8e6b8
Compare
markylaing
reviewed
Sep 2, 2025
This allows filtering out the TLS handshake error messages which might appear in case go-dqlite cancels the connection attempts prematuerely in case the leader was already found. It doesn't hide an error but instead suppresses errors which are considered to be not desired. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
This allows injecting our filter logger. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
This allows calling the daemon's internal remote's Addresses func which is now required as part of the log filtering to gather a list of cluster member addresses. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
roosterfish
force-pushed
the
dqlite_fixes
branch
from
September 2, 2025 08:32
12576db to
9c77dfe
Compare
markylaing
reviewed
Sep 2, 2025
Contributor
Author
|
@markylaing when you are happy with the PR please leave it open and don't merge it yet. |
Contributor
Author
|
What I have mentioned in this comment isn't a full alternative unfortunately. However we could think about using I'll add another PR for this and will merge this one now. |
This was referenced Sep 3, 2025
tomponline
added a commit
to canonical/lxd
that referenced
this pull request
Sep 4, 2025
When using only the outer context's deadline as timeout for the actual TLS dial, we ignore cases in which the outer context gets cancelled which should also cancel the TLS dial. In case the target addr is offline, the dial function will wait until the timeout (outer context's deadline) instead of returing early in case the context already got cancelled. This is critical in cases where `go-dqlite` opens up connections in parallel to all members to identify the current leader. If the leader was found, all other attempts are cancelled. A similar fix was put in place for Microcluster in canonical/microcluster#470. The additional log filtering added in Microcluster (see the PR) doesn't seem to be that critical in LXD. Whilst Microcluster uses `go-dqlite`'s `app` construct when opening up connections, LXD doesn't and instead uses the `client` package directly. This results in the `http: TLS handshake ...: EOF` errors not being triggered as permanent as in Microcluster as the connections are handled differently by `go-dqlite`. Those errors can potentially be seen, but their default log level is set to [`info`](https://github.com/canonical/lxd/blob/main/lxd/endpoints/network_util.go#L24) so they don't appear in the regular LXD daemon log by default. The already existing log filter [hooks directly](https://github.com/canonical/lxd/blob/main/lxd/endpoints/network.go#L175) into the endpoint's `http.Server`. I tried to reproduce this many times and only ever saw those `http: TLS handshake ...` errors appearing infrequently when forming the LXD cluster or when restarting the cluster members. So ultimately I don't think we need extra handling for those. In Microcluster they sometimes appeared with every heartbeat (default 10s).
roosterfish
added a commit
that referenced
this pull request
Sep 8, 2025
Follow up on #470 to also account for IPv6 addresses. I initially thought this is working for IPv4 and IPv6 but more tests have shown that in case of IPv6 we need to handle a newline and the additional brackets.
markylaing
added a commit
that referenced
this pull request
Sep 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See canonical/go-dqlite#398 for reference.
Closes canonical/go-dqlite#398.
Microcluster's custom dial function passed into
go-dqlitemight cause TLS handshake errors on the target members in case the context passed into the dial function is cancelled early on. That might be the case when a leader was already found, so all the other parallel connection attempts to the other members are cancelled.Whilst this is not a functional issue we should have extra handling to prevent seeing those error messages in the Microcluster daemon logs.
Furthermore the passed context's deadline is now used properly. Setting it whilst using the same context the deadline is coming from doesn't bring much value.
In addition a potential leak of the request's response body is fixed.