docker: cache bearer tokens#234
Conversation
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Yeah, something like that.
docker.io seems to expire tokens in 5 minutes, which does seem a bit risky.
| // | ||
| // debugging: https://github.com/containers/image/pull/211#issuecomment-273426236 and follows up | ||
| func (c *dockerClient) setupRequestAuth(req *http.Request) error { | ||
| if c.token != "" { |
There was a problem hiding this comment.
Move this into the case "bearer" I think.
runcom
force-pushed
the
cache-tokens
branch
2 times, most recently
from
a9de767 to
0a9d8ea
Compare
|
@mtrmac PTAL - followed https://github.com/docker/distribution/blob/master/docs/spec/auth/token.md and what docker does also. |
|
copying a multi-layers image shows a gain of around 4-5 seconds: # w/o this patch
./skopeo copy docker://registry oci:registry2 0.31s user 0.24s system 2% cpu 21.696 total
# with this patch
./skopeo copy docker://registry oci:registry 0.27s user 0.20s system 2% cpu 17.360 total |
| if err != nil { | ||
| return err | ||
| now := time.Now() | ||
| if now.After(c.tokenExpiration) || c.token == nil { |
There was a problem hiding this comment.
Switch the order of the checks; c.tokenExpiration is uninitialized if c.token == nil (and the c.token == nil check is more efficient, but that should not be the decisive criterion).
Is the now variable necessary? AFAICS it has a single user.
| type bearerToken struct { | ||
| Token string `json:"token"` | ||
| ExpiresIn int `json:"expires_in"` | ||
| IssuesAt time.Time `json:"issued_at"` |
| if token.ExpiresIn < minimumTokenLifetimeSeconds { | ||
| token.ExpiresIn = minimumTokenLifetimeSeconds | ||
| logrus.Debugf("Increasing token expiration to: %d seconds", token.ExpiresIn) | ||
| } |
There was a problem hiding this comment.
(This works just fine. It could also be worth considering the case when Issue[sd]At is in the past, possibly even more than minimumTokenLifetimeSeconds in the past. But the current code, in that case, uses the token exactly once, which is a perfectly reasonable behavior. The alternative of extending the token to be valid at least minimumTokenLifetimeSeconds seems to be much worse.)
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
|
@mtrmac fixed your comments. |
|
👍 Thanks! (Also, a nice performance improvement.) |
|
👍 |
|
Nice lets get new skopeo builds. |
|
Awesome. |
|
Nice! Can we pull this into cri-o? |
|
@mrunalp I will (likely later today or tomorrow morning, we need to update many of the deps in CRI-O I had opened an issue iirc) |
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
4 participants
@mtrmac PTAL, just WIP for now as I have to handle tokens refreshing and stuff like that.
Signed-off-by: Antonio Murdaca runcom@redhat.com