Bug 1982498: default to podman credential configs #893

atiratree · 2021-08-09T17:20:08Z

we should first try podman location for detecting credentials and then check the docker ones

relevant docs:
https://man.archlinux.org/man/community/containers-common/containers-auth.json.5.en
http://docs.podman.io/en/latest/markdown/podman-login.1.html

openshift-ci · 2021-08-09T17:20:10Z

@atiratree: An error was encountered searching for bug 1982498 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


could not unmarshal response body: invalid character '<' looking for beginning of value

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1982498: default to podman credential configs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-09T17:23:26Z

@atiratree: This pull request references Bugzilla bug 1982498, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-09T17:24:29Z

@atiratree: An error was encountered querying GitHub for users with public email (wzheng@redhat.com) for bug 1982498 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: connect: connection refused

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2021-08-09T17:24:42Z

/bugzilla refresh

openshift-ci · 2021-08-09T17:25:21Z

@atiratree: This pull request references Bugzilla bug 1982498, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @wzheng1

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2021-08-09T17:26:31Z

/assign @soltysh

xiuwang · 2021-08-10T02:07:12Z

/bugzilla refresh

openshift-ci · 2021-08-10T02:07:16Z

@xiuwang: This pull request references Bugzilla bug 1982498, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @xiuwang

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2021-08-10T09:51:44Z

/retest

atiratree · 2021-08-10T12:38:06Z

/retest

xiuwang · 2021-08-11T07:50:24Z

Build oc from this pr, and report two premerged bugs,
https://bugzilla.redhat.com/show_bug.cgi?id=1992474
https://bugzilla.redhat.com/show_bug.cgi?id=1992468

This pr didn't account the situation when ${XDG_RUNTIME_DIR} and ${XDG_CONFIG_HOME} not set in host. Then the podman login auth will located to a replacement directory with similar capabilities . Like this:
DEBU[0000] Returning credentials from /run/containers/0/auth.json

atiratree · 2021-08-11T18:13:34Z

@xiuwang Thanks for the feedback. I have reassessed the logic we have in oc and decided to use the same logic as podman uses. The logic itself is quite complex and is fortunately shared via github.com/containers/image library.

Although bear in mind that even podman doesn't fallback to other locations when ${REGISTRY_AUTH_FILE} or --authfile is specified. So the oc is using the same convention.

Regarding the oc login I think it is not feasible without breaking backwards compatibility (please see the BZ for more details).

atiratree · 2021-08-11T21:11:36Z

/retest

atiratree · 2021-08-12T11:01:57Z

/retest

xiuwang · 2021-08-17T07:14:45Z

Hi @atiratree Thanks for update.
I check the oc cmd with new commits.
the option has included ~/.dockercfg file, but oc doesn't query this file when other target files not existing.

-a, --registry-config='': Path to your registry credentials. Alternatively env variable ${REGISTRY_AUTH_FILE} can be
also specified. Defaults to ${XDG_RUNTIME_DIR}/containers/auth.json, ${XDG_CONFIG_HOME}/containers/auth.json,
/run/containers/${UID}/auth.json, ${DOCKER_CONFIG}, ~/.docker/config.json, ~/.dockercfg

xiuwang · 2021-08-17T07:16:02Z

This issue https://bugzilla.redhat.com/show_bug.cgi?id=1992474 doesn't be fixed yet.

atiratree · 2021-08-17T11:15:29Z

Hi @atiratree Thanks for update.
I check the oc cmd with new commits.
the option has included ~/.dockercfg file, but oc doesn't query this file when other target files not existing.

@xiuwang
I cannot reproduce this. Both options are working fine for me - when other configs exist or not exist. Are you sure you are using the right format? kubernetes/kubernetes#12626 (comment)

Can you please share how are you reproducing this and what errors you are getting?

soltysh · 2021-08-17T15:38:23Z

pkg/cli/image/manifest/manifest.go

@@ -51,7 +51,7 @@ type SecurityOptions struct {
 }

 func (o *SecurityOptions) Bind(flags *pflag.FlagSet) {
-	flags.StringVarP(&o.RegistryConfig, "registry-config", "a", o.RegistryConfig, "Path to your registry credentials (defaults to ~/.docker/config.json)")
+	flags.StringVarP(&o.RegistryConfig, "registry-config", "a", o.RegistryConfig, "Path to your registry credentials. Alternatively env variable ${REGISTRY_AUTH_FILE} can be also specified. Defaults to ${XDG_RUNTIME_DIR}/containers/auth.json, ${XDG_CONFIG_HOME}/containers/auth.json, /run/containers/${UID}/auth.json, ${DOCKER_CONFIG}, ~/.docker/config.json, ~/.dockercfg")


We can't change the order, since that might break current users. We'll have to do it gradually:

Add podman locations after .docker/config.json, we talked with @atiratree about it, it'll be done by re-using the current logic to read .docker/config.json and add everything else using github.com/containers/image library. We just need to ensure that the former has higher precedence than the latter.

Add a warning when using the old loading order.

Add support for the new order, but behind an env var, CONTAINERS_AUTH_ORDER or something like that. This would basically drop the legacy support we currently have for .docker/config.json and just use github.com/containers/image/. This would apply both to reading and writing as well.

Don't forget:

Add release notes about the loading order changes including the env var to enable it - CONTAINERS_AUTH_ORDER. Mention supporting REGISTRY_AUTH_FILE as well.
Call out that in 2 release, we'll flip CONTAINERS_AUTH_ORDER to on by default.

@soltysh I think I have resolved all of the issues. Can you please rereview?

regarding the release notes, I will write them just after we confirm the name of the env variable

xiuwang · 2021-08-31T02:43:12Z

@atiratree After I used the previous format for .dockercfg, the cmd could get the auth.
Validate ~/.docker/config.json (deprecated),${XDG_RUNTIME_DIR}/containers/auth.json, ${XDG_CONFIG_HOME}/containers/auth.json, /run/containers/${UID}/auth.json, ${DOCKER_CONFIG}, ~/.dockercfg . And Defaults can be changed via REGISTRY_AUTH_PREFERENCE env variable.

/label qe-approved

atiratree · 2021-09-02T14:01:30Z

/bugzilla refresh

openshift-ci · 2021-09-02T14:01:34Z

@atiratree: This pull request references Bugzilla bug 1982498, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "4.10.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2021-10-01T18:21:20Z

/retest

atiratree · 2021-10-01T20:03:06Z

/bugzilla refresh
/retest

openshift-ci · 2021-10-01T20:03:10Z

@atiratree: This pull request references Bugzilla bug 1982498, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @xiuwang

In response to this:

/bugzilla refresh
/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2021-10-15T15:08:10Z

/retest

atiratree · 2021-10-27T11:34:12Z

rebased

atiratree · 2021-11-03T11:40:06Z

/retest

- introduce REGISTRY_AUTH_FILE env variable as an alternative to --registry-config - introduce REGISTRY_AUTH_PREFERENCE env variable for switching preference for default auth config location (podman, docker)

…tials - introduce REGISTRY_AUTH_FILE env variable as an alternative to --registry-config - introduce REGISTRY_AUTH_PREFERENCE env variable for switching preference for default auth config location (podman, docker)

soltysh

/lgtm
/approve
nits can be addressed in followup

soltysh · 2021-11-30T20:09:07Z

pkg/cli/image/manifest/manifest.go

@@ -50,7 +50,8 @@ type SecurityOptions struct {
 }

 func (o *SecurityOptions) Bind(flags *pflag.FlagSet) {
-	flags.StringVarP(&o.RegistryConfig, "registry-config", "a", o.RegistryConfig, "Path to your registry credentials (defaults to ~/.docker/config.json)")
+	// TODO: fix priority and deprecation notice in 4.12
+	flags.StringVarP(&o.RegistryConfig, "registry-config", "a", o.RegistryConfig, "Path to your registry credentials. Alternatively REGISTRY_AUTH_FILE env variable can be also specified. Defaults to  ~/.docker/config.json (order deprecated), ${XDG_RUNTIME_DIR}/containers/auth.json, ${XDG_CONFIG_HOME}/containers/auth.json, /run/containers/${UID}/auth.json, ${DOCKER_CONFIG}, ~/.dockercfg. Defaults can be changed via REGISTRY_AUTH_PREFERENCE env variable to docker or podman.")


Suggestion:

Path to your registry credentials. Alternatively REGISTRY_AUTH_FILE env variable can be specified.
Defaults to ~/.docker/config.json, ${XDG_RUNTIME_DIR}/containers/auth.json, ${XDG_CONFIG_HOME}/containers/auth.json, /run/containers/${UID}/auth.json, ${DOCKER_CONFIG}, ~/.dockercfg. The order can be changed via REGISTRY_AUTH_PREFERENCE env variable to docker (current default - deprecated) or podman (prioritizes podman credentials over docker) .

soltysh · 2021-11-30T20:12:32Z

pkg/helpers/image/helpers.go

+		result = RegistryAuthConfigPreference(authPreference)
+	} else {
+		// TODO: remove once deprecated in 4.12
+		klog.Warningln("Defaulting of registry auth file to \"${HOME}/.docker/config.json\" is deprecated. The default will be switched to podman config locations in 4.12")


Nit: in the future version

we don't specify exact version in these messages, in case we forget 😉

soltysh · 2021-11-30T20:14:00Z

pkg/cli/registry/login/login.go

 	"net/http"
 	"net/url"
 	"os"
 	"path/filepath"
 	"strings"

+	"github.com/containers/image/v5/pkg/docker/config"
+	imageTypes "github.com/containers/image/v5/types"


Nit: most import aliases are lowercase to differentiate that from variables.

Can we maybe make these imports be:
dockerconfig and containerstypes so it's explicit where these are coming while reading the code

soltysh · 2021-11-30T20:19:10Z

pkg/cli/registry/login/login.go

-	flag.StringVarP(&o.ConfigFile, "registry-config", "a", o.ConfigFile, "The location of the file your credentials will be stored in. Default is Docker config.json")
-	flag.StringVar(&o.ConfigFile, "to", o.ConfigFile, "The location of the file your credentials will be stored in. Default is Docker config.json")
+	flag.StringVarP(&o.ConfigFile, "registry-config", "a", o.ConfigFile, "The location of the file your credentials will be stored in. Alternatively REGISTRY_AUTH_FILE env variable can be also specified. Default is Docker config.json (deprecated). Default can be changed via REGISTRY_AUTH_PREFERENCE env variable to docker or podman.")
+	flag.StringVar(&o.ConfigFile, "to", o.ConfigFile, "The location of the file your credentials will be stored in. Alternatively REGISTRY_AUTH_FILE env variable can be also specified. Default is Docker config.json (deprecated). Default can be changed via REGISTRY_AUTH_PREFERENCE env variable to docker or podman.")


Suggestion:

The location of the file your credentials will be stored in. Alternatively REGISTRY_AUTH_FILE env variable can be specified. Defaults to ~/.docker/config.json. Default can be changed via REGISTRY_AUTH_PREFERENCE env variable to docker (current default - deprecated) or podman (prioritizes podman credentials over docker) .

This will be inline with that other place where I left similar comment.

soltysh · 2021-11-30T20:22:25Z

pkg/cli/registry/login/login.go

-	contents, err := json.MarshalIndent(cfg, "", "  ")
-	if err != nil {
+	ctx := &imageTypes.SystemContext{AuthFilePath: o.ConfigFile}
+	if err := config.SetAuthentication(ctx, o.HostPort, o.Credentials.Username, o.Credentials.Password); err != nil {


This makes the code so much easier 😅 we should've done that long time ago.

openshift-ci · 2021-11-30T20:27:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh

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:

~~OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2021-11-30T20:29:38Z

@atiratree: All pull requests linked via external trackers have merged:

openshift/oc#893

Bugzilla bug 1982498 has been moved to the MODIFIED state.

In response to this:

Bug 1982498: default to podman credential configs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot requested review from mfojtik and smarterclayton August 9, 2021 17:20

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Aug 9, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 9, 2021

openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Aug 9, 2021

openshift-ci bot requested a review from wzheng1 August 9, 2021 17:25

openshift-ci bot assigned soltysh Aug 9, 2021

openshift-ci bot requested a review from xiuwang August 10, 2021 02:07

atiratree force-pushed the podman-credentials branch from 626f543 to a224a8c Compare August 11, 2021 17:50

atiratree force-pushed the podman-credentials branch 2 times, most recently from 56abe09 to ebadfd8 Compare August 11, 2021 18:45

soltysh reviewed Aug 17, 2021

View reviewed changes

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 31, 2021

openshift-ci bot removed the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 2, 2021

openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 2, 2021

atiratree mentioned this pull request Sep 10, 2021

Bug 1858760: allow Authorization Service URL to differ from Registry URL when retrieving tokens #924

Merged

2 tasks

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Oct 1, 2021

atiratree force-pushed the podman-credentials branch from a046188 to 1c3d992 Compare October 27, 2021 11:33

use podman (containers/image) logic for loading podman/docker config

decbc0e

- introduce REGISTRY_AUTH_FILE env variable as an alternative to --registry-config - introduce REGISTRY_AUTH_PREFERENCE env variable for switching preference for default auth config location (podman, docker)

atiratree force-pushed the podman-credentials branch from 1c3d992 to e1e3858 Compare November 29, 2021 14:58

atiratree added 2 commits November 29, 2021 16:03

update vendor folder

c065372

atiratree force-pushed the podman-credentials branch from e1e3858 to c065372 Compare November 29, 2021 15:05

soltysh approved these changes Nov 30, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2021

openshift-merge-robot merged commit 94c8c68 into openshift:master Nov 30, 2021

This was referenced Dec 1, 2021

fix registry-config messages and imports #983

Merged

OCP 4.10 Release Notes Tracker openshift/openshift-docs#37586

Closed

zaneb mentioned this pull request Dec 1, 2021

Login fails if target file is empty #985

Closed

Bug 1982498: default to podman credential configs #893

Bug 1982498: default to podman credential configs #893

atiratree commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

atiratree commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

atiratree commented Aug 9, 2021

xiuwang commented Aug 10, 2021

openshift-ci bot commented Aug 10, 2021

atiratree commented Aug 10, 2021

atiratree commented Aug 10, 2021

xiuwang commented Aug 11, 2021

atiratree commented Aug 11, 2021 •

edited

atiratree commented Aug 11, 2021

atiratree commented Aug 12, 2021

xiuwang commented Aug 17, 2021

xiuwang commented Aug 17, 2021

atiratree commented Aug 17, 2021

soltysh Aug 17, 2021

atiratree Aug 19, 2021

xiuwang commented Aug 31, 2021

atiratree commented Sep 2, 2021

openshift-ci bot commented Sep 2, 2021

atiratree commented Oct 1, 2021

atiratree commented Oct 1, 2021

openshift-ci bot commented Oct 1, 2021

atiratree commented Oct 15, 2021

atiratree commented Oct 27, 2021

atiratree commented Nov 3, 2021

soltysh left a comment

soltysh Nov 30, 2021

soltysh Nov 30, 2021

soltysh Nov 30, 2021

soltysh Nov 30, 2021

soltysh Nov 30, 2021

soltysh Nov 30, 2021

openshift-ci bot commented Nov 30, 2021

openshift-ci bot commented Nov 30, 2021

Bug 1982498: default to podman credential configs #893

Bug 1982498: default to podman credential configs #893

Conversation

atiratree commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

atiratree commented Aug 9, 2021

openshift-ci bot commented Aug 9, 2021

atiratree commented Aug 9, 2021

xiuwang commented Aug 10, 2021

openshift-ci bot commented Aug 10, 2021

atiratree commented Aug 10, 2021

atiratree commented Aug 10, 2021

xiuwang commented Aug 11, 2021

atiratree commented Aug 11, 2021 • edited

atiratree commented Aug 11, 2021

atiratree commented Aug 12, 2021

xiuwang commented Aug 17, 2021

xiuwang commented Aug 17, 2021

atiratree commented Aug 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiuwang commented Aug 31, 2021

atiratree commented Sep 2, 2021

openshift-ci bot commented Sep 2, 2021

atiratree commented Oct 1, 2021

atiratree commented Oct 1, 2021

openshift-ci bot commented Oct 1, 2021

atiratree commented Oct 15, 2021

atiratree commented Oct 27, 2021

atiratree commented Nov 3, 2021

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 30, 2021

openshift-ci bot commented Nov 30, 2021

atiratree commented Aug 11, 2021 •

edited