Skip to content

Conversation

@MbolotSuse
Copy link
Contributor

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
Copy link
Contributor Author

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

@MbolotSuse
Copy link
Contributor Author

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

  • Added a check to getCertificate which marks a connection as ready if:
    • We successfully retrieved a cert for this connection (cert is not nil and err is nil)
    • This listener was configured to close connections on cert change (l.conns is not nil)
    • We have a current connection
  • Removes the logic in loadCertificate which marks a connection which forced a cert change as ready. This is not done by getCertificate, and would be redundant in the new structure.

@MbolotSuse
Copy link
Contributor Author

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

Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

Nice job on the tests 👍

Most connections were not marked as ready despite having retrieved
a valid cert. This change makes all connections which succesfully
retrieved a cert get marked as ready
@brandond brandond merged commit 7001abf into rancher:master Oct 19, 2022
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.

4 participants