rancher / dynamiclistener

Apache License 2.0
17 stars 61 forks source link

Marking additional connections as ready #66

Closed MbolotSuse closed 2 years ago

MbolotSuse commented 2 years ago

Most connections were not marked as ready despite having retrieved a valid cert. This change makes all connections which successfully retrieved a cert get marked as ready

Related to rancher/rancher#39094

MbolotSuse commented 2 years ago

Still working on some tests, but would appreciate any comments on the approach/functionality as I put together some tests for getCertificate

MbolotSuse commented 2 years ago

Overview

This pr is related to rancher/rancher#39094. Basically, not every connection which got a valid cert was marked as ready when a valid cert was retrieved for it. This lead to issues where the UI maintained some connections with an old cert event though a newer cert was being used for new connections. Since the old cert was still being used, this prevented user's Browsers from recognizing that it needed to re-prompt the user to accept the new self-signed cert, leading to constant websocket disconnect/reconnect attempts. For more information on this issue, see this comment.

This PR aims to fix this by marking any connection which we retrieved a valid cert for as ready. Since loadCert is only called with an active connection in one place, we only need to perform this check in one side function (getCertificate). We also only attempt to mark a connection as ready if we have an active connection (to avoid nil pointer exceptions) and if l.conns has been initializing (indicating that consuming function requested that we close connections on cert change - see here for more).

Changes

MbolotSuse commented 2 years ago

Got some tests added, and also pushed a commit which bumps the go version for this to 1.19.