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

Give auto-update ability to use per-container authfile specified by label. #6188

Conversation

neVERberleRfellerER
Copy link
Contributor

This makes podman auto-update read REGISTRY_AUTH_FILE environment variable from systemd service unit. It is useful with gitlab container repositories where multiple repositories share same domain but have different deploy tokens.

Snippet:

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Environment=REGISTRY_AUTH_FILE=/path/to/someauthfile.json

Signed-off-by: Ondřej Kraus [email protected]

@neVERberleRfellerER
Copy link
Contributor Author

neVERberleRfellerER commented May 12, 2020

@vrothberg This is basic implementation of functionality I like to have in autoupdate. It is just stripped down version of code referenced in #6159 . I think unit generator changes (arbitrary --env) and auto-update changes should be separated since they don't depend on each other and with this PR REGISTRY_AUTH_FILE is treated specially.
Does it make sense to you? I am not sure about reusing the same envvar, but it makes sense for services generated with --new where container uses --pull=always.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Out of curiosity. Why not use podman auto-update --authfile instead?

The PR is aiming at each container being able to specify its own auth file for autoupdates. In case they are using the same file, I recommend using --authfile.

Regarding the current implementation. As we are already specifying the auto-update policy via labels, I prefer to add such an authfile via a label as well: io.containers.autoupdate.authfile=/path/auth.json.

@neVERberleRfellerER
Copy link
Contributor Author

Out of curiosity. Why not use podman auto-update --authfile instead?
The PR is aiming at each container being able to specify its own auth file for autoupdates.

Because, in one quite specific use case, I really want separate authfile for each container, because I have bunch of images in gitlab repo in different namespaces (reasons for this separation are historic and I don't want to mess with it more than necessary) accessible by different credentials (basically this https://gitlab.com/gitlab-org/gitlab/-/issues/22718). I could also create new user to run another rootless podman, but I'd rather not (there is no other gain - services in question share some resources anyway). Alternatively I can just call auto-update multiple times with different --authfile and filter logs so 403s won't scare me (as long as auto-update wont stop after encountering 403 - not sure if it does or doesn't).
Of course, best solution would be to add support to specifiy image path in authfiles, eg:

{
	"auths": {
		"my-registry.example.com/somegroup/somename": {
			"auth": "token1"
		},
		"my-registry.example.com": {
			"auth": "token2"
		}
	}
}

where it would use token1 for matching path prefix and token2 for everything else in the same registry, but it's not supported in Docker and in the end, it really only matter during updates, since I can use --authfile when pulling images manually.

@vrothberg
Copy link
Member

Thanks for the details. It makes perfect sense in this context!

@neVERberleRfellerER neVERberleRfellerER force-pushed the autoupdate-systemd-envvar branch from 68ff357 to 62642b4 Compare May 12, 2020 15:57
@neVERberleRfellerER neVERberleRfellerER changed the title [WIP] Make auto-update use REGISTRY_AUTH_FILE from systemd unit. Make auto-update use REGISTRY_AUTH_FILE from systemd unit. May 12, 2020
@neVERberleRfellerER neVERberleRfellerER force-pushed the autoupdate-systemd-envvar branch from 62642b4 to c82c443 Compare May 13, 2020 10:16
@vrothberg
Copy link
Member

Regarding the current implementation. As we are already specifying the auto-update policy via labels, I prefer to add such an authfile via a label as well: io.containers.autoupdate.authfile=/path/auth.json.

I still prefer to use --label "io.containers.autoupdate.authfile=$PATH rather than looking up the authfile in the env. The policy sis configured the same way. We used an env variable for the systemd unit as it may be helpful beyond auto updates.

Other than that, LGTM.

@neVERberleRfellerER neVERberleRfellerER changed the title Make auto-update use REGISTRY_AUTH_FILE from systemd unit. WIP Make auto-update use REGISTRY_AUTH_FILE from systemd unit. May 17, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2020
@neVERberleRfellerER neVERberleRfellerER force-pushed the autoupdate-systemd-envvar branch from c82c443 to 61c3822 Compare May 17, 2020 09:31
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: neVERberleRfellerER
To complete the pull request process, please assign mheon
You can assign the PR to them by writing /assign @mheon in a comment when ready.

The full list of commands accepted by this bot can be found 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

@neVERberleRfellerER neVERberleRfellerER changed the title WIP Make auto-update use REGISTRY_AUTH_FILE from systemd unit. Make auto-update use REGISTRY_AUTH_FILE from systemd unit. May 17, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2020
@neVERberleRfellerER
Copy link
Contributor Author

Sorry, I somehow missed the part of your comment suggesting use of label instead of environment variable. I completely agree.
I have updated my PR to use label and rebased it on top of current master.

@neVERberleRfellerER neVERberleRfellerER force-pushed the autoupdate-systemd-envvar branch from 61c3822 to 9177c89 Compare May 17, 2020 09:38
@neVERberleRfellerER neVERberleRfellerER changed the title Make auto-update use REGISTRY_AUTH_FILE from systemd unit. Give auto-update ability to use per-container authfile specified by label. May 17, 2020
@rhatdan
Copy link
Member

rhatdan commented May 17, 2020

I still don't understand why this needs to be a label.

Why not

$ podman run --label "io.containers.autoupdate=image" \
    --authfile=/some/authfile.json" \
    -d busybox:latest top

Rather then

$ podman run --label "io.containers.autoupdate=image" \
    --label "io.containers.autoupdate.autfile=/some/authfile.json" \
    -d busybox:latest top

Then autoupdate could just look at the authfile associated with the container? If this is not recorded in the containers config, we could start recording it?

@neVERberleRfellerER
Copy link
Contributor Author

neVERberleRfellerER commented May 17, 2020

It's possible to get --authfile from CreateCommand, but it puts envvar authfile configuration into disadvantage (which isn't a problem for me). I am not aware of any existing universal record of authfile used during container creation, but I can try to create one.

UPDATE: possible authfile-in-config here #6254

@vrothberg
Copy link
Member

I still don't understand why this needs to be a label.
[...]
Then autoupdate could just look at the authfile associated with the container? If this is not recorded in the containers config, we could start recording it?

I am okay with doing that implicitly 👍 Note that it's important to establish a priority over which authfile is being:

  • If the container config has an authfile use it.
  • Otherwise, check if auto-update --authfile was used.
  • Else, use the default.

@rhatdan
Copy link
Member

rhatdan commented May 18, 2020

Ok, I will go along with you guys.
/approve
/lgtm
Thanks @neVERberleRfellerER

@mheon
Copy link
Member

mheon commented May 18, 2020

LGTM, for reference

@mheon mheon added 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. labels May 18, 2020
@openshift-merge-robot openshift-merge-robot merged commit 1332c8b into containers:master May 18, 2020
@neVERberleRfellerER neVERberleRfellerER deleted the autoupdate-systemd-envvar branch May 18, 2020 20:57
@vrothberg
Copy link
Member

Thanks for the great contributions, @neVERberleRfellerER !

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants