discovery: add port parameter

[iaguis]

This allows to perform discovery in arbitrary ports.

[kelseyhightower]

@iaguis Thanks for the PR. I've been working on the fetch command lately and ran into this issue while testing the fetch code, so I'm a big supporter of this change.

First a couple of notes. Since you are proposing a change to the spec this PR should include an update to the spec. Second can you provide more details in the commit message?

discovery: add port parameter

Currently the ACI spec only supports discovery on ports 80 and 443, this does not allow .....

Fix the problem by adding support for ...

[iaguis]

Updated commit message and spec, I hope I explained it right as writing is not my strong point :)

[jonboulle]

I don't want to add it to this part of the spec. I think if anything it should fall under #4 in a suggested format for convenience etc; then we can reference that from here.

[jonboulle]

@iaguis how would you propose to disambiguate between ports and versions in the AppString? For example example.com:22 - is that name=example.com,port=22 or name=example.com,version=22 ?

[jonboulle]

(thanks for the PR! We might just need to think this through a little more before figuring out how to land it)

[iaguis]

I thought AppString must have something more than the host name so I split it from the first slash so port number is to the left and version to the right. If that's not the case there's no way to disambiguate without changing how the version is specified or making ports mandatory.

Maybe adding a --discovery-port flag to rkt fetch and actool discovery will do.

[jonboulle]

I thought AppString must have something more than the host name

I admit that the example seemed a little contrived, but unfortunately there's no such restriction on an app name today, and it seems it would be somewhat arbitrary to add it.

I think using custom ports is a sufficiently atypical use case that having another flag would be fine; the whole point of discovery is really to make the default case super simple and so it is a reasonable expectation that http/https are on their standard ports.

[iaguis]

I updated the PR with the new flag

[philips]

@iaguis I think that something like "https-port" and "http-port" would be reasonable. Leaving it as "discovery-port" is a little to ambiguous.

[iaguis]

Do you mean something like this?

USAGE:
        actool discover [--http-port PORT] [--https-port PORT] [--insecure] APP...
OPTIONS:
        --http-port=0           Port to connect when performing discovery using HTTP. If unset or set to 0, defaults to 80. Sets insecure.
        --https-port=0          Port to connect when performing discovery HTTPS. If unset or set to 0, defaults to 443.
        --insecure=false        Allow insecure non-TLS downloads over http

[jonboulle]

LGTM, cc @vbatts

[philips]

I slept on this and we want to let people have a mapping of names to discovery endpoints and a default rendevuous point for all non-matched discovery entries.

The two use cases I see are:

1. I want all discovery for example.corp to go to example.corp:9044
2. I want all discovery for everything to go to https://discovery-proxy.example.corp instead of going directly to the originating requestor.

So, in the case above I could see two options:

USAGE:
        actool discover [--discovery-proxy ] [--discovery-host-map ACName,host=foo,port=8000,scheme=http] [--insecure] APP...
OPTIONS:
        --discovery-host-map=      Can be provided multiple times, explicit map of a prefix to a host/port/scheme for discovery. e.g. example.corp,port=8000,scheme=http
        --discovery-proxy      Proxy all HTTP requests for discovery through this proxy
        --insecure=false        Allow insecure non-TLS downloads over http

Thoughts?

[jonboulle]

Stepping back: what is the purpose of actool discover? Is it just a tool to play around with image discovery to see how it works, or is it a canonical/reference implementation that can be expected to implement the full set of prescribed/recommended discovery functionality?

[philips]

@jonboulle I see the actool as a way for a user to test out all that the discovery mechanism can offer. Essentially it is our second line debugging tool when a user files a bug against rkt/jetpack/etc to test that things are configured correctly on their end.

[jonboulle]

OK, so I guess that the map/proxy concepts are something we would describe in the (forthcoming!) "implementation guides"? rather than the spec itself.

[philips]

@jonboulle Yea, we need to start on that implementation guide.

[jonboulle]

@philips what do you want to do with this?

[philips]

I think we need to figure out some sort of mapping of host to discovery and support proxies. I don't know how this looks for the actool at the moment though.

[sgotti]

I like the idea of the map of a prefix to a host/port/scheme. This can be very useful for a local repo hosting a copy/modified images (for example a mirror of coreos.com/etcd) without changing the appname and also for testing purposes (to fake discovery in rkt tests needed for example by rkt/rkt#661).

[vbatts]

@philips @jonboulle where is this implementation guide being tracked?

[jonboulle]

@vbatts I added a first cut here - https://github.com/appc/spec/blob/master/GUIDE.md

[cgonyeo]

Someone wants to do image discovery over a non-default port in acpush. Ignoring proxies, just the ability to specify a port via flag to use during discovery would be useful, so any chances of reviving this?

[jonboulle]

@dgonyeo sure, go for it ;-)