Repository: osbuild/osbuild
Pull request #1573 (2024-02-06 17:29:14)

sources(curl): download multiple URLs with the same curl command
This PR is based on the excellent work of @supakeen in https://github.com/osbuild/osbuild/pull/1549 and the findings in https://github.com/osbuild/osbuild/pull/1023

Simon found that curl has a lot of fork/exec overhead in the way we use it. The other issue with our current use of curl is that we do not reuse connections and the overhead of the connection setup (especially for https) is there. 

Newer versions of curl (7.68+) have a nice new feature called `--parallel` that will reuse connections and download 50 streams in parallel by default (this can be controlled via the --parallel-max option). Since we want some more features (like --write-out %{json}) we actually need curl 7.75+ to use --parallel in a nice way. But that is okay because RHEL9/Centos9 have 7.76 as their default (but of course I can bring it back if people feel strongly about this).

Note that this PR originally also parallelized the old forking() curl but I removed this because it seems like a lot of extra work just for RHEL8 which is (AIUI) in maintenance mode.

The numbers I see are relatively modest for me though, for me it's mostly I/O bound as my connections is not that fast maybe someone with a fast connection can also test this? I ran:
````
$ rm -rf .osbuild/ ; time python3 -m osbuild  --libdir . ./test/data/manifests/fedora-container.json 
...
real	0m28.433s
user	0m6.179s
sys	0m4.821s


[upstream/main at v117-13-gaf0e8490]
$ rm -rf .osbuild/ ; time python3 -m osbuild  --libdir . ./test/data/manifests/fedora-container.json 
...
real	0m32.849s
user	0m17.607s
sys	0m6.236s
`` 
and see broadly the same total time except that the "user" time is significantly lower.

[Build on top of https://github.com/osbuild/osbuild/pull/1767 - once that is merged this one will be a lot shorter]

supakeen: Even modest improvement is improvement. Can you rebase this and fix `tox.ini` so I can approve? :)
github-actions[bot]: This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.
bcl: Remove the stale label for now, I still think this is a good idea. When there is time :)
supakeen: I feel like; while it is useful for its performance improvements over the current curl stage, it seems to make the curl stage a lot more complex with all the setup, configuration files, and all these things as compared to an approach that doesn't go through a curl binary.

Would it perhaps be a good idea to instead use `libcurl` through `pycurl`, this is likely shipped in RHEL and gets rid of the subprocesses.
supakeen: Sources run outside the build root if I recall correctly. You are right that this would apply cleanly to old manifests as it improves the existing curl source while only new manifests could use differently named sources.
ondrejbudai: Interesting, `httpx` is comparable to `curl` on my setup. :thinking: I think I'm still in favor of this PR, it will provide a very interesting self-contained speed improvement. Alternatively, we can focus on finishing https://github.com/osbuild/osbuild/pull/1304 which will bring support for handling mirror fallbacks properly.

## old curl
7.72user 3.37system 1:26.58elapsed 12%CPU (0avgtext+0avgdata 30384maxresident)k
344inputs+0outputs (31major+542192minor)pagefaults 0swaps

## new curl
1.68user 1.38system 0:44.46elapsed 6%CPU (0avgtext+0avgdata 30780maxresident)k
0inputs+0outputs (0major+12908minor)pagefaults 0swaps

## new curl with tweaks (parallel-immediate)
1.97user 1.51system 0:47.14elapsed 7%CPU (0avgtext+0avgdata 38396maxresident)k
0inputs+0outputs (0major+17992minor)pagefaults 0swaps

## httpx
8.82user 1.93system 0:51.83elapsed 20%CPU (0avgtext+0avgdata 82976maxresident)k
32inputs+1264outputs (0major+24259minor)pagefaults 0swaps

supakeen: > Interesting, `httpx` is comparable to `curl` on my setup. 🤔 I think I'm still in favor of this PR, it will provide a very interesting self-contained speed improvement. Alternatively, we can focus on finishing #1304 which will bring support for handling mirror fallbacks properly.
> ## old curl
> 
> 7.72user 3.37system 1:26.58elapsed 12%CPU (0avgtext+0avgdata 30384maxresident)k 344inputs+0outputs (31major+542192minor)pagefaults 0swaps
> ## new curl
> 
> 1.68user 1.38system 0:44.46elapsed 6%CPU (0avgtext+0avgdata 30780maxresident)k 0inputs+0outputs (0major+12908minor)pagefaults 0swaps
> ## new curl with tweaks (parallel-immediate)
> 
> 1.97user 1.51system 0:47.14elapsed 7%CPU (0avgtext+0avgdata 38396maxresident)k 0inputs+0outputs (0major+17992minor)pagefaults 0swaps
> ## httpx
> 
> 8.82user 1.93system 0:51.83elapsed 20%CPU (0avgtext+0avgdata 82976maxresident)k 32inputs+1264outputs (0major+24259minor)pagefaults 0swaps

Is this on the same manifest? I'd assume the percentage overhead of what needs to be done for curl to be larger the faster the pipe becomes.

And yes, this PR is still a nice speed up.
ondrejbudai: > Is this on the same manifest? I'd assume the percentage overhead of what needs to be done for curl to be larger the faster the pipe becomes.

I'm not sure which manifest you used. I used the fedora-boot one. Not sure if that matters a lot, it's good to have multiple data points I think. :)
supakeen: > > Is this on the same manifest? I'd assume the percentage overhead of what needs to be done for curl to be larger the faster the pipe becomes.
> 
> I'm not sure which manifest you used. I used the fedora-boot one. Not sure if that matters a lot, it's good to have multiple data points I think. :)

fedora-boot :)
achilleas-k: > They will only pull in curl (the binary) in the buildroot(AIUI?)

Sources run on the host before osbuild sets up a buildroot or bubblewrap containers.
Anything used by a source becomes a dependency of osbuild.
supakeen: What if we add the ability for some pipelines to do networking so we can have a conceptual `netroot` as well ;)

/me runs away.
github-actions[bot]: This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.
mvo5: Had to do another rebase/conflict resolution because of https://github.com/osbuild/osbuild/pull/1809 
ondrejbudai: There's a fixup! commit in the PR, otherwise LGTM.
mvo5: > There's a fixup! commit in the PR, otherwise LGTM.

Sorry for that! I removed that one now and it should be good again.

Patch:
diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl
index 05ee0781..e6f016f7 100755
--- a/sources/org.osbuild.curl
+++ b/sources/org.osbuild.curl
@@ -19,13 +19,17 @@ up the download.
 """
 
 import concurrent.futures
+import contextlib
 import os
+import pathlib
 import platform
+import re
 import subprocess
 import sys
 import tempfile
+import textwrap
 import urllib.parse
-from typing import Dict
+from typing import Dict, List, Tuple
 
 from osbuild import sources
 from osbuild.util.checksum import verify_file
@@ -93,6 +97,156 @@ SCHEMA = """
 """
 
 
+def curl_has_parallel_downloads():
+    """
+    Return true if curl has all the support we needed for parallel downloading
+    (this include --write-out "%{json}" too
+    """
+    output = subprocess.check_output(["curl", "--version"], universal_newlines=True)
+    first_line = output.split("\n", maxsplit=1)[0]
+    m = re.match(r'^curl (\d+\.\d+\.\d+)', first_line)
+    if not m:
+        print(f"WARNING: cannot parse curl version from '{first_line}'", file=sys.stderr)
+        return False
+    major, minor, _ = m.group(1).split(".")
+    if int(major) > 7:
+        return True
+    # --parallel got added in 7.68
+    # --write-out "%{exitcode} is 7.75
+    if int(major) == 7 and int(minor) >= 75:
+        return True
+    return False
+
+
+def _quote_url(url: str) -> str:
+    purl = urllib.parse.urlparse(url)
+    path = urllib.parse.quote(purl.path)
+    quoted = purl._replace(path=path)
+    return quoted.geturl()
+
+
+def gen_curl_download_config(config_path: pathlib.Path, chksum_desc_tuple: List[Tuple[str, Dict]]):
+    with open(config_path, "w", encoding="utf8") as fp:
+        # global options
+        fp.write(textwrap.dedent(f"""\
+        user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
+        silent
+        speed-limit = 1000
+        connect-timeout = 30
+        fail
+        location
+        """))
+        proxy = os.getenv("OSBUILD_SOURCES_CURL_PROXY")
+        if proxy:
+            fp.write(f'proxy = "{proxy}"\n')
+        fp.write("\n")
+        # per url options
+        for checksum, desc in chksum_desc_tuple:
+            url = _quote_url(desc.get("url"))
+            fp.write(f'url = "{url}"\n')
+            fp.write(f'output = "{checksum}"\n')
+            secrets = desc.get("secrets")
+            if secrets:
+                ssl_ca_cert = secrets.get('ssl_ca_cert')
+                if ssl_ca_cert:
+                    fp.write(f'cacert = "{ssl_ca_cert}"\n')
+                ssl_client_cert = secrets.get('ssl_client_cert')
+                if ssl_client_cert:
+                    fp.write(f'cert = "{ssl_client_cert}"\n')
+                ssl_client_key = secrets.get('ssl_client_key')
+                if ssl_client_key:
+                    fp.write(f'key = "{ssl_client_key}"\n')
+            insecure = desc.get("insecure")
+            if insecure:
+                fp.write('insecure\n')
+            else:
+                fp.write('no-insecure\n')
+            fp.write("\n")
+
+
+def try_parse_curl_line(line):
+    line = line.strip()
+    print(line)
+    if not line.startswith("osbuild-dl\x1c"):
+        print(f"WARNING: unexpected prefix in {line}", file=sys.stderr)
+        return None
+    _, url, filename, exitcode, errormsg = line.split("\x1c")
+    return {
+        "url": url,
+        "filename_effective": filename,
+        "exitcode": int(exitcode),
+        "errormsg": errormsg,
+    }
+
+
+def validate_and_move_to_targetdir(tmpdir, targetdir, checksum, origin):
+    """
+    Validate that the checksum of the file with the filename
+    "checksum" in tmpdir matches and move into target dir. The
+    "origin" parameter is purely information to generate better
+    errors.
+    """
+    if not verify_file(f"{tmpdir}/{checksum}", checksum):
+        raise RuntimeError(f"checksum mismatch: {checksum} {origin}")
+    # The checksum has been verified, move the file into place. in case we race
+    # another download of the same file, we simply ignore the error as their
+    # contents are guaranteed to be  the same.
+    with contextlib.suppress(FileExistsError):
+        os.rename(f"{tmpdir}/{checksum}", f"{targetdir}/{checksum}")
+
+
+def fetch_many_new_curl(tmpdir, targetdir, dl_pairs):
+    curl_config_path = f"{tmpdir}/curl-config.txt"
+    gen_curl_download_config(curl_config_path, dl_pairs)
+    curl_command = [
+        "curl",
+        "--config", curl_config_path,
+        # this adds a bunch of noise but might be nice for debug?
+        # "--show-error",
+        "--parallel",
+        # this will write out a "record" for each finished download
+        # Not using %{json} here because older curl (7.76) will write
+        # {"http_connect":000} which python cannot parse
+        "--write-out", "osbuild-dl\x1c%{url}\x1c%{filename_effective}\x1c%{exitcode}\x1cerror: %{errormsg}\n",
+    ]
+    with contextlib.ExitStack() as cm:
+        curl_p = subprocess.Popen(curl_command, encoding="utf-8", cwd=tmpdir, stdout=subprocess.PIPE)
+        # ensure that curl is killed even if an unexpected exit happens
+        cm.callback(curl_p.kill)
+        while True:
+            line = curl_p.stdout.readline()
+            # empty line means eof/process finished
+            if line == "":
+                break
+            dl_details = try_parse_curl_line(line)
+            if not dl_details:
+                continue
+            url = dl_details['url']
+            # ignore individual download errors, the overall exit status will
+            # reflect them and the caller can retry
+            if dl_details["exitcode"] != 0:
+                print(f"WARNING: failed to download {url}: {dl_details['errormsg']}", file=sys.stderr)
+                continue
+            # the way downloads are setup the filename is the expected hash
+            # so validate now and move into place
+            checksum = dl_details["filename_effective"]
+            validate_and_move_to_targetdir(tmpdir, targetdir, checksum, url)
+            # remove item from download list
+            for todo_chksum, desc in dl_pairs[:]:
+                if todo_chksum == checksum:
+                    dl_pairs.remove((checksum, desc))
+            # Workaround the lack of structured progress reporting from
+            # stages/sources. It generates messages of the form
+            #   "message": "source/org.osbuild.curl (org.osbuild.curl): Downloaded https://rpmrepo.osbuild.org/v2/mirror/public/f38/f38-x86_64-fedora-20230413/Packages/f/fonts-srpm-macros-2.0.5-11.fc38.noarch.rpm\n
+            #
+            # Without it just a long pause with no progress while curl
+            # downloads.
+            print(f"Downloaded {url}")
+        # return overall download status (this will be an error if any
+        # transfer failed)
+        return curl_p.wait()
+
+
 class CurlSource(sources.SourceService):
 
     content_type = "org.osbuild.files"
@@ -102,6 +256,7 @@ class CurlSource(sources.SourceService):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self.subscriptions = None
+        self._curl_has_parallel_downloads = curl_has_parallel_downloads()
 
     def amend_secrets(self, checksum, desc_or_url):
         if not isinstance(desc_or_url, dict):
@@ -128,77 +283,60 @@ class CurlSource(sources.SourceService):
 
         return checksum, desc
 
-    @staticmethod
-    def _quote_url(url: str) -> str:
-        purl = urllib.parse.urlparse(url)
-        path = urllib.parse.quote(purl.path)
-        quoted = purl._replace(path=path)
-        return quoted.geturl()
-
     def fetch_all(self, items: Dict) -> None:
         filtered = filter(lambda i: not self.exists(i[0], i[1]), items.items())  # discards items already in cache
         amended = map(lambda i: self.amend_secrets(i[0], i[1]), filtered)
 
+        if self._curl_has_parallel_downloads:
+            self._fetch_all_new_curl(amended)
+        else:
+            self._fetch_all_old_curl(amended)
+
+    def _fetch_all_new_curl(self, dl_pairs):
+        dl_pairs = list(dl_pairs)
+        if len(dl_pairs) == 0:
+            return
+
+        # Download to a temporary sub cache until we have verified the checksum. Use a
+        # subdirectory, so we avoid copying across block devices.
+        with tempfile.TemporaryDirectory(prefix="osbuild-unverified-file-", dir=self.cache) as tmpdir:
+            # some mirrors are sometimes broken. retry manually, because we could be
+            # redirected to a different, working, one on retry.
+            return_code = 0
+            for _ in range(10):
+                return_code = fetch_many_new_curl(tmpdir, self.cache, dl_pairs)
+                if return_code == 0:
+                    break
+            else:
+                failed_urls = ",".join([itm[1]["url"] for itm in dl_pairs])
+                raise RuntimeError(f"curl: error downloading {failed_urls}: error code {return_code}")
+
+    def _fetch_all_old_curl(self, amended):
         with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
             for _ in executor.map(self.fetch_one, *zip(*amended)):
                 pass
 
     def fetch_one(self, checksum, desc):
-        secrets = desc.get("secrets")
-        insecure = desc.get("insecure")
-        url = self._quote_url(desc.get("url"))
-        proxy = os.getenv("OSBUILD_SOURCES_CURL_PROXY")
+        url = _quote_url(desc.get("url"))
         # Download to a temporary sub cache until we have verified the checksum. Use a
         # subdirectory, so we avoid copying across block devices.
         with tempfile.TemporaryDirectory(prefix="osbuild-unverified-file-", dir=self.cache) as tmpdir:
             # some mirrors are sometimes broken. retry manually, because we could be
             # redirected to a different, working, one on retry.
             return_code = 0
-            arch = platform.machine()
             for _ in range(10):
-                curl_command = [
-                    "curl",
-                    "--silent",
-                    "--speed-limit", "1000",
-                    "--connect-timeout", "30",
-                    "--fail",
-                    "--location",
-                    "--user-agent", f"osbuild (Linux.{arch}; https://osbuild.org/)",
-                    "--output", checksum,
-                ]
-                if proxy:
-                    curl_command.extend(["--proxy", proxy])
-                if secrets:
-                    if secrets.get('ssl_ca_cert'):
-                        curl_command.extend(["--cacert", secrets.get('ssl_ca_cert')])
-                    if secrets.get('ssl_client_cert'):
-                        curl_command.extend(["--cert", secrets.get('ssl_client_cert')])
-                    if secrets.get('ssl_client_key'):
-                        curl_command.extend(["--key", secrets.get('ssl_client_key')])
-
-                if insecure:
-                    curl_command.append("--insecure")
-
-                # url must follow options
-                curl_command.append(url)
-
-                curl = subprocess.run(curl_command, encoding="utf-8", cwd=tmpdir, check=False)
+                curl_config_path = f"{tmpdir}/curl-config.txt"
+                gen_curl_download_config(curl_config_path, [(checksum, desc)])
+                curl = subprocess.run(
+                    ["curl", "--config", curl_config_path],
+                    encoding="utf-8", cwd=tmpdir, check=False)
                 return_code = curl.returncode
                 if return_code == 0:
                     break
             else:
                 raise RuntimeError(f"curl: error downloading {url}: error code {return_code}")
 
-            if not verify_file(f"{tmpdir}/{checksum}", checksum):
-                raise RuntimeError(f"checksum mismatch: {checksum} {url}")
-
-            # The checksum has been verified, move the file into place. in case we race
-            # another download of the same file, we simply ignore the error as their
-            # contents are guaranteed to be  the same.
-            try:
-                os.rename(f"{tmpdir}/{checksum}", f"{self.cache}/{checksum}")
-            except FileExistsError:
-                pass
+            validate_and_move_to_targetdir(tmpdir, self.cache, checksum, url)
             # Workaround the lack of structured progress reporting from
             # stages/sources. It generates messages of the form
             #   "message": "source/org.osbuild.curl (org.osbuild.curl): Downloaded https://rpmrepo.osbuild.org/v2/mirror/public/f38/f38-x86_64-fedora-20230413/Packages/f/fonts-srpm-macros-2.0.5-11.fc38.noarch.rpm\n


Test patch:
diff --git a/osbuild/testutil/net.py b/osbuild/testutil/net.py
index 6dc8acca..092b0965 100644
--- a/osbuild/testutil/net.py
+++ b/osbuild/testutil/net.py
@@ -34,6 +34,14 @@ class SilentHTTPRequestHandler(http.server.SimpleHTTPRequestHandler):
     def log_message(self, *args, **kwargs):
         pass
 
+    def do_GET(self):
+        # silence errors when the other side "hangs up" unexpectedly
+        # (our tests will do that when downloading in parallel)
+        try:
+            super().do_GET()
+        except (ConnectionResetError, BrokenPipeError):
+            pass
+
 
 class DirHTTPServer(ThreadingHTTPServer):
     def __init__(self, *args, directory=None, simulate_failures=0, **kwargs):
diff --git a/sources/test/test_curl_source.py b/sources/test/test_curl_source.py
index 9611df7f..0daea2c1 100644
--- a/sources/test/test_curl_source.py
+++ b/sources/test/test_curl_source.py
@@ -1,10 +1,10 @@
 #!/usr/bin/python3
 
 import hashlib
-import pathlib
+import platform
 import re
 import shutil
-import subprocess
+import textwrap
 from unittest.mock import patch
 
 import pytest
@@ -82,14 +82,24 @@ def test_curl_source_amend_secrets_subscription_mgr(sources_service):
     assert desc["secrets"] == "secret-for-http://localhost:80/a"
 
 
-def test_curl_download_many_fail(sources_service):
+@pytest.fixture(name="curl_parallel")
+def curl_parallel_fixture(sources_module, sources_service, request):
+    use_parallel = request.param
+    if use_parallel and not sources_module.curl_has_parallel_downloads:
+        pytest.skip("system curl does not support parallel downloads")
+    sources_service._curl_has_parallel_downloads = use_parallel  # pylint: disable=protected-access
+    yield sources_service
+
+
+@pytest.mark.parametrize("curl_parallel", [True, False], indirect=["curl_parallel"])
+def test_curl_download_many_fail(curl_parallel):
     TEST_SOURCES = {
         "sha:1111111111111111111111111111111111111111111111111111111111111111": {
             "url": "http://localhost:9876/random-not-exists",
         },
     }
     with pytest.raises(RuntimeError) as exp:
-        sources_service.fetch_all(TEST_SOURCES)
+        curl_parallel.fetch_all(TEST_SOURCES)
     assert str(exp.value) == 'curl: error downloading http://localhost:9876/random-not-exists: error code 7'
 
 
@@ -114,24 +124,26 @@ def make_test_sources(fake_httpd_root, port, n_files):
     return sources
 
 
-def test_curl_download_many_with_retry(tmp_path, sources_service):
+@pytest.mark.parametrize("curl_parallel", [True, False], indirect=["curl_parallel"])
+def test_curl_download_many_with_retry(tmp_path, curl_parallel):
     fake_httpd_root = tmp_path / "fake-httpd-root"
 
     simulate_failures = 2
     with http_serve_directory(fake_httpd_root, simulate_failures=simulate_failures) as httpd:
         test_sources = make_test_sources(fake_httpd_root, httpd.server_port, 5)
 
-        sources_service.cache = tmp_path / "curl-download-dir"
-        sources_service.cache.mkdir()
-        sources_service.fetch_all(test_sources)
+        curl_parallel.cache = tmp_path / "curl-download-dir"
+        curl_parallel.cache.mkdir()
+        curl_parallel.fetch_all(test_sources)
         # we simulated N failures and we need to fetch K files
         assert httpd.reqs.count == simulate_failures + len(test_sources)
     # double downloads happend in the expected format
     for chksum in test_sources:
-        assert (sources_service.cache / chksum).exists()
+        assert (curl_parallel.cache / chksum).exists()
 
 
-def test_curl_download_many_chksum_validate(tmp_path, sources_service):
+@pytest.mark.parametrize("curl_parallel", [True, False], indirect=["curl_parallel"])
+def test_curl_download_many_chksum_validate(tmp_path, curl_parallel):
     fake_httpd_root = tmp_path / "fake-httpd-root"
 
     with http_serve_directory(fake_httpd_root) as httpd:
@@ -140,14 +152,15 @@ def test_curl_download_many_chksum_validate(tmp_path, sources_service):
         # match the checksum
         (fake_httpd_root / "1").write_text("hash-no-longer-matches", encoding="utf8")
 
-        sources_service.cache = tmp_path / "curl-download-dir"
-        sources_service.cache.mkdir()
+        curl_parallel.cache = tmp_path / "curl-download-dir"
+        curl_parallel.cache.mkdir()
         with pytest.raises(RuntimeError) as exp:
-            sources_service.fetch_all(test_sources)
+            curl_parallel.fetch_all(test_sources)
         assert re.search(r"checksum mismatch: sha256:.* http://localhost:.*/1", str(exp.value))
 
 
-def test_curl_download_many_retries(tmp_path, sources_service):
+@pytest.mark.parametrize("curl_parallel", [True, False], indirect=["curl_parallel"])
+def test_curl_download_many_retries(tmp_path, curl_parallel):
     fake_httpd_root = tmp_path / "fake-httpd-root"
 
     with http_serve_directory(fake_httpd_root) as httpd:
@@ -155,73 +168,132 @@ def test_curl_download_many_retries(tmp_path, sources_service):
         # remove all the sources
         shutil.rmtree(fake_httpd_root)
 
-        sources_service.cache = tmp_path / "curl-download-dir"
-        sources_service.cache.mkdir()
+        curl_parallel.cache = tmp_path / "curl-download-dir"
+        curl_parallel.cache.mkdir()
         with pytest.raises(RuntimeError) as exp:
-            sources_service.fetch_all(test_sources)
+            curl_parallel.fetch_all(test_sources)
         # curl will retry 10 times
         assert httpd.reqs.count == 10 * len(test_sources)
         assert "curl: error downloading http://localhost:" in str(exp.value)
 
 
-class FakeCurlDownloader:
-    """FakeCurlDownloader fakes what curl does
-
-    This is useful when mocking subprocess.run() to see that curl gets
-    the right arguments. It requires test sources where the filename
-    matches the content of the file (e.g. filename "a", content must be "a"
-    as well) so that it can generate the right hash.
-    """
-
-    def __init__(self, test_sources):
-        self._test_sources = test_sources
+def test_curl_user_agent(tmp_path, sources_module):
+    config_path = tmp_path / "curl-config.txt"
+    test_sources = make_test_sources(tmp_path, 80, 2)
 
-    def faked_run(self, *args, **kwargs):
-        download_dir = pathlib.Path(kwargs["cwd"])
-        for chksum, desc in self._test_sources.items():
-            # The filename of our test files matches their content for
-            # easier testing/hashing. Alternatively we could just pass
-            # a src dir in here and copy the files from src to
-            # download_dir here but that would require that the files
-            # always exist in the source dir (which they do right now).
-            content = desc["url"].rsplit("/", 1)[1]
-            (download_dir / chksum).write_text(content, encoding="utf8")
-        return subprocess.CompletedProcess(args, 0)
+    sources_module.gen_curl_download_config(config_path, test_sources.items())
+    assert config_path.exists()
+    assert 'user-agent = "osbuild (Linux.x86_64; https://osbuild.org/)"' in config_path.read_text()
 
 
 @pytest.mark.parametrize("with_proxy", [True, False])
-@patch("subprocess.run")
-def test_curl_download_proxy(mocked_run, tmp_path, monkeypatch, sources_service, with_proxy):
+def test_curl_download_proxy(tmp_path, monkeypatch, sources_module, with_proxy):
+    config_path = tmp_path / "curl-config.txt"
     test_sources = make_test_sources(tmp_path, 80, 2)
-    fake_curl_downloader = FakeCurlDownloader(test_sources)
-    mocked_run.side_effect = fake_curl_downloader.faked_run
 
     if with_proxy:
         monkeypatch.setenv("OSBUILD_SOURCES_CURL_PROXY", "http://my-proxy")
-    sources_service.cache = tmp_path / "curl-cache"
-    sources_service.cache.mkdir()
-    sources_service.fetch_all(test_sources)
-    for call_args in mocked_run.call_args_list:
-        args, _kwargs = call_args
-        if with_proxy:
-            idx = args[0].index("--proxy")
-            assert args[0][idx:idx + 2] == ["--proxy", "http://my-proxy"]
-        else:
-            assert "--proxy" not in args[0]
-
-
-@patch("subprocess.run")
-def test_curl_user_agent(mocked_run, tmp_path, sources_service):
-    test_sources = make_test_sources(tmp_path, 80, 2,)
-    fake_curl_downloader = FakeCurlDownloader(test_sources)
-    mocked_run.side_effect = fake_curl_downloader.faked_run
-
-    sources_service.cache = tmp_path / "curl-cache"
-    sources_service.cache.mkdir()
-    sources_service.fetch_all(test_sources)
-
-    for call_args in mocked_run.call_args_list:
-        args, _kwargs = call_args
-        idx = args[0].index("--user-agent")
-        assert "osbuild" in args[0][idx + 1]
-        assert "https://osbuild.org/" in args[0][idx + 1]
+    sources_module.gen_curl_download_config(config_path, test_sources.items())
+    assert config_path.exists()
+    if with_proxy:
+        assert 'proxy = "http://my-proxy"\n' in config_path.read_text()
+    else:
+        assert "proxy" not in config_path.read_text()
+
+
+TEST_SOURCE_PAIRS_GEN_DOWNLOAD_CONFIG = [
+    (
+        # sha256("0")
+        "sha256:5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9",
+        {
+            "url": "http://example.com/file/0",
+        },
+    ), (
+        # sha256("1")
+        "sha256:6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b",
+        {
+            "url": "http://example.com/file/1",
+            "insecure": True,
+        },
+    ), (
+        # sha256("2")
+        "sha256:d4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35",
+        {
+            "url": "http://example.com/file/2",
+            "secrets": {
+                "ssl_ca_cert": "some-ssl_ca_cert",
+            },
+        },
+    ), (
+        # sha256("3")
+        "sha256:4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce",
+        {
+            "url": "http://example.com/file/3",
+            "secrets": {
+                "ssl_client_cert": "some-ssl_client_cert",
+                "ssl_client_key": "some-ssl_client_key",
+            },
+        },
+    ),
+]
+
+
+def test_curl_gen_download_config(tmp_path, sources_module):
+    config_path = tmp_path / "curl-config.txt"
+    sources_module.gen_curl_download_config(config_path, TEST_SOURCE_PAIRS_GEN_DOWNLOAD_CONFIG)
+
+    assert config_path.exists()
+    assert config_path.read_text(encoding="utf8") == textwrap.dedent(f"""\
+    user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
+    silent
+    speed-limit = 1000
+    connect-timeout = 30
+    fail
+    location
+
+    url = "http://example.com/file/0"
+    output = "sha256:5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9"
+    no-insecure
+
+    url = "http://example.com/file/1"
+    output = "sha256:6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b"
+    insecure
+
+    url = "http://example.com/file/2"
+    output = "sha256:d4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35"
+    cacert = "some-ssl_ca_cert"
+    no-insecure
+
+    url = "http://example.com/file/3"
+    output = "sha256:4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce"
+    cert = "some-ssl_client_cert"
+    key = "some-ssl_client_key"
+    no-insecure
+
+    """)
+
+
+# fc39
+NEW_CURL_OUTPUT = """\
+curl 8.2.1 (x86_64-redhat-linux-gnu) libcurl/8.2.1 OpenSSL/3.1.1 zlib/1.2.13 libidn2/2.3.7 nghttp2/1.55.1
+Release-Date: 2023-07-26
+Protocols: file ftp ftps http https
+Features: alt-svc AsynchDNS GSS-API HSTS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz SPNEGO SSL threadsafe UnixSockets
+"""
+
+# centos-stream8
+OLD_CURL_OUTPUT = """\
+curl 7.61.1 (x86_64-redhat-linux-gnu) libcurl/7.61.1 OpenSSL/1.1.1k zlib/1.2.11 nghttp2/1.33.0
+Release-Date: 2018-09-05
+Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
+Features: AsynchDNS IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy
+"""
+
+
+@patch("subprocess.check_output")
+def test_curl_has_parallel_download(mocked_check_output, sources_module):
+    mocked_check_output.return_value = NEW_CURL_OUTPUT
+    assert sources_module.curl_has_parallel_downloads()
+
+    mocked_check_output.return_value = OLD_CURL_OUTPUT
+    assert not sources_module.curl_has_parallel_downloads()