Mirrors support

[mtrmac]

- What I did
Implemented support for operator/v1alpha1.ImageContentSourcePolicy, added by openshift/api#354 , to support multiple mirrors for pulling OpenShift images.

NOTE: The CRI interface does not support passing more than one set of credentials for pulling an image, i.e. the pull secret, if any, only applies to the primary image location (the one explicitly included in the pull spec). If any of the mirrors need credentials, the credentials must be available on the node, probably via ControllerConfig.spec.pullSecret. That implies that configuring per-tenant credentials for accessing the mirrors is not currently possible.

To make this work at least for the OpenShift images, this PR also hard-codes CRI-O’s global_auth_file option to point to the pull secret file also used by kubelet, which provides access to OpenShift mirrors.

- How to verify it
WIP. There are fairly extensive unit tests, but I didn’t try this end-to end yet at all.

- Description for the changelog
(Depending on which variant is chosen):

• Added support for operator/v1alpha1.ImageContentSourcePolicy, allowing to configure mirrors for image registries

[cgwalters]

This is totally fine but it'd also be nice to update the PR description with at least a sentence or two for what the end goal is - think of it like the "seed" upon which an eventual proper commit message could grow 😄 - offhand I'm guessing something like:

We're working on supporting fetching container images from mirrors.

[mtrmac]

This is totally fine but it'd also be nice to update the PR description with at least a sentence or two for what the end goal is - think of it like the "seed" upon which an eventual proper commit message could grow 😄

Updated now. Still WIP, notably completely untestedonly with a unit test.

I really don’t know much about what I’m doing, any pointers would be appreciated.

Specific questions, for now:

• What should the eventual API look like: this adds two separate commits adding members to vendor/github.com/openshift/api/config/v1/types_image.go, we need one of them only.
• I suppose those members will have to be added to the openshift/api repository. Or should this instead introduce a new object / some other kind of change in the machineconfiguration.openshift.io namespace?
• The mirror support requires using (a current version of) the v2 registries.conf format, which implies requiring a new enough version of CRI-O (and Podman?) (Neither of which exist at this moment.). Can we just assume that the version will be new enough (and enforce that in some repository by updating MCO and CRI-O/… in lockstep), or does this code need to detect and transparently support older versions of the two tools as well (degrading to no-mirror operation)?

[umohnani8]

/retest

[mtrmac]

/retest

[cgwalters]

This work is targeted at 4.2 right?

Can you link to any more information on the dependent crio work?

It looks like this depends on openshift/api#354 - may be good to do a separate PR to update our vendored openshift/api as a prep step.

[mtrmac]

This work is targeted at 4.2 right?

Yes.

Can you link to any more information on the dependent crio work?

cri-o/cri-o#2494 + cri-o/cri-o#2510 for 1.14,
cri-o/cri-o#2439 + cri-o/cri-o#2511 on master.

It looks like this depends on openshift/api#354 - may be good to do a separate PR to update our vendored openshift/api as a prep step.

Will do, at least to get the other effects of dep ensure -update out of this PR — but note that the API repo changes are pure additions, and only really happen when this PR adds the uses of those subpackages.

[mtrmac]

It looks like this depends on openshift/api#354 - may be good to do a separate PR to update our vendored openshift/api as a prep step.

#936 . This PR now includes code from that branch, so don’t merge it as it is; after #936 is merged, I’ll rebase on top of master again.

[mtrmac]

/retest

[mtrmac]

Dropping the WIP marker, reportedly this works, and the new behavior now has tests at least as good as any other aspect of the controller (in particular, the tests now actually verify expected contents of the generated registries.conf).

Still, I don’t yet fully understand the test scaffolding, so, please, make sure to review that the “Fix TestContainerRuntimeConfigUpdate and TestImageConfigUpdate” commit makes sense.

[openshift-ci-robot]

@mtrmac: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-etcd-quorum-loss	69bab1e939794ed5914f2c73601ac2a279d5d8a9	link	/test e2e-etcd-quorum-loss

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

[umohnani8]

/retest

[umohnani8]

Tested this out on a cluster and everything works fine. Tried out adding multiple ImageContentSourcePolicy objects with overlaps in the mirrors defined. Tested deleting the CRs one by one and registries.conf was populates as expected. And it plays well with any insecure and blocked registries added to the cluster wide Image CR.
The only thing I think that is potentially missing is checking whether there was an actual change to all the ImageContentSourcePolicy CRs when a resync happens. This way we can prevent rebooting the nodes when a resync happens every few minutes if nothing with the registries.conf file has changed. We do a check for the Image CR, which was added here #461. cc @mtrmac

[mtrmac]

The only thing I think that is potentially missing is checking whether there was an actual change to all the ImageContentSourcePolicy CRs when a resync happens. This way we can prevent rebooting the nodes when a resync happens every few minutes if nothing with the registries.conf file has changed. We do a check for the Image CR, which was added here #461. cc @mtrmac

I thought exactly that path, in particular the

machine-config-operator/pkg/controller/container-runtime-config/container_runtime_config_controller.go

Line 627 in aed3d2a

if !isNotFound && equality.Semantic.DeepEqual(registriesIgn, mc.Spec.Config) {

check, should already work prevent no-op changes. Is that not enough?

OTOH, I can’t get TestICSPUpdate to stop the MCS object updates just by not modifying ICSP, but so far that seems to be more likely to be a test issue (the difference is actually in OSImageURL being "" vs. "dummy:///").

[umohnani8]

@mtrmac ah yes, that should do it. I missed it, my bad.
Everything LGTM then, this should be ready to merge once tests pass @runcom @kikisdeliveryservice :)

[umohnani8]

@runcom @cgwalters tests are green!

[kikisdeliveryservice]

this LGTM

will let walters/runcom give the final approval

[cgwalters]

Because any users without credentials were already able to use the
kubelet's config.json secrets

Hmm...not as of #827 right?

[mtrmac]

Because any users without credentials were already able to use the
kubelet's config.json secrets

Hmm...not as of #827 right?

The above is not talking about non-container users with local accounts on the node, but about users of the cluster with the ability to create pods (who should, typically, have no access outside of their containers at all). For such users, the kubelet automatically uses the kubelet’s secret file if the user does not explicitly provide a pull secret (and the kubelet is, before this PR, the only process on the node that actually needs access to the cluster pull secret file), and sends the credentials to CRI-O to cause the image to be pulled.

[cgwalters]

/approve

[cgwalters]

So...this is a lot to take in. I'd like to see a higher level design doc if one exists. But the feature is critical, and I think we can learn more after it lands in master.
/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [cgwalters]

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

[kikisdeliveryservice]

So...this is a lot to take in. I'd like to see a higher level design doc if one exists. But the feature is critical, and I think we can learn more after it lands in master.

+1