-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Re-enable cross-repository blob mounts #1793
Conversation
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryClient.java
Outdated
Show resolved
Hide resolved
jib-core/src/test/java/com/google/cloud/tools/jib/http/AuthorizationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good.
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryAuthenticator.java
Outdated
Show resolved
Hide resolved
jib-core/src/test/java/com/google/cloud/tools/jib/http/AuthorizationTest.java
Outdated
Show resolved
Hide resolved
jib-core/src/test/java/com/google/cloud/tools/jib/http/AuthorizationTest.java
Outdated
Show resolved
Hide resolved
jib-core/src/test/java/com/google/cloud/tools/jib/http/AuthorizationTest.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
* @return true if the repository was covered | ||
*/ | ||
public boolean canAccess(String repository, String access) { | ||
// if null then we assume that all repositories are granted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth explaining this in the Javadoc too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't understand why "null" is valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other non-Docker Container Registry Token methods don't provide this information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the method name and the Javadoc can still be confusing. I think we just need to clarify (and justify). For example, I can create an Authorization
instance with fromBasicCredentials("my-username", "my-passwd")
. Then canAccess("any repository", "pull or push or anything")
will always return true. I feel this is not intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about making this specific to the blob-mount: canAttemptBlobMount(repository)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That technically works. Man, naming is hard. I see depending on the name, we may want to relocate this class from the .http
package. (Relocation can be done later.) This class is meant for holding the very string value for the Authorization HTTP header (i.e., for Authorization: <some schema> <token value>
) as explained in the class Javadoc, hence was holding the two fields schema
and token
. I thought repositoryGrants
was OK, because it is actually information in token
. But if we incorporate the notions of BLOb or cross-repo mount, it may make sense to get this out of .http
.
That said, I don't have a strong opinion about what the name should be. Maybe the logic for canAttemptBlobMount
can be outside of this method, I don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point. I'll move this into RegistryClient
— it's only called from pushBlobStep
.
- be more explicit for JWT payload source - use JsonTemplateMapper rather than third-party library for parsing JWT - simplify RegistryClient factory use
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/http/Authorization.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM.
* @return true if the repository was covered | ||
*/ | ||
public boolean canAccess(String repository, String access) { | ||
// if null then we assume that all repositories are granted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the method name and the Javadoc can still be confusing. I think we just need to clarify (and justify). For example, I can create an Authorization
instance with fromBasicCredentials("my-username", "my-passwd")
. Then canAccess("any repository", "pull or push or anything")
will always return true. I feel this is not intuitive.
Moved the Docker Registry Bearer Token related code into RegistryClient, though I kept the test separate as |
jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryClient.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious as to why we are "re-enabling", did it work in the past?
Do you think we should hide this behind a flag for now? |
okay, so makes sense to enable by default |
I somehow fat-fingered the commit button before I could edit that mess of a commit message. Blech. |
Life goes on |
Re-enables
mount
/from
for #169. The speed-up can be immense. Building and pushing an image withopenjdk:8
as the base image to a new Docker Hub image takes more than 3 minutes, whereas withmount
/from
it takes about 8 seconds.This implementation is is similar in spirit to #1024, but we do some additional checking to ensure we have permission to request a blob mount. Basically the Docker Registry Bearer Token includes the set of repositories and their granted permission. Private repositories won't be included if we don't have permission.
High-level summary of the changes:
Authorization
now parses the provided Bearer Token to maintain a list of repositories and their granted permissions. It provides a new method,canAccess(repository, access-scope)
. For non-bearer tokens,canAccess()
returns true.RegistryClient.pushBlob()
checks that theAuthorization
allows a cross-repository mount and ignores the mount point if not (this avoids the @chanseokoh's francium25 and francium25paran permission problem discovered previously)BuildConfiguration.newTargetImageRegistryClientFactory()
configures the targetRegistryClient
to request permissions for the base and target repositories when base and target registries are the same, which is recorded in theRegistryEndpointRequestProperties
object as a source image nameRegistryAuthenticator
requests permission for the source repository when set. A key change here is that if authorizing against the additional repository fails, we retry with only the target repository. NOTE: this doesn't seem to happen in practice with Docker Hub¸, and hence the need to check the resulting of the Docker Registry Bearer Token's access grants insteadI debated whether I should provide a property to disable blob mounts or if I should rework
RegistryClient.pushBlob()
to retry if the blob-mount fails. The Registry Spec saysFixes #169