Hi,
Thanks for the reply Abderrahim :)
FWIW, the new PR (after the migration correction) is up at:
https://github.com/apache/buildstream/pull/1434
On Fri, 2021-02-05 at 21:13 +0100, Abderrahim Kitouni wrote:
> Hi Tristan,
[...]
> > Authentication
> > --------------
> > For all of the authentication related properties, `server-cert`,
> > `client-cert` and `client-key`, these have been split out into a
> > subdictionary named "auth" for any remote configuration.
> >
> > This may allow better extensibility for alternative authentication
> > methods in the future, however right now it serves us very well to be
> > able to document the "auth" dictionary in one central place in the
> > documentation.
>
> I think it's better to keep `server-cert` out of the sub-dictionary.
> The server certificate is needed when the server is using an untrusted
> certificate regardless of whether we want to authenticate with the
> server and regardless of the method we use to authenticate (currently
> client certificate, but we may want to support some kind of access
> token in the future like buildbox)
I understand that the YAML might be one dict less wordy in a lot of
cases where you only want to download, and I also take your point that
the server cert is not exactly for the purpose of "authenticating the
client".
The main motivations behind the `auth` dict are:
* We can clearly document the authentication related attributes in a
single location.
The key/cert exchange is a bit confusing in my opinion, I wanted
to rename them in terms of "pull" and "push" credentials so that
the user could be unburdened of the knowledge of how key exchanges
work, but that is also incorrect.
I think that the client/server cert/key is all clearly the same
topic / subject material, and as it is used in multiple places
(different kinds of remote asset services, remote execution
services), it makes good sense to have a central place to
document this.
* Extensibility.
Right now we only support http/https URIs, but this could
hypothetically be extended in the future.
By having an `auth` dict we can naturally add a `kind` field
to the dict and support different types of authentication in
the future in a nice clean component.
> > Artifact and Source cache configuration
> > ---------------------------------------
> > Projects are still allowed to provide recommendations for artifact and
> > source cache servers.
> >
> > User configuration now has the ability to override them, i.e. disregard
> > artifact and source cache servers declared in projects.
> >
> > Also, it is no longer possible to declare an artifact/source cache
> > server as a dictionary, it MUST be a list.
> >
> > This choice is simply because it the dict-or-list tactic here does not
> > buy us any convenience whatsoever, and clarity that it is in fact a
> > list of dictionaries is more worthwhile.
>
> I think this makes sense.
>
> > Project Configuration
> > ~~~~~~~~~~~~~~~~~~~~~
> >
> > #
> > # This is mostly unchanged, except for the `auth`
> > #
> > artifacts:
> > - url: https://pony.com:9999
> > type: both
> > push: false
> > instance-name: this-shard
> > auth:
> > server-cert: server.crt
>
> I assume `both` here means index+storage? (as in CAS + RA)
Yes, that is the current name (I didn't change this), although we could
rename this if you have a better name in mind (now is a good time...).
> > User Configuration
> > ~~~~~~~~~~~~~~~~~~
> > We can declare global artifact configuration, which either
> > overrides or augments project recommended cache servers.
> >
> > When "augmenting", the user configuration is still at a higher priority
> > than the project recommendations (as in: user configuration caches will
> > be consulted *first* when interacting with remotes).
>
> I think this makes a lot of sense, but is the reverse of what bst1
> does. I hope there won't be objections to changing it.
Indeed.
The point is that all of user configuration can override project
suggestions, which has always been the design, e.g. see:
https://docs.buildstream.build/master/arch_data_model.html#context
The cache service configuration went a bit off the reservation in this
regard so I want to bring it back in line :)
> > #
> > # Global artifact configuration
> > #
> > artifacts:
> >
> > #
> > # Here we decide whether user configuration overrides
> > # project recommendations.
> > #
> > override-project-caches: true
>
> This would be useful as a command line argument too.
Yup, that is already included in my follow up branch :)
https://github.com/apache/buildstream/pull/1438
Cheers,
-Tristan
