Improve error handling in Quay client HTTP requests #1057
Conversation
This enhances error reporting when external Quay.io API requests fail by checking HTTP response status before attempting JSON parsing. Changes: - Add status code validation in manifest.rs before JSON deserialization - Add status code validation in tag.rs before JSON deserialization - Return clear error messages with HTTP status codes - Prevent confusing "JSON parsing" errors when getting HTML error pages This improves debugging by showing actual HTTP errors (e.g., "403 Forbidden") instead of cryptic "expected value at line 1 column 1" JSON parsing failures. Network tests now fail properly with actionable error messages when external services have authentication or permission issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com> Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
WalkthroughAdds runtime HTTP response status checks in Quay client functions and replaces server-side label filtering with client-side filtering in the Cincinnati metadata fetcher and corresponding tests; error paths now return or yield errors on non-successful responses, and label filtering is applied locally after fetching all labels. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fao89 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
quay/src/v1/tag.rs (1)
52-59: Status check behavior looks correct; consider adding more context and/or a helperThe early return on non-success statuses correctly prevents JSON parsing of error bodies and aligns with the PR goal. The behavior of the happy path is unchanged.
For maintainability and easier debugging, you might consider:
- Including minimal context (e.g., repository and/or endpoint) in the error message so callers can quickly identify which request failed when multiple Quay calls are in play.
- Extracting this status-check + error-construction pattern into a small helper on
Client(or a shared function) so other HTTP callers can reuse it and stay consistent.quay/src/v1/manifest.rs (1)
80-86: Correct HTTP error handling; consider shared helper and richer error contextThe explicit status check here cleanly avoids JSON parsing on failed responses and returns a meaningful error instead, which is exactly what you want.
To improve maintainability and diagnostics further, consider:
- Reusing a shared helper (e.g., a
send_checked/check_statuswrapper onClient) soget_labelsandstream_tagsdon’t each hand-roll the same status check and message.- Adding a bit of context (repository/manifest reference or endpoint) to the error so it’s immediately clear which label request failed when multiple calls are in flight.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
quay/src/v1/manifest.rs(1 hunks)quay/src/v1/tag.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
quay/src/v1/tag.rsquay/src/v1/manifest.rs
🧬 Code graph analysis (1)
quay/src/v1/tag.rs (1)
quay/src/v1/manifest.rs (1)
resp(88-88)
The Quay.io labels API returns 403 Forbidden when using filter parameters, but works fine without filters. This change removes the server-side filtering and implements client-side filtering instead. Changes: - Remove filter parameter from get_labels calls in both test and plugin code - Add client-side filtering using starts_with() to match expected prefixes - Tests now pass instead of failing with 403 Forbidden errors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com> Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cincinnati/src/plugins/internal/metadata_fetch_quay.rs (1)
134-139: Consider the performance impact of client-side filtering.Fetching all labels before filtering may transfer and process more data than necessary, especially if repositories have many labels that don't match the prefix. While this is a necessary workaround for the 403 error, consider:
- Adding logging to track the number of labels fetched vs. filtered
- Documenting the performance trade-off in code comments
If you'd like to add observability, consider adding a trace log:
// Filter client-side for labels matching the expected prefix let quay_labels = all_labels .into_iter() .map(Into::into) .filter(|(key, _): &(String, String)| key.starts_with(&label_filter)) .collect::<Vec<(String, String)>>(); + +trace!( + "[{}] filtered {} labels to {} matching prefix '{}'", + &release_version, + all_labels.len(), + quay_labels.len(), + &label_filter +);Note: You'll need to capture
all_labels.len()before consuming the iterator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cincinnati/src/plugins/internal/metadata_fetch_quay.rs(1 hunks)quay/tests/net/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cincinnati/src/plugins/internal/metadata_fetch_quay.rsquay/tests/net/mod.rs
🔇 Additional comments (2)
quay/tests/net/mod.rs (2)
63-72: LGTM! Test properly validates client-side filtering.The test correctly mirrors the production implementation by:
- Fetching all labels without server-side filtering
- Applying client-side filtering by prefix
- Validating the filtered results
This ensures the workaround for the 403 error is properly tested.
74-80: LGTM! Assertion correctly updated.The assertion properly validates the filtered labels against the expected result, ensuring only labels matching the "io.openshift.upgrades.graph" prefix are present.
| let all_labels = client | ||
| .get_labels( | ||
| repo.clone(), | ||
| manifestref.clone(), | ||
| Some(label_filter.clone()), | ||
| None, // Remove filter to avoid 403 Forbidden - filter client-side instead | ||
| ) | ||
| .await? | ||
| .await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 'get_labels|label.*filter' --type=rust -g '!target/**'Repository: openshift/cincinnati
Length of output: 184
🌐 Web query:
Quay.io API label filtering 403 Forbidden error
💡 Result:
Likely causes and quick checks for a 403 when filtering by labels on the Quay API
- Permission/scope issue (most common)
- The API call requires repo:read (or repo:write for label changes). Ensure the token/robot account has the needed repository permissions or is a team member of the org. Test with a token that you know has repo:read. [1][2]
- Using the wrong account type
- Robot accounts must be used for some automated operations (and must have read access to the repo). If using a user token, confirm the user is member of the org and has repo access. [1]
- Wrong endpoint/parameters or path
- The manifest/labels endpoints include repository path and manifest digest in the path; incorrectly-formed paths or query parameters can return 403/401. Verify you’re calling the documented endpoint (e.g. GET /api/v1/repository/{repository}/manifest/{manifestref}/labels with optional filter). [2]
- Authentication header or token not sent
- Confirm Authorization: Bearer is present and not being removed by proxies or client libs. Also check you’re not accidentally using a cookie/session token where an OAuth token is required. [1]
- Registry configuration / proxy or signature mismatch
- If you see 403 with XML or “SignatureDoesNotMatch” in body, it can be a backend S3/signing issue or proxy that changes requests. Check registry logs and any reverse proxy in path. (Quay docs and support notes mention S3/signature and proxy issues producing 403s.) [1][3]
How to debug (quick steps)
- Reproduce with curl (explicit header) and full verbose output:
curl -v -H "Authorization: Bearer " "https://quay.io/api/v1/repository/namespace/name/manifest//labels?filter=keyprefix" - Try a token you know has repo:read to confirm permission vs token problem.
- Check Quay server logs for the request and reason (auth/acl check or upstream storage error).
- If using a robot, confirm it’s enabled and has read on that repo (and that you didn’t use a user token that lacks org membership). [1][2]
If you want, paste (redacted) the curl command and full response (headers + body) or the exact endpoint and token type you’re using and I’ll point to the specific cause and fix.
Sources
- Quay docs — “I’m authorized but I’m still getting 403s” / common auth failures. [1]
- Red Hat / Quay API guide — manifest/labels endpoints and required authorizations. [2]
- Quay/Red Hat issues & support notes re: 403 from storage/signature problems. [3]
References
[1] Quay docs — auth failure guidance.
[2] Red Hat Quay API Guide — manifest/labels endpoints and permissions.
[3] Red Hat/Quay support notes on 403s (S3/signature/proxy).
Investigate and fix the root cause of the 403 Forbidden error instead of working around it.
The change from server-side to client-side filtering suggests an underlying authentication or permissions issue with the Quay API token/robot account. Based on Quay API documentation, 403 errors on filtered label queries typically indicate:
- The token lacks
repo:readpermissions on the repository - The robot account or user is not properly configured
- Incorrect API endpoint path or authentication headers
Client-side filtering is a workaround that masks the actual problem. Verify the credentials have appropriate permissions and that the API call is correctly formed before removing the server-side filter. This also has performance implications if filtering many labels.
🤖 Prompt for AI Agents
In cincinnati/src/plugins/internal/metadata_fetch_quay.rs around lines 126 to
132, stop masking a 403 by removing the server-side filter; investigate and fix
the root cause so the Quay API can accept filtered queries. Check the
credentials used by the client: verify the token/robot account has repo:read
(and other required) permissions on the target repo, confirm the robot account
is correctly provisioned, and ensure the request uses the correct Quay API
endpoint and authentication headers. Add explicit handling/logging for 403
responses to surface permission errors (include API path, token identifier, and
response body) and restore the server-side filter once permissions/endpoint/auth
are corrected to avoid client-side filtering and the associated performance
cost.
|
@fao89: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
This enhances error reporting when external Quay.io API requests fail by checking HTTP response status before attempting JSON parsing.
Changes:
This improves debugging by showing actual HTTP errors (e.g., "403 Forbidden") instead of cryptic "expected value at line 1 column 1" JSON parsing failures.
Network tests now fail properly with actionable error messages when external services have authentication or permission issues.
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4 noreply@anthropic.com
Signed-off-by: Fabricio Aguiar fabricio.aguiar@gmail.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED