make dockercfg secrets handle multiple urls including service DNS #9306

deads2k · 2016-06-13T19:03:44Z

Replacement for #6385, builds on #9209

Let's see what the [test]s do.

This has watches on services (maybe routes later) and secrets. No work is done until the caches are up to date.

After the service (and future routes) queue up work using a single key. That workqueue consumer checks the cache for the current value of the docker registry URLs. If it has changed, it lists all dockercfg secrets and puts them into a the secretQueue.

The secretQueue holds references to dockercfg secrets. An item is dequeued, checked if it needs to be updated to the new docker registry URLs, and if so, it updates.

deads2k · 2016-06-13T19:04:04Z

@liggitt You had the original, want this one too?

liggitt · 2016-06-13T20:29:59Z

@liggitt You had the original, want this one too?

sure, if no one gets to it first

deads2k · 2016-06-15T19:10:53Z

yum re[test]

deads2k · 2016-06-29T17:12:23Z

@Kargakis take a look at another controller?

0xmichalis · 2016-06-30T10:29:28Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

@@ -37,14 +41,16 @@ type DockercfgControllerOptions struct {
 	// If zero, re-list will be delayed as long as possible
 	Resync time.Duration

-	DefaultDockerURL string
+	// DockerURLsIntialized is used to send a signal to this controller that it has the correct set of docker urls


Can you add the source of the signal (DockerRegistryServiceController) to the doc?

0xmichalis · 2016-06-30T11:53:54Z

First pass looks good, but I want to give it another eye later

cc: @miminar @legionus this may interest you

0xmichalis · 2016-07-01T09:08:05Z

pkg/serviceaccounts/controllers/create_dockercfg_secrets.go

+		return
+	}
+
+	go e.serviceAccountController.Run(stopCh)


Can you move this out of the wait function?

Can you move this out of the wait function?

Sure, I hadn't fully sorted out the waiting mechanism when I wrote this.

deads2k · 2016-07-01T13:21:01Z

comments addressed.

deads2k · 2016-07-01T20:26:10Z

#9444 re[test]

0xmichalis · 2016-07-04T08:44:12Z

pkg/serviceaccounts/controllers/docker_registry_service.go

+		dockerCredentials = dockercfgMap[existingDockercfgSecretLocations.List()[0]].Password
+	}
+	if len(dockerCredentials) == 0 {
+		tokenSecret, err := e.client.Secrets(dockercfgSecret.Namespace).Get(dockercfgSecret.Annotations[ServiceAccountTokenSecretNameKey])


Can't you pull this from the cache? Is there any reason for the live call?

Can't you pull this from the cache? Is there any reason for the live call?

No reason I can think of. Nice catch.

0xmichalis · 2016-07-04T08:46:31Z

One more question, LGTM otherwise

deads2k · 2016-07-05T12:00:28Z

[merge]

openshift-bot · 2016-07-05T12:00:30Z

Evaluated for origin test up to 834622c

openshift-bot · 2016-07-05T13:22:19Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5775/)

openshift-bot · 2016-07-05T13:50:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5775/) (Image: devenv-rhel7_4527)

deads2k · 2016-07-05T13:58:19Z

#9512 re[merge]

openshift-bot · 2016-07-05T14:00:39Z

Evaluated for origin merge up to 834622c

smarterclayton · 2016-07-11T17:34:16Z

Why was this merged? This is obviated by migrate.

smarterclayton · 2016-07-11T17:34:52Z

Or is this supposed to only handle the case where no docker registry has been installed?

deads2k · 2016-07-11T18:22:01Z

Or is this supposed to only handle the case where no docker registry has been installed?

Written to handle one, happens to handle both. The code doesn't get simpler by eliminating it.

Also, why make someone run a command if they don't have to and this structure makes it possible and transparent to operate against routes if we choose to add it.

smarterclayton · 2016-07-11T19:00:43Z

In person discussion:

This (once the route watching part is added) removes the need for a
config variable that lists all possible names for secrets, something
liggitt and I discussed about migration that would be required
It means end users and end administrators can just create the route and
wait
It does have the downside that secrets grow much larger

We still have to have some persistent config that identifies the string
value that the API server will use to write into storage. That value still
has to be migrated. We still have to be able to migrate arbitrary secrets
for external registries. We could potentially use an annotation on the
service (resolved once per process) to record that. We also have the need
to figure out how export can expose an external image URL without having
all images in the cluster go through the router. Not going through the
router is important - unless contentRedirect is on, we can't afford to
bottleneck the cluster on proxying through an edge router. We could have
the router name resolve to the internal name if DNS supported CNAMEs from
router names to service IPs.

On Mon, Jul 11, 2016 at 2:22 PM, David Eads [email protected]
wrote:

Or is this supposed to only handle the case where no docker registry has
been installed?

Written to handle one, happens to handle both. The code doesn't get
simpler by eliminating it.

Also, why make someone run a command if they don't have to and this
structure makes it possible and transparent to operate against routes if we
choose to add it.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#9306 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_pxRWFWo0kvW2rbTj9kFEHSAnBrBYks5qUonLgaJpZM4I0oqG
.

liggitt self-assigned this Jun 13, 2016

deads2k force-pushed the convert-docker-registry-service branch 4 times, most recently from 55856ae to 269d586 Compare June 15, 2016 17:45

This was referenced Jun 17, 2016

refactor dockerfcfg secret controller to make it easier to handle routes #6385

Closed

Add a migration framework for mutable resources #9240

Merged

deads2k force-pushed the convert-docker-registry-service branch from 269d586 to 26ca796 Compare June 22, 2016 20:01

0xmichalis reviewed Jun 30, 2016
View reviewed changes

0xmichalis reviewed Jul 1, 2016
View reviewed changes

deads2k force-pushed the convert-docker-registry-service branch from 26ca796 to 7711762 Compare July 1, 2016 13:20

deads2k force-pushed the convert-docker-registry-service branch from 7711762 to f1b4f46 Compare July 1, 2016 17:22

0xmichalis reviewed Jul 4, 2016
View reviewed changes

handle multiple names for docker registry

834622c

deads2k force-pushed the convert-docker-registry-service branch from f1b4f46 to 834622c Compare July 5, 2016 12:00

openshift-bot merged commit eec443b into openshift:master Jul 5, 2016

sferich888 mentioned this pull request Aug 12, 2016

Cannot push to registry based on dns #8581

Closed

deads2k deleted the convert-docker-registry-service branch September 6, 2016 17:17

make dockercfg secrets handle multiple urls including service DNS #9306

make dockercfg secrets handle multiple urls including service DNS #9306

Uh oh!

Conversation

deads2k commented Jun 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Jun 13, 2016

Uh oh!

liggitt commented Jun 13, 2016

Uh oh!

deads2k commented Jun 15, 2016

Uh oh!

deads2k commented Jun 29, 2016

Uh oh!

0xmichalis Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Jun 30, 2016

Uh oh!

0xmichalis Jul 1, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jul 1, 2016

Uh oh!

deads2k commented Jul 1, 2016

Uh oh!

0xmichalis Jul 4, 2016

Choose a reason for hiding this comment

Uh oh!

deads2k Jul 5, 2016

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Jul 4, 2016

Uh oh!

deads2k commented Jul 5, 2016

Uh oh!

openshift-bot commented Jul 5, 2016

Uh oh!

openshift-bot commented Jul 5, 2016

Uh oh!

openshift-bot commented Jul 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Jul 5, 2016

Uh oh!

openshift-bot commented Jul 5, 2016

Uh oh!

smarterclayton commented Jul 11, 2016

Uh oh!

smarterclayton commented Jul 11, 2016

Uh oh!

deads2k commented Jul 11, 2016

Uh oh!

smarterclayton commented Jul 11, 2016

Uh oh!

Uh oh!

deads2k commented Jun 13, 2016 •

edited

Loading

deads2k Jul 1, 2016 •

edited

Loading

openshift-bot commented Jul 5, 2016 •

edited

Loading