Arvados

22777-named-proxy-port @ 73d6a792ae3573b2b3bfda0bb14fa308524252d0 -- developer-run-tests: #4773

22777-named-proxy-port @ 06da3a82653ea26964f3023b7a9f552c220b5ff2 -- developer-run-tests: #4774

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • ✅ if a link exists with link_class=published_port, head_uuid=C, properties={port:1234}, name=foobar, then incoming requests to https://foobar.{container-web-services-external-url}/ get routed to port 1234 on container C, just like we already do with incoming requests to https://C-1234.{container-web-services-external-url}/
  • ✨ incoming requests to https://{container-uuid}-{port}.example.com/ are routed to the indicated container/port only if the ContainerWebServices.ExternalURL wildcard is https://*.example.com/. Previously this worked with an arbitrary vhost beginning with {uuid}-{port} (so the proxy feature could be tested before the config entry existed) but it could become a DNS-rebinding attack vector. Although the recommended Nginx configuration already provides equivalent protection, it seems prudent to check here too.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • Added #22861 for caching token, permission, and port name lookups.
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • Automated tests added.
  • Updated existing tests that relied on routing https://{uuid}-{port}.any-string-here/ regardless of config.
• New or changed UX/UX and has gotten feedback from stakeholders.
  • n/a
• Documentation has been updated.
  • Added a link_class=published_port section to the links API page, with a link to the existing discussion on the container requests API page.
  • I suspect we will want to rearrange the docs a bit when all the parts are in place, i.e., after
    • Feature #22613: Update install scripts/docs to enable external access to HTTP services in containers
    • Feature #22706: show published ports on workbench process page
    • possibly Feature #22861: Cache ContainerHTTPProxy port names, access levels, and permissions
• Behaves appropriately at the intended scale (describe intended scale).
  • At this point the feature is suitable for small-scale / development use (see above comment about caching / #22861)
• Considered backwards and forwards compatibility issues between client and server.
  • ✅
• Follows our coding standards and GUI style guidelines.
  • ✅

This started with a bit of refactoring, see 230aa0d369363efa017abf8a58555f3d95cdac73 message.

On https://dev.arvados.org/issues/22551 we wrote that the head_uuid of the link would be the container request, but this seems to be expecting the container. Was that a deliberate design change or an oversight?

Oops, that was an accident. It should be a container request UUID. Will update.

• refuse to create/update the link ("must be a container request uuid")
• implement it so either a container uuid or a container request uuid works, error out on other object types
• allow creating the link with a container uuid (or any other object type), but it doesn't actually achieve anything

Tom Clegg wrote in #note-11:

Oops, that was an accident. It should be a container request UUID. Will update.

The tests we added in #22581 use container UUIDs. I'll fix that to align with real usage, but how should we handle this?

Got it, so I might have made the mistake first and you propagated it, or we made a subsequent change to the design. No big deal, good thing we caught it.

• refuse to create/update the link ("must be a container request uuid")

Since it's a really easy Rails validation hook, this would be my preference.

• implement it so either a container uuid or a container request uuid works, error out on other object types
• allow creating the link with a container uuid (or any other object type), but it doesn't actually achieve anything

I can't think of any situation where you would actually want either of these options.

22777-named-proxy-port @ 19a01e8303cfbef9f64e581287ac4f291062ad37 -- developer-run-tests: #4806

22777-named-proxy-port @ c45117f5aae5ee6aea9a848ffc92b48eab201dd7 -- developer-run-tests: #4807

• ✅ if link_class=published_port, then head_uuid must be a container request UUID
• ✅ generic endpoint @https://{uuid}-{port}.domain/ works whether UUID is a container request UUID or a container UUID -- I figure this is strictly more useful than leaving it as it was (container UUID only) or matching the published_port rule (container request UUID only).
• ✅ documentation updated

The documentation should be consistent and use "https://*.containers.zzzzz.example.com/" so then the published_port example would be "https://servicename.containers.zzzzz.example.com".

Rest LGTM