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
fix registry auth regression #97
Conversation
introduced with commit: c30d664 Signed-off-by: Jonas Galvão Xavier <jonas.agx@gmail.com>
validated against: |
@@ -98,6 +91,10 @@ func prepareRemoteOptions(ref name.Reference, registryOptions *image.RegistryOpt | |||
authenticator := registryOptions.Authenticator(ref.Context().RegistryStr()) | |||
if authenticator != nil { | |||
opts = append(opts, remote.WithAuth(authenticator)) | |||
} else { |
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.
Are we putting this here now because we still want InsecureSkipTLSVerify
to be respected from the config?
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.
Thanks for adding this back!
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.
It was removed mistakenly, I expanded the PR description to avoid confusion.
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.
LGTM
@@ -98,6 +91,10 @@ func prepareRemoteOptions(ref name.Reference, registryOptions *image.RegistryOpt | |||
authenticator := registryOptions.Authenticator(ref.Context().RegistryStr()) | |||
if authenticator != nil { | |||
opts = append(opts, remote.WithAuth(authenticator)) | |||
} else { | |||
// use the Keychain specified from a docker config file. | |||
log.Debugf("no registry credentials configured, using the default keychain") |
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.
maybe Infof
?
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 vote would be to keep it as debug. (my mental model: info = let me know big-picture execution stages, debug = tell me more about decision points)
While adding Podman I mistakenly removed a registry condition[1]. The problem was spotted by tests in Syft and Grype.
Question: should that behavior be enforced by tests in Stereoscope?
Also added extra unit tests around Podman.
[1] commit: c30d664
Signed-off-by: Jonas Galvão Xavier jonas.agx@gmail.com