-
Notifications
You must be signed in to change notification settings - Fork 315
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
HTTPS Clone
fails with remote pointer not found
using Go transport
#836
Comments
I am facing the same issue. I wonder if this has something to do with compiling libgit2 with the openSSL and libSSH libraries. The language around these is a bit confusing, so I wonder if this is one potential problem |
Following the introduction of Go native transports, the configuration to link against these libraries was disabled in this repository: https://github.com/libgit2/git2go/blob/main/script/build-libgit2.sh#L76-L77. However, even after spending hours trying various configuration options, we have not been able to get a working version of |
I am not sure how go native transports work, but let me tinker around and see if I can get it to work. I am still relatively new to this CGO and cross-language compiling. I will keep posting my findings, lets hope something works :P |
After a bit of tinkering, I have turned on the HTTPS flag now when I try to clone, I get the error
this leads me to believe that simply linking the OpenSSL library is the issue now |
UPDATE: I got it to work, (ie, it is not throwing any more SSL errors) Currently the new bug is
EDIT: I will update my process of getting this whole thing to work once I iron out the kinks |
@hiddeco using your test, I am able to get it working with recompiling the vendored libgit2 with my setup is probably slightly different from yours since I am packaging gogit2 as a submodule, but I believe that is the minimal tweak required. also, my tests were cached, so I had to use |
yikes, this seems to be caused by the fact that libgit2 duplicates the remote while cloning, so git2go can't match the original |
But why does it work with the openssl libraries activated?
…________________________________
From: lhchavez ***@***.***>
Sent: Monday, October 11, 2021 5:28:44 PM
To: libgit2/git2go ***@***.***>
Cc: Yashodhan Ghadge ***@***.***>; Comment ***@***.***>
Subject: Re: [libgit2/git2go] HTTPS `Clone` fails with `remote pointer not found` using Go transport (#836)
yikes, this seems to be caused by the fact that libgit2 duplicates the remote while cloning, so git2go can't match the original Remote object to the one passed into the callback :/ we probably need a different way of matching the remote instead of by-pointer.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#836 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACCHNM4CNQ73LKSYNNPUK2LUGLYBZANCNFSM5E6KTZ2A>.
|
that uses a different codepath that doesn't involve the remote callbacks. |
Oh, if you have some pointers (pun intended) could you let me know? I would like to work on this if it does turn out to be a bug. |
it is indeed a bug. what should happen is that given that we have no way of recovering the original and add a test, haha. |
wow, I just tried to go through the code paths, and yes it's pretty complicated. My question is, if the pointer in the smartTransportCallback is in fact a copy, and has nothing to do with the original *git_remote, then, we can safely free it right? it won't affect libgit2's cloning process since that will be performed on the original *git_remote pointer. (I am super new to C bindings, and libgit2, so if it sounds stupid, it probably is) solution 2, is it possible to fix this pointer allocation on libgit2's side? can we submit an upstream PR that returns the same objects that are passed to the cloning process? |
we can't ^^;; that pointer is owned by libgit2 and if we free it we will cause a double free D: https://github.com/libgit2/libgit2/blob/5bd49aeeeb7a69f85249b970ac4e2c7011b7127d/src/clone.c#L407-L421 (the last line is the call to
i don't think so, that copy was introduced very intentionally: libgit2/libgit2@3c60768 |
right, so it landlocks on all sides. What code path does it take when the OpenSSL / libssh2 is compiled against libgit2? cause it seems to work just fine along that code path. Can we try to mimic what happens along that path? |
we can't: that codepath is done 100% in libgit2 D: anything that crosses the Go/C boundary needs to go through a very different codepath. |
right, if you don't mind, can you explain what code path is taken for the Go/C cloning process? I lose it the minute it goes to C.git_clone(args). Perhaps, then I can spend some time to think about this, and maybe try to make an educated guess and propose a solution. Cause I'm all out of ideas rn |
yeah, if OpenSSL / libssh2 are involved, that's pretty much it: once it goes to when this goes through Go, https://github.com/libgit2/libgit2/blob/b7bad55e4bb0a285b073ba5e02b01d3f522fc95d/src/clone.c#L348 calls Line 56 in 9b15518
Remote , and then there's some more shenanigans with the Transports. f1fa96c is a good place to look at how the transports themselves are plumbed through and then b983e1d outlines the HTTPS-only part.
|
thanks for the pointers :P. I will trace this and try to think of a solution. |
sounds good! #836 (comment) contains a partial outline of what the solution might end up looking like. |
I followed the chain, from what I can see, the callback is only triggered when it is explicitly passed. I trace it further to now just to check if my working theory is on track, these lines will trigger the go transport callbacks, right? so, let's say we run a simple clone code from golang, and don't pass any callback funcs for remoteCallbacks, it will try to use the native C default callbacks and invoke the HTTPS C transports. But if we do pass the callbacks in golang, it will trigger those instead, thus now bringing the transports control back in golang. If this working theory is incorrect please let me know. I think if I understand this right, I might get closer to a solution EDIT: I am trying to understand most of this code from libgit2 master and the version linked in the git2go submodule so there is a slight chance the code links may not perfectly match up |
it will try to use the native C default callbacks for remotes, but transports are a completely different beast that use different codepaths: If the remote object already has a transport assigned to it or the remote callbacks have a transport factory, it will use those. otherwise, it will use
control can be passed back to golang even without the remote callbacks: it can also be returned when the transports callbacks are registered.
the outline of the solution in #836 (comment) is more or less as follows:
But there's something that I didn't think of beforehand: We would also need to create a |
Right with this, I think can start work to get a pr opened. Give me a few days to get this up. I'm now quite clear on what's happening to a large degree |
update! i just realized that the |
This is good to know. Cause I'll be honest. I'm thinking I've gone in over my head, trying to create a pr. Now that there is less changes to make it is always good :P |
I have created a PR, I think this is the bare minimum change like you listed out, I will add tests for this (I need a valid GitHub url that I can add in the test, will this repo's link itself be okay?) |
@codexetreme bit of unsolicited advice - based on experience relying on real GitHub repositories for tests (and moving away from them) - I would use a mocking Git server using a project like https://github.com/sosedoff/gitkit In addition, thank you very much for taking care of this, as I was out of time 🙇 🥇 |
ah I thought so too. Just pushed a test where I was checking if github was up :P. in anycase, your(@lhchavez) solution works !!!!!! I will get the mocking server up in the test code now |
okay, so the mocking server has no HTTPS support, which means, we'll need to add it externally via some other mechanism for the test (since the point is to test HTTPS transport :P) EDIT: any suggestions/ideas? |
The library I mentioned has support for HTTP/S, but you have to create some tiny wrappers to make it work, see https://github.com/fluxcd/pkg/blob/main/gittestserver/server.go#L134 for our take :-). |
Is it possible that during this fix, a detail around the
However, our code which makes use of
|
Ah, looks like the SSH issue would be solved by switching to |
once you switch to plus, I am going to use this lib in my own personal project too, and your use case for using this lib is the similar to mine :P, any errors you face, I will face too |
The
For HTTPS, no matter what changes I am trying to make, none of the callbacks seem to work (username and password, custom CA, etc.). What we are working on is OSS by the way, and can be consulted at: fluxcd/source-controller#465 (which depends on |
nice! I'll take a look for contributing too:) for this
this I think I know why, when the code for making the fake remote is executed, data for callbacks from the original remote pointer is lost, I can possibly test my hypothesis and update |
can you link a small test in the fluxCD codebase I can use to make a quick and dirty test(with HTTPS and callbacks)? |
If you run the |
got it :) let me try to make some changes to the libgit code on my side and see if that test works with my assumed hypothesis |
I think your hypothesis is right by the way, but this should either be documented, or properly addressed. The workaround for us would be to first initialize an empty repository, then add a remote, and then call fetch, but having a working |
the way I understand the git code (at least the non C parts) is that here func smartTransportCallback(
errorMessage **C.char,
out **C.git_transport,
owner *C.git_remote,
handle unsafe.Pointer,
) C.int {
registeredSmartTransport := pointerHandles.Get(handle).(*RegisteredSmartTransport)
remote, ok := remotePointers.get(owner)
if !ok {
// create a new empty remote and set it
// as a weak pointer, so that control stays in golang
remote = createNewEmptyRemote()
remote.weak = true
}
... the issue is since the go code does not get access to the originally created object (which should be the owners arg), I am not sure how to propagate the callbacks you are setting in the code to this bit of code. |
UPDATES: @lhchavez can you please assist on how to fix this? |
If it is too hard to fix, we'll resort to #836 (comment). But I think this limitation should in this case be documented, so that other people don't fall into the same trap. |
makes sense, maybe a small note in the README, but I think this is a major point that needs to be solved cause moving forward, if go transports are to work, they have to work with these callbacks, (I am probably just missing something really small and obvious :P) let's wait for the maintainers/owners to lend their feedback till then |
Another error for the mix, I rewrote the callbacks to be more like the git2go tests in: This has made a new error surface in which the connection simply seems to get closed prematurely:
|
…sport (#836) (#842) (#846) Fixes: #836 Changes: * adding a weak bool param for Remote * create a new remote in the smartTransportCallback incase one is not found (cherry picked from commit 0e8009f) Co-authored-by: Yashodhan Ghadge <[email protected]> Co-authored-by: lhchavez <[email protected]>
…sport (#836) (#842) (#847) Fixes: #836 Changes: * adding a weak bool param for Remote * create a new remote in the smartTransportCallback incase one is not found (cherry picked from commit 0e8009f) Co-authored-by: Yashodhan Ghadge <[email protected]> Co-authored-by: lhchavez <[email protected]>
…sport (#836) (#842) (#848) Fixes: #836 Changes: * adding a weak bool param for Remote * create a new remote in the smartTransportCallback incase one is not found (cherry picked from commit 0e8009f) Co-authored-by: Yashodhan Ghadge <[email protected]> Co-authored-by: lhchavez <[email protected]>
…sport (#836) (#842) (#849) Fixes: #836 Changes: * adding a weak bool param for Remote * create a new remote in the smartTransportCallback incase one is not found (cherry picked from commit 0e8009f) Co-authored-by: Yashodhan Ghadge <[email protected]> Co-authored-by: lhchavez <[email protected]>
@hiddeco did this comment about premature closing get resolved? did you find a workaround? per: #836 (comment) We are seeing |
Given:
Clone
) initialize aRepository
before they run a testClone
seem to be using a local file pathI am not really sure if this is due to a misuse of the API, or a bug. In any case, the following used to work before not too long ago with
git2go/v31
and backing crypto / transport dependencies:While attempting to update to
git2go/v32
however, and trying to depend on less C libraries, this now returns aremote pointer not found
error:Following the breadcrumbs, it seems to end on https://github.com/libgit2/git2go/blob/main/clone.go#L43 returning
-7
(which makes me wonder even more).The text was updated successfully, but these errors were encountered: