*: implement additional pull registries #674

runcom · 2017-07-20T08:01:48Z

Signed-off-by: Antonio Murdaca runcom@redhat.com

runcom · 2017-07-20T08:02:14Z

I need feedback from tests/CI and still need to add integration tests, do not merge (but review appreciated)

runcom · 2017-07-20T08:30:25Z

@mrunalp @rhatdan PTAL

rh-atomic-bot · 2017-07-20T10:18:05Z

87/87 passed on RHEL - Passed.
121/121 k8s node-e2e passed on RHEL - Passed.

87/87 passed on Fedora - Passed.
121/121 k8s node-e2e passed on Fedora - Passed.

87/87 passed on CentOS - Passed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/777/fullresults.txt

rh-atomic-bot · 2017-07-20T10:35:41Z

0/0 passed on RHEL - Passed.
0/0 k8s node-e2e passed on RHEL - Failed.

0/0 passed on Fedora - Passed.
0/0 k8s node-e2e passed on Fedora - Failed.

0/0 passed on CentOS - Passed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/778/fullresults.txt

rhatdan · 2017-07-20T12:16:32Z

pkg/storage/image.go


+	if len(additionalRegistries) == 0 {
+		// TODO(runcom): make docker.io a CONST
+		additionalRegistries = []string{"docker.io"}


Should we add a default registry to replace docker.io and put this in the config file. Then add additional registries, that way we don't hard code docker.io into the tool?
I think we should use Trust/signing to block registries, so there is no reason to add that functionality here.

indeed, I will change the config file to have "docker.io" in it by default so that we don't hardcode docker.io

Perhaps the flag should just be registries, and we allow users to add a list of registries, with docker.io as the default.

Sounds good :)

rhatdan · 2017-07-20T12:17:51Z

pkg/storage/image_regexp.go

@@ -0,0 +1,123 @@
+package storage


Should this be in storage or pkg/registries?

It's about image naming, irrespective of registries I guess

rhatdan · 2017-07-20T12:19:41Z

server/image_pull.go

+		options := &copy.Options{
+			SourceCtx: &types.SystemContext{},
+		}
+		// a not empty username should be sufficient to decide whether to send auth


// Specifiying a username indicates the user intends to send authentication to the registry.

@runcom Can you change the comment above to match my previous comment.

rh-atomic-bot · 2017-07-20T12:28:40Z

87/87 passed on RHEL - Passed.
121/121 k8s node-e2e passed on RHEL - Passed.

87/87 passed on Fedora - Passed.
121/121 k8s node-e2e passed on Fedora - Passed.

87/87 passed on CentOS - Passed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/779/fullresults.txt

TomSweeneyRedHat · 2017-07-20T13:44:23Z

pkg/storage/image_regexp.go

+	separatorRegexp = match(`(?:[._]|__|[-]*)`)
+
+	// nameComponentRegexp restricts registry path component names to start
+	// with at least one letter or number, with following parts able to be


Suggestion: For these more complicated regexp's, it would be nice to have an example of what a valid object would look like in the comments.

Also have you written tests for these? It would be nice to have a test fail is someone later edits one of these and messes it up.

I have to write a huge comment at the beginning of this as this is a forked version from Docker/distribution. Nobody should never edit this, as for test coverage, this should stay as is.

runcom · 2017-07-31T16:09:51Z

Updated not-to hardcode docker.io in go files, now we just generate the config with "docker.io" just to maintain compatibility but hopefully we can get rid of that and indeed distros can generate config w/o docker at all. I'll add tests and changes to man pages now.

runcom · 2017-07-31T17:50:35Z

Added changes to man pages. Just need to add tests (which I'm working on). @mrunalp @rhatdan PTAL (hopefully, we can tag beta after this :))

rhatdan · 2017-07-31T17:51:25Z

cmd/crio/config.go

+# image (e.g. fedora:rawhide). "docker.io" is left here as a default registry.
+# Users can remove it and use whatever default registry they need.
+registries = [
+  "docker.io",


I am not sure this will work, since config reads the host config and will append docker.io onto a list even if docker.io is already on the list. Might be better off leaving this to the distribution.

docker.io should go at the end not the beginning.

I don't know, Docker.io is still a default since w/o this string the config isn't a working config. One can do "crio config" and get a broken config since no registries are configured. How do we deal with that?

I think it will still default to docker.io in containers/image? My problem with this is if I wanted to specify no registies, config will still show docker.io. And if I have docker.io in my config, this code will show
docker.io docker.io

Even if it still defaults to docker.io in containers/image we need to have a way to still have a default for docker.io when pulling unqualified images because that's just what people expect (and what we do in RHEL right now, unless people actually block docker.io).
I'm fine leaving this to distro but bear in mind crio is going to error out if nobody add a registry here.

@rhatdan could you provide an example of using this PR and having the double "docker.io" entry there?

@rhatdan the double "docker.io" issue is not an issue (the last command totally overrides the docker.io entry also):

$ cat test.config| grep docker.io "docker.io", $ sudo ./crio --config test.config --debug --registry docker.io --registry test.redhat.com DEBU[2017-08-01 10:28:44.630508363+02:00] runcom [docker.io test.redhat.com] $ sudo ./crio --configtest.config --debug --registry test.rhel.com DEBU[2017-08-01 10:29:49.679527671+02:00] runcom [test.rhel.com]

You can see docker.io isn't added twice.

I added a ;piece of code to remove duplicates though.

What does
$ sudo ./crio --config test.config --registry test.rhel.com | grep docker.io
Show?
What does the list of registries show? It should show docker.io and test.rhel.com from my understanding of the code. But if I remove docker.io from test.config and run the same test does it show just test.rhel.com and NO docker.io? That is the problem, but it is probably not a big deal since the config command is probably only run in rpm.

What does
$ sudo ./crio --config test.config --registry test.rhel.com | grep docker.io
Show?
What does the list of registries show? It should show docker.io and test.rhel.com from my understanding of the code

@rhatdan this shows just test.rhel.com - is that ok? CLI flags overrides config.

But if I remove docker.io from test.config and run the same test does it show just test.rhel.com and NO docker.io? That is the problem

it will still shows NO docker.io

rhatdan · 2017-07-31T17:52:20Z

cmd/crio/config.go

 {{ range $opt := .InsecureRegistries }}{{ printf "\t%q,\n" $opt }}{{ end }}]

+# registries is used to specify registries to be used when pulling an unqualified
+# image (e.g. fedora:rawhide). "docker.io" is left here as a default registry.


Remove comment about docker.io.

rhatdan · 2017-07-31T17:53:14Z

cmd/crio/main.go

 		},
+		cli.StringSliceFlag{
+			Name:  "registry",
+			Usage: "list of registies used when pulling unqualified images",


list of registries to prepend when pulling unqualified images.

Registry to be prepended when pulling unqualified images, can be specified multiple times.

rhatdan · 2017-07-31T17:53:57Z

cmd/crio/main.go

 		}

+		if len(config.Registries) == 0 {
+			logrus.Fatal("no registries configured. Please add registries with the --registry flag")


Is this actually an error, or does it just force users to use fully qualified images

I'm torn between leaving this error or do as you said, force people to use fully qualify images. However, I think we should go with the latter (error out) so we don't risk to break kubernetes at all (since many users still use unqualified images in prod)

rhatdan · 2017-07-31T17:55:07Z

docs/crio.8.md

  Print usage statement

+**--insecure-registry=**
+  List of registries to enable insecure communication with


--insecure-registry=[] Enable insecure registry communication, i.e., enable un-encrypted and/or untrusted communication. List of insecure registries can contain an element with CIDR notation to specify a whole subnet. Insecure registries accept HTTP and/or accept HTTPS with certificates from unknown CAs. Enabling --insecure-registry is useful when running a local registry. However, because its use creates security vulnerabilities it should ONLY be enabled for testing purposes. For increased security, users should add their CA to their system's list of trusted CAs instead of using --insecure-registry.

rhatdan · 2017-07-31T17:56:04Z

docs/crio.8.md

  CRIO root dir (default: "/var/lib/containers/storage")

+**--registry**=""
+  List of registries to be used when pulling an unqualified image


Is this a list or something that can be specified multiple times?

This is the same as insecure registries flag, so it's a flag which can be specified multiple times.

Then the comment should say
Registry host which will be prepended to unqualified images, can be specified multiple times.

rhatdan · 2017-07-31T17:56:38Z

docs/crio.conf.5.md


+**insecure_registries**=""
+  List of registries to enable insecure communication with
+


--insecure-registry=[] Enable insecure registry communication, i.e., enable un-encrypted and/or untrusted communication. List of insecure registries can contain an element with CIDR notation to specify a whole subnet. Insecure registries accept HTTP and/or accept HTTPS with certificates from unknown CAs. Enabling --insecure-registry is useful when running a local registry. However, because its use creates security vulnerabilities it should ONLY be enabled for testing purposes. For increased security, users should add their CA to their system's list of trusted CAs instead of using --insecure-registry.

rhatdan · 2017-07-31T17:57:07Z

docs/crio.conf.5.md

  Image which contains the pause executable (default: "kubernetes/pause")

+**registries**=""
+  List of registries to be used when pulling an unqualified image


If it is a list how do I specify it?

I'll add "comma separated list of registeies"

rh-atomic-bot · 2017-08-01T01:58:49Z

81/81 passed on RHEL - Passed.
121/121 k8s node-e2e passed on RHEL - Passed.

81/81 passed on Fedora - Passed.
121/121 k8s node-e2e passed on Fedora - Passed.

81/81 passed on CentOS - Passed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/890/fullresults.txt

rhatdan · 2017-08-01T13:11:31Z

pkg/storage/image.go

+	}
+
+	if len(cleanRegistries) == 0 {
+		return nil, errors.New("no registries configured. Please add registries with the --registry flag")


Why am I required to have a registry defined? Can't a user say that he always requires fully qualified names?

I'm fine with that - @mrunalp wdyt?

👍 to allowing fully qualified names always.

rhatdan · 2017-08-01T13:12:25Z

server/image_pull.go

+		options := &copy.Options{
+			SourceCtx: &types.SystemContext{},
+		}
+		// a not empty username should be sufficient to decide whether to send auth


@runcom Can you change the comment above to match my previous comment.

rhatdan · 2017-08-01T13:13:12Z

server/server.go

 	}

-	imageService, err := storage.GetImageService(store, config.DefaultTransport, config.InsecureRegistries)
+	imageService, err := storage.GetImageService(store, config.DefaultTransport, config.InsecureRegistries, config.Registries)


nit: shouldn't registries come before insecureregistries.

mmm they are unrelated, I guess it's personal taste, if you want I'll have Registries comes first

I guess so, thats why it was a nit.

rhatdan · 2017-08-01T13:15:08Z

pkg/storage/image.go

+	// normalize the unqualified image to be domain/repo/image...
+	images := []string{}
+	for _, r := range svc.registries {
+		if r == dockerRegistry {


Is this really necessary? Won't containers/image treat docker.io://imageName correctly?

yes, but I guess it's cleaner to pass down the unqualified image (as I do here) instead of passing down "docker.io/imageName"

I would rather remove the reference to docker.io and allow containers/image to do its thing. You are building in assumptions about containers/image that could change in the future. I think we should treat them all the same.

runcom · 2017-08-01T13:35:24Z

@baude brought up that we should use the global /etc/containers/registries.conf instead of having cri-o's config specify its own additional registries. @mrunalp @rhatdan wdyt? I'm super fine with this. Other tools can read that.

rh-atomic-bot · 2017-08-01T13:58:16Z

81/81 passed on RHEL - Passed.
121/121 k8s node-e2e passed on RHEL - Passed.

81/81 passed on Fedora - Passed.
121/121 k8s node-e2e passed on Fedora - Passed.

81/81 passed on CentOS - Passed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/899/fullresults.txt

rhatdan · 2017-08-01T14:17:44Z

My pull request is doing the same thing, in that we are defaulting storage to /etc/containers/storage.conf and then allowing crio.conf to override it, but by default content comes out of registries.

I agree with @baude but I think /etc/containers/registry.conf, (We should use it for buildah as well.)
It should become part of skopeo-containers, not atomic though. Since we don't want to force atomic on everyone. Perhaps the definition should be stored in containers/image.

runcom · 2017-08-01T15:35:20Z

Discussed with @rhatdan @baude, we won't hold this PR for now. We finish this, ship beta, meanwhile we build up a library to parse the registries.conf file and provides primitives to cri-o to gather additional registries.

rhatdan · 2017-08-01T18:28:22Z

LGTM

runcom · 2017-08-01T18:36:41Z

Just push-forced with latest changes - @mrunalp PTAL

mrunalp · 2017-08-01T19:00:28Z

This looks fine. Let us wait for the tests to return.

mrunalp · 2017-08-01T21:59:55Z

Test failure: pulling image failed: rpc error: code = 2 desc = no registries configured while trying to pull an unqualified image

runcom · 2017-08-01T22:09:17Z

Yeah, got that, I'm fixing it (and writing the blog post simultaneously)

rh-atomic-bot · 2017-08-02T03:20:58Z

6/7 passed on RHEL - Failed.
0/1 k8s node-e2e passed on RHEL - Failed.

6/7 passed on Fedora - Failed.
0/1 k8s node-e2e passed on Fedora - Failed.

30/54 passed on CentOS - Failed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/909/fullresults.txt

rh-atomic-bot · 2017-08-02T10:23:59Z

30/31 passed on RHEL - Failed.
0/1 k8s node-e2e passed on RHEL - Failed.

30/31 passed on Fedora - Failed.
0/1 k8s node-e2e passed on Fedora - Failed.

30/31 passed on CentOS - Failed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/913/fullresults.txt

rh-atomic-bot · 2017-08-02T12:38:37Z

38/43 passed on RHEL - Failed.
121/121 k8s node-e2e passed on RHEL - Passed.

38/43 passed on Fedora - Failed.
121/121 k8s node-e2e passed on Fedora - Passed.

38/43 passed on CentOS - Failed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/914/fullresults.txt

runcom · 2017-08-02T14:26:48Z

let's wait for CI now that Travis's green here

mrunalp · 2017-08-02T14:30:28Z

Needs rebase :/

runcom · 2017-08-02T14:33:33Z

rebasing..........

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

rh-atomic-bot · 2017-08-02T16:03:03Z

38/40 passed on RHEL - Failed.
2/24 k8s node-e2e passed on RHEL - Failed.

38/40 passed on Fedora - Failed.
2/21 k8s node-e2e passed on Fedora - Failed.

38/40 passed on CentOS - Failed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/915/fullresults.txt

runcom · 2017-08-02T16:07:03Z

mmmm looking at tests failures. in CI (weird actually)

runcom · 2017-08-02T16:11:38Z

can't see how CI failed and Travis did not, maybe it was an old run of this PR

mrunalp · 2017-08-02T16:33:58Z

yeah, could have been an old run. Can you check if a new run is scheduled?

runcom · 2017-08-02T18:21:53Z

there are 6+ builds in the queue and can't check parameters of those 😕

rh-atomic-bot · 2017-08-02T22:20:25Z

81/81 passed on RHEL - Passed.
121/121 k8s node-e2e passed on RHEL - Passed.

81/81 passed on Fedora - Passed.
121/121 k8s node-e2e passed on Fedora - Passed.

81/81 passed on CentOS - Passed.
0/0 k8s node-e2e passed on CentOS - Failed.

Log - https://aos-ci.s3.amazonaws.com/kubernetes-incubator/cri-o/crio-integration-tests-prs/919/fullresults.txt

runcom · 2017-08-02T22:30:05Z

Alright CI finally came back @mrunalp

mrunalp · 2017-08-03T00:42:46Z

LGTM

runcom · 2017-08-03T12:30:37Z

...adding some integration tests also

k8s-ci-robot added the cncf-cla: yes label Jul 20, 2017

runcom force-pushed the additional-registries branch 3 times, most recently from 9c42a7c to 54aa085 Compare July 20, 2017 08:29

runcom force-pushed the additional-registries branch from 54aa085 to 4e164e8 Compare July 20, 2017 09:44

runcom force-pushed the additional-registries branch from 4e164e8 to b32e73a Compare July 20, 2017 11:06

rhatdan reviewed Jul 20, 2017

View reviewed changes

TomSweeneyRedHat reviewed Jul 20, 2017

View reviewed changes

runcom force-pushed the additional-registries branch from b32e73a to 35b1a26 Compare July 31, 2017 16:08

runcom force-pushed the additional-registries branch from 35b1a26 to 877a641 Compare July 31, 2017 17:49

rhatdan reviewed Jul 31, 2017

View reviewed changes

runcom force-pushed the additional-registries branch 2 times, most recently from 76c6274 to 3a6a62b Compare August 1, 2017 08:43

rhatdan reviewed Aug 1, 2017

View reviewed changes

runcom changed the title ~~[WIP] *: implement additional pull registries~~ *: implement additional pull registries Aug 1, 2017

runcom force-pushed the additional-registries branch from 3a6a62b to 7787d67 Compare August 1, 2017 18:36

runcom force-pushed the additional-registries branch 2 times, most recently from 7a65d8d to d173e1f Compare August 2, 2017 09:45

runcom force-pushed the additional-registries branch 2 times, most recently from 6a37548 to ec450e6 Compare August 2, 2017 12:17

runcom force-pushed the additional-registries branch from ec450e6 to b4cf00f Compare August 2, 2017 13:00

*: implement additional pull registries

a35727c

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the additional-registries branch from b4cf00f to a35727c Compare August 2, 2017 14:38

mrunalp merged commit 66d96e7 into cri-o:master Aug 3, 2017

runcom deleted the additional-registries branch August 3, 2017 07:48

wking mentioned this pull request Apr 2, 2018

Add documentation of files used by cri-o #1476

Merged


		insecure_registries=""
		List of registries to enable insecure communication with

*: implement additional pull registries #674

*: implement additional pull registries #674

Uh oh!

Conversation

runcom commented Jul 20, 2017

Uh oh!

runcom commented Jul 20, 2017

Uh oh!

runcom commented Jul 20, 2017

Uh oh!

rh-atomic-bot commented Jul 20, 2017

Uh oh!

rh-atomic-bot commented Jul 20, 2017

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!

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!

rh-atomic-bot commented Jul 20, 2017

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!

runcom commented Jul 31, 2017

Uh oh!

runcom commented Jul 31, 2017

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!

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!

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!

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!