Bug #6263

[Git] Can't push via arv-git-httpd

Added by Bryan Cosca almost 6 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Git hosting
Target version:
Start date:
06/08/2015
Due date:
% Done:

63%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Description

$ git push origin b303
Counting objects: 7, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (4/4), 491 bytes, done.
Total 4 (delta 2), 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/b303
To https://git.afkia.arvadosapi.com/ward/ward.git
! [remote rejected] b303 -> b303 (hook declined)
error: failed to push some refs to 'https://git.afkia.arvadosapi.com/ward/ward.git'


Subtasks

Task #7062: Test env var behavior with stubResolvedTom Clegg

Task #7054: Replicate, confirm which versions and run scripts are in playResolvedTom Clegg

Task #7061: Set REMOTE_PORT in arv-git-httpdResolvedTom Clegg

Task #7049: Review 6263-arv-gitoliteResolvedTom Clegg

Task #7143: Add gitolite integration testResolvedTom Clegg

Task #7151: Review 6263-go-coverage (arvados-dev)ResolvedTom Clegg

Task #7183: Review 6263-gitolite-test branches on arvados and arvados-devResolvedTom Clegg


Related issues

Has duplicate Arvados - Bug #7009: When using git clone "https" instead of "git@", user is unable to push to masterDuplicate08/17/2015

Associated revisions

Revision 8e17ce53
Added by Tom Clegg over 5 years ago

Merge branch '6263-arv-gitolite' refs #6263

Revision eb73e211
Added by Tom Clegg over 5 years ago

Merge branch '6263-gitolite-test' refs #6263

Revision c6f9a3fc
Added by Tom Clegg over 5 years ago

Merge branch '6263-gitolite-test' refs #6263

Revision c6f9a3fc
Added by Tom Clegg over 5 years ago

Merge branch '6263-gitolite-test' refs #6263

Revision a4e96f86
Added by Tom Clegg over 5 years ago

Merge branch '6263-go-coverage' refs #6263

Revision a4e96f86
Added by Tom Clegg over 5 years ago

Merge branch '6263-go-coverage' refs #6263

History

#1 Updated by Ward Vandewege almost 6 years ago

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

#2 Updated by Tom Clegg almost 6 years ago

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.

#3 Updated by Ward Vandewege almost 6 years ago

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'

#4 Updated by Brett Smith over 5 years ago

  • Project changed from Arvados Private to Arvados
  • Subject changed from git push origin perl problem to [Git] Can't push via arv-git-httpd
  • Category set to Git hosting
  • Target version set to 2015-09-02 sprint

#5 Updated by Brett Smith over 5 years ago

  • Story points set to 1.0

This is expected to be address in our Gitolite deployment—either the way we set it up, or in the Gitolite code itself. Currently this isn't expected to involve changing arv-git-httpd at all.

#6 Updated by Tom Clegg over 5 years ago

  • Assigned To set to Tom Clegg

#7 Updated by Tom Clegg over 5 years ago

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.

#8 Updated by Tom Clegg over 5 years ago

  • Status changed from New to In Progress

#9 Updated by Tom Clegg over 5 years ago

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.)

#10 Updated by Radhika Chippada over 5 years ago

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)”?

#11 Updated by Tom Clegg over 5 years ago

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.
We do use gitolite in production, so in that sense it's proved. Currently the integration tests only use plain git, so in that sense it isn't proved. It would certainly be good to add integration tests with gitolite, but that seemed too daunting to add here. I suppose our options include:
  • 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)”?
Hm, that is confusing:
  • 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.

#12 Updated by Radhika Chippada over 5 years ago

  • 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.

#13 Updated by Tom Clegg over 5 years ago

re. 6263-go-coverage (arvados-dev|fe19ce4b) -- example coverage report

#14 Updated by Tom Clegg over 5 years ago

  • Status changed from In Progress to Feedback

#15 Updated by Brett Smith over 5 years ago

  • Target version changed from 2015-09-02 sprint to 2015-09-16 sprint
  • Story points changed from 1.0 to 0.5

Moving to accommodate the remaining reviews.

#16 Updated by Radhika Chippada over 5 years ago

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 :).

#17 Updated by Radhika Chippada over 5 years ago

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

#18 Updated by Tom Clegg over 5 years ago

  • Status changed from Feedback to Resolved

Also available in: Atom PDF