Allow -listen-http, -listen-https and -listen-proxy to be given more than once

Per issue #13 (closed), sometimes you want to listen on more than one port for each type of listener. Add support for doing so.

Added 1 commit:

• 70d98260 - Run go fmt. Oops

Added 1 commit:

• bda4e1f2 - go vet: Avoid shadowing the file descriptor

mentioned in issue #13 (closed)

OK, we didn't know about make verify! My bad for not running it before committing anything.

Could we consider the cyclomatic complexity failure to be spurious? I'm not clear on why 9 was chosen; with a complexity of 10 (just one more!) it passes. Or I'm happy to reduce the depth if necessary.

Naive test coverage is still < 40% though, and with the pointer-to-fd changes in daemon.go and app_config.go I'd quite like to up that before removing the WIP tag. What do we think to a minimal integration test harness that runs the compiled binary and verifies that each port responds with a 404 Not Found, or a 200 for a fixture site?

Added 2 commits:

• 3e142ea5 - Store listener file descriptors as []uintptr instead of []*uintptr
• 1e31b2f1 - Rename the manystrings type to MultiStringFlag and move to its own file

The []uintptr refactoring suggested by @ayufan reduced cyclomatic complexity enough for make verify to pass, which is a bonus!

Added 1 commit:

• 54211fb1 - Pass listener fds into goroutines explicitly instead of relying on closures

Added 1 commit:

• eafee0b1 - Re-introduce daemonListenFd and have daemonListenFds modify both its arguments

@lupine Looks awesome! 👍

Do you have ideas how we could test this changes, maybe write some automated tests?

@lupine Nice work! Just a few more things from me as well:

• Can you add a changelog item?
• Can you make sure this is documented? It can get a quick mention in the README, but should definitely be at http://doc.gitlab.com/ee/pages/administration.html
• Can you add support for this to the Omnibus package? See the "Omnibus installations" section under http://doc.gitlab.com/ee/pages/administration.html#setting-up-gitlab-pages

@ayufan Thanks! For testing, I was thinking about a simple integration harness - execute the binary with known parameters and ensure the HTTP(S) ports respond with the right things. It's quick, and it'll cover the parent/child interaction in daemon.go (although we'd need to run the binary as a privileged user).

Let me know if you don't like this, and I'll have a bit more of a think. Should have time to work in it tonight or Saturday. A brief look suggests other options are going to include tests for unexported functions and a lot of surgery to daemon.go - to get more-fragile tests that cover less ground.

MultiStringFlag, at least, can have a short unit test. I'll add that :-). Omnibus and documentation support too.

It's quick, and it'll cover the parent/child interaction in daemon.go (although we'd need to run the binary as a privileged user).

I think that we could use any upper ports, and test it as unprivileged user :)

Morning! If we're not passing -daemon-uid and -daemon-gid then we can certainly test as an unprivileged user, but then the code in daemon.go doesn't get touched. I experimented with passing my own uid and gid, but the chroot() call gets EPERM, at the very least.

I was planning on allowing both privileges and unprivileged operation, and using high ports is definitely the right way to go in the latter case :)

Added 1 commit:

• 18a1a5df - Add simple acceptance tests for the gitlab-pages binary.

Added 1 commit:

• 4731dfe4 - Fix 'make acceptance' for go1.5

Added 1 commit:

• 61cf51c2 - Add documentation for providing -listen-http and friends multiple times.

mentioned in merge request omnibus-gitlab!769 (closed)

Added 1 commit:

• b8fd2f53 - Add a simple test for MultiStringFlag

https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/383 https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/769

Added 2 commits:

• 59bc7038 - Put test RSA key + certificate somewhere common
• 671ad307 - Start running acceptance tests against HTTPS

Added 1 commit:

• d5f6cec0 - Add support for dropping privileges if the acceptance tests are run under sudo

Added 1 commit:

• d62af32b - Update CHANGELOG and VERSION

OK, I think that's everything. I've not changed the .gitlab-ci.yml to run sudo -E make acceptance because I don't know if it'll work or not, offhand.

Let me know what you think :-)

Added 1 commit:

• 5a75678a - Remove an accidentally-committed debug branch in a test

Unmarked this merge request as a Work In Progress

I think we're all happy with everything here, so should we merge it?

Ping @ayufan, could you check if anything else is needed here?

@nick.thomas

Could you rebase it?

Added 13 commits:

• 5a75678a...62b22d77 - 8 commits from branch gitlab-org:master
• 33fca097 - Allow -listen-http, -listen-https and -listen-proxy to be given more than once
• 6b6eed8c - Add acceptance tests for the gitlab-pages binary.
• e5d0aa9a - Add documentation for providing -listen-http and friends multiple times.
• 411cab50 - Add a simple test for MultiStringFlag
• c2f0c476 - Update CHANGELOG and VERSION

Added 1 commit:

• b8ece518 - Add the gitlab-pages binary to gitignore

@ayufan have a significant rebase :)

I did this on my personal account, so may as well finish it in the same way. Four months isn't as long as I thought it was :D

@ayufan @DouweM any objections if I merge this? I'm a bit leery of "JFDI" since I wrote the code!

I particularly want the acceptance tests!

assigned to @ayufan

merged

mentioned in commit bc4aa822