Arvados

Confirmed as a bug when trying to push to a repo checked out via arv-git-httpd

gitlab seems to have addressed this by patching gitolite. The patch didn't make it upstream.

It looks like the solution might be just setting setting some env vars when running arv-git-httpd (so it can pass them on to git and hence gitolite):

GL_BYPASS_ACCESS_CHECKS=1
PERLLIB=/usr/share/gitolite3/lib

This is just from reading source. I haven't tested at all.

Tom Clegg wrote:

gitlab seems to have addressed this by patching gitolite. The patch didn't make it upstream.

It looks like the solution might be just setting setting some env vars when running arv-git-httpd (so it can pass them on to git and hence gitolite):

[...]

This is just from reading source. I haven't tested at all.

Hrm, that doesn't seem to fix it:

$ git push
Counting objects: 5, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 291 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: Empty compile time value given to use lib at hooks/update line 6
remote: Use of uninitialized value in require at hooks/update line 7.
remote: Can't locate Gitolite/Hooks/Update.pm in @INC (@INC contains:  /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at hooks/update line 7.
remote: BEGIN failed--compilation aborted at hooks/update line 7.
remote: error: hook declined to update refs/heads/master
To https://git.qr1hi.arvadosapi.com/wardv.git
 ! [remote rejected] master -> master (hook declined)
error: failed to push some refs to 'https://git.qr1hi.arvadosapi.com/wardv.git'

Gitolite needs

GITOLITE_HTTP_HOME=/var/lib/gitolite
GL_BYPASS_ACCESS_CHECKS=1

It also emits warnings unless it has SERVER_ADDR and REMOTE_PORT, which Go's CGI library doesn't provide by itself.

Go's CGI library, and therefore arvados-git-httpd, does not pass through environment variables to the CGI program unless listed in InheritEnv.

6263-arv-gitolite @ b20aa99 passes through the GL vars, so you can set them in your run script. It sets SERVER_ADDR, but doesn't address REMOTE_PORT. (We can get REMOTE_PORT by building with Go 1.5.)

1de4f2f adds GL_BYPASS_ACCESS_CHECKS=1 to the run script in the install guide (GITOLITE_HTTP_HOME was already there).

Meanwhile, as a workaround, you can put a wrapper like this in /etc/sv/arvados-git-httpd/gitolite-wrapper like this

#!/bin/bash
export GL_BYPASS_ACCESS_CHECKS=1
export GITOLITE_HTTP_HOME=/var/lib/gitolite
export SERVER_ADDR=localhost
exec /usr/share/gitolite3/gitolite-shell

...make it executable, and call the wrapper instead of git in the arvados-git-httpd command in the run script.

...
exec chpst -e "${envdir}" -u git:git arvados-git-httpd -address=:9001 -git-command=`pwd`/gitolite-wrapper -repo-root=/var/lib/gitolite/repositories

This seems to make everything work on 4xphq.

6feefc5 splits the git cgi stuff out of auth_handler.go into git_handler.go, and adds tests for the new env vars in git_handler_test.go.

(Also addressed REMOTE_PORT after all, so silencing gitolite warnings doesn't have to block on upgrading CI to Go1.5.)

Just a few comments about doc and tests.

• does doc.go in arv-git-httpd need to be updated? Espeicially, the -git-command portion?
  • While at it, I have a question about the doc for “-address [host]:[port]”? It says “Each can be a name, a number, or empty.” Can the port be “name” (by name I am thinking string)?

• This comment “gitHandler is an http.Handler that invokes git-http-backend (or whatever backend is configured) via CGI” => Did we test with any other backend or just with git http backend? Wondering if we should not say this unless we proved it as yet.

• Even if I remove all the code below the comment “Copy the wrapped cgi.Handler, so these request-specific variables don't leak into the next request” in git_handler.go, the tests pass. The tests do not seem to test past the net.SplitHostPort block. Is it possible to add tests that test the “handlerCopy.ServeHTTP(w, r)”?

Radhika Chippada wrote:

• does doc.go in arv-git-httpd need to be updated? Espeicially, the -git-command portion?

Yes, indeed. Added reference to gitolite-shell, updated formatting, and (perhaps most useful) added a link to the doc.arvados.org page.

• While at it, I have a question about the doc for “-address [host]:[port]”? It says “Each can be a name, a number, or empty.” Can the port be “name” (by name I am thinking string)?

Yes, you can specify a port by name ("-address :http") if http is listed in /etc/services.

• This comment “gitHandler is an http.Handler that invokes git-http-backend (or whatever backend is configured) via CGI” => Did we test with any other backend or just with git http backend? Wondering if we should not say this unless we proved it as yet.

• Do not merge this until gitolite integration tests are added
• Merge this, do integration tests in another branch (but still part of this ticket)
• Put integration tests on a separate ticket.

I'm inclined to go with the middle option because this bug (presumably) would have been prevented if we had integration tests with gitolite. WDYT?

• Even if I remove all the code below the comment “Copy the wrapped cgi.Handler, so these request-specific variables don't leak into the next request” in git_handler.go, the tests pass. The tests do not seem to test past the net.SplitHostPort block. Is it possible to add tests that test the “handlerCopy.ServeHTTP(w, r)”?

• If I do that, I can't even compile: "git_handler.go:42: remoteHost declared and not used"
• If I enable coverage profiling, Go tells me the code is covered.
• The git_handler_test.go code sure looks like it's testing this.
• Without that, it seems like the CGI program would never be run, and none of the integration tests would pass...?

I was at 6feefc5 before, now at 4b8da3c.

• Tom said: "I'm inclined to go with the middle option because this bug (presumably) would have been prevented if we had integration tests with gitolite. WDYT?"

If we could have prevented or caught the bug beforehand with an integration test, that asserts that we need a test. If it is not a big amount of additional undertaking, I definitely think we should consider it as part of addressing this issue in a separate branch (and merge the code without blocking on the test). Please do either option 2 or 3 as appropriate. Thanks.

• The test now fails if I comment out the said code in git_handler.go. So, this is good. Thanks.

LGTM.

Branch 6263-go-coverage looks awesome. It is great to see coverage report. I tested go and non-go areas and saw no issues testing with this update. Thanks.

Just one question. In the display of the coverage report (file:///.../arvados/tmp/coverage-services_keepstore.html), the background color is black and foreground has various colors. The "low coverage" areas with gray color foreground were hard to read on black background. Is it possible to configure the display background color to white? This is just a thought and absolutely not a big deal. I really love the coverage report either way :).

Branches 6263-gitolite-test in arvados and arvados-dev repositories LGTM. Thanks.