Skip to content

Conversation

@sjenning
Copy link
Contributor

@mrunalp @eparis @derekwaynecarr

static pods do not use fully qualified image names and docker.io is not in the registry search list in /etc/containers/registries.conf by default. We need to inject it.

TNC change will also be needed if we want this change in the masters/workers.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sjenning
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: rajatchopra

If they are not already assigned, you can assign the PR to them by writing /assign @rajatchopra in a comment when ready.

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:

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

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 11, 2018
# and 'registries.block'.

[registries.search]
registries = ['docker.io', 'registry.access.redhat.com']
Copy link
Member

Choose a reason for hiding this comment

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

should this order be flipped? i thought ordering mattered here.

# and 'registries.block'.

[registries.search]
registries = ['docker.io', 'registry.access.redhat.com']
Copy link
Contributor

Choose a reason for hiding this comment

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

r.a.r.c might need to come first, so no one hijacks one of our image names on docker.io.

# and 'registries.block'.

[registries.search]
registries = ['docker.io', 'registry.access.redhat.com']
Copy link
Member

Choose a reason for hiding this comment

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

docker.io could come after registry.access.redhat.com

#
# Docker only
[registries.block]
registries = []
Copy link
Member

Choose a reason for hiding this comment

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

The docs are not super clear, but it seems like we could accomplish this same result by leaving this block (and registries.insecure) off entirely. If that's the case, I'd rather have the file content be just:

# Configure container registries.  Docs in containers-registries.conf(5).

[registries.search]
registries = ['registry.access.redhat.com', 'docker.io']

or similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking I tested this out and it does appear that you can remove these sections completely.

@sjenning sjenning force-pushed the inject-dockerio-registry branch from 2110857 to 5ca14a1 Compare September 11, 2018 18:04
@sjenning
Copy link
Contributor Author

flipped the registry ordering

@sjenning
Copy link
Contributor Author

/hold
I think just fully qualifying the images might be better https://github.com/coreos-inc/tectonic-operators/pull/459

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2018
@sjenning
Copy link
Contributor Author

/hold cancel
we still want this for backward compatibility

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2018
@sjenning
Copy link
Contributor Author

/retest

@wking
Copy link
Member

wking commented Sep 11, 2018

I think just fully qualifying the images might be better coreos-inc/tectonic-operators#459

We push most (but not all) of our images to both docker.io and quay.io, right? I'm not sure how registry lookup works with this search approach; if one registry is dead do we move on to the next? It would be nice if we had a centralized place where we could set preferred/alternate/fallback registries for both robustness (no single point of failure) and optimization (prefer a local caching registry to conserve bandwidth). I dunno if this registries.search approach would be a good way to do that or not, but it seems like it might be. Thoughts?

@eparis
Copy link
Member

eparis commented Sep 11, 2018

I'm generally against this idea from a philisophical PoV. We are putting system wide configuration in individual nodes. is this what is used for unqualified openshift containers? if not, can we instead just 'ban' unqualified static pods?

@sjenning
Copy link
Contributor Author

sjenning commented Sep 11, 2018

@eparis personally, i'd like to make the search list empty by default, basically forcing fully qualified image paths, but that isn't a great experience for customers that already have unqualified pod spec or are trying to run sample pod specs from upstream documentation.

@eparis
Copy link
Member

eparis commented Sep 11, 2018

Do we do qualification in openshift/kube or only in the runtime?

@wking
Copy link
Member

wking commented Sep 11, 2018

I'm generally against this idea from a philisophical PoV.

Can you elaborate? Do you want to force blobs for an image to come from https://index.docker.io/v2/library/docker/blobs/${DIGEST}? Or are you ok with those coming from other places and only concerned about signatures (e.g. from Notary, etc.)? Maybe you're concerned about an insecure proxy leaking pull secrets or content pulled using those pull secrets to unauthorized clients? Or maybe we want to put in a policy for openshift/* that requires signatures from an OpenShift release OpenPGP key? Forcing everything through a single registry and signature server lets us punt on those sorts of questions, but launching a libvirt cluster would be so much faster if I could serve blobs from a local cache (and we'll probably have similar needs for clients who want to set up a cluster behind an air gap).

@sjenning
Copy link
Contributor Author

Do we do qualification in openshift/kube or only in the runtime?

The dockershim in the kubelet will actually UNqualify image paths with docker.io explicitly specified leading to this long standing issue https://bugzilla.redhat.com/show_bug.cgi?id=1561989

If we are using cri-o, however, there is no qualification of image paths. The image path defined in the pod spec is passed through unchanged.

@crawford
Copy link
Contributor

We don't need this configuration on the bootstrap node since we don't run user workloads. All of our images should be fully qualified. Closing in favor of openshift/machine-config-operator#52 (this will apply the change to all of the masters and the workers).

/close

@openshift-ci-robot
Copy link
Contributor

@crawford: Closing this PR.

Details

In response to this:

We don't need this configuration on the bootstrap node since we don't run user workloads. All of our images should be fully qualified. Closing in favor of openshift/machine-config-operator#52 (this will apply the change to all of the masters and the workers).

/close

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.

@eparis
Copy link
Member

eparis commented Sep 12, 2018

@wking In general I'm opposed to doing any node level configuration. Files on disks, even if they get laid down by the MC* tooling, are a source of skew/mis-configuration that can bite us in unexpected ways. So I'd rather we find a way to not have to ever set files on disks.

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

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants