Skip to content

image/docker: use unified configfile search for cert directories#746

Merged
Luap99 merged 1 commit intocontainers:mainfrom
jankaluza:certs.d
Apr 30, 2026
Merged

image/docker: use unified configfile search for cert directories#746
Luap99 merged 1 commit intocontainers:mainfrom
jankaluza:certs.d

Conversation

@jankaluza
Copy link
Copy Markdown
Member

@jankaluza jankaluza commented Apr 7, 2026

Switch dockerCertDir to use the new
configfile.ContainersResourceDirs for resolving certificate directories.

@github-actions github-actions Bot added storage Related to "storage" package image Related to "image" package labels Apr 7, 2026
@jankaluza jankaluza mentioned this pull request Apr 8, 2026
Closed
Luap99
Luap99 reviewed Apr 8, 2026
View reviewed changes
Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread image/docker/docker_client.go Outdated
@jankaluza
Copy link
Copy Markdown
Member Author

jankaluza commented Apr 9, 2026

@Luap99 , thanks for the review :-). I will change the code once we finish discussions for all the suggested changes.

mtrmac
mtrmac reviewed Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

(Just a random note for now)

Comment thread image/docker/docker_client.go
@Luap99 Luap99 added the podman 6 breaking changes that should go only into podman 6 only label Apr 21, 2026
Luap99
Luap99 reviewed Apr 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

With the latest GetSearchPaths() API do we even get anything new from this ContainersResourceDirs?

I guess the simple thing is to just reuse GetSearchPaths() and not do any pkg/configfile changes?

Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
@jankaluza
Copy link
Copy Markdown
Member Author

jankaluza commented Apr 28, 2026

@Luap99 , I rewrote it to use GetSearchPaths, but I had to introduce File.ExtraDropInDirectories to inject the /etc/docker/certs.d between XDG directories and /etc directories.

Luap99
Luap99 reviewed Apr 28, 2026
View reviewed changes
Comment thread image/docker/docker_client.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
mtrmac
mtrmac reviewed Apr 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Looks reasonable overall.

Comment thread image/docs/containers-certs.d.5.md
Comment thread image/docker/docker_client_test.go Outdated
Comment thread image/docker/docker_client.go Outdated
Comment thread storage/pkg/configfile/parse_test.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
Comment thread storage/pkg/configfile/parse.go Outdated
@jankaluza
Copy link
Copy Markdown
Member Author

jankaluza commented Apr 29, 2026

Podman and buildah tests with this PR applied:

Luap99
Luap99 approved these changes Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

mtrmac
mtrmac reviewed Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • Commit message still talks about ExtraDropInDirectories.

I’m fine with merging now and fixing things afterwards if it made a difference to the vendor dance or other dependent work.

Comment thread image/docker/docker_client_test.go Outdated
@jankaluza
image/docker: use unified configfile search for cert directories
c0d0173 
Switch `dockerCertDir` to use the `configfile.GetSearchPaths` for
resolving certificate directories.

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
@packit-as-a-service
Copy link
Copy Markdown

packit-as-a-service Bot commented Apr 30, 2026

Packit jobs failed. @containers/packit-build please check.

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Apr 30, 2026

@mtrmac Did you change the skopeo CI setup?

ERROR: Somehow quay.io/libpod/skopeo_cidev:c20260425t010036z-f43f42d14 is not based on fedora-45. (contrib/cirrus/runner.sh:76 in _run_setup())

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Apr 30, 2026

Anyhow failure is unrelated to this PR so I am ok force merging in the interest of getting this done.

@Luap99 Luap99 merged commit 06cbc5d into containers:main Apr 30, 2026
22 of 24 checks passed
@mtrmac
Copy link
Copy Markdown
Contributor

mtrmac commented Apr 30, 2026

@mtrmac Did you change the skopeo CI setup?

ERROR: Somehow quay.io/libpod/skopeo_cidev:c20260425t010036z-f43f42d14 is not based on fedora-45. (contrib/cirrus/runner.sh:76 in _run_setup())

One part of the explanation is https://github.com/containers/skopeo/pull/2861/changes#r3163314632 (and that whole PR), but that doesn’t explain why it suddenly started breaking.

Should be fixed by #809 .

mtrmac
mtrmac reviewed Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM, for the record. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mtrmac mtrmac mtrmac left review comments

@Luap99 Luap99 Luap99 approved these changes

Assignees

No one assigned

Labels

image Related to "image" package podman 6 breaking changes that should go only into podman 6 only storage Related to "storage" package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jankaluza @Luap99 @mtrmac