Support Flat Repository Format

[alexconrey]

Problem

When working with "flat repository format" external repositories, the assumption made by rules_distroless is to use the known repository hierarchy within dists directory. See: https://wiki.debian.org/DebianRepository/Format#Flat_Repository_Format

In a flat repository, everything is "flat" within the top-level of the repository and as such - the {}/dists/{}/{}/binary-{}/Packages.{}".format(url, dist, comp, arch, file_type) logic will always fail to find the Package indices.

Example: https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/

Solution

Add supporting logic around Package index fetch to:

• Default to the nominal Debian Repository Format, which includes all files in dists/ or pool/
• Fall back to flat repository format if retrievals from dists/... default path fail. Uses path provided in sources.url YAML for respective repository as base for URL when pulling package files

Notes

• A flat repository will end up producing a similar WARNING output as a result of these changes:

WARNING: Download from https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/dists/bullseye-cran40//binary-amd64/Packages.xz failed: class java.io.FileNotFoundException GET returned 404 Not Found
WARNING: Download from https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/Packages.xz failed: class java.io.FileNotFoundException GET returned 404 Not Found
WARNING: Download from https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/dists/bullseye-cran40//binary-amd64/Packages.gz failed: class java.io.FileNotFoundException GET returned 404 Not Found
WARNING: Download from https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/dists/bullseye-cran40//binary-arm64/Packages.xz failed: class java.io.FileNotFoundException GET returned 404 Not Found
WARNING: Download from https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/dists/bullseye-cran40//binary-arm64/Packages.gz failed: class java.io.FileNotFoundException GET returned 404 Not Found

• Testing this specific case required adding a package which contains over a dozen dependencies. To ensure accurate testing, I included it in this PR. I think this could go either way with including these tests upstream, this may be a case where the feature is implemented but a warning in a known-working case is less than desirable for contributors making upstream changes
• Specific test package put image over layer depth limits. Implemented workaround noted in: FR: max depth exceeded when trying to load images that have many apt dependencies #36 (pkg_tar)
• pkg_tar requires rules_pkg inclusion, added to MODULE.bazel

[thesayyn]

can we not add a dependency here?

[jjmaestro]

@thesayyn what about moving it to the bottom of the file and marking it as a dev_dependency? Would that work?

[thesayyn]

How does flat repositories play with different architectures? in the manifest format we support specifying architectures matrix which i don't see being applied here.

I believe we should disallow flat repositories to be combined with non flat repositories to prevent confusion.

[jjmaestro]

I don't think disallowing flat repos mixed with "classic" repos is really needed.

You can have flat repos with multiple architectures (e.g. https://cloud.r-project.org/bin/linux/debian/bullseye-cran40/ has i386 and amd64 packages). The NVIDIA repos are "a bit weirder" since they are flat and have multiple architectures (e.g. https://developer.download.nvidia.com/compute/cuda/repos/debian12/x86_64/ has i386 and amd64 packages) but they also seem to have separate flat repos for "other architectures" (e.g. https://developer.download.nvidia.com/compute/cuda/repos/debian12/sbsa/ has arm64 packages).

Personally, as I mentioned in #64 (comment) if there are specific packages in specific architectures I'd just separate them and use another manifest.

[thesayyn]

Hmm, that makes this even more confusing. How does flat repos interact with arches? for instance if you want to install i386 and amd64 of a package from https://developer.download.nvidia.com/compute/cuda/repos/debian12/x86_64/ ?

I don't see how this PR allows discovery of arches in flat repositories as of today.

[jjmaestro]

@thesayyn ah, maybe you are right and there's more fixes needed. I'll try to test the PR and see where it breaks.

[jjmaestro]

@thesayyn OK, I think I got it, the main issue that I found so far (and it's been a very quick look so maybe there's more issues / corner cases) is that in _parse_package_index we force arch to be the one we are using in the for-loop so that's a problem for Package indexes that contain multiple architectures as we will get the package info from a different arch.

This is actually happening in this PR and the proof is in the lock and, easier to see, in the test itself :D Check my comment in examples/debian_snapshot/test_linux_amd64.yaml, the new expectedOutput for the r-base-core package is set as i386 instead of the correct amd64 precisely because the current code in this PR is selecting the wrong package architecture:

(...)
      - r-base-core/now 4.4.1-1~bullseyecran.0 i386 \[installed,local\]

Now, the fix that I would propose here is the following:

diff --git a/apt/private/package_index.bzl b/apt/private/package_index.bzl
index 34f8202..8d6f922 100644
--- a/apt/private/package_index.bzl
+++ b/apt/private/package_index.bzl
@@ -77,7 +77,8 @@ def _parse_package_index(state, contents, arch, root):
 
         if len(pkg.keys()) != 0:
             pkg["Root"] = root
-            util.set_dict(state.packages, value = pkg, keys = (arch, pkg["Package"], pkg["Version"]))
+            arch_ = arch if pkg["Architecture"] == "all" else pkg["Architecture"]
+            util.set_dict(state.packages, value = pkg, keys = (arch_, pkg["Package"], pkg["Version"]))
             last_key = ""
             pkg = {}

IMHO this "is not great" because it's duping the packages that are valid across all architectures N times (as many as architectures we have in the manifest). It's not too bad, it's just some storage space wasted and "some mismatch" between the repo structure that is on disk (the per-arch folders) VS the actual contents of the tar files (this can be seen in the dpk-status in the test, e.g. tzdata is also all but the folder and the "rules_distroless arch" for the package is "amd64" or whatever other arch we are using). But... I don't see how to easily fix this without major changes (and even then I don't know how I'd do it).

[ericlchen1]

Is this still being worked on? If not, I'll take a look at this and probably make a separate PR since we also need this functionality (preferably without the patch we are using).

[jjmaestro]

This is not following the Debian repo format spec that you mentioned. In there, they show the two sources.list formats, one for "canonical" repos:

deb uri distribution [component1] [component2] [...]

and one for flat repos:

deb uri directory/

I think the convention is that directory always ends with the slash / and, while distribution can have slashes, it never ends with a slash.

IMHO we should use the same in the YAML manifest, something like:

version: 1

sources:
  - channel: bullseye main
    url: https://snapshot-cloudflare.debian.org/archive/debian/20240210T223313Z
  - channel: bullseye-cran40/
    url: https://cloud.r-project.org/bin/linux/debian

archs:
  - amd64

packages:
  - bash
  - r-base-core

and use the slash to distinguish between "canonical" and flat repos.

Also, although the sources.list spec doesn't seem to require components, we are also parsing them so I guess that's another difference that should be made in the parsing of the manifest (and validation).

[thesayyn]

i agree with your findings there.

[jjmaestro]

I think the issue with the max depth exceeded is not the only problem of using this package / repo for testing.

It seems that the versions move quite fast... I've tried running the tests of this PR and they initially failed because the version in the lock file didn't exist, so I had to update the lock. Ideally, the tests should be done with a snapshot repo that will "always" exist, or at the very least, run without a lock (and don't test for the specific version installed in the image).

Also,, the R-project repo doesn't have arm64 packages (it only has i386 and amd64, and a few all). Thus, the tests will fail for arm64.

I think it would be best to create a separate "debian_flat_repo" test that uses a lockless repo and only runs for amd64.

Or, preferably, find another flat repo that also has a snapshot... probably not an easy task! 😅 If I come up with such a repo, I'll comment here as well.

[jjmaestro]

i386 is the wrong architecture here, this is caused by a bug in _parse_package_index, see my comments before in this commit, in apt/private/package_index.bzl.

[jjmaestro]

Found a way to easily avoid having to flatten stuff here in the test and depend on rules_pkg, etc.

You can simply use the :data target of the package so that you avoid pulling in all of the dependencies :) Sure, that's "broken" for a real-life image but it'll work to show that the specific package is installed and thus it can be used to validate the test without having to bring in more dependencies and potentially have slower tests, etc.

[jjmaestro]

@alexconrey there's quite a few changes to make... IMHO from "first to last":

• separate test to avoid using arm64 since it's not a supported architecture for the r-project.org repo
• fix the bug in _parse_package_index
• detect flat VS "canonical" repos in the manifest and avoid the 404 warnings
• nit: to avoid the max depth issue and to avoid using rules_pkg and/or flattening large amounts of dependencies, we can use the :data part of r-base-core in the test.

At this point, and given that there's been a few attempts at adding support for flat repos (this PR plus #64 and #67 that I could find) I'm a bit wary of doing yet-another-PR... but I'm also happy to do it and get it done ASAP.

@thesayyn what do you think? would it be OK to create "one 💍 PR to merge them all"? 😅

[jjmaestro]

OK, so I finally did try to write a PR that adds support for multi-arch and single-arch flat repos and addresses the issue with the NVIDIA CUDA flat repos. Hopefully that PR superseeds the others and can be merged and 1 + 3 PRs closed 😄

@alexconrey please have a look and let me know what you think and if #86 works for you. Happy to address any issues or concerns if there's something that breaks in your use-case!