[systemd-devel] [PATCH 5/5] import/pull-dkr: V2 Image specification + manifest support

[Pavel Odvody]

On Fri, 2015-05-15 at 15:54 +0200, Lennart Poettering wrote:
> On Thu, 07.05.15 17:47, Pavel Odvody (podvody at redhat.com) wrote:
> 
> >  
> >  #define HEADER_TOKEN "X-Do" /* the HTTP header for the auth token */ "cker-Token:"
> > -#define HEADER_REGISTRY "X-Do" /*the HTTP header for the registry */ "cker-Endpoints:"
> > +#define HEADER_REGISTRY "X-Do" /* the HTTP header for the registry */ "cker-Endpoints:"
> > +#define HEADER_DIGEST "Do" /* the HTTP header for the manifest digest */ "cker-Content-Digest:"
> > +#define HEADER_USER_AGENT_V2 "User-Agent: do" /* otherwise we get
> > load-balanced(!) to a V1 registyry */ "cker/1.6.0"
> 
> I hope we can get rid of this. I'd prefer if we wouldn't have to 
> pretend to be other software.

As of today we can, kudos to Vincent for pushing this.

> 
> > +#define HEADER_BEARER_REALM "https://auth.doc" /* URL which we query for a bearer token */ "ker.io/token"
> > +#define HEADER_BEARER_SERVICE "registry.doc" /* the service we
> > query the token for */ "ker.io"
> 
> These two are problematic, we really shouldn't hardcode them. First of
> all users might want to use other servers here. More importantly
> though, to my knowledge the service agreements of the company
> providing those server does not allow usage like this. IANAL, but I
> really would prefer staying out of any legal issues here. We can make
> the defaults for this configurable via ./configure switches but we
> should not carry these URLs by default. If downstream distros are
> willing to take the legal risk of encoding the URLs by default then,
> they are welcome to via the configure switches, but upstream I really
> don't want to see that...
> 
> Also, can you quickly explain how the three servers relate to each
> other? Is this documented anywhere?
> 

I'm currently in the process of rewriting these to use the same domain
name as is passed in --dkr-index-url, so they'll look like:
${PROTOCOL}auth.${ADDRESS}/[v2]/token
and registry.${ADDRESS} respectively.

These are somehow documented here:
https://docs.docker.com/registry/spec/auth/token/

> >  
> > +        if (strategy == PULL_V2) {
> > +                char **k = NULL;
> > +                STRV_FOREACH(k, i->ancestry) {
> > +                        char *d = strjoina(i->image_root, "/.dkr-",
> > *k, NULL);
> 
> Using alloca() (which strjoina() uses internally) within loops is not
> OK, since deallacation happens at the end of the function call, not at
> the end of the {} block. 
Oh, didn't know that, will change.

> > +
> > +                r = json_parse((const char *)j->payload, &doc);
> 
> Why the cast? void, and non-const automatically upgrade to const and
> any type...

I think that j->payload is uint8_t* and the function expects const
char*, what do you thing about changing the signature of json_parse to

int json_parse(const void* payload, size_t size, json_variant **rv);

?

> 
> >  
> > +enum PullStrategy { PULL_V1, PULL_V2 };
> 
> So far we typedef'ed enums for exported types, and prefixed both with
> a namespacing prefix. Also, why call this a "strategy"? Does dkr
> upstream call it that way? Shouldn't this be "protocol" or so?
> 
> typedef enum DkrProtocolVersion {
>        DKR_PROTOCOL_V1,
>        DKR_PROTOCOL_V2,
> } DkrProtocolVersion;

Nope, entirely my choice of a name. Protocol sounds more like switching
between HTTP/S, FTP or something, while the underlying network protocols
has stayed the same, only the "strategy" how the image is pulled has
changed. But I have no problem with changing that.

> 
> > +
> > +typedef struct DkrSignature {
> > +        char *curve;
> > +
> > +        char *key_id;
> > +        char *key_type;
> > +
> > +        char *x;
> > +        char *y;
> > +
> > +        char *algorithm;
> > +
> > +        char *signature;
> > +        char *protected;
> > +} DkrSignature;
> 
> This is not used so far, is it?
> 
> > +
> > +//typedef struct DkrHistory {
> > +//      char *image_id;
> > +//      char *parent_id;
> > +//} DkrHistory;
> 
> 
> And neither is this? Also, we don't use C++ comments in our code,
> i.e. /* this */ is preferred over // this...

Yep, I'll get rid of these.

> 
> Lennart
> 


-- 
Pavel Odvody <podvody at redhat.com>
Software Engineer - EMEA ENG Developer Experience
5EC1 95C1 8E08 5BD9 9BBF 9241 3AFA 3A66 024F F68D
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150515/dfef4cb7/attachment.sig>