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

Experimental managed transport for libgit2 operations #606

Merged
merged 8 commits into from
Mar 16, 2022

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Mar 9, 2022

libgit2 network operations are blocking and do not provide timeout nor context capabilities, leading to users reporting that source-controller can hang indefinitely.

By using managed transport, golang primitives such as http.Transport and net.Dial can be used to ensure timeouts are enforced.

This will initially be guarded behind a feature switch via environment variable EXPERIMENTAL_GIT_TRANSPORT.

Fixes: #402

This has been also tested against IAC and should fix fluxcd/image-automation-controller#286 and fluxcd/image-automation-controller#282. The tested were based off fluxcd/image-automation-controller#325 - thanks @darkowlzz.


This work should be followed-up by fluxcd/pkg#245, to consolidate the git implementation code and have it extracted away from the controllers.

@pjbgf
Copy link
Member Author

pjbgf commented Mar 9, 2022

So far manual tests seem to be working as expected. Tests used github for SSH and auth-less HTTPS, and Azure DevOps for HTTPS with basic auth.
image

@pjbgf pjbgf self-assigned this Mar 10, 2022
@pjbgf pjbgf force-pushed the managed-transport-libgit130 branch from ddf5af9 to dec736e Compare March 11, 2022 14:21
@pjbgf pjbgf changed the title Use managed transport for git operations Use managed transport for libgit2 operations Mar 11, 2022
@pjbgf pjbgf marked this pull request as ready for review March 11, 2022 14:30
@hiddeco
Copy link
Member

hiddeco commented Mar 11, 2022

Happy with the direction this is headed at for experimental introduction. Couple of things I would like to see:

  1. Tests in Go to at least add some coverage to the newly introduced (sub-)module.
  2. Some end-to-end test to confirm the wiring is working as expected. Preferably in the Go reconciler tests (due to speed), but this may be hard to factor in at present due to the experimental flag. In that case, I would be happy with an end-to-end test case as well (which may be cumbersome as well due to having to patch the deployment 🤷‍♂️).

On an additional note, and as already mentioned on Slack. The introduction of this opens up the discussion if we do want to leverage the same trick for go-git, which has a likewise way of setting up transports (https://github.com/go-git/go-git/blob/bf3471db54b0255ab5b159005069f37528a151b7/plumbing/transport/client/client.go#L36). This could then eventually solve more advanced authentication methods that have been requested (e.g. TLS) which I can't find the issue ref for at the moment.

@hiddeco hiddeco changed the title Use managed transport for libgit2 operations Experimental managed transport for libgit2 operations Mar 11, 2022
@hiddeco hiddeco added enhancement New feature or request area/git Git related issues and pull requests labels Mar 11, 2022
@pjbgf pjbgf force-pushed the managed-transport-libgit130 branch 4 times, most recently from 82d1638 to 229b116 Compare March 14, 2022 16:02
@hiddeco hiddeco self-requested a review March 14, 2022 16:22
@pjbgf pjbgf force-pushed the managed-transport-libgit130 branch from 229b116 to 31e28bf Compare March 14, 2022 16:25
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.

This looks good to me for experimental introduction, thanks a bunch @pjbgf 🥇 💯

@hiddeco hiddeco requested a review from darkowlzz March 15, 2022 10:06
@pjbgf pjbgf force-pushed the managed-transport-libgit130 branch 2 times, most recently from bf6cb1c to e98bd36 Compare March 15, 2022 14:43
Paulo Gomes and others added 8 commits March 16, 2022 16:22
libgit2 network operations are blocking and do not provide timeout nor context capabilities,
leading for several reports by users of the controllers hanging indefinitely.

By using managed transport, golang primitives such as http.Transport and net.Dial can be used
to ensure timeouts are enforced.

Co-Authored-by: Sunny <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
The initial implementation was based off upstream, which cause
an initial request to fail, and only then the credentials would
be added into the request.

Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
For backwards compatibility, support for HTTP redirection is enabled when targeting
the same host, and no TLS downgrade took place.

Signed-off-by: Paulo Gomes <[email protected]>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!
Very interesting work on securing the redirects. 🥇

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 enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Image Automation controller silently stops working Azure DevOps: Source controller getting stuck
3 participants