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

libgit2: Disable connection caching #713

Merged
merged 1 commit into from
May 13, 2022

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented May 6, 2022

Connection caching was a feature created to resolve upstream issues raised from concurrent ssh connections.
Some scenarios were based on multiple key exchange operations happening at the same time.

This PR removes the connection caching, and instead:

  • Services Session.StdoutPipe() as soon as possible, as it is a known source of blocking SSH connections.
  • Reuse SSH connection within the same subtransport, eliminating the need for new handshakes when talking
    with the same server.
  • Simplifies the entire transport logic for better maintainability.

This also fixes the panic below when connecting to AWS CodeCommit with Managed Transport.
Note that this logic is not captured by the panic recovery due to its execution path not starting from the Checkout func.
Additional changes are being made to completely remove the dependency to SmartCertificateCheck, which will ensure this does not happen at all going forwards. But that will be dealt with in subsequent PRs.

panic: invalid pointer handle: 0x7fd35c005e80

goroutine 470 [running]:
github.com/libgit2/git2go/v33.(*HandleList).Get(0xab1c5ed5923f82a4?, 0x7fd35c005e80)
        /home/levi/go/pkg/mod/github.com/libgit2/git2go/v33@v33.0.9/handles.go:64 +0x12e
github.com/libgit2/git2go/v33.certificateCheckCallback(0x240ca1cc0fc19dc6?, 0xc000a17080, 0x1, 0x4a7484aa2de92c6f?, 0x76f988da5cb0a9dc?)
        /home/levi/go/pkg/mod/github.com/libgit2/git2go/v33@v33.0.9/remote.go:445 +0x3d
github.com/libgit2/git2go/v33._Cfunc_git_transport_smart_certificate_check(0x7fd35c0085f0, 0xc000a17080, 0x1, 0x7fd3a0001050)
        _cgo_gotypes.go:8790 +0x4c
github.com/libgit2/git2go/v33.(*Transport).SmartCertificateCheck.func9(0x7fd3a0001050?, 0x29?, 0x90?, 0x86062dda3eacc8fb?)
        /home/levi/go/pkg/mod/github.com/libgit2/git2go/v33@v33.0.9/transport.go:170 +0x67
github.com/libgit2/git2go/v33.(*Transport).SmartCertificateCheck(0xe28033dd618b378?, 0xc000efdcb0, 0x1, {0xc000e4df50, 0x29})
        /home/levi/go/pkg/mod/github.com/libgit2/git2go/v33@v33.0.9/transport.go:170 +0x325
github.com/fluxcd/source-controller/pkg/git/libgit2/managed.(*sshSmartSubtransport).Action.func1({0xc000e4df50, 0x29}, {0x306c073?, 0x7?}, {0x33cd170, 0xc000d91380})
        /home/levi/go/src/github.com/fluxcd/source-controller/pkg/git/libgit2/managed/ssh.go:208 +0x1bc
golang.org/x/crypto/ssh.(*handshakeTransport).client(0xc000b806e0, {0x33c54f8, 0xc000479960}, 0x1b?)
        /home/levi/go/pkg/mod/golang.org/x/crypto@v0.0.0-20220427172511-eb4f295cb31f/ssh/handshake.go:698 +0x113
golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange(0xc000b806e0, {0xc000d76fc0, 0x101, 0x101})
        /home/levi/go/pkg/mod/golang.org/x/crypto@v0.0.0-20220427172511-eb4f295cb31f/ssh/handshake.go:611 +0x3f4
golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop(0xc000b806e0)
        /home/levi/go/pkg/mod/golang.org/x/crypto@v0.0.0-20220427172511-eb4f295cb31f/ssh/handshake.go:301 +0xa6
created by golang.org/x/crypto/ssh.newClientTransport
        /home/levi/go/pkg/mod/golang.org/x/crypto@v0.0.0-20220427172511-eb4f295cb31f/ssh/handshake.go:135 +0x236

@pjbgf pjbgf added the area/git Git related issues and pull requests label May 6, 2022
@pjbgf pjbgf added this to the GA milestone May 6, 2022
@pjbgf
Copy link
Member Author

pjbgf commented May 6, 2022

Test images based on version 830771f:

  • source-controller: quay.io/paulinhu/source-controller:v0.24.4-cacheless@sha256:61930cad1da900f209b396f20c2f7740ff32b5cf1bb4ab7892200790c00a5f4b
  • image-automation-controller: quay.io/paulinhu/image-automation-controller:v0.22.2-cacheless@sha256:87823667cfc4c6e395d996ceaee92a1b5059a8950884f2c3aa49488dcbed81f5

UPDATE: changed IAC image to v0.22.2-cacheless

@pjbgf
Copy link
Member Author

pjbgf commented May 12, 2022

New image for source controller based on 6d51758:
ghcr.io/fluxcd/source-controller:rc-6d517589

UPDATE: replace image with official release candidate version.

@pjbgf
Copy link
Member Author

pjbgf commented May 13, 2022

Removed "enable managed transport" commit from this PR, we shall treat it as a separate PR.

go.mod Outdated Show resolved Hide resolved
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Besides the nitpick, this looks good to me. Thanks @pjbgf 🙇

Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

just one nitpick, otherwise it LGTM! 🚀

pkg/git/libgit2/managed/ssh.go Outdated Show resolved Hide resolved
Connection caching was a feature created to resolve
upstream issues raised from concurrent ssh connections.
Some scenarios were based on multiple key exchange
operations happening at the same time.

This PR removes the connection caching, and instead:
- Services Session.StdoutPipe() as soon as possible,
  as it is a known source of blocking SSH connections.
- Reuse SSH connection within the same subtransport,
  eliminating the need for new handshakes when talking
  with the same server.
- Simplifies the entire transport logic for better
  maintainability.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
@pjbgf pjbgf merged commit e180b3c into fluxcd:main May 13, 2022
@pjbgf pjbgf deleted the libgit2-cacheless-conns branch May 13, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants