Add git ls-remote to validate the remote GIT repository #5573

mfojtik · 2015-11-02T13:05:33Z

Fixes: #4564
Fixes: #5944

@bparees @soltysh this will use git ls-remote command to check the source repository URL. It should pickup all credentials we have for the GIT in case the repo is private. It also perform check if the repository is reachable. Proxy vars should be picked up as well (I didn't tested this).

0xmichalis · 2015-11-02T13:09:04Z

pkg/build/builder/source.go

@@ -27,6 +26,10 @@ const (
 	urlCheckTimeout = 16 * time.Second
 )

+var (
+	GitAuthenticationError, GitNotFoundError error


Aren't these nil?

yeah, should stop drinking

These should be error types. See for example

origin/pkg/deploy/controller/deploymentconfig/controller.go

Line 45 in 3cf7f3c

type fatalError string

soltysh · 2015-11-02T13:14:34Z

Other than what Michalis already pointed out, looks good.

mfojtik · 2015-11-02T13:17:36Z

@soltysh @Kargakis thanks, errors fixed.

mfojtik · 2015-11-02T13:18:12Z

[test]

soltysh · 2015-11-02T13:18:48Z

LGTM

0xmichalis · 2015-11-02T13:21:54Z

pkg/build/builder/source.go

+	succ := make(chan bool, 1)
+	go func() {
+		out, err = cmd.CombinedOutput()
+		succ <- true


We don't really care about this value so this should be an empty struct.

0xmichalis · 2015-11-02T13:22:23Z

one more nit, LGTM otherwise

0xmichalis · 2015-11-02T13:23:52Z

Actually can you add a test case for this as well?

bparees · 2015-11-02T13:25:27Z

if this works with proxies, you should remove the logic that skips this check when a proxy is set.

mfojtik · 2015-11-02T13:45:25Z

i fixed everything, but do not merge this yet, need to test if the proxy stuff works as expected

liggitt · 2015-11-02T13:47:52Z

pkg/build/builder/source.go

+// remote repository failed to authenticate.
+// Since this is calling the 'git' binary, the proxy settings should be
+// available for this command.
+func checkRemoteGit(url string, timeout time.Duration) error {


I thought the original issue was a long hang when an invalid IP or host was specified... does this fail fast in cases like that?

I see the timeout, but not a test exercising it... is that possible?

@liggitt it hangs when it was asking for password in case you have private repos.... and yes, there is no test exercising this, something I have to follow up

@mfojtik there was also an issue with long hangs when the hostname didn't exist (or rather the hostname existed, but nothing was listening at the other end), as @liggitt was saying.

@bparees the timeout should take care of that issue IMHO, but agree with @liggitt we should have extended test that exercise this... will need some fake server

i can, also I think github is asking me for a password even if I request "https://github.com/openshift/foo"

Per #3624 it would be good not to rely on external services, using Jordan's idea of httptest returning 401 is preferable, imho.

@soltysh @liggitt already done, the test is not depending on anything external now

@soltysh i would not be that optimistic, I might revert the github test to have positive case ;-)

mfojtik · 2015-11-02T14:15:16Z

@bparees tested with proxy/without proxy and by accident tested also the timeout... it all works, so this is mergeable now

mfojtik · 2015-11-02T14:26:55Z

[test] again

smarterclayton · 2015-11-02T14:42:33Z

pkg/build/builder/source.go

-	if !testConnection {
-		return nil
+
+	if strings.Contains(string(out), "Authentication failed") {


Use a switch

mfojtik · 2015-11-02T15:42:20Z

@liggitt @bparees test case added

bparees · 2015-11-02T15:50:32Z

@mfojtik this works when going through a proxy? the scenario that was broken before that lead to reworking this was a git server that could only be reached via proxy. (the testing that was done which was insufficient was accessing a git server via a proxy, when that git server could also be reached w/o using the proxy)

liggitt · 2015-11-02T16:08:36Z

pkg/build/builder/source_test.go

+	if err != nil {
+		t.Errorf("expected no error, got %q", err)
+	}
+	err = checkRemoteGit("https://254.254.254.254/foo/bar", 10*time.Second)


lower the time limit, we don't want this test to take 30 seconds to run

20 seconds. one of those tests won't normally hit the timeout.

I didn't say it would take 30 seconds. I said I didn't want it to take 30 seconds, which I don't 😁

well it won't, so desire met? :)

there are two now :-) the first one should never hit the timeout, I will lower the second one :-) is that ok?

mfojtik · 2015-11-03T09:33:25Z

@liggitt @bparees rebased and test fixed (no more github ;-)

mfojtik · 2015-11-03T11:24:56Z

[test]

mfojtik · 2015-11-18T13:33:17Z

pkg/build/builder/source.go

+		// ignored. This command will replace git:// with https:// in that case.
+		if strings.HasPrefix(gitSource.URI, "git://") {
+			glog.V(4).Infof("Using https:// instead of git:// prefix for %q", gitSource.URI)
+			out, err := exec.Command("git", "config", "--global", `url."https://".insteadOf git://`).CombinedOutput()


@bparees sanity check required

liggitt · 2015-11-18T14:30:40Z

pkg/build/builder/source.go

+
+		// When the protocol is git:// the HTTPS_PROXY and HTTP_PROXY variables are
+		// ignored. This command will replace git:// with https:// in that case.
+		if strings.HasPrefix(gitSource.URI, "git://") {


this is wrong... the git:// protocol is not http/https-based, and should not be using http_proxy/https_proxy. It's over a completely different port, so switching to https:// could actually break the check (https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols#The-Git-Protocol)

@liggitt the command bellow this will set it global, so the clone (if https succeed) will work normally.

openshift-bot · 2015-11-18T15:49:05Z

continuous-integration/openshift-jenkins/test Waiting: Determining build queue position

mfojtik · 2015-11-18T15:53:29Z

[testonlyextended][extended:core(s2i build with proxy)]

openshift-bot · 2015-11-18T16:10:20Z

Evaluated for origin test up to ebbbcc0

openshift-bot · 2015-11-18T16:20:22Z

Evaluated for origin testonlyextended up to ebbbcc0

openshift-bot · 2015-11-18T17:44:46Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7212/) (Extended Tests: core(s2i build with proxy))

openshift-bot · 2015-11-18T18:14:00Z

continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7218/) (Extended Tests: core(s2i build with proxy))

mfojtik · 2015-11-18T18:24:13Z

@bparees ready for review/merge

bparees · 2015-11-18T21:13:06Z

[merge]

mfojtik · 2015-11-19T08:00:21Z

[merge]

mfojtik · 2015-11-19T08:57:15Z

[merge] !

openshift-bot · 2015-11-19T10:00:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4058/) (Image: devenv-rhel7_2742)

mfojtik · 2015-11-19T10:11:05Z

@bparees @csrwng each merge failed with different flaky test :-)

This time, new-app is the winner:

I1119 04:46:11.727845     509 newapp.go:533] Using "https://github.com/openshift/ruby-hello-world" as the source for build
error: installing "installable:file" requires that you grant the image access to run with your credentials

Previously it was new-app again:

!!! Error in test/cmd/newapp.sh:65
    '[ "$(oc new-app --search --image-stream=postgresql | grep -E "Tags:\s+9.2, 9.4, latest")" ]' exited with status 1

And before it was timeout starting openshift server :-)

csrwng · 2015-11-19T13:51:38Z

The merge failure is an integration test flake:

--- FAIL: TestConcurrentBuildControllers-2 (6.84s)
    buildcontroller_test.go:109: Error: received unexpected build status: New

which is assigned to me: #5937
I'm making it a priority to get it fixed this morning.

mfojtik · 2015-11-19T14:04:50Z

[merge]

(an innocent chicken was sacrificed)

openshift-bot · 2015-11-19T14:05:16Z

Evaluated for origin merge up to ebbbcc0

Merged by openshift-bot

0xmichalis reviewed Nov 2, 2015
View reviewed changes

mfojtik force-pushed the fix-git-check branch from 2f72f01 to 480f286 Compare November 2, 2015 13:16

0xmichalis reviewed Nov 2, 2015
View reviewed changes

mfojtik force-pushed the fix-git-check branch from 480f286 to 92d12fd Compare November 2, 2015 13:36

liggitt reviewed Nov 2, 2015
View reviewed changes

smarterclayton reviewed Nov 2, 2015
View reviewed changes

mfojtik force-pushed the fix-git-check branch 2 times, most recently from 7e614a1 to 4724788 Compare November 2, 2015 15:41

liggitt reviewed Nov 2, 2015
View reviewed changes

mfojtik force-pushed the fix-git-check branch from 4724788 to 460e849 Compare November 3, 2015 09:32

mfojtik force-pushed the fix-git-check branch from 460e849 to 317425e Compare November 3, 2015 11:02

bparees changed the title ~~Add git ls-remote to validate the remote GIT repository~~ [POST-3.1] Add git ls-remote to validate the remote GIT repository Nov 3, 2015

mfojtik force-pushed the fix-git-check branch from 6dcf89e to a83fd22 Compare November 18, 2015 12:08

mfojtik mentioned this pull request Nov 18, 2015

The httpProxy and httpsProxy settings does not work when source URI has git:// prefix #5944

Closed

mfojtik reviewed Nov 18, 2015
View reviewed changes

mfojtik force-pushed the fix-git-check branch 3 times, most recently from 34c27af to 611ad20 Compare November 18, 2015 14:17

liggitt reviewed Nov 18, 2015
View reviewed changes

mfojtik force-pushed the fix-git-check branch 2 times, most recently from 61e1ad0 to d69b2b0 Compare November 18, 2015 15:52

Add extended test for proxy

ebbbcc0

mfojtik force-pushed the fix-git-check branch from d69b2b0 to ebbbcc0 Compare November 18, 2015 16:06

openshift-bot pushed a commit that referenced this pull request Nov 19, 2015

Merge pull request #5573 from mfojtik/fix-git-check

aa8e6ba

Merged by openshift-bot

openshift-bot merged commit aa8e6ba into openshift:master Nov 19, 2015

gabemontero mentioned this pull request Nov 19, 2015

better url check, better avoid DialTCP frailities for non-http protocols #5956

Merged

mfojtik deleted the fix-git-check branch September 5, 2018 21:07

Add git ls-remote to validate the remote GIT repository #5573

Add git ls-remote to validate the remote GIT repository #5573

Conversation

mfojtik commented Nov 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Nov 2, 2015

mfojtik commented Nov 2, 2015

mfojtik commented Nov 2, 2015

soltysh commented Nov 2, 2015

Choose a reason for hiding this comment

0xmichalis commented Nov 2, 2015

0xmichalis commented Nov 2, 2015

bparees commented Nov 2, 2015

mfojtik commented Nov 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Nov 2, 2015

mfojtik commented Nov 2, 2015

Choose a reason for hiding this comment

mfojtik commented Nov 2, 2015

bparees commented Nov 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Nov 3, 2015

mfojtik commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Nov 18, 2015

mfojtik commented Nov 18, 2015

openshift-bot commented Nov 18, 2015

openshift-bot commented Nov 18, 2015

openshift-bot commented Nov 18, 2015

openshift-bot commented Nov 18, 2015

mfojtik commented Nov 18, 2015

bparees commented Nov 18, 2015

mfojtik commented Nov 19, 2015

mfojtik commented Nov 19, 2015

openshift-bot commented Nov 19, 2015

mfojtik commented Nov 19, 2015

csrwng commented Nov 19, 2015

mfojtik commented Nov 19, 2015

openshift-bot commented Nov 19, 2015