Skip to content

[storage] Modify redirect policy to follow 10 redirects #1688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

Fricounet
Copy link
Contributor

Relevant Issue (if applicable)

If there are Issues related to this PullRequest, please list it.

Details

Please describe the details of PullRequest.

From 2378d07#diff-c9f1f654cf0ba5d46a4ed25d8bb0ea22c942840c6693d31927a9fd912bcb9456R125-R131 it seems that the redirect policy of the http client has always been to not follow redirects. However, this means that pulling from registries which have redirects when pulling blobs does not work. This is the case for instance on GCP's former container registries that were migrated to artifact registries. Additionally, containerd's behavior is to follow up to 10 redirects https://github.com/containerd/containerd/blob/main/core/remotes/docker/resolver.go#L596 so it makes sense to use the same value.

I found out about this issue when attempting to pull nydus images from a private GCP artifact registry using nydus with containerd in kubernetes.

Initially, I was seing a lot of warning logs like:

[2025-04-24 13:22:20.703987 +00:00] WARN [/src/backend/mod.rs:132] Read from backend failed: Registry(Transport(reqwest::Error { kind: Decode, source: Error { kind: WriteZero, message: "failed to write whole buffer" } })), retry count 3

And after digging into the issue and enabling debug logs in nydusd, I see the following logs:

[2025-04-24 13:22:21.123051 +00:00] DEBUG [/src/backend/connection.rs:714] cache-flusher Request: GET https://eu.gcr.io/v2/<image path>/blobs/sha256:0a94c564728231d5d749ddc1247ca4aefddb0476d5c5a0f27a7ad9d911aebc55 headers: Some({"range": "bytes=276448-5925616"}), proxy: false, data: false, duration: 36ms
[2025-04-24 13:22:21.125804 +00:00] DEBUG [/reqwest-0.11.27/src/async_impl/client.rs:2488] redirect policy disallowed redirection to 'https://eu.gcr.io/artifacts-downloads/namespaces/<gcp-project>/repositories/eu.gcr.io/downloads/<very long redacted path>'

Which led me to believe that the issue was with the redirects. I tried my change in a custom build and I can confirm that is does solve the issue for gcp registries.

Types of changes

What types of changes does your PullRequest introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation Update (if none of the other choices apply)

Checklist

Go over all the following points, and put an x in all the boxes that apply.

  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@Fricounet Fricounet requested a review from a team as a code owner April 24, 2025 15:57
@Fricounet Fricounet requested review from adamqqqplay, hsiangkao and power-more and removed request for a team April 24, 2025 15:57
From dragonflyoss@2378d07#diff-c9f1f654cf0ba5d46a4ed25d8bb0ea22c942840c6693d31927a9fd912bcb9456R125-R131
it seems that the redirect policy of the http client has always been to not follow redirects. However, this means that pulling from registries which have redirects when pulling blobs does not work. This is the case for instance on GCP's former container registries that were migrated to artifact registries.
Additionally, containerd's behavior is to follow up to 10 redirects https://github.com/containerd/containerd/blob/main/core/remotes/docker/resolver.go#L596 so it makes sense to use the same value.

Signed-off-by: Baptiste Girard-Carrabin <[email protected]>
@Fricounet Fricounet force-pushed the fricounet/registry-follow-redirects branch from 6670da5 to 356a2c3 Compare April 24, 2025 15:59
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.45%. Comparing base (21206e7) to head (356a2c3).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
- Coverage   55.45%   55.45%   -0.01%     
==========================================
  Files         197      197              
  Lines       55754    55756       +2     
  Branches    47176    47178       +2     
==========================================
- Hits        30920    30917       -3     
- Misses      23235    23238       +3     
- Partials     1599     1601       +2     
Files with missing lines Coverage Δ
storage/src/backend/connection.rs 43.51% <100.00%> (+0.21%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@BraveY BraveY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. LGTM!

@BraveY BraveY merged commit 67bf8b8 into dragonflyoss:master Apr 27, 2025
45 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants