-
Notifications
You must be signed in to change notification settings - Fork 173
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
Fixes #333 Handle parallel calls to add with osxkeychain #334
base: master
Are you sure you want to change the base?
Fixes #333 Handle parallel calls to add with osxkeychain #334
Conversation
When `docker login` is called in parallel, some of the calls fail with the error `The specified item already exists in the keychain`. This PR checks for this specific error in `osxkeychain.Add` and returns ok. If one process loses the race in this case, the desired credentials were saved by another process and we can ignore the error. Signed-off-by: Justin Randell <justin.p.randell@gmail.com>
49860e5
to
6e1f8f7
Compare
case errCredentialsAlreadyExist: | ||
// If docker login is called in parallel, we may try to | ||
// save the same credentials twice. In that case, return | ||
// ok, because although we lost the race, the desired | ||
// credentials were saved by another process. | ||
return nil |
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'm a bit on the fence if this is the right thing to do here. While the related ticket is about the same credentials being stored concurrently, it's also possible that different credentials are being stored for the same registry, in which case an error may be most appropriate.
Perhaps we could add a ErrConflict
for this, and a utility to detect that, so that a client could choose to handle it;
docker-credential-helpers/credentials/error.go
Lines 100 to 111 in db1da9d
// IsCredentialsMissingServerURL returns true if the error | |
// was an errCredentialsMissingServerURL. | |
func IsCredentialsMissingServerURL(err error) bool { | |
var target errCredentialsMissingServerURL | |
return errors.As(err, &target) | |
} | |
// IsCredentialsMissingServerURLMessage checks for an | |
// errCredentialsMissingServerURL in the error message. | |
func IsCredentialsMissingServerURLMessage(err string) bool { | |
return strings.TrimSpace(err) == errCredentialsMissingServerURLMessage | |
} |
@laurazard any thoughts?;
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.
Apologies, took me a while to work my way here. I'll TAL.
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.
My intuition is that we should handle this a la EINTR
, i.e. "things happened while your code was blocked on some call and you should handle it".
Otherwise, like @thaJeztah mentioned, we have no guarantees that the credentials saved were the desired ones by this process.
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.
@beejeebus what do you think?
When
docker login
is called in parallel, some of the calls fail with the errorThe specified item already exists in the keychain
.This PR checks for this specific error in
osxkeychain.Add
and returns ok. If one process loses the race in this case, the desired credentials were saved by another process and we can ignore the error.