bug-1827506: Implement the storage interface for GCS.

[smarnach]

This pull request implements the storage interface for Google Cloud Storage and makes the code backend-agnostic.

• All tests that make use of storage backends are run for both S3 and GCS.
• Each test now uses a dedicated bucket. This ensures test isolation, but most importantly the tests no longer interfere with the running devstack, which I found to be quite annoying. This is mainly achieved via the parametrized symbol_storage fixture.
• The s3_helper and botomock fixtures are no longer needed and have been removed.
• I switched the development environment to use GCS by default.

Next steps:

• Decide what to do about the different Content-Encoding behaviour between S3 and GCS, as discussed in bug-1827506. I'm currently still waiting for a response from Google's support.
• Decide what additional tests we need.
• Update the documentation.
• Add new environment variables to stage before merging this PR.

[smarnach]

Original comment

The issue with accessing the GCS emulator in the local dev environment has been resolved. I needed to introduce an nginx container to simulate a CDN anyway.

When testing this locally, there is a little inconvenience. For S3, we have this hack to make testing from the browser (and from the command line outside of the container) convenient. For the GCS emulator, the same hack doesn't work: We have to specify the host name it listens for public download requests on, and we can only specify a single host name. That single host name has to be gcs-emulator for the tests to work. Rewriting this to localhost doesn't help, since the GCS emulator will reject public requests for localhost.

On my system, I worked around the issues by adding gcs-emulator to my /etc/hosts file:

127.0.0.1 localhost gcs-emulator

If that also works on macOS, maybe that's good enough?

Some other solutions I can think of:

• Patch the GCS emulator to allow multiple public hosts.
• Run nginx in front of the GCS emulator and rewrite requests.
• Proxy Docker's internal DNS resolver to the host computer.

All of these alternative solutions are more complex than I like.

[smarnach]

I split out all commits that are for other bugs into separate PRs. Many of the PRs needed to be filed against other branches, so we now have to merge the commits in this order:

• bug-1906597: Remove all uses of Upload.bucket_endpoint_url. #2961
• This PR.
• bug-1906959: Allow GCS buckets to be behind a CDN. #2964
• bug-1905455: Enable anonymous access to storage buckets. #2965

The other PRs I filed are completely orthogonal to everything else and can be merged at any time.

[smarnach]

Local testing is easier using the code in #2964 than this branch (see the comment above).

[willkg]

This PR is pretty substantial and makes some big changes to the codebase to make it possible to implement GCS storage. We knew going into this that it would be hard to review and require multiple back-and-forths. That's how big PRs go.

I left a few comments on minor things we should change that are worth doing in this PR.

There are two comments that probably require bigger changes:

The first is how StorageError is structured. I'm pretty sure the structure in this PR will cause us a lot of pain when looking at problems in Sentry. We should do something about that in this PR.

The second is how upload data is handled in tests. I think I want us to change that before I review the rest of the test code and validate the changes.

[willkg]

This gets used by the "upload now" ui view. I bet we could remove this ui view and the API endpoints it uses. I wrote up a bug to consider doing that.

https://bugzilla.mozilla.org/show_bug.cgi?id=1909013

[willkg]

We talked about this on Zoom. I'm going to do a pass to minimally fix the StorageError issue and part of the issue with the tests that I had. Then I'll finish up the review.

[willkg]

^^^ That rebases against main tip and addresses my concerns with StorageError and the tests.

@smarnach Do the last 3 commits look ok to you? If so, then we have a couple of __repr__ suggestions and then I can review this again.

[smarnach]

@smarnach Do the last 3 commits look ok to you? If so, then we have a couple of __repr__ suggestions and then I can review this again.

Thanks a lot! The commits look good to me. I particularly like that the symfile tests and uploads now use valid data.

[willkg]

In testing, everything looked fine.

Validation:

1. ran unit tests 👍
2. ran system tests 👍
3. looked at upload data
   1. regular uploads are marked correctly 👍
   2. try uploads are marked correctly 👍
4. looked at file upload data
   1. compressed vs. uncompressed looks good 👍
   2. try vs. non try have the right public url 👍
   3. at the bottom of the upload file for try, it says "Try Symbols: No" which is wrong--but this is broken in stage, too. I wrote up bug 1910180 😒
   4. public url is something like http://localhost:8000/TestBaseProfiler/F9E38D3857BEE30A4339CC3B15892CF00/TestBaseProfiler.sym?try and clicking on it redirects to https://gcs-emulator:8001/publicbucket/try/v1/TestBaseProfiler/F9E38D3857BEE30A4339CC3B15892CF00/TestBaseProfiler.sym which fails. I think this gets fixed in pull bug-1905455: Enable anonymous access to storage buckets. #2965. 🤷
5. i uploaded a pdb sym file
   1. verified sym file header extraction works 👍
   2. verified codeinfo lookup works 👍

We dropped checking the Location value in the download tests. I'm not sure why we did that. I kind of think we should be checking the Location value. After we land this, we can talk about it and maybe implement it in a followup PR.

I think this is good. Let's land it!

[willkg]

Seems like we should be testing the HTTP 302 response Location. Something like this:

def test_client_happy_path(client, db, symbol_storage, metricsmock):
    upload = UPLOADS["compressed"]
    upload.upload(symbol_storage)

    bucket = symbol_storage.upload_backend.bucket
    prefix = symbol_storage.upload_backend.prefix

    url = reverse(
        "download:download_symbol",
        args=(upload.debug_file, upload.debug_id, upload.sym_file),
    )

    response = client.get(url)
    assert response.status_code == 302

    parsed = urlparse(response["location"])
    assert parsed.netloc in ["localstack:4566", "gcs-emulator:8001"]
    assert parsed.path == (
        f"/{bucket}/{prefix}/{upload.debug_file}/{upload.debug_id}/{upload.sym_file}"
    )

    metricsmock.assert_histogram_once(
        "tecken.symboldownloader.file_age_days",
        value=0,
        tags=["storage:regular", "host:testnode"],
    )

    response = requests.get(response.headers["location"])
    assert response.status_code == 200
    assert response.content == upload.original_body

    response = client.head(url)
    assert response.status_code == 200
    assert response["Access-Control-Allow-Origin"] == "*"
    assert response["Access-Control-Allow-Methods"] == "GET"

We can add this in a follow-up PR unless there's some compelling reason I'm not seeing for not doing that.

[smarnach]

Determining the actual redirect location gets more complex with the next PR that adds a potential CDN. At that point, we'd also need to check whether the storage backend has public_url set and take that into account. It's possible, but the question is how useful it is – we'd basically be re-implementing some logic from the application code in the tests and check that both implementations have the same result.

For this reason I added the code to actually download the symbols file and compare the body to what we uploaded. If we get the correct file back, that seems to be a much stronger indication that the redirect location is correct.

[smarnach]

@willkg There were two outstanding comments from your first review that I addressed in the last two commits. I think this is ready to land now – could you take a final look at the last two PRs?

[smarnach]

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1910363 to rename the loggers in Tecken.

[willkg]

Looks good!

[smarnach]

Quoting from the description of this PR:

Add new environment variables to stage before merging this PR.

I forgot to do that. I'll file a PR.