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

Create drop in file for ContainerRuntimeSearchRegistries #2276

Merged
merged 1 commit into from Dec 4, 2020

Conversation

umohnani8
Copy link
Contributor

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

- What I did
A new option, containerRuntimeSearchRegistries, has been added
to the images.config.openshift.io CRD. Implement this new option
in the ctrcfg controller so that when users set this in the cluster
wide Image CR, we create a drop-in file at /etc/containers/registries.conf.d
that sets unqualified-search-registries to the given list. This overwrites
the unqualified-search-registries list in registries.conf and lets the user
decide specify which registries to check when using short names for an image.

This PR is for the UnqualifiedSearchRegistries support epic.

- How to verify it
Start a 4.7 cluster and add the containerRuntimeSearchRegistries field to the cluster wide Image CR. Once that deploys, you should see a drop-in file at /etc/containers/registries.conf.d with the unqualified-search-registries field set to the list from the CR. When you try to pull an image using short name, it should only try to pull from the registries specified in that list.

- Description for the changelog

Create drop in file for ContainerRuntimeSearchRegistries option when set in the cluster wide Image CR

@umohnani8
Copy link
Contributor Author

@kikisdeliveryservice
Copy link
Contributor

@umohnani8
Copy link
Contributor Author

@kikisdeliveryservice no it does not, that doc is only for the ctrcfg CRD. This change is for the images.config.openshift.io CRD, which lives in openshift/api and the ctrcfg controller watches it for changes.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2020
@kikisdeliveryservice
Copy link
Contributor

/assign @mtrmac

@kikisdeliveryservice
Copy link
Contributor

/retest

@haircommander
Copy link
Member

LGTM, chances we could get an e2e test? (I'm not sure what tests already exist for registires.conf)

@umohnani8
Copy link
Contributor Author

We have the mock tests for this, which I have updated for this new option. Will work on adding an e2e for the Image CRD in a follow up PR

@haircommander
Copy link
Member

/approve

waiting on @mrunalp or @mtrmac for a more knowledgeable review

Copy link
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.

ACK to the little extent I understand MCO.

And to demonstrate how little I understand it: Why is this fully Go code and not just one of the file templates generated by pkg/controller/template? Is it undesirable to change the user-visible ControllerConfigSpec object, is the extra step generating that object undesirable, is it just that this is by far less code, or some other reason?

@umohnani8
Copy link
Contributor Author

The users want to be able to overwrite the unqualified-search-registries list with their own list and we are exposing this option in the images.config.openshift.io CRD. So this PR checks for this new option now in the cluster wide Image CR and creates a drop-in file at /etc/containers/registries.conf.d with the new unqualified-search-registries list. Hence the go code similar to what we do for the crio.conf drop-in files. The template approach wouldn't work here if I understand your comment correctly.

We renamed the user facing option to be containerRuntimeSearchRegistries as unqualifiedSearchRegistries was a bit confusing plus this list only works for the container runtime, builds and imagestream imports won't be using it for short names.

@mtrmac
Copy link
Contributor

mtrmac commented Dec 2, 2020

The users want to be able to overwrite the unqualified-search-registries list with their own list and we are exposing this option in the images.config.openshift.io CRD. So this PR checks for this new option now in the cluster wide Image CR and creates a drop-in file at /etc/containers/registries.conf.d with the new unqualified-search-registries list.

Sure.

The template approach wouldn't work here if I understand your comment correctly.

Why not? It seems to work for things like AdditionalTrustBundle. Is it the conditional existence of the drop-in file? (I understand why policy.json and registries.conf are not templates, the data has to be fairly non-trivially modified; still, why is the logic in pkg/controller and not pkg/operator?)

Or alternatively, why do things like AdditionalTrustBundle (or #2025 ) need to go through the extra ControllerConfigSpec CRD step and it isn’t all done in pkg/controller?

I’m not at all saying this PR is wrong (it’s certainly the lower-risk approach, given all the existing infrastructure accessing all the relevant CRDs, and given the past pain dealing with the bootstrap process and render differences during that time), I just don’t understand the difference in approach.

@mtrmac
Copy link
Contributor

mtrmac commented Dec 2, 2020

I just don’t understand the difference in approach.

… which makes me unqualified to be reviewing PRs here; just to be very explicit, this is not a request to reimplement.

@kikisdeliveryservice
Copy link
Contributor

I just don’t understand the difference in approach.

… which makes me unqualified to be reviewing PRs here; just to be very explicit, this is not a request to reimplement.

SBT, you came up as suggested by bots

@umohnani8
Copy link
Contributor Author

/retest

@umohnani8
Copy link
Contributor Author

@yuqi-zhang @sinnykumari would appreciate a review here when you have a few please

Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, 1 concern regarding failure reporting

pkg/controller/container-runtime-config/helpers.go Outdated Show resolved Hide resolved
A new option, containerRuntimeSearchRegistries, has been added
to the images.config.openshift.io CRD. Implement this new option
in the ctrcfg controller so that when users set this in the cluster
wide Image CR, we create a drop-in file at /etc/containers/registries.conf.d
that sets unqualified-search-registries to the given list. This overwrites
the unqualified-search-registries list in registries.conf and lets the user
decide specify which registries to check when using short names for an image.

Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kikisdeliveryservice, umohnani8, yuqi-zhang

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 [kikisdeliveryservice,yuqi-zhang]

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

@umohnani8
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@kikisdeliveryservice
Copy link
Contributor

/test e2e-aws-serial

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Dec 4, 2020

@umohnani8: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-workers-rhel7 882709a link /test e2e-aws-workers-rhel7

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.

@kikisdeliveryservice
Copy link
Contributor

/test e2e-aws-serial

@kikisdeliveryservice
Copy link
Contributor

/skip

@openshift-merge-robot openshift-merge-robot merged commit 4a005cb into openshift:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants