Arvados

I improved this by refactoring the "is it a ContainerWebServices request?" code so there's only one copy of it.

Testing was a bit painful.

Updated the nginx conf used by integration tests to add a "listen" directive when needed to support ExternalPortMin/Max.

Fixed the gateway logic to accept 127.0.0.1 as a real gateway address when using the loopback driver.

Fixed the a-d-c worker code to parse the instance address as either host or host:port when constructing the gateway address (the loopback driver uses 127.0.0.1:23456 to target its custom ssh server, and the gateway was advertising itself as [127.0.0.1:23456]:34562).

Unfortunately the integration test cluster (arvados-server boot using a-d-c with loopback driver with RuntimeEngine: singularity) still does not support ContainerWebServices, even after updating arvados-server boot to pass through ARVADOS_TEST_PRIVESC to its subprocesses. I think this would work with passwordless sudo, but with non-passwordless sudo, crunch-run doesn't have access to the terminal to ask for a password. Even if we run a sudo command from the test case, sudo needs to ask again from inside crunch-run. Rather than changing our implicit PRIVESC requirement from "sudo" to "passwordless sudo", I figured it would be more reasonable to switch these integration tests from singularity to docker, which doesn't have this problem.

(We still have some "skip if docker not available" in other tests, but I think docker is a test dependency now -- right? If not, I should reconsider this.)

23044-web-services-routing @ 67a347424611983af738e086d61331e65f4f88d7 -- developer-run-tests: #4837

wb integration test failure is surely unrelated (see attached logs) -- retry run-tests-services-workbench2-integration: #615

23044-web-services-routing @ 67a347424611983af738e086d61331e65f4f88d7 -- developer-run-tests: #4837 + run-tests-services-workbench2-integration: #615

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • ✅ refactor routing code to de-duplicate
  • ✨ fixed some integration testing things to enable the feature to work in test clusters (see #note-5)
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • n/a
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • ✅ integration test now exercises the whole controller code path and a real nginx config
• New or changed UI/UX has gotten feedback from stakeholders.
  • n/a
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • ✅ n/a
• Considered backwards and forwards compatibility issues between client and server.
  • ✅ n/a
• Follows our coding standards and GUI style guidelines.
  • ✅

Tom Clegg wrote in #note-5:

(We still have some "skip if docker not available" in other tests, but I think docker is a test dependency now -- right? If not, I should reconsider this.)

In general I think tests are allowed to assume they can run Docker containers, yes. At the same time, I do think it's nice when tests properly declare their dependencies and report when those dependencies are not met. That makes it easier to debug issues that arise if there's a disconnect between the test host configuration and the tests.

I think the tests that skip in the absence of Docker are a gesture towards that, but we have no consistent philosophy on this and there are a million improvements we could make in this direction if we wanted.

This branch is based off a relatively old main. It would've been nice to rebase before the test+review. Under the circumstances I think this is pretty low risk (nobody else has been working on controller lately) and I'm fine with you merging, but I'm flagging it just because I would like us to try to start getting better about this. Thanks.

23044-web-services-routing @ 32681fc9b3362f7a8e309b5fac18e316b12fee01 -- developer-run-tests: #4839

• merge main
• fail tests early if docker info fails

Tom Clegg wrote in #note-9:

23044-web-services-routing @ 32681fc9b3362f7a8e309b5fac18e316b12fee01 -- developer-run-tests: #4839

• merge main
• fail tests early if docker info fails

Well, what I meant by my earlier comment that I wasn't explicit enough about is, we should decide together what's the right way to handle missing test dependencies, and then implement that going forward. But sure, this is an improvement. LGTM, thanks.