-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add --compat-auth-file to login/logout #1731
Add --compat-auth-file to login/logout #1731
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/auth/auth.go
Outdated
// If the Docker configuration exists in the default ~/.docker/config.json location, | ||
// we DO NOT write to it; instead, we update auth.json in the default path. | ||
// Only if the user explicitly sets DOCKER_CONFIG, we write to that config.json. | ||
if dockerConfig := os.Getenv("DOCKER_CONFIG"); dockerConfig != "" { |
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.
If REGISTRY_AUTH_FILE and DOCKER_CONFIG is set, then we use DOCKER_CONFIG? We should probably at least logrus_debug this.
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.
Oops, I’ll fix that.
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.
Hopefully fixed.
@@ -45,7 +45,7 @@ type LogoutOptions struct { | |||
// GetLoginFlags defines and returns login flags for containers tools | |||
func GetLoginFlags(flags *LoginOptions) *pflag.FlagSet { | |||
fs := pflag.FlagSet{} | |||
fs.StringVar(&flags.AuthFile, "authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override") | |||
fs.StringVar(&flags.AuthFile, "authfile", "", "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override") |
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 like to reveal this information to the user via
podman login --help
Why are you removing this information?
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.
We need to detect whether the user incorrectly passed --authfile …
--compat-auth-file …
, and setting a default value here interferes with that.
Also, GetDefaultAuthFile
does not do what it suggests; on almost all systems it returns ""
.
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.
The idea is revealing the default, if user specifies the --authfile or --docker-auth-file, then they modified the defaults.
This should be showing $XDG_RUNTIME_DIR/UID/auth.json almost everywhere unless there is a bug.
It would also show the correct value if DOCKER_AUTH_FILE or REGISTRY_AUTH_FILE environment is set.
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.
But that’s not how it works.
On the read paths (not login
/logout
, this has always been incorrect because “no --authfile
” and --authfile=$the_default_from_--help
” don’t do the same thing: the former searches for auth.json
in /run
, and in ~/.config
, and for two formats of Docker-defined files. The latter only reads that one file. (I think fairly strongly that this behavior is correct.)
And, on both read paths and login/logout, if a credential helper is configured in registries.conf
, the value of --authfile
(and of the new --compat-auth-file
) might be effectively ignored. (I don’t know whether that’s desirable, it’s just what the code does.)
I see some value in displaying the paths that will be used in --help
; but doing that by setting “the default value of the flag”, and in effect having the flag always set and non-empty, is EDIT not an accurate way to do that.
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.
Change
fs.StringVar(&flags.AuthFile, "authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
To
fs.String("authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
Then during the main function set the values only if they changed, otherwise rely on the regular processioning as if they were "".
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.
- That doesn’t actually implement the feature you want:
GetDefaultAuthFile
returns""
on most systems. (and it must continue to do so, given how it is currently called in Buildah/Podman, otherwise Podman would stop searching in any of the ~3 other locations.) - And a description of the default behavior into
--help
as a default value of--authfile
is actively confusing, as described above. The default behavior is a credential helper user and/or a file search, passing any--authfile
value disables that search (and not the credential helper use) - As a special case of that confusion, see how
$DOCKER_CONFIG
is handled.GetDefaultAuthFile
must continue to read that environment variable, because Podman and Buildah “credential reader” commands rely on that and removing that behavior would be a regression, butlogin
/logout
must *EDIT not use that value in--authfile
, because they need to write in the compatibility format.
I don’t object to idea of showing default locations in --help
. But that feature mostly doesn’t exist right now; it is not the topic of this PR; and I don’t think showing the default locations (note the plural!) in the default value of --authfile
can work.
77dd5ce
to
c36a4fc
Compare
These flags should be ignored if the customer did not set it. We should add accessors to see if the fields are set if flags().Changed { |
Yes, that is an approach to display a “default” value for |
c36a4fc
to
2e22005
Compare
> go get github.com/containers/image/v5@main > go mod tidy && go mod vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that it can be used with a future --compat-auth-file option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We _need_ to know whether the user explicitly set an option or not, because --authfile and --compat-auth-file conflict. Also, --authfile= (unset) and --authfile=$the_default_auth_json have different effects, so setting the default value of the option is _not_ a practical way to show what the system would do if the option were not specified. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
2e22005
to
44e6c07
Compare
Rebased and now no longer depends on c/image from a fork. PRs in users all pass tests, and have been manually tested to interoperate with Docker:
Please review & merge. |
LGTM |
pkg/auth/auth.go
Outdated
case authFile != "" && dockerCompatAuthFile != "": | ||
return nil, errors.New("--authfile and --compat-auth-file can not be set simultaneously") | ||
case authFile != "": | ||
if authFile == defaultDockerConfigPath { | ||
logrus.Warn("--authfile points to ~/.docker/config.json, but the file format is not fully compatible; use --compat-auth-file instead") |
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 am not sure how this package is used everywhere but pointing to cli options in error messages may not be optimal here. I see it used on the podman server API side (without any auth file option there so not a real issue but it might be an issue in the future). It is also used in the bindings test with an authfile so pointing to a cli option there would not make any sense.
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.
You’re completely right that conceptually, auth.go
can be used without cli.go
, so it should not assume any specific option names.
Can you suggest a practical option-name-independent wording? Especially in this message, my first stab is credential file is set to ~/.docker/config.json, but the file format is not fully compatible; specify it using the (Docker?) compatibility credential file option instead
, which is way too long, and not all that helpful for users looking for the option name.
Looking at the callers, most users users of pkg/auth
just call GetDefaultAuthFile
and CheckAuthFile
; the non-trivial callers are just the top-level CLI login/logout, except:
- Podman’s
pkg/api/handlers/compat/auth.go
. In that case neitherAuthFile
norCompatAuthFile
is specified, so these messages should not be reached.- (From a quick read without looking at the history, it seems to me that this API handler should not deal with auth files at all (like
auth.Login
always reads them), and instead of the CLI login it should just calldocker.CheckAuth
directly; andLoginOptions.NoWriteBack
could then be removed.)
- (From a quick read without looking at the history, it seems to me that this API handler should not deal with auth files at all (like
- Podman’s
pkg/bindings/test/auth_test.go
. Awkward, but those errors should not happen with code as is, and future maintainers can hopefully deal with it.
So I think that referring to the cli.go
option names directly is a a clearly imperfect but hopefully-acceptable compromise, but that’s just because I can’t think of good wording; I’ll be happy to change it to any other suggested text.
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.
Perhaps "saving credentials to ~/.docker/config.json but not using Docker-compatible credential format"?
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.
@@ -45,7 +47,8 @@ type LogoutOptions struct { | |||
// GetLoginFlags defines and returns login flags for containers tools | |||
func GetLoginFlags(flags *LoginOptions) *pflag.FlagSet { | |||
fs := pflag.FlagSet{} | |||
fs.StringVar(&flags.AuthFile, "authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override") | |||
fs.StringVar(&flags.AuthFile, "authfile", "", "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override") | |||
fs.StringVar(&flags.DockerCompatAuthFile, "compat-auth-file", "", "path of a Docker-compatible config file to update instead") |
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.
Should this be named compat-authfile
? Ii is inconsistent to have authfile
as one word and then another option split it as auth-file
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 very weak preference is to keep the dash, and to think of the previous authfile
as a mistake.
The spelling with the dash was also suggested by someone else in the design doc, and widely approved there — though I’m not 100% clear all of the reviewers were focused on the dash question.
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.
Yeah I can live with either one as well, I just noticed the inconsistency when looking at this here.
If people didn't complain in the design doc then I am fine keeping it like this.
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.
can we rename authfile
to auth-file
and add an alias to maintain authfile
? Otherwise, I find it confusing too that one has the dash and one hasn't
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.
Migrating to --auth-file
is a good idea.
Sadly this is not the only location, --authfile
is hard-coded all over the place in commands that read credentials (e.g. https://github.com/containers/podman/blob/1d49773bb82a9cb93dbcf652fa613fb9295fd609/cmd/podman/images/push.go#L85 / https://github.com/containers/podman/blob/1d49773bb82a9cb93dbcf652fa613fb9295fd609/cmd/podman/images/push.go#L178 , 28 other instances in Podman outside of this subpackage).
I’m fine with doing that work but I’d prefer to do that after 4.8; --compat-auth-file
is a release blocker and I’m worried that changing the flag name everywhere would require several CI iterations (to adjust tests and to deal with possible bugs, or even to add tests for the alias), and Buildah/Podman CI can manage about 2 iterations a day.
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 am OK with the plan of migrating after 4.8 given our current timing issues
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 have filed containers/podman#20693 to make sure the --auth-file
renaming does not get lost.
Code LGTM; I think all remaining concerns are about exact wording of error/warning messages and the name of the option. |
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Note that the warning can't be disabled. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
44e6c07
to
eb481dc
Compare
/lgtm |
... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to include containers/image#2173, containers/common#1731 and containers/buildah#5143 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>
See individual commit messages for details.
Depends on an unmerged c/image PR, and was not yet tested in consumers.