Skip to content
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

prefer podman authentication file locations #3345

Merged

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Mar 27, 2023

we want to remove preference for docker auth files in favor of podman ones in openshift/oc#1376. Since ci-operator depends on the cli (oc) we need to first merge the changes here before the changes in oc can be merged. This change was announced in 4.10 and a proper warning was shown when using oc commands that work with registries since then.

  • podman ~/.docker/config.json is deprecated in favor of podman authentication file locations (default is ${XDG_RUNTIME_DIR}/containers/auth.json)
  • oc registry login will try to write to this location so XDG_RUNTIME_DIR environment variable must be present and the XDG_RUNTIME_DIR directory created/accessible. Places that need to manipulate with ~/.docker/config.json for backwards compatibility reasons need to specify --to or --registry-config
  • other oc commands that lookup credentials from registry authentication files will first try the podman locations and if the credentials are not found, oc will fallback and check ~/.docker/config.json

- podman ~/.docker/config.json is deprecated in favor of podman
  authentication file locations (default is
  ${XDG_RUNTIME_DIR}/containers/auth.json)
- oc registry login will try to write to this location so
  XDG_RUNTIME_DIR environment variable must be present and the XDG_RUNTIME_DIR
  directory created/accessible. Places that need to manipulate with
  ~/.docker/config.json for backwards compatibility reasons need to
  specify --to or --registry-config
- other oc commands that lookup credentials from registry authentication files
  will first try the podman locations and if the credentials are not found,
  oc will fallback and check ~/.docker/config.json
@@ -433,7 +438,7 @@ objects:

# mirror the release image and override the release image to point to the mirrored one
mkdir /tmp/.docker && cp /etc/openshift-installer/pull-secret /tmp/.docker/config.json
oc registry login
oc registry login --to /tmp/.docker/config.json
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to mention the file explicitly to support the pre 4.10 jobs

@jmguzik
Copy link
Contributor

jmguzik commented Mar 29, 2023

/cc @bbguimaraes
Could have a look at this as well?

@openshift-ci openshift-ci bot requested a review from bbguimaraes March 29, 2023 09:18
Copy link
Contributor

@bbguimaraes bbguimaraes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than a single question. Cancel the hold whenever you like.

/lgtm
/hold

Comment on lines +146 to +147
- name: XDG_RUNTIME_DIR
value: /tmp/home/run
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any oc registry login calls in either the test or teardown containers, are they done by openshift-tests / the installer? Will those calls also need a --to argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbguimaraes These variables are not needed at this time. I am adding this so we that do not have to add them later in case someone needs to call the oc registry login, or have other functionality that depends on this common directory. We may also decide that we want to change/remove the .docker/config.json preferences in the code. Although I am not sure how likely that is. I can remove the support for XDG_RUNTIME_DIR to slim down the PR if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, this code is only used for legacy tests anyway.

Feel free to /hold cancel whenever you want this deployed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks. Next one to go is this one: openshift/release#37726

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023
@atiratree
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 70674ce and 2 for PR HEAD 993a786 in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 17, 2023

@atiratree: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 94102e6 into openshift:master Apr 17, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants