Add parsing for registries.conf wildcard entries

[umohnani8]

Signed-off-by: Urvashi Mohnani umohnani@redhat.com

- What I did
The registries.conf file now supports wildcard entries.
Add validation for possible entries for insecure, blocked,
and allowed and vendor in updated runtime-utils for handling
the wildcard entries correctly.
Add some more test cases for wildcard entries.

- How to verify it
Add wildcard entries such as ".foo.com" to the cluster wide Image CR for insecure, blocked, or allowed registries. When matching certificates, if we are pulling from "example.foo.com", it will match the settings for ".foo.com" to it. So users don't need to have a long list of entries in their registries.conf file if they all match to a common dns name.

- Description for the changelog
Add support for registries.conf wildcard entries.

[umohnani8]

@kikisdeliveryservice PTAL

[kikisdeliveryservice]

@umohnani8 is this... the minimum set of changes for the re-vendoring? Because I'm kind of concerned at updating& adding 1200 files.

[umohnani8]

/retest

[umohnani8]

@kikisdeliveryservice yup, I just had to update two repos and go mod pulled in everything else on its own based on that.

[sinnykumari]

We are now also parsing /etc/containers/registries.conf to determine whether a change made is safe to skip node drain or not https://github.com/openshift/machine-config-operator/blob/master/pkg/daemon/drain.go#L166 , especially, for icsp related changes. Do you think this PR may require some update in our logic?

[mtrmac]

@umohnani8 is this... the minimum set of changes for the re-vendoring? Because I'm kind of concerned at updating& adding 1200 files.

A lot of that is a problematic dependency chain that brings the full containers/storage driver set, via containers/image#1146 . It’s not fundamentally necessary, but I’m also not sure we can fix it today. I can take a stab at it if it is important.

[mtrmac]

We are now also parsing /etc/containers/registries.conf to determine whether a change made is safe to skip node drain or not https://github.com/openshift/machine-config-operator/blob/master/pkg/daemon/drain.go#L166 , especially, for icsp related changes. Do you think this PR may require some update in our logic?

Is this supposed to work only on MCO-generated data, or in general, including user-provided ignition files?

If the latter, oh boy, that looks rather brittle, possibly breaking any time a new option is added to the config file.
I would strongly recommend at least defaulting to drain on any completely unrecognized config file entries, like containers/image#1284 . And for config file entries known to sysregistriesv2 but not to MCO (right now, CredentialHelpers+the short-name configuration), probably rework this to work via a single top-level deep equality comparison, only editing out known-acceptable differences.

---

Either way, the code that uses reg.Location for a lookup key, and compares entries for unexpected changes, must prefer reg.Prefix if it set; runtime-utils now generates configuration like that.

[sinnykumari]

Is this supposed to work only on MCO-generated data, or in general, including user-provided ignition files?

This doesn't come into picture for initial boot. MCO is parsing and determining the change performed on cluster as a day2 operation if registries.conf content changed between old and newly generated rendered-config.

I would strongly recommend at least defaulting to drain on any completely unrecognized config file entries, like
containers/image#1284 . And for config file entries known to sysregistriesv2 but not to MCO (right now, CredentialHelpers+the short-name configuration, probably rework this to work via a single top-level deep equality comparison, only editing out known-acceptable differences.

+1
I agree that current parsing is quite delicate for getting new field being introduced. Tried best to handle all available structs and types I got after parsing the toml. Perhaps, need to revisit the implementation.

[mtrmac]

Either way, the code that uses reg.Location for a lookup key, and compares entries for unexpected changes, must prefer reg.Prefix if it set; runtime-utils now generates configuration like that.

To be very explicit, I think this is a blocker for merging.

[umohnani8]

@mtrmac I accounted for the Prefix if it has a value 3e408f1#diff-58db213cf6f6259a621f8d3421b5e8af55ed71c9f52c15b255f66f0b1e78a946R226, PTAL

[mtrmac]

ACK, one more location.

[mtrmac]

Is this supposed to work only on MCO-generated data, or in general, including user-provided ignition files?

This doesn't come into picture for initial boot. MCO is parsing and determining the change performed on cluster as a day2 operation if registries.conf content changed between old and newly generated rendered-config.

Yes, but users can add new MachineConfig resources that contain ignition content, overriding what the MCO runtime-config mechanism creates, can’t they? (I don’t know, that might be unsupported, but IIRC some people are sharing such recipes.)

[mtrmac]

/lgtm

Thanks!

[umohnani8]

@sinnykumari @kikisdeliveryservice can I please get an approve here :)

[sinnykumari]

Is this supposed to work only on MCO-generated data, or in general, including user-provided ignition files?

This doesn't come into picture for initial boot. MCO is parsing and determining the change performed on cluster as a day2 operation if registries.conf content changed between old and newly generated rendered-config.

Yes, but users can add new MachineConfig resources that contain ignition content, overriding what the MCO runtime-config mechanism creates, can’t they? (I don’t know, that might be unsupported, but IIRC some people are sharing such recipes.)

Possible but are they supported? I don't have full picture. I thought in OCP this would ideally be managed only by MCO by applying ctrcfg. @umohnani8 can you confirm this?

If this is true, I would suggest to hold this PR for 4.10 where we can work on improving drain behaviour accordingly.

[umohnani8]

Users can override the registries.conf setting by applying a MC that adds a drop-in file at /etc/containers/registries.conf.d. We don't recommend this method and always ask users to use the ICSP or Image CR for any registries.conf changes.

We don't recommend and support the MC way, but I believe that method has been shared in the past when the user absolutely had to use it. I don't think we need to hold this PR because of that. if the MC method is being used at all, it would be for only a limited number of clusters (most probably less than 5 cases).

[sinnykumari]

Thanks @umohnani8 for confirming! I am fine with this to go in as we don't see any known issues.
gcp-op test has failed, looks like it needs some fixing.

 INFO[2021-07-23T16:30:50Z] go test -failfast -timeout 90m -v${WHAT:+ -run="$WHAT"} ./test/e2e/
# pkg-config --cflags  -- devmapper
Package devmapper was not found in the pkg-config search path.
Perhaps you should add the directory containing `devmapper.pc'
to the PKG_CONFIG_PATH environment variable
No package 'devmapper' found
pkg-config: exit status 1 

[umohnani8]

@sinnykumari the test should be fixed now. Can I please get an approve and lgtm here

[sinnykumari]

Yeah, looks like test is fixed now. Since, later changes made was only in Makefile to fix gcp-op test and @mtrmac has already added lgtm for container runtime specific changes. Should be good to get merged.

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, sinnykumari, umohnani8

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 [sinnykumari]

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

[openshift-ci]

@umohnani8: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-disruptive	798d71f	link	/test e2e-aws-disruptive
ci/prow/e2e-aws-techpreview-featuregate	798d71f	link	/test e2e-aws-techpreview-featuregate
ci/prow/okd-e2e-aws	798d71f	link	/test okd-e2e-aws
ci/prow/e2e-aws-upgrade-single-node	798d71f	link	/test e2e-aws-upgrade-single-node
ci/prow/e2e-aws-single-node	798d71f	link	/test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel7	798d71f	link	/test e2e-aws-workers-rhel7
ci/prow/e2e-aws-serial	798d71f	link	/test e2e-aws-serial

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.

[mtrmac]

@umohnani8 is this... the minimum set of changes for the re-vendoring? Because I'm kind of concerned at updating& adding 1200 files.

containers/storage#972 + containers/image#1312 will reduce the effect, with that this PR would count 749 modified files (and a lot of that is dependency upgrades, not just net-new additions). We should go further than that to exclude the compression implementations entirely, but that part of containers/image#1146 (and its impact on the stable API) looks much more difficult.

[mtrmac]

@umohnani8 is this... the minimum set of changes for the re-vendoring? Because I'm kind of concerned at updating& adding 1200 files.

containers/storage#972 + containers/image#1312 will reduce the effect

#2695 .