Remove dependency on libgit2 credentials callback #727
Conversation
aryan9600
force-pushed
the
improve-managed
branch
2 times, most recently
from
4858774
to
bba2317
Compare
aryan9600
force-pushed
the
improve-managed
branch
2 times, most recently
from
5945880
to
18b1802
Compare
aryan9600
force-pushed
the
improve-managed
branch
from
18b1802
to
116aca7
Compare
aryan9600
force-pushed
the
improve-managed
branch
from
3855434
to
1a32e3c
Compare
aryan9600
force-pushed
the
improve-managed
branch
from
1a32e3c
to
e3790c3
Compare
There was a problem hiding this comment.
Awesome work @aryan9600! Thank you for working on this. 🙇 🙇
LGTM
Injects transport and auth options at the transport level directly to bypass the inbuilt credentials callback because of it's several shortcomings. Moves some of the pre-existing logic from the reconciler to the checkout implementation. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com> Co-authored-by: Paulo Gomes <paulo.gomes@weave.works>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
aryan9600
force-pushed
the
improve-managed
branch
from
e3790c3
to
972d1ca
Compare
|
This has been tested against several SaaS providers, using a wide range of authentication algorithms. The average reconciliation time (sub second) has improved a lot compared to the previous released source controller version.
|
There was a problem hiding this comment.
LGTM
Thanks @aryan9600 and @pjbgf for these awesome improvements!
| // When we get rid of unmanaged transports, we can get rid of this branching as well. | ||
| if managed.Enabled() { | ||
| if opts.TransportOptionsURL == "" { | ||
| return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.") |
There was a problem hiding this comment.
This same error is used multiple times. It'd be good to have an error variable for it to prevent any mistakes in the string and easily change the message.
And it could just be errors.New(), no formatting required.
| // The branching lets us establish a clear code path to help us be certain of the expected behaviour. | ||
| // When we get rid of unmanaged transports, we can get rid of this branching as well. | ||
| if managed.Enabled() { | ||
| if opts.TransportOptionsURL == "" { |
There was a problem hiding this comment.
In CheckoutBranch, a nil check is performed for this option. That's missing here.
| "") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to fetch remote '%s': %w", | ||
| managed.EffectiveURL(url), gitutil.LibGit2Error(err)) |
There was a problem hiding this comment.
Since the passed in URL is now the original URL, we don't need to use EffectiveURL() here. If the url variable is not overwritten above, we still have the original URL.
There was a problem hiding this comment.
The passed in URL is original but it's always overwritten here: https://github.com/fluxcd/source-controller/blob/main/pkg/git/libgit2/checkout.go#L96
| ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto}, | ||
| }) | ||
| url = opts.TransportOptionsURL | ||
| remoteCallBacks = managed.RemoteCallbacks() |
There was a problem hiding this comment.
The above code is repeated everywhere. We can have a helper function to do the same, reducing the risk of making any mistakes in any of the checkouts.
| g.Expect(err).ToNot(HaveOccurred()) | ||
| g.Expect(cc.String()).To(Equal(git.DefaultBranch + "/" + commit.Id().String())) | ||
| g.Expect(git.IsConcreteCommit(*cc)).To(Equal(true)) | ||
| } |
There was a problem hiding this comment.
Unlike in TestCheckoutBranch_checkoutUnmanaged(), looks like we are no longer testing various scenarios of checkout for managed transport anymore like non default branch, and non existing branch checkout.
| name string | ||
| branch string | ||
| filesCreated map[string]string | ||
| lastRevision string |
There was a problem hiding this comment.
filesCreated is not being used any more.
lastRevision doesn't apply to this test as unmanaged transport doesn't support no-op clone.
| ) | ||
|
|
||
| const testRepositoryPath = "../testdata/git/repo" | ||
|
|
||
| func TestMain(m *testing.M) { | ||
| managed.InitManagedTransport(logr.Discard()) | ||
| } |
There was a problem hiding this comment.
TestMain() not calling m.Run() results in none of the tests in this whole package to run.
| // It's a field of AuthOptions despite not providing any kind of authentication | ||
| // info, as it's the only way to sneak it into git.Checkout, without polluting | ||
| // it's args and keeping it generic. | ||
| TransportOptionsURL string |
There was a problem hiding this comment.
This leaks implementation details into the generic options and should be configured via other means.
There was a problem hiding this comment.
Added this to the issue tracking the consolidation of the git implementation.
This PR removes our dependency on the inbuilt libgit2 credentials callback used in managed transport. This helps us:
It refactors the pre-existing logic, such that we don't call
Checkoutwith the fake url, as it really degrades code readability (especially for newcomers). Instead, a new field is added toAuthOptions, which is then is used by the transport action to uniquely identify the correct target URL and the correct credentials, letting us move the logic from the reconciler to the libgit2 package (where it rightfully belongs).It separates the managed transports and unmanaged transports code, to make it easier for us in the future to remove the unmanaged transport code. It also enables
OptimizedGitClones, whenGitManagedTransportis enabled.It fixes proxy handling for the managed http transport and expands the tests to cover the same.
Signed-off-by: Sanskar Jaiswal jaiswalsanskar078@gmail.com