Skip to content
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

resolver: add better pooling and custom authenticator #1636

Merged
merged 8 commits into from
Aug 14, 2020

Conversation

tonistiigi
Copy link
Member

Changes all remote resolvers access through the new pooling functions.

Resolvers should now be always reused for concurrent access. Repeated access reuse tokens if they are still valid and if they have permissions to access them. Token expiring does not cause the build to fail anymore. The fallback order of request methods has been changed to avoid an extra request.

Some copy-paste in here can be removed when someone merges containerd/containerd#4445

In a follow-up I want to extend this to move token requests to the client. Could also add some authentication prediction for common registries but that probably will not be that significant atm.

@sipsma Added WithSession method to update existing resolver but didn't change anything about managing remotes yet. We can discuss these follow up changes after this is merged.

@tonistiigi
Copy link
Member Author

hmm. some failed to load cache key errors in ci

@tonistiigi
Copy link
Member Author

green now

@@ -71,7 +71,7 @@ require (
)

replace (
github.com/containerd/containerd => github.com/containerd/containerd v1.4.0-beta.2.0.20200728183644-eb6354a11860
github.com/containerd/containerd => github.com/containerd/containerd v1.4.0-beta.2.0.20200730150746-fa1220fce33f
Copy link
Member

Choose a reason for hiding this comment

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

Can we just vendor rc.1 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No objections for vendoring newer containerd. Was just trying to keep changes in this PR minimal and specific to the auth feature.

util/resolver/pool.go Show resolved Hide resolved
util/resolver/pool.go Outdated Show resolved Hide resolved
util/resolver/pool.go Outdated Show resolved Hide resolved
@tonistiigi
Copy link
Member Author

@sipsma Thanks. Updated

Copy link
Collaborator

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

LGTM

@crazy-max
Copy link
Member

crazy-max commented Sep 13, 2020

@tonistiigi I've got the same issue with GitHub Container Registry. Switching to image=moby/buildkit:master image seems to work.

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.

4 participants