Implement opaque ETag in Compact Index to avoid falling back to old index in servers with different etag implementations #7122

martinemde · 2023-10-29T19:20:02Z

What was the end-user or developer problem that led to this PR?

Building on @simi's branch for opaque etags (related to #6632) and merging in my work with DigestIO in the CompactIndex client at #7039 (closed).

The problem that originally surfaced here was when other compact index sources did not calculate their etags the same way as rubygems.org and S3. This would result in the compact index never doing incremental updates, which is much slower. As a solution we decided to use the Digest, and then the Repr-Digest header and allow etags to be opaque.

What is your fix for the problem, implemented in this PR?

Change the compact index client to verify compact index file downloads using the Repr-Digest or Digest header. Uses sha-256 right now but opens up the possibility to support any algorithm going forward. This is a change from assuming that the etag is an MD5 sum of the full file body and using it to verify the content.

The compact index client now stores etags in a separate file, allowing them to be completely opaque, as intended. If an etag file does not exist, we fallback to generating the md5 of the file on disk and saving that to the etag file. After the first pass, the etag will either be validated or replaced, resuming normal operation of the compact index client.

The Repr-Digest header to conform to Structured Fields in HTTP Headers RFC 8941, but we also read Digest which comes from Fastly/S3 for all compact index files and is (incorrectly if you're picky) wrapped in double quotes. Once the changes are deployed to fastly, this client will read from Repr-Digest preferentially.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

bundler/lib/bundler/compact_index_client/updater.rb

bundler/lib/bundler/compact_index_client/cache.rb

segiddins · 2023-11-01T18:21:17Z

bundler/lib/bundler/shared_helpers.rb

@@ -193,6 +193,21 @@ def digest(name)
      Digest(name)
    end

+    def checksum_for_file(path, digest)


should this just take in an array of digest symbols and then return a hash?

We only use it in one place only for the MD5 of the version info files for the compact index. The server side of the compact index is hard coded to MD5 as well. The most correct thing is probably to move this completely into the compact index client and only generalize if we need to later.

segiddins · 2023-11-01T18:22:45Z

bundler/lib/bundler/compact_index_client/cache.rb

@@ -76,6 +84,16 @@ def info_path(name)
        end
      end

+      def info_etag_path(name)
+        name = name.to_s
+        if /[^a-z0-9_-]/.match?(name)


so... question. since this is new, might it be easier to only do the #{name}-#{digest} file name, and then only need to maintain a single directory of etags?

What is the purpose of this logic? @simi?

I was about to keep it the same as other data we store

rubygems/bundler/lib/bundler/compact_index_client/cache.rb

Line 71 in f8e87eb

if /[^a-z0-9_-]/.match?(name)

. Not every gem name is unique when transformed into file name, this is to bandaid this AFAIK.

So then is @segiddins suggesting that we use MD5 of every name instead of having both? Could we just do the MD5 of the name without the prefix?

bundler/lib/bundler/compact_index_client.rb

bundler/lib/bundler/compact_index_client/updater.rb

segiddins · 2023-11-01T18:29:01Z

bundler/lib/bundler/compact_index_client/updater.rb


-          headers["If-None-Match"] = etag_for(local_temp_path)
+          headers["If-None-Match"] = local_etag_temp_path.read


should we strip! here?

strip! and chomp! both return nil if there's nothing to strip or chomp, which makes this pretty ugly:

etag = local_etag_path.read etag.chomp! headers["If-None-Match"] = etag

this is one line but the to_proc might negate the benefit of using chomp! over chomp:

headers["If-None-Match"] = local_etag_path.read.tap(&:chomp!)

segiddins · 2023-11-01T18:30:07Z

bundler/lib/bundler/compact_index_client/updater.rb

        local_temp_path = local_temp_path.sub(/$/, ".tmp")

+        headers["Want-Content-Digest"] = WANT_DIGEST


I'm not seeing Want-Content-Digest listed at all on MDN, and it says Want-Digest is deprecated https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Want-Digest

It's only a draft 🤷‍♂️ though, to be fair the Content-Digest header seems to come from the same draft.

We could totally skip this header. It's just future proofing.

segiddins · 2023-11-01T18:32:17Z

bundler/lib/bundler/compact_index_client/updater.rb

+      # Unwrap surrounding colons (byte sequence) or quotes (string).
+      # The wrapping characters must be matched or we return nil.
+      # Converts hex to base64 based on field type assumptions.
+      def parse_structured_value(value)


this definitely needs some tests

bundler/lib/bundler/compact_index_client/updater.rb

martinemde · 2023-11-02T05:19:08Z

Hey @segiddins & @simi, I refactored this pretty significantly. I mostly replaced the giant method with two smaller methods. I also cut down on the pending temporary files, clearly separated what was called "retrying" logic, and removed the nested call to update which made the file management weird.

The behavior changes a little, but I think for the best: In the case where a range request is ignored or not available, it no longer attempts the full request twice (it was requesting a range 0- which is the same as a full request, then it would do the same thing again, whereas the intention, I think, was to try a partial request, then try the full request).

simi · 2023-11-02T10:28:46Z

@martinemde this is on my list, I'll do some local testing first.

martinemde · 2023-11-02T15:13:03Z

I'm still working on fixing and adding tests. I think the mocked ones are a good way to test the more subtle behavior.

martinemde

Nitpicks of my own code that I want to remember to fix.

bundler/lib/bundler/compact_index_client/cache_file.rb

bundler/lib/bundler/compact_index_client/updater.rb

segiddins

Nice! took me a while to read through the changes, but I think this is both good future-proofing, as well as fully backwards compatible. well done!

bundler/lib/bundler/compact_index_client/cache_file.rb

This changes the CompactIndexClient to store etags received from the compact index in separate files rather than relying on the MD5 checksum of the file as the etag. Smoothes the upgrade from md5 etags to opaque by generating them when no etag file exists. This should reduce the initial impact of changing the caching behavior by reducing cache misses when the MD5 etag is the same. Eventually, the MD5 behavior should be retired and the etag should be considered completely opaque with no assumption that MD5 would match.

simi · 2023-11-27T01:35:26Z

bundler/lib/bundler/compact_index_client.rb

@@ -5,7 +5,13 @@

 module Bundler
  class CompactIndexClient
+    # NOTE: MD5 is here not because we expect a server to respond with it, but


Should this be TODO instead?

I'm not sure. I mentioned this temporary md5 thing in a bunch of places so maybe we'll remember to get it.

I mean I do rarely scan codebase for NOTE, but I do often for TODO. I'll try to keep this in my mind as well for now as a candidate to update at some point.

Ah, I don't usually leave TODO in PRs, but that's a good point if you actually search for it. We could update.

simi · 2023-11-27T01:40:30Z

bundler/lib/bundler/compact_index_client/cache.rb

@@ -76,8 +81,19 @@ def info_path(name)
        end
      end

+      def info_etag_path(name)
+        name = name.to_s
+        info_etag_root.join("#{name}-#{SharedHelpers.digest(:MD5).hexdigest(name).downcase}")


❤️ love this doesn't follow need for 2 directories (one with special characters) in the end and unifies it.

simi · 2023-11-27T01:41:42Z

Thanks a lot for taking over @martinemde. Well done 💪.

martinemde · 2023-11-27T01:48:44Z

@simi and @segiddins thanks for reviewing and supporting this, and especially thanks @simi for jumping on a call to go over this code. Now let's just hope it rolls out smoothly.

deivid-rodriguez · 2023-11-27T13:30:19Z

This is awesome, thanks for all the work here to not just quickly patch this and move on, but to fix it the right way!

martinemde requested review from simi and segiddins October 29, 2023 19:20

martinemde commented Oct 29, 2023

View reviewed changes

bundler/lib/bundler/compact_index_client/updater.rb Outdated Show resolved Hide resolved

martinemde mentioned this pull request Oct 29, 2023

Use DigestIO to reduce I/O when parsing compact index file digests #7039

Closed

4 tasks

simi reviewed Oct 29, 2023

View reviewed changes

bundler/lib/bundler/compact_index_client/updater.rb Outdated Show resolved Hide resolved

martinemde force-pushed the martinemde/digestio-opaque-etag branch 3 times, most recently from cd0b0a2 to a823684 Compare October 29, 2023 22:58

segiddins reviewed Nov 1, 2023

View reviewed changes

martinemde force-pushed the martinemde/digestio-opaque-etag branch from 94e0179 to 14a0b4e Compare November 2, 2023 05:09

martinemde force-pushed the martinemde/digestio-opaque-etag branch 2 times, most recently from 613b8ef to 06b95be Compare November 2, 2023 05:31

martinemde mentioned this pull request Nov 2, 2023

Return Repr-Digest header in compact index rubygems/rubygems.org#4174

Merged

martinemde commented Nov 4, 2023

View reviewed changes

martinemde force-pushed the martinemde/digestio-opaque-etag branch 2 times, most recently from c17960b to 7ca3f7b Compare November 4, 2023 19:50

martinemde mentioned this pull request Nov 8, 2023

bundle lock/install --checksums #7137

Closed

4 tasks

martinemde changed the title ~~Opaque ETag using DigestIO to calculate checksums~~ Opaque ETag in Compact Index Nov 13, 2023

martinemde marked this pull request as ready for review November 26, 2023 22:58

martinemde force-pushed the martinemde/digestio-opaque-etag branch from 800ecd4 to 5298b97 Compare November 26, 2023 22:58

martinemde requested review from segiddins and simi November 26, 2023 22:59

martinemde force-pushed the martinemde/digestio-opaque-etag branch from 5298b97 to 9fdf6fe Compare November 26, 2023 23:00

segiddins approved these changes Nov 26, 2023

View reviewed changes

bundler/lib/bundler/compact_index_client/cache_file.rb Outdated Show resolved Hide resolved

martinemde force-pushed the martinemde/digestio-opaque-etag branch from 9fdf6fe to a3ef6d9 Compare November 27, 2023 00:19

martinemde enabled auto-merge November 27, 2023 00:59

martinemde merged commit 0339d53 into master Nov 27, 2023
66 checks passed

martinemde deleted the martinemde/digestio-opaque-etag branch November 27, 2023 01:27

simi reviewed Nov 27, 2023

View reviewed changes

simi mentioned this pull request Nov 27, 2023

Fix bundler compact index resolution with jfrog cloud artifactory #6632

Closed

4 tasks

deivid-rodriguez added the bundler: bug fix label Nov 27, 2023

deivid-rodriguez changed the title ~~Opaque ETag in Compact Index~~ Implement opaque ETag in Compact Index to avoid falling back to old index in servers with different etag implementations Nov 27, 2023

deivid-rodriguez mentioned this pull request Feb 2, 2024

Bundler::GemNotFound: Could not find X with Artifactory virtual gem repo + rubygems.org remote repo #7451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement opaque ETag in Compact Index to avoid falling back to old index in servers with different etag implementations #7122

Implement opaque ETag in Compact Index to avoid falling back to old index in servers with different etag implementations #7122

martinemde commented Oct 29, 2023 •

edited

segiddins Nov 1, 2023

martinemde Nov 1, 2023

segiddins Nov 1, 2023

martinemde Nov 2, 2023

simi Nov 2, 2023

martinemde Nov 2, 2023

segiddins Nov 1, 2023

martinemde Nov 2, 2023

segiddins Nov 1, 2023

martinemde Nov 1, 2023 •

edited

martinemde Nov 2, 2023

segiddins Nov 1, 2023

martinemde commented Nov 2, 2023 •

edited

simi commented Nov 2, 2023

martinemde commented Nov 2, 2023

martinemde left a comment

segiddins left a comment

simi Nov 27, 2023

martinemde Nov 27, 2023

simi Nov 27, 2023

martinemde Nov 27, 2023

simi Nov 27, 2023

simi commented Nov 27, 2023

martinemde commented Nov 27, 2023

deivid-rodriguez commented Nov 27, 2023


		headers["If-None-Match"] = etag_for(local_temp_path)
		headers["If-None-Match"] = local_etag_temp_path.read

		local_temp_path = local_temp_path.sub(/$/, ".tmp")

		headers["Want-Content-Digest"] = WANT_DIGEST

Implement opaque ETag in Compact Index to avoid falling back to old index in servers with different etag implementations #7122

Implement opaque ETag in Compact Index to avoid falling back to old index in servers with different etag implementations #7122

Conversation

martinemde commented Oct 29, 2023 • edited

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinemde Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinemde commented Nov 2, 2023 • edited

simi commented Nov 2, 2023

martinemde commented Nov 2, 2023

martinemde left a comment

Choose a reason for hiding this comment

segiddins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simi commented Nov 27, 2023

martinemde commented Nov 27, 2023

deivid-rodriguez commented Nov 27, 2023

martinemde commented Oct 29, 2023 •

edited

martinemde Nov 1, 2023 •

edited

martinemde commented Nov 2, 2023 •

edited