Support gs:// and https:// build sources in cvd fetch and cvd load

[larsers]

Replaces #2238.

# a dist directory published to a bucket that artifacts are discovered from
# the listing, including the host package:
cvd fetch --default_build=gs://my-bucket/dist/

# artifacts published to any web server, e.g. by a CI system; the
# default build here names one zip, so the host package has its own URL:
cvd fetch \
  --default_build=https://ci.example.com/builds/42/aosp_cf_x86_64_phone-img-9099.zip \
  --host_package_build=https://ci.example.com/builds/42/cvd-host_package.tar.gz

# the same strings work in cvd load configs:
"disk": { "default_build": "gs://my-bucket/dist/" }

A URL build source is an artifact namespace, the same way an Android Build target or a local directory is one. GcsBuild and HttpBuild join the Build variant behind the existing BuildApi interface. A new CompositeBuildApi routes each build to its API, so the fetch logic has no URL-conditional branches. A URL ending in '/' names the directory holding a build's artifacts and for gs:// the contents come from the bucket listing. A URL naming one object is a build of that artifact, where the existing {selector} syntax picks a member out of a zip and #sha256=<hex> pins the digest. Both forms are probed at GetBuild time, so a bad bucket or an expired signature fails before any bytes move. The tree already handles gs:// prefixes this way in LuciBuildApi, this generalizes that to user supplied locations.

Credentials for gs:// resolve independently of the Android Build credential: cvd login token, service account file, ~/.boto, GCE metadata, then anonymous. Public buckets need no setup. https:// sends no credentials and a pre-signed URL carries its own. Query strings are kept out of logs and records. Downloads are cached per artifact, keyed on the object generation or ETag. They are verified against #sha256= and the GCS md5Hash, and an interrupted download resumes with Range/If-Range. A plain https:// directory has no listing, so its artifacts are named explicitly and are not cached.

The first seven commits are standalone fixes the feature depends on and each is reviewable on its own. They are split out as #3070.

Unit tests cover the new code. e2etests/cvd/cvd_url_build_tests is a hermetic e2e suite driving cvd fetch and cvd load against a local TLS server so it needs no Android Build access.

[jemoreira]

Replaces #2238.

# a dist directory published to a bucket that artifacts are discovered from
# the listing, including the host package:
cvd fetch --default_build=gs://my-bucket/dist/

# artifacts published to any web server, e.g. by a CI system:
cvd fetch \
  --default_build=https://ci.example.com/builds/42/aosp_cf_x86_64_phone-img-9099.zip \
  --host_package_build=https://ci.example.com/builds/42/cvd-host_package.tar.gz

# the same strings work in cvd load configs:
"disk": { "default_build": "gs://my-bucket/dist/" }

There is a problem with this example (haven't seen the code yet): --default_build is expected to have everything, including the host packages, so if --host_package is not provided cvd should look for it in the default build.

[jemoreira]

Reviewed the first 7 commits so far, those can be merged independently of the rest, so consider making another PR with just those.

[jemoreira]

nit: it should be possible to avoid passing otatools_inferred as an extra parameter all over the place by making the otatools build something like std::optional<struct{Build build; bool inferred}>. I believe FetchContext can just copy that optional and its OtaToolsBuild can return it too.

[larsers]

Restructured in #3070 , which carries this commit: Builds::otatools is now std::optional<OtaTools> holding the build and the inferred flag, and FetchContext::OtaToolsBuild() returns both, so the extra parameters are gone...

[Databean]

Haven't reviewed everything, but left some comments.

The first six commits are standalone fixes the feature depends on and each is reviewable on its own.

Can they be separate PRs?

[Databean]

We already have DetectGceEnvironment which instead tries to determine if http://metadata.google.internal is reachable. Can these two approaches be consolidated?

[larsers]

Hmm..I guess the detection code could share a home, but both checks are maybe worth keeping?

Afaik, they answer different questions. DetectGceEnvironment queries the metadata server, which metrics needs anyway for the zone and project id, and it reports an error rather than "not GCE" when the request fails. The credential ladder only needs the static "is this machine GCE" fact to decide whether the metadata token endpoint is worth trying, and the DMI product name answers that without a network round trip or a timeout...

Wdyt about moving GCE detection out of host/libs/metrics into a shared library, keep both checks there, and let metrics use the DMI check to skip the HTTP probe on machines that are not GCE?

I would prefer do that as a follow up to keep this PR contained...

[Databean]

Can this accept a std::string_view parameter instead since it immediately converts the parameter and never uses the original?

[larsers]

Done.

[Databean]

Downloading files with HTTP range requests is already implemented in ZipSourceFromUrl, is it possible to reuse this?

There's also LazilyLoadedFile for data loaded piecemeal from a backing reader and cached in a source.

[larsers]

ZipSourceFromUrl is what I decided to use for random access...The FileReader on both new APIs goes through it, with a new overload that takes a known size to skip the HEAD request, and CachingBuildApi::FileReader wraps the source in CacheZipSource, which is built on LazilyLoadedFile.

This file covers the other access pattern, downloading a whole artifact to disk, so one streaming GET instead of a ranged request per read chunk, resume across process restarts keyed on If-Range, a lock against concurrent invocations, and error response bodies never reaching the artifact path. Neither existing helper does that job, and one ranged request per libzip read would be slow for multi-gigabyte image zips.

It does reuse more than it did when you read it first time around... a download with nothing to resume against is now just a call to HttpGetToFile, and only the resume path keeps its own request loop, because HttpGetToFile hides the partial file that the offset and the lock both need...

[Databean]

Can the Fd type get an Result<struct stat> Fstat() function so that this can call CF_EXPECT(fd->Fstat()) instead?

[larsers]

Done... Fd has an Fstat() now, in its own commit, and this file uses Fd instead of SharedFD, so the dup is gone.

[Databean]

Is there multithreading happening around downloading these files, or is this about multiple concurrent user invocations? What are these locks intended to protect against?

[larsers]

No threads... A fetch downloads sequentially. The locks are for concurrent cvd invocations. The artifact cache is shared, so anything that brings up several devices on one host at once, so a test harness, CI, or a service driving cvd, can have more than one fetch running against it. Cache paths are keyed on the object generation, so two fetches of the same build land on the same .part file.... So flock picks one downloader and the others wake up to either the finished file or a resumable part file. I can add a comment saying so.

[Databean]

Originally in the scenarios where otatools is implicitly requested, a later step in assemble_cvd depends on being able to invoke executables in otatools to combine images. If there is a mismatch on cases where they are inferred and cases where they are necessary, it should be removed from the set of inferred cases.

[larsers]

I tried to understand where those executables come from... From what I can tell BuildSuperImage in super_image_mixer.cc runs build_super_image from HostBinaryPath and passes --path=DefaultHostArtifactsPath(""), and both of those resolve to the host package directory rather than the otatools/ directory fetch extracts.

As far as I can tell nothing in assemble_cvd reads that directory at all, and the TODO in RebuildSuperImage about otatools/bin/merge_target_files suggests the same. The rebuild also only runs when there is a system build. So in the kernel plus default case we infer an otatools build that nothing goes on to use, which looks like the mismatch I think you had in mind.

If that is right, this tolerance is not needed. The system case can stay strict, and this part can go away along with the inference. The tolerance is also indiscriminate... It cannot tell a missing otatools.zip from a download that failed for some other reason...

Is there any external consumer that depends on a kernel mix fetch producing the otatools directory?

If not, I'll swap this commit in #3070 for one that drops the inference.

[Databean]

I tried with kernel and system mixed builds, and it doesn't look like either depends on otatools being there in practice.

# system mix
$ cvd fetch \
  --default_build=git_main/cf_x86_64_only_phone-trunk_staging-userdebug \      
  --host_package_build=git_main/cf_x86_64_only_phone-trunk_staging-userdebug \
  --system_build=git_main/gsi_x86_64-trunk_staging-userdebug \          
  --target_directory=$HOME/dl_mix
$ cd $HOME/dl_mix
$ rm -rf otatools
$ cvd create --product_path=$PWD --host_path=$PWD
$ cvd clear
# kernel mix
$ cvd fetch \
   --default_build=git_main/aosp_cf_x86_64_only_phone-trunk_staging-userdebug \
   --target_directory=$HOME/dl_kernel \
   --kernel_build=aosp_kernel-common-android-mainline/kernel_virt_x86_64
$ cd $HOME/dl_mix
$ rm -rf otatools
$ cvd create --product_path=$PWD --host_path=$PWD
$ cvd clear

so it seems fine to only download otatools when explicitly requested, and avoid tracking the inferred state.

[larsers]

OK, thanks for testing the combinations. I'll swap the tolerance commit in #3070 for one that drops the inference entirely, so otatools is only fetched when --otatools_build asks for it, and the inferred tracking goes away with it.

[Databean]

It looks like this function distinguishes between two categories of builds, but the function name and return values of true and false do not describe the categories. Only the function comment describes the categories.

This should either return an enum class or the categories should be clear from the function name.

[larsers]

It is a predicate on artifact names rather than build categories... So both parts accept the same kind of archive, named with or without a build id. I've renamed it to NameMatchesZipKind with a comment that spells out the two accepted shapes.

If you would rather have an enum for the two name shapes I can add one, though nothing downstream distinguishes them.

[Databean]

This is redundant with the following CF_EXPECTF call.

[larsers]

Done... Folded into the scheme allowlist check, keeping the cleartext http hint in its message.

[Databean]

It might be worth splitting up AndroidBuildApi at this point between DeviceBuild and DirectoryBuild, so it's not a special case handling two different types of Builds.

[larsers]

Sure...As this is the last place where one class handles two Build types. The change I made for your BuildApi interface comment already narrows it anyway... So the Android, GCS and HTTP APIs no longer implement the shared interface and each takes its own build type, and AndroidBuildApi handles its two through typed overloads. What is left deciding between build kinds is the composite's dispatch.

Splitting DeviceBuild and DirectoryBuild into separate classes means reworking code that predates this work... If you would rather have it here, I can do that instead?

[Databean]

It kind of breaks the BuildApi interface to have many arguments that fail at runtime for particular BuildApi implementations. See the Liskov substitution principle.

What do you think of changing the BuildApi interface to be templated, like

template<typename BuildStringType, typename BuildType>
class BuildApi {
public:
  Result<BuildType> GetBuild(const BuildStringType&);
  // etc
};

class CompositeBuildApi : public BuildApi<BuildString, Build> {
  // ...
}

class HttpBuildApi : public BuildApi<HttpBuildString, HttpBuild> {
  // ...
}

Then CompositeBuildApi can still direct calls to the correct implementations, and the implementations don't have to consider invalid alternatives.

The most difficult part will be dealing with CachingBuildApi, which will probably need to be templated as well with more of the implementation moved into the header.

[larsers]

You're right... The invalid alternative part should probably not exist. I went one step simpler than the template, the leaf classes no longer implement BuildApi at all. Each exposes only methods in its own types, which already existed as private overloads, so CompositeBuildApi does the variant dispatch with std::visit and stays the only BuildApi the fetch code sees; and the caching wrap moved above the composite, so CachingBuildApi and the BuildApi interface are unchanged at the variant level and nothing needed templating. The invalid combinations are now unrepresentable at compile time.

If you would still prefer to have the templated interface, (e.g. so the caching layer can wrap each leaf separately again), I can switch to it...

[larsers]

Replaces #2238.

# a dist directory published to a bucket that artifacts are discovered from
# the listing, including the host package:
cvd fetch --default_build=gs://my-bucket/dist/

# artifacts published to any web server, e.g. by a CI system:
cvd fetch \
  --default_build=https://ci.example.com/builds/42/aosp_cf_x86_64_phone-img-9099.zip \
  --host_package_build=https://ci.example.com/builds/42/cvd-host_package.tar.gz

# the same strings work in cvd load configs:
"disk": { "default_build": "gs://my-bucket/dist/" }

There is a problem with this example (haven't seen the code yet): --default_build is expected to have everything, including the host packages, so if --host_package is not provided cvd should look for it in the default build.

It does work that way. With no --host_package_build the host package comes from the default build: GetHostBuild falls back to the first target's default build, and for the directory form the package is found in the listing, checked before any download starts (CheckHostPackagePresent). The https example names a single img zip as its default build, so that build holds no host package and the example passes one explicitly. I clarified the example comment.

[larsers]

Reviewed the first 7 commits so far, those can be merged independently of the rest, so consider making another PR with just those.

Split out as #3070. This PR rebases onto it after it merges, dropping to just the feature commits.

[larsers]

Haven't reviewed everything, but left some comments.

The first six commits are standalone fixes the feature depends on and each is reviewable on its own.

Can they be separate PRs?

Yes. The first seven commits are now #3070, as requested. Each commit there is a standalone fix, so if one PR per fix is easier to review I can split them further. This branch gets rebased and pushed once that PR merges, and that push includes the fixes from the inline comments here.