Arvados

22581-api-service-support @ commit:6f334adc80b53abe0c508904123040b8ca209591

22581-api-service-support @ 6b707d3e5234130e574ff6435190ad4cd11a67a0

developer-run-tests: #4714

22581-api-service-support @ 36dfde98aa901334d056f73c85db32cd25ab426e

developer-run-tests: #4718

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • adds new columns to containers and container requests as described in #22551
  • adds validation for service and published_ports columns
  • adds unique index to database for the "published_port" link_class
  • adds Service and PublishedPort fields to the Go SDK
  • adds Services.ContainerWebServices to Config file
  • the one design change I made was to add an additional field "initial_path" in the spec for a published port (alongside "access" and "label") which should be used when composing the entry URL the user clicks on, making it possible to point the browser at an arbitrary path in the web app and not only at the root.
• 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.
  • added unit test for validating published_ports
  • added unit test that 'name' is unique for records with link_class: published_port
  • I started working on the client features (#22706) starting from this unmerged branch and have it all working together in that (also unmerged) development branch, so I know this branch's features are fit for purpose
  • Working on the client side was also gave me the idea that "initial_path" was a good idea
• New or changed UX/UX and has gotten feedback from stakeholders.
  • n/a
• Documentation has been updated.
  • yes, updated API docs for containers with a new section describing the syntax and usage of published_ports
  • Did not update the documentation for link_class: published_port because it has not yet been implemented controller, and thus would be misleading.
• Behaves appropriately at the intended scale (describe intended scale).
  • This adds new columns/fields to ever container request and container, but most of the time they will be empty, so it is only a marginal increase in record size.
• Considered backwards and forwards compatibility issues between client and server.
  • API revision was bumped up, otherwise these features have no effect on clients that are not aware of them.
• Follows our coding standards and GUI style guidelines.
  • yes

This branch could be merged as is, but there's a couple of outstanding features in Controller that are required to meet our original design for this feature -- support for access control (being able to specify 'public' access to services) and support for service aliases using a published_port link. Would it make sense to go ahead and extend this branch to do that? Or merge this branch and then follow up?

In source:doc/_includes/_container_published_ports.liquid:

Example URL should use example cluster id zzzzz, not zzz.

"Each key in the hash is a string with the integer value of a port" -> how about "Each key is a port number" (see example)? This seems inherently awkward because we're trying to get across that it must be encoded as a string (not an int) but also must be a number (not a service name). Maybe fewer words, plus the nice clear example, is the best way?

If "private", the client connecting to the container should provide an Arvados API token

This should be "must", not "should", right?

We should mention that in the "private" case, access is only granted to the same user who submitted the container request(s) corresponding to the container. The current wording suggests any valid Arvados token will suffice.

We should mention that any port not listed in published_ports is still accessible just as if it were listed with {"access":"private"}, the only difference is that it won't be shown in Workbench.

source:doc/api/methods/container_requests.html.textile.liquid and source:doc/api/methods/containers.html.textile.liquid

published_ports should not depend on "If service is true, ...". Published ports are also useful for non-service containers. Update validate_service in source:services/api/app/models/container_request.rb.

source:services/api/app/controllers/arvados/v1/schema_controller.rb

Should document the "initial_path" key.

source:services/api/app/models/container_request.rb

Most of validate_service would make more sense as validate_published_ports.

"value '#{v}' must be an dict" -> "...must be an object"?

• key 'http' must be an integer in the range ... but keys can't be integers, they must be strings. Maybe "key must be a decimal port number ..."?
• value 80.access must be one of 'public' or 'private' but was '"peanuts"' has an extra layer of quotes

Should we reject published_ports with unrecognized/misspelled keys, say "tls":true ?

Should we reject keys like "80.1" and "80+80" and " 80 " and "\n80"? (s.to_i == 80 for all of these, so I think they will be accepted by the current code)

source:services/api/test/unit/container_request_test.rb

Some of the "published_ports validation" tests aren't really doing what the comments suggest. For example, in the "invalid access" test, the update is already invalid regardless of the bogus "access" value, because "initial_path" is missing.

We could also test that "label":"" is OK here.

there's a couple of outstanding features in Controller that are required [...] Would it make sense to go ahead and extend this branch to do that?

• Feature #22613: Update install scripts/docs to enable external access to HTTP services in containers
• Feature #22706: declare published ports in CWL & render on workbench process page
• Feature #22760: Allow container http proxy without token when published_ports.*.access=public

I'm not keen on the "don't merge until everything else is done" idea. Splitting the doc updates to #22613 also seems too annoying.

Maybe we should just add "note: some behaviors described below are not fully implemented, see issue X" in the docs, and remove when done? FWIW, #22760 seems trivial, but the other two seem to have some potential to take longer.

Tom Clegg wrote in #note-16:

In source:doc/_includes/_container_published_ports.liquid:

Example URL should use example cluster id zzzzz, not zzz.

Fixed.

"Each key in the hash is a string with the integer value of a port" -> how about "Each key is a port number" (see example)? This seems inherently awkward because we're trying to get across that it must be encoded as a string (not an int) but also must be a number (not a service name). Maybe fewer words, plus the nice clear example, is the best way?

Fixed.

If "private", the client connecting to the container should provide an Arvados API token

This should be "must", not "should", right?

Fixed.

We should mention that in the "private" case, access is only granted to the same user who submitted the container request(s) corresponding to the container. The current wording suggests any valid Arvados token will suffice.

Fixed.

We should mention that any port not listed in published_ports is still accessible just as if it were listed with {"access":"private"}, the only difference is that it won't be shown in Workbench.

Fixed.

published_ports should not depend on "If service is true, ...". Published ports are also useful for non-service containers. Update validate_service in source:services/api/app/models/container_request.rb.

Ok, I changed it.

source:services/api/app/controllers/arvados/v1/schema_controller.rb

Should document the "initial_path" key.

Done

source:services/api/app/models/container_request.rb

Most of validate_service would make more sense as validate_published_ports.

Renamed it.

"value '#{v}' must be an dict" -> "...must be an object"?

The error messages here seem a little awkward

• key 'http' must be an integer in the range ... but keys can't be integers, they must be strings. Maybe "key must be a decimal port number ..."?
• value 80.access must be one of 'public' or 'private' but was '"peanuts"' has an extra layer of quotes

I reworded the error messages a bit.

Should we reject published_ports with unrecognized/misspelled keys, say "tls":true ?

Sure, added a check.

Should we reject keys like "80.1" and "80+80" and " 80 " and "\n80"? (s.to_i == 80 for all of these, so I think they will be accepted by the current code)

Added a check.

source:services/api/test/unit/container_request_test.rb

Some of the "published_ports validation" tests aren't really doing what the comments suggest. For example, in the "invalid access" test, the update is already invalid regardless of the bogus "access" value, because "initial_path" is missing.

We could also test that "label":"" is OK here.

Fixed up the tests.

22581-api-service-support @ a27b23093310534e28575bce7449ff062713388e

developer-run-tests: #4734