libgit2: Configured libgit2 clone ProxyOptions #524

racdev · 2021-12-15T22:11:33Z

This configures ProxyOptions for all libgit2 Checkout functions when cloning and configures the options based on current environment settings using the http.ProxyFromEnvironment function.

Refs: #131

I haven't added any tests as they would only really be testing the http.ProxyFromEnvironment function.
However when I tried to create some tests, it appeared as though the go test caching meant the proxy environment values set for each test were being cached and hence the tests always failed. I don't have a huge amount of experience developing in go so that might just be an issue on my part - happy to be told/shown how I could get such tests to work.

hiddeco · 2021-12-16T11:57:47Z

Thanks a lot for your contribution 💯 🙇

Just a heads up that due to most maintainers starting their holidays next week, and libgit2 being a critical and complex area of the controller. I am (sadly) afraid it's highly likely this won't be (properly) looked at before the beginning of the new year.

For the tests, it may be worth trying to run them using -count 1 added to go test .., which will invalidate the cache on each run.

racdev · 2021-12-16T16:19:07Z

No worries.

I did try adding -count 1, but it still didn't work. I think the issue is that the underlying library looks up and caches the Environment variable values once, so once the tests run the values are set, so I can't set them differently to test different scenarios. The library itself has a private reset mechanism that it uses in its own tests as far as I can tell: https://github.com/golang/go/blob/master/src/net/http/transport.go#L825

au2001 · 2021-12-17T16:59:11Z

Hi,

libgit2 has a built-in option which tries to detect the proxy from the Git configuration and then from environment variables.
The second part is not explicitly stated in the documentation but you can see the relevant code here and here.

I have made an example of implementation here: https://github.com/au2001/fluxcd-source-controller/pull/1/files
I tested that change on a real-world cluster and can confirm it works as expected.
This change makes use of libgit2's feature so it does not require any additional import and very little code.
@hiddeco Maybe this would allow a merge sooner than you mentioned?

With this PR, the behavior of libgit2 would be closer to the one of the go-git implementation and would help fix #131 which has been open for over a year, as well as fluxcd/image-automation-controller#279 potentially.

hiddeco · 2021-12-17T19:49:29Z

Given this is my last hour of availability for this year, I won't be able to help this move any further I am afraid. However, if tested and proved to work, it could be moved forward by others (cc: @darkowlzz).

I would still like to see at least a simple test added, as we have been working really hard on getting the test coverage up to an acceptable rate.

std4453 · 2021-12-18T14:32:57Z

Hi,

libgit2 has a built-in option which tries to detect the proxy from the Git configuration and then from environment variables. The second part is not explicitly stated in the documentation but you can see the relevant code here and here.

I have made an example of implementation here: https://github.com/au2001/fluxcd-source-controller/pull/1/files I tested that change on a real-world cluster and can confirm it works as expected. This change makes use of libgit2's feature so it does not require any additional import and very little code. @hiddeco Maybe this would allow a merge sooner than you mentioned?

With this PR, the behavior of libgit2 would be closer to the one of the go-git implementation and would help fix #131 which has been open for over a year, as well as fluxcd/image-automation-controller#279 potentially.

@au2001 What about opening a PR on on source-controller?

racdev · 2021-12-20T16:35:09Z

I've also confirmed @au2001 code works in our clusters. Happy to update this PR to work using the Auto option if @au2001 doesn't want to raise his own? I'm a bit stuck on how to test it though so any hints would be appreciated (I can only think of actually cloning a real remote repo via a running proxy and testing somehow that the proxy is used based on env values)

Admittedly I did notice the Auto proxy type, but (mistakenly) assumed that was the default - that will teach me.

au2001 · 2021-12-20T16:59:28Z

@au2001 What about opening a PR on on source-controller?

I've also confirmed @au2001 code works in our clusters. Happy to update this PR to work using the Auto option if @au2001 doesn't want to raise his own? I'm a bit stuck on how to test it though so any hints would be appreciated (I can only think of actually cloning a real remote repo via a running proxy and testing somehow that the proxy is used based on env values)

Admittedly I did notice the Auto proxy type, but (mistakenly) assumed that was the default - that will teach me.

I can raise a PR but as @hiddeco stated, at least a test (and maybe some documentation?) will have to be written.
If you're willing to do that @racdev, I hereby grant all rights on the code written in arl-sh#1 for use in this PR and in this repository.

As for tests, most that I can think of would just be testing libgit2 and not source-controller itself so I'm not sure.
You could start a proxy (e.g. https://github.com/elazarl/goproxy), then clone a repository by setting the environment variables to that proxy's URL and finally make sure all expected HTTP requests have gone through the proxy and not directly to the Git server.

darkowlzz · 2022-01-04T15:32:01Z

Hi, thanks for the patch and all the helpful information around this. I've been looking into this and thinking about ways to test it. I will be trying it out myself in a while.
pkg/git/strategy/strategy_test.go contains tests for both the git implementations. A test for this could go there for both go-git and libgit2. The other tests in there create a test git server and perform actions against it. We need such tests to ensure that things are functional and we find out when they stop working due to some change. It is testing the git implementations themselves in some ways, but it's helpful to test them with the flux specific wrappers, to ensure they actually work with how we configure them.

As recommended above, using https://github.com/elazarl/goproxy or something simpler, we should be able to create a simple proxy that adds things that are necessary for a successful checkout, like the credentials. We can create a test git server with basic auth (examples are in the existing tests) and not provide the credentials to the checkout strategy, but let the proxy add the credentials to the outgoing request. Test cases could be to try checkout directly without credentials, which would fail, and then try checkout using the proxy and that should succeed.
I'm going to find out more about related things and post here if I find any other useful information.

@racdev you can update the PR with @au2001 's recommendations. And thank you both for all the help in flux and git2go.

darkowlzz · 2022-01-05T07:17:28Z

Got some updates.

I tried the proxy option in the test code in strategy_test.go, starting with just http git server and found out that it doesn't work. Libgit2 just ignores the proxy. Found out that it only works for https git server as per libgit2/libgit2#5650 . I guess we'll have to document this properly.

Continuing the testing for https git server, I created a local proxy using goproxy. The basic default usage of goproxy provided in the examples work only for http requests. Enabling AlwaysMitm results in certificate verification failure. More info in elazarl/goproxy#426 (comment). I concluded that we can read the requests with a custom handler but can't modify them when it's https. Considering this, I created a custom goproxy.HttpsHandler to check if libgit2 sends request through the proxy by checking the request host, comparing with the git server address, and user-agent.

Following is a code snippet from my testing. It creates a goproxy server after creating the git server and sets HTTPS_PROXY env vars for libgit2 to use. The https handler toggles a boolean to convey that it received the expected request from libgit2, which is checked at the end of the test.

// Create git server.
...

proxyGotRequest := false

// Run a proxy server.
proxy := goproxy.NewProxyHttpServer()
proxy.Verbose = true

var httpsHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) {
    // Check if the host matches with the git server address and the user-agent is the expected git client.
    userAgent := ctx.Req.Header.Get("User-Agent")
    if strings.Contains(gitServer.HTTPAddress(), host) && strings.Contains(userAgent, "libgit2") {
        proxyGotRequest = true
    }
    return goproxy.OkConnect, host
}
proxy.OnRequest().HandleConnect(httpsHandler)

proxyServer := http.Server{
    Addr:    "localhost:9999",
    Handler: proxy,
}
// Run the proxy server in a separate goroutine.
go func() {
    proxyServer.ListenAndServe()
}()
defer proxyServer.Close()

// Set the proxy env var.
os.Setenv("HTTPS_PROXY", fmt.Sprintf("http://%s", proxyServer.Addr))
defer func() {
    os.Unsetenv("HTTPS_PROXY")
}()

...

// At the end of the test, check if the proxy received the expected request.
g.Expect(proxyGotRequest).To(BeTrue())

This succeeds with some warnings:

2022/01/05 12:33:53 [001] INFO: Running 1 CONNECT handlers
2022/01/05 12:33:53 [001] INFO: on 0th handler: &{0 <nil> 0xda4c80} 127.0.0.1:34279
2022/01/05 12:33:53 [001] INFO: Accepting CONNECT to 127.0.0.1:34279
2022/01/05 12:33:53 request: GET 127.0.0.1:34279/bar/test-reponame/info/refs?service=git-upload-pack
2022/01/05 12:33:53 request: GET 127.0.0.1:34279/bar/test-reponame/info/refs?service=git-upload-pack
2022/01/05 12:33:53 request: POST 127.0.0.1:34279/bar/test-reponame/git-upload-pack
2022/01/05 12:33:53 [001] WARN: Error copying to client: readfrom tcp 127.0.0.1:51448->127.0.0.1:34279: splice: connection reset by peer
2022/01/05 12:33:53 [001] WARN: Error copying to client: readfrom tcp 127.0.0.1:9999->127.0.0.1:46302: splice: broken pipe

And fails if HTTPS_PROXY is set to some random address where proxy server isn't running:

        Unexpected error:
            <*fmt.wrapError | 0xc000214280>: {
                msg: "unable to clone: failed to connect to localhost: Connection refused",
                err: <*git.GitError | 0xc000214260>{
                    Message: "failed to connect to localhost: Connection refused",
                    Class: 2,
                    Code: -1,
                },
            }
            unable to clone: failed to connect to localhost: Connection refused
        occurred

Tried the same for go-git, but it doesn't work. I saw that go-git has some support for proxy using SOCKS5 for ssh. Maybe we need to document about it as well.

This may be a little more involved than expected. Please let me know if I can help more with the tests.

darkowlzz · 2022-01-05T07:22:00Z

With this PR, the behavior of libgit2 would be closer to the one of the go-git implementation and would help fix #131 which has been open for over a year, as well as fluxcd/image-automation-controller#279 potentially.

@au2001 Have you been able to use HTTPS_PROXY with go-git? If yes, how did you configure it? I'll have to go through go-git more to investigate further.

std4453 · 2022-01-05T07:24:49Z

@darkowlzz With `source-controller` and `go-git`, I can simply set `HTTPS_PROXY` on the pod and see it work.

darkowlzz · 2022-01-05T07:28:49Z

With source-controller and go-git, I can simply set HTTPS_PROXY on the pod and see it work.

I'm unable to reproduce it manually for some reason. The proxy doesn't receive any request.
go-git clones the repo directly.

au2001 · 2022-01-05T07:58:33Z

@au2001 Have you been able to use HTTPS_PROXY with go-git? If yes, how did you configure it? I'll have to go through go-git more to investigate further.

I have not yet tested go-git with an HTTPS proxy, my specific use case uses an unauthenticated HTTP proxy on port 443
I can test using an HTTPS proxy with and without authentication in a few hours on both go-git and libgit2 with the patched versions of source-controller and image-automation-controller

Did you try to use the lowercase version of the variables, https_proxy and make sure to set no_proxy to an empty value?

Edit: Sorry, my mistake. I did use HTTP_PROXY and HTTPS_PROXY with go-git and only tried HTTPS_PROXY with libgit2. As you stated, plain HTTP proxies with libgit2 are not supported with the proposed patch.
Here is the exact configuration I am using (see the whole patch):

HTTPS_PROXY=http://1.2.3.4:443
HTTP_PROXY=http://1.2.3.4:443
HTTPS_PROXY=http://1.2.3.4:443
FTP_PROXY=http://1.2.3.4:443
NO_PROXY=localhost,.local,.local.,127.0.0.0/8,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16
https_proxy=http://1.2.3.4:443
http_proxy=http://1.2.3.4:443
https_proxy=http://1.2.3.4:443
ftp_proxy=http://1.2.3.4:443
no_proxy=localhost,.local,.local.,127.0.0.0/8,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16

racdev · 2022-01-05T09:53:22Z

@darkowlzz is the git server running on localhost, because if so then the default ProxyFromEnvironment being used by the http Client in go-git will ignore it as it is a special case that localhost is ignored.

https://github.com/golang/go/blob/master/src/net/http/transport.go#L437

Also the proxy determination is cached (which is the issue I was having when trying to test the helper function in this PR) - don't know if that is an issue for your tests or not

darkowlzz · 2022-01-05T11:04:28Z

@darkowlzz is the git server running on localhost, because if so then the default ProxyFromEnvironment being used by the http Client in go-git will ignore it as it is a special case that localhost is ignored.

@racdev Yes, it was on localhost. Tried a few things:

Updated /etc/hosts to have a different name. It worked for libgit2 as before, but not for go-git.
Ran the same goproxy server on a different physical machine in the same network. Used the IP of the machine, it worked for libgit2 but not for go-git.

I'm not sure what's going on. Need to investigate further.

Just to isolate and verify the go http transport ignoring localhost proxy thought, I wrote a small program to verify the behavior:

package main

import (
    "fmt"
    "net/http"
    "os"
)

func main() {
    os.Setenv("HTTP_PROXY", "http://localhost:8899")
    resp, err := http.Get("http://example.com")
    if err != nil {
        panic(err)
    }
    fmt.Println(resp)
}

It seems to be going through the proxy. I see the logs:

2022/01/05 16:23:46 [004] INFO: Got request / example.com GET http://example.com/
2022/01/05 16:23:46 [004] INFO: Sending request GET http://example.com/
2022/01/05 16:23:46 [004] INFO: Received response 200 OK
2022/01/05 16:23:46 [004] INFO: Copying response to client 200 OK [200]
2022/01/05 16:23:46 [004] INFO: Copied 1256 bytes to client error=<nil>

Changing to HTTPS_PROXY and https://example.com also works.
Noticed that setting HTTP_PROXY and https://example.com doesn't go through the proxy.

Also the proxy determination is cached (which is the issue I was having when trying to test the helper function in this PR) - don't know if that is an issue for your tests or not

I haven't noticed any caching issue yet.
You can try it for yourself. Here's the proxy code:

package main

import (
    "log"
    "net/http"

    "github.com/elazarl/goproxy"
)

func main() {
    proxy := goproxy.NewProxyHttpServer()
    proxy.Verbose = true
    log.Fatal(http.ListenAndServe(":8899", proxy))
}

You can run the simple client code agains this proxy and see if you observe the same behavior.

Since this is working, I'm suspicious about something specific to go-git that's ignoring the proxy settings.

darkowlzz · 2022-01-05T11:15:18Z

Did you try to use the lowercase version of the variables, https_proxy and make sure to set no_proxy to an empty value?

@au2001 I just tried https_proxy and it works. Setting no_proxy to empty didn't make much of a difference, proxy works with libgit2 but not work go-git.

Will investigate more into what's going on with go-git and my setup.

racdev · 2022-01-05T11:23:52Z

@darkowlzz The localhost ignore is based on the request URL, not the proxy URL, so your code will work as your request isn't to localhost (the proxy running on localhost is fine). Your comment reads as though you changed the proxy to not be on localhost, is that correct?

If the git server/repo URL is localhost in the tests then the proxy will be ignored as I understand it.

darkowlzz · 2022-01-05T12:00:01Z

@darkowlzz The localhost ignore is based on the request URL, not the proxy URL, so your code will work as your request isn't to localhost (the proxy running on localhost is fine). Your comment reads as though you changed the proxy to not be on localhost, is that correct?

@racdev Sorry, my bad. I'm able to reproduce it now. Thanks a lot 😅

In this case, I think we'll have to clone a public repo. I was testing with a small repo that go-git uses for it's own testing: https://github.com/git-fixtures/basic.git.
I need to look up more, I think there's a way to not bind to 127.0.0.1 but something else like 127.5.0.1, which won't be ignored. I'll report back if I find out more about what I'm referring to.

So, with this, we know that go-git works, libgit2 works for https only.

racdev · 2022-01-05T12:05:40Z

Yeah I had come to the conclusion that a public repo clone would be required to test. I will see if I can get some time this evening to update this PR with something.

au2001 · 2022-01-05T13:06:19Z

In this case, I think we'll have to clone a public repo. I was testing with a small repo that go-git uses for it's own testing: https://github.com/git-fixtures/basic.git.
I need to look up more, I think there's a way to not bind to 127.0.0.1 but something else like 127.5.0.1, which won't be ignored. I'll report back if I find out more about what I'm referring to.

@darkowlzz Since we already have a proxy running, why not try to clone a repository at example.com and make the proxy connect to localhost instead? That way, the local Git server is used, but go-git and libgit2 see example.com as URL.

Also, from what I can see, the SSL certificate used for HTTPS cloning already contains example.com and www.example.com as valid hosts.

darkowlzz · 2022-01-05T14:53:35Z

@darkowlzz Since we already have a proxy running, why not try to clone a repository at example.com and make the proxy connect to localhost instead? That way, the local Git server is used, but go-git and libgit2 see example.com as URL.

Also, from what I can see, the SSL certificate used for HTTPS cloning already contains example.com and www.example.com as valid hosts.

@au2001 That would be great if it works. In my experiments with goproxy earlier today, I found that when it's https, the request can't be modified. I tried adding some headers but on the server side, the received requests don't contain the headers added by the proxy. But it works for http requests. Please share more information if you know how to achieve it.

Regarding the idea of binding to something like 127.5.0.1, looks like go ignores that as well. So, I'm fine with a public repo for just this proxy test. But if anyone can find a workaround and make it work with a local repo, that'd be much better, less dependency on public infrastructure.

au2001 · 2022-01-05T16:53:07Z

@au2001 That would be great if it works. In my experiments with goproxy earlier today, I found that when it's https, the request can't be modified. I tried adding some headers but on the server side, the received requests don't contain the headers added by the proxy. But it works for http requests. Please share more information if you know how to achieve it.

@darkowlzz Indeed goproxy does not let you modify the content of the request without setting up MITM.
But you can choose where the request is sent (which server the proxy connects to), which is all we need.
The second return value in return goproxy.OkConnect, host indicates to connect to the same host the user typed.
Simply replacing that with localhost:xxxx (where xxxx is the Git server's port) will effectively redirect example.com to localhost.

I tried to use your test code and adapt it, but I can't seem to make go-git use the proxy, no matter what URL I use. It works fine with libgit2 though.
I will look into it a bit but I'm sure you can get it to work before me given that I have almost no experience in Go.

Here is a PoC redirecting github.com to a.github.io (which serves a certificate valid on github.com):

package main

import (
	"fmt"
	"io/ioutil"
	"net/http"
	"os"
	"time"

	"github.com/elazarl/goproxy"
)

func main() {
	proxy := goproxy.NewProxyHttpServer()
	proxy.Verbose = true

	var httpsHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) {
		if host == "github.com:443" { // The port is always added, even if the user doesn't specify it.
			return goproxy.OkConnect, "a.github.io:443"
		}
		return goproxy.RejectConnect, host // Prevent all connections except to github.com - just to be safe.
	}
	proxy.OnRequest().HandleConnect(httpsHandler)

	proxyServer := http.Server{
		Addr:    "localhost:9999",
		Handler: proxy,
	}

	go proxyServer.ListenAndServe()
	defer proxyServer.Close()

	os.Setenv("HTTPS_PROXY", fmt.Sprintf("http://%s", proxyServer.Addr))
	defer os.Unsetenv("HTTPS_PROXY")

	time.Sleep(1 * time.Second) // Wait for the proxy server to start.

	res, err := http.Get("https://github.com")
	if err != nil {
		panic(err)
	}

	defer res.Body.Close()
	body, err := ioutil.ReadAll(res.Body)
	if err != nil {
		panic(err)
	}
	fmt.Println(string(body)) // Displays the GitHub.io 404 page and not github.com's home page.
}

darkowlzz · 2022-01-06T07:12:55Z

Thanks for trying that @au2001 . I too managed to make it work for libgit2 based on your method but the proxy seems to be ignored with go-git even for example.com. Tried a few things, but couldn't get it to work.

racdev · 2022-01-06T10:11:59Z

I also couldn't get go-git to work (still think it is something to do with the ProxyFromEnvironment caching), but libgit2 was ok.

NO_PROXY didn't work for libgit2 though, although I think that is because the version of libgit2 in use doesn't have this update in it for NO_PROXY libgit2/libgit2#6026 ?

If we cant get go-git proxy tests to work, do we just go with adding tests for the libgit2 implementation (which is what this PR specifically targets)? Or do we want to keep trying to get go-git working too?

au2001 · 2022-01-06T10:36:37Z

I also couldn't get go-git to work (still think it is something to do with the ProxyFromEnvironment caching), but libgit2 was ok.

Caching could definitely be an issue later on, but I believe it is not the (only) cause of the issue here.
In my tests, I tried exclusively running a single HTTPS clone with go-git on a freshly started VM with the HTTPS_PROXY environment variable manually added at startup (before even running the test).
go-git still did not follow that environment variable and did not go through the proxy.

It works fine with go-git through HTTP and libgit2 through HTTPS.

NO_PROXY didn't work for libgit2 though, although I think that is because the version of libgit2 in use doesn't have this update in it for NO_PROXY libgit2/libgit2#6026 ?

I was able to bypass the proxy using NO_PROXY. You can see it is already implemented in libgit2 here.
Which value are you using?

A test for NO_PROXY on each implementation would also probably be interesting.

racdev · 2022-01-06T11:24:37Z

I was able to bypass the proxy using NO_PROXY. You can see it is already implemented in libgit2 here.
Which value are you using?

I don't think that code is in the version of libgit2 that the controller is using. I am perhaps (more than likely) completely wrong, but as far as I can see the makefile uses libgit2 version 1.1.1 which would be this, that doesn't have anything in there for NO_PROXY: https://github.com/libgit2/libgit2/blob/v1.1.1/src/remote.c#L789

Same story if you follow the version of git2go v31.6.1 and its libgit2 version: https://github.com/libgit2/git2go/tree/v31.6.1/vendor

A test for NO_PROXY on each implementation would also probably be interesting.

Yeah, I did want to add a NO_PROXY test, I was trying to simply use example.com for the no proxy value and testing to make sure the request didn't go through the proxy when set, but it always did in my tests (for libgit2).

darkowlzz · 2022-01-06T11:38:05Z

Looks right to me, libgit2/libgit2@e5a3277 this change is only present in v1.2.0 and v1.3.0, and we are using v1.1.1 due to some issues in the newer versions of git2go.

If we cant get go-git proxy tests to work, do we just go with adding tests for the libgit2 implementation (which is what this PR specifically targets)? Or do we want to keep trying to get go-git working too?

@racdev We have seen that it all works fine with remote/public repos. Due to the situation, let's use a public repo for just proxy tests. In the future, if we figure out how to make it work locally, we can change it.

au2001 · 2022-01-07T12:16:53Z

Instead of adding a sleep, I would recommend we can start a listener synchronously and run only the serve in a goroutine, passing it the listener:

@darkowlzz Yes, that's a way better solution. Thanks.

So for 1) I'll change the URL to be HTTPS (as my original test request was - I should have spotted the need to change the proxy setup with the change of protocol) as I think the majority of use cases will be to an HTTPS endpoint and libgit2 doesnt support HTTP proxying so wouldnt be valid for a NO_PROXY test of that (once supported), so using HTTPS will be consistent once libgit2 can be tested too.

@racdev Agreed, that makes sense.

I will also add a shorter checkout timeout for the NO_PROXY test as otherwise we have to wait the default 30s for the checkout to fail, unless there's something obviously wrong with that logic?

@racdev Seems fine to me. I actually was using a 5 seconds timeout too during most of my testing, but forgot to add it back.

It seems like this is no longer needed. I verified that these tests are not affecting the other tests by running all the other tests in the same package together. So, it's okay to have theses tests in the same single test file, but it's also fine to keep proxy related tests in a separate file. We can just remove this comment.

@darkowlzz I'm not sure if I'm doing something wrong/different than you but when moving the proxy test function to strategy_test.go, I get:

--- FAIL: TestCheckoutStrategyForImplementation_Proxied (6.07s)
    --- FAIL: TestCheckoutStrategyForImplementation_Proxied/gogit_HTTP_PROXY (0.36s)
        strategy_test.go:665:
            Unexpected error:
                <*fmt.wrapError | 0xc000318160>: {
                    msg: "unable to clone 'http://example.com/bar/test-reponame': repository not found",
                    err: <*errors.errorString | 0xc0001ebad0>{
                        s: "repository not found",
                    },
                }
                unable to clone 'http://example.com/bar/test-reponame': repository not found
            occurred
    --- FAIL: TestCheckoutStrategyForImplementation_Proxied/gogit_HTTPS_PROXY (0.47s)
        strategy_test.go:668:
            Expected
                <bool>: false
            to equal
                <bool>: true

Which indicates empty environment variables were cached by the previous tests and the proxy isn't used.

darkowlzz · 2022-01-07T12:50:10Z

@au2001 I pasted the whole test function in strategy_test.go and just been running go test -v github.com/fluxcd/source-controller/pkg/git/strategy -count 1, even increasing the count to 3-4 and it never fails. It runs all the unrelated tests and the proxy tests multiple times at once without any issue.

I've also been running make test to run the tests of the whole project and it works fine for me.

go.mod

au2001 · 2022-01-07T13:04:26Z

@au2001 I pasted the whole test function in strategy_test.go and just been running go test -v github.com/fluxcd/source-controller/pkg/git/strategy -count 1, even increasing the count to 3-4 and it never fails. It runs all the unrelated tests and the proxy tests multiple times at once without any issue.

@darkowlzz If I place the proxy tests at the top of the file, they are ran first and thus the cached variables are the good ones.
I'm not sure why the other tests still succeed. I would think they would have failed to connect to the proxy.
But when I try to place the proxy tests last, they fail, even using your command.

Is it working for you even with proxy tests last?

darkowlzz · 2022-01-07T13:07:31Z

Is it working for you even with proxy tests last?

@au2001 Yes, fails when added at the very end with the same unable to clone, repository not found error.

darkowlzz · 2022-01-07T13:15:03Z

I'm not sure why this is happening. But this proves that there's some interference when the tests are in the same file.
So, @racdev please ignore my comment about removing the comment about separate file.

pkg/git/strategy/strategy_proxy_test.go

darkowlzz · 2022-01-07T13:37:28Z

Discovered another related issue with the test ordering, if the proxy tests are in a separate file, such that the file name in sorted order leads to the proxy tests run after the other tests, it fails.
Currently, strategy_proxy_test.go runs before strategy_test.go so we don't see an issue.
But if the files are renamed like strategy1_test.go and strategy2_test.go, the proxy tests in strategy2_test.go, it fails because the proxy tests run after the other tests.

au2001 · 2022-01-07T13:46:36Z

Discovered another related issue with the test ordering, if the proxy tests are in a separate file, such that the file name in sorted order leads to the proxy tests run after the other tests, it fails. Currently, strategy_proxy_test.go runs before strategy_test.go so we don't see an issue. But if the files are renamed like strategy1_test.go and strategy2_test.go, the proxy tests in strategy2_test.go, it fails because the proxy tests run after the other tests.

Good catch. That's an issue though.
Could the tests be excluded from go test ./... in the Makefile and ran right after in a separate command?
Would that be enough?

darkowlzz · 2022-01-07T14:19:04Z

Could the tests be excluded from go test ./... in the Makefile and ran right after in a separate command?

Yes, I think I mentioned this a few days ago. We can skip just the proxy tests and run them later separately.
One way to do it is using build tags. But I'm afraid that may introduce build tags elsewhere and updates to unrelated files.

After playing around with build tags and other options, it looks like moving the proxy tests into a separate package helps. In separate package, it doesn't matters if it runs before or after the other tests in other packages.

So, I created a directory within strategy dir, pkg/git/strategy/proxy and moved the proxy test file in there. Made the necessary changes like change the package name at the top of the file, change the testdata path (../testdata) and import "github.com/fluxcd/source-controller/pkg/git/strategy", since the test is in a separate directory now. Ran the tests with:

go test github.com/fluxcd/source-controller/pkg/git/strategy/... -count 1
ok      github.com/fluxcd/source-controller/pkg/git/strategy    5.015s
ok      github.com/fluxcd/source-controller/pkg/git/strategy/proxy      0.973s

It succeeded even though proxy tests ran at the end.
This may still need some more testing, but I feel this is much better than introducing build tags and running tests separately.

darkowlzz · 2022-01-07T14:31:51Z

A separate package also enables easy ignoring of packages using go list, something like:

$ go test -v `go list ./... | grep -v proxy` -count 1

That'll test all the packages except for the proxy package.

$ go list ./... | grep -v proxy
github.com/fluxcd/source-controller
github.com/fluxcd/source-controller/controllers
github.com/fluxcd/source-controller/internal/fs
github.com/fluxcd/source-controller/internal/helm
github.com/fluxcd/source-controller/internal/helm/chart
github.com/fluxcd/source-controller/internal/helm/getter
github.com/fluxcd/source-controller/internal/helm/repository
github.com/fluxcd/source-controller/pkg/gcp
github.com/fluxcd/source-controller/pkg/git
github.com/fluxcd/source-controller/pkg/git/gogit
github.com/fluxcd/source-controller/pkg/git/libgit2
github.com/fluxcd/source-controller/pkg/git/strategy
github.com/fluxcd/source-controller/pkg/sourceignore

This pattern was very common many years ago when go didn't ignore the vendor directory for running tests.

racdev · 2022-01-10T17:25:06Z

Think I have made the updates as required 🙏

darkowlzz

@racdev Everything looks and works great.
Sorry that I couldn't add some more suggestions last week. Added a few minor suggestions for improvements. Hope that's okay.

pkg/git/strategy/proxy/strategy_proxy_test.go

racdev · 2022-01-17T20:57:58Z

Apologies for the delay in making updates. Hopefully I have updated as required.

darkowlzz

LGTM!
Thanks a lot.

pkg/git/strategy/proxy/strategy_proxy_test.go

This configures ProxyOptions for all libgit2 Checkout functions when cloning and configures the options based on current environment settings using the git2go.ProxyTypeAuto option. Refs: fluxcd#131 Signed-off-by: Robert Clarke <rob@robertandrewclarke.com> Co-authored-by: Aurélien GARNIER <aurelien.garnier@atos.net>

hiddeco

Thank you very much @racdev 🙇 💯

stefanprodan · 2022-01-19T11:11:57Z

We should document this in the API spec and update the gogit vs libgit2 table.

hiddeco added area/git Git related issues and pull requests enhancement New feature or request labels Dec 16, 2021

au2001 mentioned this pull request Dec 23, 2021

Configure libgit2 fetch and push ProxyOptions fluxcd/image-automation-controller#285

Merged

darkowlzz reviewed Jan 7, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

darkowlzz reviewed Jan 7, 2022

View reviewed changes

pkg/git/strategy/strategy_proxy_test.go Outdated Show resolved Hide resolved

racdev force-pushed the libgit2-proxy-support branch 2 times, most recently from 1a7417f to 9ddfd4c Compare January 10, 2022 17:20

darkowlzz reviewed Jan 11, 2022

View reviewed changes

racdev force-pushed the libgit2-proxy-support branch from 8e2219b to 59c5747 Compare January 17, 2022 20:56

darkowlzz approved these changes Jan 18, 2022

View reviewed changes

darkowlzz requested review from hiddeco and stefanprodan January 18, 2022 12:56

hiddeco reviewed Jan 18, 2022

View reviewed changes

pkg/git/strategy/proxy/strategy_proxy_test.go Outdated Show resolved Hide resolved

pkg/git/strategy/proxy/strategy_proxy_test.go Outdated Show resolved Hide resolved

racdev force-pushed the libgit2-proxy-support branch from 59c5747 to 3d276b6 Compare January 18, 2022 19:36

hiddeco approved these changes Jan 19, 2022

View reviewed changes

hiddeco merged commit 3ca05e1 into fluxcd:main Jan 19, 2022

racdev deleted the libgit2-proxy-support branch January 19, 2022 13:57

darkowlzz mentioned this pull request Jan 19, 2022

docs: Add git proxy support docs #547

Merged

pjbgf mentioned this pull request Mar 29, 2022

source controller needs proxy to access github.com when running inside corporate network #131

Closed

aryan9600 mentioned this pull request Nov 23, 2022

add support for env proxying when using HTTPS with custom CAs fluxcd/go-git#9

Merged

libgit2: Configured libgit2 clone ProxyOptions #524

libgit2: Configured libgit2 clone ProxyOptions #524

Conversation

racdev commented Dec 15, 2021

hiddeco commented Dec 16, 2021

racdev commented Dec 16, 2021

au2001 commented Dec 17, 2021 • edited Loading

hiddeco commented Dec 17, 2021 • edited Loading

std4453 commented Dec 18, 2021

racdev commented Dec 20, 2021

au2001 commented Dec 20, 2021 • edited Loading

darkowlzz commented Jan 4, 2022 • edited Loading

darkowlzz commented Jan 5, 2022

darkowlzz commented Jan 5, 2022

std4453 commented Jan 5, 2022 via email • edited Loading

darkowlzz commented Jan 5, 2022 • edited Loading

au2001 commented Jan 5, 2022 • edited Loading

racdev commented Jan 5, 2022 • edited Loading

darkowlzz commented Jan 5, 2022 • edited Loading

darkowlzz commented Jan 5, 2022 • edited Loading

racdev commented Jan 5, 2022

darkowlzz commented Jan 5, 2022

racdev commented Jan 5, 2022

au2001 commented Jan 5, 2022

darkowlzz commented Jan 5, 2022 • edited Loading

au2001 commented Jan 5, 2022 • edited Loading

darkowlzz commented Jan 6, 2022

racdev commented Jan 6, 2022

au2001 commented Jan 6, 2022 • edited Loading

racdev commented Jan 6, 2022

darkowlzz commented Jan 6, 2022

au2001 commented Jan 7, 2022

darkowlzz commented Jan 7, 2022 • edited Loading

au2001 commented Jan 7, 2022

darkowlzz commented Jan 7, 2022

darkowlzz commented Jan 7, 2022

darkowlzz commented Jan 7, 2022

au2001 commented Jan 7, 2022

darkowlzz commented Jan 7, 2022 • edited Loading

darkowlzz commented Jan 7, 2022 • edited Loading

racdev commented Jan 10, 2022

darkowlzz left a comment

Choose a reason for hiding this comment

racdev commented Jan 17, 2022

darkowlzz left a comment

Choose a reason for hiding this comment

hiddeco left a comment

Choose a reason for hiding this comment

stefanprodan commented Jan 19, 2022

au2001 commented Dec 17, 2021 •

edited

Loading

hiddeco commented Dec 17, 2021 •

edited

Loading

au2001 commented Dec 20, 2021 •

edited

Loading

darkowlzz commented Jan 4, 2022 •

edited

Loading

std4453 commented Jan 5, 2022 via email •

edited

Loading

darkowlzz commented Jan 5, 2022 •

edited

Loading

au2001 commented Jan 5, 2022 •

edited

Loading

racdev commented Jan 5, 2022 •

edited

Loading

darkowlzz commented Jan 5, 2022 •

edited

Loading

darkowlzz commented Jan 5, 2022 •

edited

Loading

darkowlzz commented Jan 5, 2022 •

edited

Loading

au2001 commented Jan 5, 2022 •

edited

Loading

au2001 commented Jan 6, 2022 •

edited

Loading

darkowlzz commented Jan 7, 2022 •

edited

Loading

darkowlzz commented Jan 7, 2022 •

edited

Loading

darkowlzz commented Jan 7, 2022 •

edited

Loading