Hi all, On Tue, 2024-02-27 at 19:00 +0900, Tristan van Berkom wrote: > On Fri, 2024-02-23 at 18:33 +0900, Tristan van Berkom wrote: > > I now have the following PRs up: > > Implement SourceMirror plugins: > https://github.com/apache/buildstream/pull/1890 > > DownloadableFileSource to support "auth-header-format" > https://github.com/apache/buildstream/pull/1895 >
I've added a few comments. They look reasonable to me overall. > In Jürg's previous reply on the last proposal, he asks: > > Could custom source mirror plugins also support URLs that are not > aliased? > > https://lists.apache.org/thread/y2hoobkxdh38wytcfhpgcy4c93r2hd6w > > Unfortunately not with the current implementation. > > [...] > > This would allow for projects to use custom authorization in the > default case, fetching and tracking from the default URLs. > > I feel like this could be extended in a subsequent addition, and does > not necessarily block these patches, but I defer to reviewers if they > have anything to add here. I agree that actual support for this could be done in the future. I've mentioned in a review comment a potential tweak to make this a bit easier. > Second... I'm not sure I'm completely satisfied with the currently > implemented API in #1895 (nor am I completely satisfied with the mock > http server I used to create the test case here). > > In upstream python, we can observe how "Basic Authentication" is done > with the AbstractBasicAuthHandler (and HTTPBasicAuthHandler which is > the concrete class): > > [...] > > There may be a purpose to all this extra code - if we were to be more > stringent here, we would probably want to implement our own > "HTTPBearerAuthHandler" in the DownloadableFileSource code, and have > it > run more stringent checks, rather than simply adding a header that > says: > > "Authentication: Bearer <token>" As I understand it, bearer token authorization is actually simpler and doesn't require the 401 retry logic that basic auth needs. So unless anyone speaks up with specific concerns, I think the current approach is sufficient and Bazel doing the same is also a good indication. If necessary, also parsing the supplied auth header does not seem completely unreasonable. Cheers, Jürg
