fix: handle caching when detecting a repo requires auth #4471
Conversation
Jib may in some circumstances incorrectly determine that a repository does not require authentication. Take for instance the case where caching is used for the manifests or other components of a base image. If the initial request succeeds then Jib assumes the repository does not require authentication. In the case of a cache miss where authentication is required, this may result in a 401 which is not handled and fails the build. Jib should use the /v2/ endpoint to initially determine if the repository requires authentication. Fixes: GoogleContainerTools#4385
Summary of ChangesHello @gibfahn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Jib could fail to authenticate with a container registry if an initial unauthenticated request succeeded (e.g., due to cached metadata), but subsequent operations (like downloading layers) required authentication. The core change introduces a flexible, on-demand authentication strategy. Instead of failing immediately on a 401 Unauthorized error, the system now attempts to dynamically re-authenticate the RegistryClient using a configurable exception handler, ensuring a more resilient build process when interacting with authenticated registries. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication handling for base image pulling to address caching issues, which could lead to incorrect assumptions about a repository's authentication requirements. The introduction of the RegistryUnauthorizedExceptionHandler interface is a solid design improvement, making the authentication logic more flexible and centralizing on-demand authentication. The changes effectively resolve the described issue and improve the robustness of the authentication flow. I have a couple of suggestions to improve code style by removing some unconventional brace patterns that affect readability.
| // Double indentation keeps code at same level as original code | ||
| { | ||
| { | ||
| final EventHandlers eventHandlers = registryClient.getEventHandlers(); | ||
| final String imageReference = ex.getImageReference(); | ||
| String wwwAuthenticate = ex.getHttpResponseException().getHeaders().getAuthenticate(); | ||
| if (wwwAuthenticate != null) { | ||
| eventHandlers.dispatch( | ||
| LogEvent.debug("WWW-Authenticate for " + imageReference + ": " + wwwAuthenticate)); | ||
| registryClient.authPullByWwwAuthenticate(wwwAuthenticate); | ||
| return new ImagesAndRegistryClient( | ||
| pullBaseImages(registryClient, progressDispatcher.newChildProducer()), | ||
| registryClient); | ||
|
|
||
| } else { | ||
| // Not getting WWW-Authenticate is unexpected in practice, and we may just blame the | ||
| // server and fail. However, to keep some old behavior, try a few things as a last resort. | ||
| // TODO: consider removing this fallback branch. | ||
| if (credential != null && !credential.isOAuth2RefreshToken()) { | ||
| eventHandlers.dispatch( | ||
| LogEvent.debug("Trying basic auth as fallback for " + imageReference + "...")); | ||
| registryClient.configureBasicAuth(); | ||
| try { | ||
| return new ImagesAndRegistryClient( | ||
| pullBaseImages(registryClient, progressDispatcher.newChildProducer()), | ||
| registryClient); | ||
| } catch (RegistryUnauthorizedException ignored) { | ||
| // Fall back to try bearer auth. | ||
| } | ||
| } | ||
|
|
||
| eventHandlers.dispatch( | ||
| LogEvent.debug("Trying bearer auth as fallback for " + imageReference + "...")); | ||
| registryClient.doPullBearerAuth(); | ||
| return new ImagesAndRegistryClient( | ||
| pullBaseImages(registryClient, progressDispatcher.newChildProducer()), | ||
| registryClient); | ||
| if (!registryClient.doPullBearerAuth()) { | ||
| eventHandlers.dispatch( | ||
| LogEvent.error("Failed to bearer auth for pull of " + imageReference)); | ||
| throw ex; | ||
| } | ||
| } | ||
|
|
||
| // If another exception occurs, use the default behavior | ||
| return RegistryClient::defaultRegistryUnauthorizedExceptionHandler; | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of double braces {{ ... }} and the accompanying comment is unconventional and harms code readability. While it might be intended to minimize diff noise, prioritizing long-term code clarity is generally better. Future maintainers could find this pattern confusing. I recommend removing the extra braces and the comment.
final EventHandlers eventHandlers = registryClient.getEventHandlers();
final String imageReference = ex.getImageReference();
String wwwAuthenticate = ex.getHttpResponseException().getHeaders().getAuthenticate();
if (wwwAuthenticate != null) {
eventHandlers.dispatch(
LogEvent.debug("WWW-Authenticate for " + imageReference + ": " + wwwAuthenticate));
registryClient.authPullByWwwAuthenticate(wwwAuthenticate);
} else {
// Not getting WWW-Authenticate is unexpected in practice, and we may just blame the
// server and fail. However, to keep some old behavior, try a few things as a last resort.
// TODO: consider removing this fallback branch.
eventHandlers.dispatch(
LogEvent.debug("Trying bearer auth as fallback for " + imageReference + "..."));
if (!registryClient.doPullBearerAuth()) {
eventHandlers.dispatch(
LogEvent.error("Failed to bearer auth for pull of " + imageReference));
throw ex;
}
}
// If another exception occurs, use the default behavior
return RegistryClient::defaultRegistryUnauthorizedExceptionHandler;| { | ||
| if (ex.getHttpResponseException().getStatusCode() | ||
| != HttpStatusCodes.STATUS_CODE_UNAUTHORIZED | ||
| || !isBearerAuth(authorization.get()) | ||
| || !isBearerAuth(registryClient.authorization.get()) | ||
| || ++bearerTokenRefreshes >= MAX_BEARER_TOKEN_REFRESH_TRIES) { | ||
| throw ex; | ||
| } | ||
|
|
||
| // Because we successfully did bearer authentication initially, getting 401 here probably | ||
| // means the token was expired. | ||
| String wwwAuthenticate = ex.getHttpResponseException().getHeaders().getAuthenticate(); | ||
| authorization.set(refreshBearerAuth(wwwAuthenticate)); | ||
| registryClient.authorization.set(registryClient.refreshBearerAuth(wwwAuthenticate)); | ||
|
|
||
| return () -> this; | ||
| } |
There was a problem hiding this comment.
This extra pair of braces is unnecessary and makes the code slightly harder to read. It's best to remove it for simplicity and to maintain a consistent coding style.
if (ex.getHttpResponseException().getStatusCode()
!= HttpStatusCodes.STATUS_CODE_UNAUTHORIZED
|| !isBearerAuth(registryClient.authorization.get())
|| ++bearerTokenRefreshes >= MAX_BEARER_TOKEN_REFRESH_TRIES) {
throw ex;
}
// Because we successfully did bearer authentication initially, getting 401 here probably
// means the token was expired.
String wwwAuthenticate = ex.getHttpResponseException().getHeaders().getAuthenticate();
registryClient.authorization.set(registryClient.refreshBearerAuth(wwwAuthenticate));
return () -> this;1 participant
Thank you for your interest in contributing! For general guidelines, please refer to
the contributing guide.
Please follow the guidelines below before opening an issue or a PR:
Jib may in some circumstances incorrectly determine that a repository
does not require authentication. Take for instance the case where
caching is used for the manifests or other components of a base image.
If the initial request succeeds then Jib assumes the repository does not
require authentication. In the case of a cache miss where authentication
is required, this may result in a 401 which is not handled and fails the
build. Jib should use the /v2/ endpoint to initially determine if the
repository requires authentication.
Fixes: #4385