Use unified configfile parser for policy.json by jankaluza · Pull Request #711 · containers/container-libs

jankaluza · 2026-03-23T12:23:58Z

This PR switches policy.json loading to the unified storage/pkg/configfile parser instead of maintaining separate lookup logic in image/signature.

The SignaturePolicyPath support is preserved, so explicit callers continue to work as before. The policy loader also now uses CONTAINERS_POLICY_JSON for environment-based override. The CONTAINERS_POLICY_JSON_OVERRIDE remains ignored for policy.json because drop-in loading is disabled for that this configuration file.

It also extends the configfile API to return the paths it searched. This is showed to users when there is no policy.json found.

Fixes: #207
Fixes: #202

common/libimage/manifests/manifests_test.go

image/signature/policy_config.go

jankaluza · 2026-03-23T12:50:59Z

@Luap99 , Hi, could you please check if this PR makes a sense? I also added two questions for parts of code I'm not sure about...

image/types/types.go

common/pkg/config/config.go

mtrmac

Just an extremely brief skim.

mtrmac · 2026-03-23T20:01:09Z

image/signature/policy_config.go

-		return filepath.Join(sys.RootForImplicitAbsolutePaths, systemPolicyPath), nil
+	var policy *Policy
+	found := false
+	for item, err := range configfile.Read(&policyFiles) {


This loop does not work; there is no merging. Is the intent to discard the policy values until the last one?!

right, since we did not wanted to define merging rules the design doc says no drop in support and as such the main file will only ever be one per current parsing rules

Reading more precisely, this loop should only iterate at most once; do we want to assert that?

image/signature/policy_config_test.go

common/pkg/config/config.go

common/libimage/manifests/manifests_test.go

common/libimage/copier.go

image/signature/policy_config.go

Luap99

it would be good to if you can squash the re-add SignaturePolicyPath commits, i.e when reviewing this it makes no sense to look at something getting remove to be added again. It also break git bisect most likely when vendoring from another repo.

image/signature/policy_config.go

Luap99 · 2026-03-24T12:11:47Z

storage/pkg/configfile/parse.go

 		}

-		if conf.EnvironmentName != "" {
+		if conf.EnvironmentName != "" && !conf.DoNotLoadDropInFiles {


please add a new test case here for that behavior

And document that behavior! It’s rather unexpected based on plain reading of the current option (although there is some logic to it).

Probably not, but maybe we’d want s/DoNotLoadDropInFiles/DropInFilesUnsupported/ ?!

I am in favour of naming it DropInFilesUnsupported, though then I guess we would want to rename DoNotLoadMainFiles as well to MainFilesUnsupported? I suppose we could rename that outside of the scope of this PR if wanted.

I kind of think of them as different dimensions: “do not load a file at $this path” is more of a directive vs. “the consumer cannot process multiple drop-ins regardless of names” is more of a capability declaration, but that’s a very weak reasoning.

I’m also quite fine with leaving the name as is.

I would remove that in another PR if possible to not add more unrelated changes to this one if that's fine for you. I added the documentation part, that's good point :-).

mtrmac

Thanks!

image/signature/policy_config.go

image/signature/policy_paths_common.go

mtrmac · 2026-03-24T16:58:17Z

storage/pkg/configfile/parse.go

 		}

-		if conf.EnvironmentName != "" {
+		if conf.EnvironmentName != "" && !conf.DoNotLoadDropInFiles {


And document that behavior! It’s rather unexpected based on plain reading of the current option (although there is some logic to it).

Probably not, but maybe we’d want s/DoNotLoadDropInFiles/DropInFilesUnsupported/ ?!

storage/pkg/configfile/parse.go

image/signature/policy_config.go

image/signature/policy_config_test.go

mtrmac · 2026-03-24T17:30:14Z

image/signature/policy_config_test.go

-		// Path overridden
-		{&types.SystemContext{SignaturePolicyPath: nondefaultPath}, false, true, nondefaultPath, ""},
-		// Root overridden
+	for _, test := range []tc{


I’m rather unhappy about no longer being able to truly test SystemContext == nil and SystemContext{}, but I also don’t see any good way to do that. This will just have to trust configfile.Read for that.

At least, please test (SystemContext == nil, SystemContext{})+(XDG_CONFIG_HOME,CONTAINERS_POLICY_CONF), enough to confirm that we got what we wanted (while we don’t control what exists in /etc and /usr)

AFAICS there are no tests for SystemContext == nil and SystemContext{}.

Still no test for SystemContext == nil?

image/signature/policy_config_test.go

image/signature/policy_config.go

storage/pkg/configfile/parse.go

mtrmac

I didn’t carefully read the added tests or think about coverage, I’m hoping for a simpler version first.

There are also a bunch of earlier outstanding review comments.

storage/pkg/configfile/parse_test.go

storage/pkg/configfile/parse.go

mtrmac · 2026-03-30T18:15:44Z

image/signature/policy_config_test.go

-		// Path overridden
-		{&types.SystemContext{SignaturePolicyPath: nondefaultPath}, false, true, nondefaultPath, ""},
-		// Root overridden
+	for _, test := range []tc{


AFAICS there are no tests for SystemContext == nil and SystemContext{}.

image/signature/policy_config_test.go

image/signature/policy_config.go

packit-as-a-service · 2026-03-31T09:47:06Z

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

Luap99

(commit 3 should be squashed into commit 1 and that one needs WIP stripped)

image/signature/policy_config.go

storage/pkg/configfile/parse.go

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

When no policy.json is found, `DefaultPolicy()`` previously returned a generic error without indicating where the system looked for the file. This commit introduces `configfile.ReadWithPaths()` to track all attempted config file locations during iteration. It uses this in DefaultPolicy() to include the searched paths in the error message when no policy file is found. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

This commit does the following: - The `setup()` function now returns the SystemContext. - The hardcoded check for "no policy file found" has moved to the test itself. - Fixture is used for SignaturePolicyPath. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

This commit introduces `File.ErrorIfNotFound`. If it is true, the Read returns an error which contains all the paths it tried when searching for a config file. The ReadWithPaths is replaced by this new logic. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Luap99 · 2026-04-08T15:39:57Z

@mtrmac are you fine with the new approach suggested by me?

@jankaluza Generally speaking I would like to see a "clean" history, i.e. not add ReadWithPaths just to remove it again as this makes reviewing harder but lets with for @mtrmac feedback first

jankaluza · 2026-04-09T04:38:57Z

@Luap99 , yeah, I will squash the commits before merge.

mtrmac

Sure, the ErrorIfNotFound approach is (given the signature-side test) fine. We can rework storage/configfile as needed. ACK overall.

Some outstanding nits:

#711 (comment) (that would not have test coverage, that’s fine)
#711 (comment)
#711 (comment)
#711 (comment)

please squash, per @Luap99 .

github-actions bot added common Related to "common" package image Related to "image" package labels Mar 23, 2026

jankaluza commented Mar 23, 2026

View reviewed changes

common/libimage/manifests/manifests_test.go Outdated Show resolved Hide resolved

jankaluza commented Mar 23, 2026

View reviewed changes

image/signature/policy_config.go Show resolved Hide resolved

Luap99 reviewed Mar 23, 2026

View reviewed changes

image/types/types.go Show resolved Hide resolved

Luap99 reviewed Mar 23, 2026

View reviewed changes

common/pkg/config/config.go Show resolved Hide resolved

mtrmac reviewed Mar 23, 2026

View reviewed changes

common/libimage/copier.go Show resolved Hide resolved

mtrmac reviewed Mar 23, 2026

View reviewed changes

image/signature/policy_config.go Show resolved Hide resolved

github-actions bot added the storage Related to "storage" package label Mar 24, 2026

jankaluza changed the title ~~WIP: Use unified configfile parser for policy.json~~ Use unified configfile parser for policy.json Mar 24, 2026

Luap99 reviewed Mar 24, 2026

View reviewed changes

This was referenced Mar 24, 2026

Multiple copies of the default signature policy path #226

Open

containers-policy.json: provide default config in /usr/ #207

Open

policy.json overwrite not honouring $XDG_CONFIG_HOME #202

Open

mtrmac reviewed Mar 24, 2026

View reviewed changes

Luap99 linked an issue Mar 24, 2026 that may be closed by this pull request

Provide a customization point for policy.json #49

Open

Luap99 mentioned this pull request Mar 24, 2026

Provide a customization point for policy.json #49

Open

This was linked to issues Mar 25, 2026

policy.json overwrite not honouring $XDG_CONFIG_HOME #202

Open

containers-policy.json: provide default config in /usr/ #207

Open

mtrmac reviewed Mar 26, 2026

View reviewed changes

storage/pkg/configfile/parse.go Outdated Show resolved Hide resolved

storage/pkg/configfile/parse.go Outdated Show resolved Hide resolved

jankaluza force-pushed the policy.json branch 3 times, most recently from 89fd676 to d9da724 Compare March 30, 2026 14:07

mtrmac reviewed Mar 30, 2026

View reviewed changes

jankaluza force-pushed the policy.json branch from 8888d15 to 12ae511 Compare March 31, 2026 09:46

Luap99 requested changes Apr 1, 2026

View reviewed changes

image/signature/policy_config.go Outdated Show resolved Hide resolved

storage/pkg/configfile/parse.go Outdated Show resolved Hide resolved

Use unified configfile parser for policy.json

b8b5734

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

jankaluza added 10 commits April 2, 2026 13:06

Unify the DefaultPolicy tests into single test.

6e253a3

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Fix formatting and use CONTAINERS_POLICY_JSON.

ae6b09f

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Add test for root + SignaturePolicyPath.

acc9008

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Document DoNotLoadDropInFiles ignoring _OVERRIDE variables

63d0fc2

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Add test for policy.json readall error

988bbf9

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Single test for both Read and ReadWithPaths.

81f5682

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

Remove useless policy_paths_*.go files.

b575526

Signed-off-by: Jan Kaluza <jkaluza@redhat.com>

jankaluza force-pushed the policy.json branch from 30d9835 to ff4e0c0 Compare April 2, 2026 11:06

jankaluza mentioned this pull request Apr 9, 2026

image/docker: use unified configfile for registries.d #753

Open

mtrmac reviewed Apr 9, 2026

View reviewed changes

Conversation

jankaluza commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jankaluza commented Mar 23, 2026

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

packit-as-a-service bot commented Mar 31, 2026

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jankaluza commented Mar 23, 2026 •

edited

Loading