Skip to content

Conversation

@cardyok
Copy link
Contributor

@cardyok cardyok commented Apr 14, 2023

when max_concurrent_download is specified, get requests are not sent immediately for all layers. Token might timeout and we need to try to fetch it again.

eg. max_concurrent_download = 3, fooimg:latest has 4 blob layers, network is poor and first 3 layers took 2hr to finish downloading. when 4th layer begins, it tries to reused the token in host(which has already expired), invalid_token err is thrown.

@k8s-ci-robot
Copy link

Hi @cardyok. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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/test-infra repository.

@cardyok cardyok changed the title [Draft] try fetch token again when token timeouts [WIP] try fetch token again when token timeouts Apr 14, 2023
@cardyok cardyok marked this pull request as draft April 14, 2023 08:00
when max_concurrent_download is specified, get requests are not sent immediately for all layers. Token might timeout and we need to try to fetch it again.

Signed-off-by: Cardy.Tang <zuniorone@gmail.com>
@cardyok cardyok force-pushed the remote_token_resolve branch from 847b62d to 296d67e Compare April 19, 2023 06:29
@cardyok cardyok marked this pull request as ready for review April 19, 2023 06:30
@cardyok cardyok changed the title [WIP] try fetch token again when token timeouts try fetch token again when token timeouts Apr 19, 2023
@cardyok
Copy link
Contributor Author

cardyok commented Apr 20, 2023

not sure if this change is appropriate, ptal @fuweid @dmcgowan

return err
}

if c.Parameters["error"] == "invalid_token" {
Copy link
Member

Choose a reason for hiding this comment

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

is it from special platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it from special platform?

Not sure if this is the generic err message protocol for all platforms, but I verified dockerhub and alibaba container registry both gave this errcode when invalid or expired token is provided in header.

Copy link
Member

Choose a reason for hiding this comment

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

invalid_token is defined in RFC 6750, as one of the three error codes that could be returned by the server.

But just wonder why invalidAuthorization didn't return an error for this case?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like this could be slotted into invlaidAuthorization here:

@k8s-ci-robot
Copy link

PR needs rebase.

Details

Instructions 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/test-infra repository.

@github-actions
Copy link

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

@github-actions github-actions bot added the Stale label Apr 21, 2024
@fuweid
Copy link
Member

fuweid commented Apr 21, 2024

closed by #8735

@fuweid fuweid closed this Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants