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

[Lennart Poettering]

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.

> +#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?

>  
> +        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. 
> +
> +                r = json_parse((const char *)j->payload, &doc);

Why the cast? void, and non-const automatically upgrade to const and
any type...

>  
> +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;

> +
> +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...

Lennart

-- 
Lennart Poettering, Red Hat