Skip to content

SS-1076 improve image pull error #368

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
merged 11 commits into from
Jul 17, 2025
Merged

Conversation

churnikov
Copy link
Contributor

@churnikov churnikov commented Jul 10, 2025

Link to related charts pull request ScilifelabDataCentre/serve-charts#101
TODO:

This pull request introduces a feature to validate Docker image architectures for both Docker Hub and GitHub Container Registry (GHCR). The changes include adding new utility functions, validators, and settings to ensure Docker images are built for the correct CPU architecture (amd64). It also integrates a feature flag to toggle this validation.

Docker image architecture validation:

  • Added a new feature to validate Docker image architectures (amd64) in validate_ghcr_image and validate_docker_image functions. This validation is controlled by the docker_image_architecture_validator feature flag using the waffle library. [1] [2]

New utilities for Docker and GHCR integration:

  • Introduced DockerHubAuthenticator and GHCRAuthenticator classes for handling authentication with Docker Hub and GHCR, respectively. These classes retrieve Bearer tokens for API requests.
  • Added utility functions like get_manifest_list, get_config_blob, and get_image_architecture to fetch and parse image manifests and configurations to determine supported architectures.

Configuration updates:

  • Added new environment variables (DOCKER_HUB_TOKEN, DOCKER_HUB_USERNAME, GITHUB_API_USERNAME) to studio/settings.py for authentication with Docker Hub and GHCR.

Checklist

If you're unsure about any of the items below, don't hesitate to ask. We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

  • This pull request is against develop branch (not applicable for hotfixes)
  • I have included a link to the issue on GitHub or JIRA (if any)
  • I have included migration files (if there are changes to the model classes)
  • I have included, reviewed and executed tests (unit and end2end) to complement my changes
  • I have updated the related documentation (if necessary)
  • I have added a reviewer for this pull request
  • I have added myself as an author for this pull request
  • In the case I have modified settings.py, then I have also updated the studio-settings-configmap.yaml file in serve-charts

Further comments

Anything else you think we should know before merging your code!

@churnikov churnikov self-assigned this Jul 10, 2025
@churnikov churnikov requested a review from a team as a code owner July 10, 2025 15:00
@churnikov churnikov added the enhancement Improvement of existing feature or request label Jul 10, 2025
@churnikov churnikov requested review from Copilot and a team and removed request for a team July 10, 2025 15:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds validation of Docker image CPU architectures and supporting utilities.

  • Introduces DockerHubAuthenticator and GHCRAuthenticator for registry token retrieval.
  • Implements get_manifest_list, get_config_blob, and get_image_architecture to fetch and parse image architectures.
  • Integrates a docker_image_architecture_validator waffle switch into validate_ghcr_image and validate_docker_image to enforce amd64 images.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
studio/settings.py Added new env vars for Docker Hub and GHCR authentication
apps/validators/container_images.py New registry auth classes and manifest/config-fetching utilities
apps/helpers.py Wired image-architecture checks behind feature flag in validators
Comments suppressed due to low confidence (1)

apps/validators/container_images.py:135

  • [nitpick] The parameter name refence is misspelled; consider renaming it to reference for clarity and consistency.
def get_image_architecture(

else:
logger.error("Unknown or unsupported manifest format!")

return architectures
Copy link
Preview

Copilot AI Jul 10, 2025

Choose a reason for hiding this comment

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

get_image_architecture can return None on unsupported manifests, but callers assume a list. Ensure it always returns a list (possibly empty) or raise an exception to prevent runtime errors.

Suggested change
return architectures
return architectures or []

Copilot uses AI. Check for mistakes.

@churnikov churnikov marked this pull request as draft July 10, 2025 15:03
churnikov and others added 3 commits July 11, 2025 12:52
- Improved documentation for authentication classes and methods.
- Added support for anonymous access in DockerHubAuthenticator (Mostly for GHCR).
- Renamed `get_image_architecture` to `get_image_architectures` for clarity.
- Introduced tests for GHCR and Docker Hub architecture validation.

Signed-off-by: Nikita Churikov <[email protected]>
@churnikov churnikov marked this pull request as ready for review July 11, 2025 11:03
@j-awada
Copy link
Contributor

j-awada commented Jul 11, 2025

@churnikov did you choose to create the flag manually? Here's where the Depictio one gets created.
But in general image arch verification sounds more like a switch than a feature flag.

@churnikov
Copy link
Contributor Author

@j-awada

But in general image arch verification sounds more like a switch than a feature flag.

My bad, I named it in commit messages feature flag, while it's already a switch. Good point thought, thank you:)

@churnikov
Copy link
Contributor Author

Here's where the Depictio one gets created.

Thanks, good idea, will do that!

Copy link
Contributor

@j-awada j-awada left a comment

Choose a reason for hiding this comment

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

Did not run this locally but LGTM.



def get_image_architectures(
*, auth: BaseRegistryAuth, repo: str, refence: str, registry: str = "registry-1.docker.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

is refence missing the r on purpose? haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops:))

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to update .env.template to include those variables there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, will do that:)

@churnikov churnikov merged commit 8b8f141 into develop Jul 17, 2025
2 checks passed
@churnikov churnikov deleted the SS-1076-improve-image-pull-error branch July 17, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants