Skip to content

oci_discovery/uri_template: Stub for URI Template expansion#7

Merged
xiekeyang merged 1 commit intoxiekeyang:masterfrom
wking:python-uri-templates
Sep 11, 2017
Merged

oci_discovery/uri_template: Stub for URI Template expansion#7
xiekeyang merged 1 commit intoxiekeyang:masterfrom
wking:python-uri-templates

Conversation

@wking
Copy link
Copy Markdown
Contributor

@wking wking commented Sep 11, 2017

Python's .format() can handle things like {host}, but there's a lot more than that in RFC 6570. Instead of rolling our own implementation, punt to the uritemplate package. The license is:

BSD-3-Clause OR Apache-2.0

which should be compatible with the OCI's Apache-2.0 requirement, even if that requirement applies to third-party dependencies, which is not clear to me.

Python's .format can handle things like {host}, but there's a lot more
than that in RFC 6570.  Instead of rolling our own implementation,
punt to the uritemplate package [1].  The license is [1,2]:

   BSD-3-Clause OR Apache-2.0

which should be compatible with the OCI's Apache-2.0 requirement [3],
even if that requirement applies to third-party dependencies, which is
not clear to me.

[1]: https://pypi.python.org/pypi/uritemplate
[2]: https://github.com/python-hyper/uritemplate/blob/3.0.0/LICENSE
[3]: https://www.opencontainers.org/about/governance
     Section 8.a
@wking wking force-pushed the python-uri-templates branch from 47093c9 to 06e2715 Compare September 11, 2017 07:25
@xiekeyang
Copy link
Copy Markdown
Owner

@wking , It seems this restriction will reject hostname of {IP}:{PORT}, which is too generally used to build local server. How can I discovery image when I set nginx server localhost:8080? I think in this stage we needn't set too much restriction.

And, the OCI index fetching workflow prompts log:

DEBUG:oci_discovery.ref_engine.oci_index_template:fetching an OCI index for 127.0.0.1:8080/app#1.0 from https://127.0.0.1:8080/oci-index/app
WARNING:oci_discovery.ref_engine_discovery:<urlopen error [SSL: UNKNOWN_PROTOCOL] unknown protocol (_ssl.c:645)>
DEBUG:oci_discovery.ref_engine_discovery:discovering ref engines via http://0.0.1:8080/.well-known/oci-host-ref-engines
WARNING:oci_discovery.ref_engine_discovery:failed to fetch http://0.0.1:8080/.well-known/oci-host-ref-engines (HTTP Error 403: Proxy_WarningPage)
DEBUG:oci_discovery.ref_engine_discovery:discovering ref engines via http://0.1:8080/.well-known/oci-host-ref-engines
WARNING:oci_discovery.ref_engine_discovery:failed to fetch http://0.1:8080/.well-known/oci-host-ref-engines (HTTP Error 403: Proxy_WarningPage)
ERROR:root:no Merkle root found for '127.0.0.1:8080/app#1.0'
{}

Actually I can fetch the index:

$ curl http://127.0.0.1:8080/oci-index/app
{
  "schemaVersion": 2,
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "size": 799,
      "digest": "sha256:e9770a03fbdccdd4632895151a93f9af58bbe2c91fdfaaf73160648d250e6ec3",
      "platform": {
        "architecture": "ppc64le",
        "os": "linux"
      },
      "annotations": {
        "org.opencontainers.image.ref.name": "1.0"
      },
      "casEngines": [
        {
          "protocol": "oci-cas-template-v1",
          "uri": "https://example.com/oci-cas/{algorithm}/{encoded:2}/{encoded}"
        }
      ]
    }
  ]
}

You are split the URI by . one by one. I know you want to resolve the template a.b.c.example.com. But I think it is some complex and not fit to IP address. Could we just check uri once in argument, and postpone the advanced resolution to later?

Now I'm debugging the workflow in README, and is confused a little by some internal mechanism.

@wking
Copy link
Copy Markdown
Contributor Author

wking commented Sep 11, 2017

It seems this restriction will reject hostname of {IP}:{PORT}, which is too generally used to build local server.

I've spun that off into #9, since it relates to the host-based image name spec, while this PR is just about the oci-index-template-v1 implementation.

How can I discovery image when I set nginx server localhost:8080?

I floated some ideas in this comment, although none of them are trivial. However, the _IP_V4_REGEXP patch I suggest there (as a local-testing workaround) is only touching one line, so while it's unfortunate to have to touch code at all for your local testing, it's a pretty small touch.

@wking
Copy link
Copy Markdown
Contributor Author

wking commented Sep 11, 2017

You are split the URI by . one by one. I know you want to resolve the template a.b.c.example.com. But I think it is some complex and not fit to IP address.

It is guarded against IP addresses (here). It's just not guarded against IPv4-address-based-authorities (without the _IP_V4_REGEXP patch suggested here). But since the host-based image name spec requires hosts and not authoritys, that's not a problem with the ancestor walker; it's a problem with the overly-liberal host-based image name parsing. So I'd rather not commit that _IP_V4_REGEXP change to master.

I think we want to keep ancestor walking to preserve the similar abd functionality.

@xiekeyang
Copy link
Copy Markdown
Owner

LGTM

@xiekeyang xiekeyang merged commit 8be7677 into xiekeyang:master Sep 11, 2017
@wking wking deleted the python-uri-templates branch September 11, 2017 10:41
wking added a commit to wking/oci-discovery that referenced this pull request Sep 20, 2017
This snuck in with 06e2715 (oci_discovery/uri_template: Stub for URI
Template expansion, 2017-09-11, xiekeyang#7).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants