Hi @iain-macdonald. 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.

might related to #8388
since currently expires_in is not an required field for registry, it might be better if we do not rely on this field.

might related to #8388 since currently expires_in is not an required field for registry, it might be better if we do not rely on this field.

That makes sense. I'll admit I'm not an expert with golang, but my understanding is if it isn't set, the default value assigned to ExpiresInSeconds will be 0 which will trigger the if-condition on line 341 and fallback to the existing logic. Is that correct, or am I misunderstanding something?

might related to #8388 since currently expires_in is not an required field for registry, it might be better if we do not rely on this field.

That makes sense. I'll admit I'm not an expert with golang, but my understanding is if it isn't set, the default value assigned to ExpiresInSeconds will be 0 which will trigger the if-condition on line 341 and fallback to the existing logic. Is that correct, or am I misunderstanding something?

yea you right it will fallback. But token expiration problem still exists if expires_in is not include in registry response

might related to #8388 since currently expires_in is not an required field for registry, it might be better if we do not rely on this field.

That makes sense. I'll admit I'm not an expert with golang, but my understanding is if it isn't set, the default value assigned to ExpiresInSeconds will be 0 which will trigger the if-condition on line 341 and fallback to the existing logic. Is that correct, or am I misunderstanding something?

yea you right it will fallback. But token expiration problem still exists if expires_in is not include in registry response

Yes, and this PR preserves the existing behavior in the case, so it's strictly an improvement for the cases where expires_in is provided.

Hey, it's been a few weeks without any feedback from the containerd team. Are there any next steps for me to take here? I've tried to follow the instructions in https://github.com/containerd/project/blob/main/CONTRIBUTING.md, but please let me know if I've missed a step or something.

Hey, checking in one more time. Let me know if there are any additional steps I need to take to get this PR reviewed. Thanks!

This change looks good to accept. I have a few questions though.

we encountered errors streaming container images using the soci-snapshotter when these auth tokens expired and weren't refreshed correctly.

What were the errors? I'm wondering if @cardyok 's PR fixes the general issue without relying on the registry.

Which registries today are setting this? (As this is only a recommended field https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2)

The errors are 401 (unauthorized) errors, and the Docker Hub registry sets this field.

@cardyok 's PR also fixes the issue we observed, Docker Hub sets the error field in its response as well, though it requires an additional roundtrip to/from the container registry to learn when access tokens have expired so it's slightly slower. That said, it's much simpler.

Is there any concern with this? It looks like a good change that can save a round trip with the registry.
I guess if we are suddnely using a field that may have a junk value in it that could cause problems.

Hey all, apologies for losing track of this for a little bit. I just rebased it, so it should be ready for review again. Thanks!

Thanks for the review, Derek. Can you suggest another reviewer so I can get this change approved and merged, or recommend different steps forward if not that?