support insecure registry in configuration reload by allencloud · Pull Request #22337 · moby/moby

allencloud · 2016-04-26T15:29:24Z

Make docker support more configuration reload, like insecure-registry.

This pr did:

add --insecure-registries configuration reload
add unit test for --insecure-registries reload
update docs about daemon's configuration reload
fix bug that 127.0.0.0/8 will be duplicated when it is in daemon.json

fixed #22332

allencloud · 2016-04-30T11:50:19Z

Any update?

thaJeztah · 2016-05-02T09:11:35Z

design LGTM

@aaronlehmann @tiborvass wdyt? This keeps the --insecure-registries a daemon-level setting, but allows it to be set on a running daemon, which makes it easier for people to use.

aaronlehmann · 2016-05-02T17:05:40Z

Yeah, I like the concept. I don't really follow how the code changes work, though. Where does serviceConfig.InsecureRegistries get populated? See newServiceConfig in registry/config.go for how this happens on startup. This gets called by

registryService := registry.NewService(cli.Config.ServiceOptions)

from daemon's start function.

My apologies if I am missing something.

icecrime · 2016-05-11T22:41:33Z

Moving to code review.

icecrime · 2016-05-11T22:42:02Z

@aaronlehmann PTAL!

aaronlehmann · 2016-05-12T01:35:47Z

@icecrime: See my question above.

yank1 · 2016-05-13T02:35:30Z

LGTM

thaJeztah · 2016-05-13T10:47:22Z

ping @allencloud could you answer @aaronlehmann's question?

allencloud · 2016-05-15T02:25:56Z

@thaJeztah @icecrime @aaronlehmann
Actually yesterday I compiled the docker binary within this pr, but it does not work for me. I will check more until it works.

andrask · 2016-05-16T11:52:05Z

Would be better to check multiple entries as the interface supports it.

thaJeztah · 2016-05-26T14:39:41Z

@allencloud were you still working on this?

allencloud · 2016-05-26T16:28:12Z

@thaJeztah
Yes, Of source. Actually in the past several days, I am on a business trip. And I will work on it tomorrow.

thaJeztah · 2016-05-26T16:38:14Z

No problem! Was going through PR's and noticed this one 👍

pikeszfish · 2016-09-07T06:52:57Z

+1 Any update? My friends.
It's quite a useful PR to me if merged.

allencloud · 2016-09-16T16:21:06Z

Hi, My friends. PTAL, reloading insecure registry is really important for users to use registries. Really sorry for bothering you. @thaJeztah @aaronlehmann @icecrime @cpuguy83 @LK4D4

aaronlehmann · 2016-09-16T16:44:14Z

I think there are concurrency issues with writing to the live config struct without locking. It looks like it could race with isSecureIndex.

@allencloud could you address this?

@thaJeztah @aaronlehmann
Oh, I am afraid race could exist. Since newRepositoryInfo would be called via ResolveRepository, while ResolveRepository would be called via daemon's field RegistryService, like repoInfo, err := daemon.RegistryService.ResolveRepository(ref).
Updated it soon.

LK4D4 · 2016-10-03T16:29:41Z

This looks sorta weird. You should use bytes.Equal or compare the output of (*IPNet).String() I think.

Definitely. In fact, when I put forward this PR, in api/types/resgistry/registry.go, there is no func (ipnet *NetIPNet) String() string implemented yet. And still separated in engine-api.

While now, I think it is better to use String(). And updated.

LK4D4 · 2016-10-03T16:31:42Z

I'm not sure how it's in other place, but it looks quite bad to ignore errors from Marshal.

Oh, Great. Thanks for your review. @LK4D4
I will update the PR soon.

Added the err judging, and add several other places related to this issue.

allencloud · 2016-10-10T10:16:44Z

Thanks for all your reviews. And I am really sorry for my delay.
Actually now I tried to updated the PR. Please let me know if we make some place better.

PTAL @thaJeztah @aaronlehmann @icecrime @cpuguy83 @LK4D4

aaronlehmann · 2016-10-10T18:57:12Z

typo: daemom's

aaronlehmann · 2016-10-10T18:59:24Z

lowercase skip

aaronlehmann · 2016-10-10T19:09:41Z

I see some other places where there may be concurrency problems:

newIndexInfo reads config.IndexConfigs without locking
(*DefaultService).ServiceConfig exposes s.config.ServiceConfig to callers. The callers won't be able to lock s.config while reading from the maps.

allencloud · 2016-10-11T03:44:01Z

@aaronlehmann
Thanks again for reviews. Concurrency is really important. I updated the PR, PTAL. Thanks.

aaronlehmann · 2016-10-11T16:44:19Z

This doesn't help. s.config.ServiceConfig is returned by the function, and then it can be read by the caller while being modified by LoadInsecureRegistries.

This function either needs to return a deep copy of ServiceConfig, or be refactored so that callers do not retrieve ServiceConfig directly (i.e. methods to look up items in the maps, which hold locks while doing the lookups).

Thanks for review @aaronlehmann
I tried the second way. PTAL

aaronlehmann · 2016-10-12T23:59:09Z

Thanks for making the changes. I think the locking is correct now. But I'm concerned it will be hard to maintain. It's not obvious which functions should acquire the lock and which ones should not. For example isSecureIndex shouldn't acquire the lock because it's called by a function that already has the lock.

It might be better to push all the locking to the methods on DefaultService. Then every exported method that touches s.config will hold the lock. I think this would make the locking scheme clearer.

allencloud · 2016-10-17T06:44:09Z

@aaronlehmann Thanks a lot for your suggestion.
I tried what you suggestion. And I think it indeed makes the locking scheme clearer.
In addition, to avoid inner call of exported functions, I add some unexported functions like tlsConfig.
PTAL, thanks.

aaronlehmann · 2016-10-17T18:09:51Z

servConfig.InsecureRegistryCIDRs = append(servConfig.InsecureRegistryCIDRs, s.config.ServiceConfig.InsecureRegistryCIDRs...)

Or allocate servConfig.InsecureRegistryCIDRs with the right size and use copy.

aaronlehmann · 2016-10-17T18:10:10Z

servConfig.Mirrors = append(servConfig.Mirrors, s.config.ServiceConfig.Mirrors...)

aaronlehmann · 2016-10-17T18:16:53Z

I know I suggested this, but now I'm not so sure. It means only one search can run at once, for example. And a running search will block calls to other DefaultService methods. I didn't realize that some of these methods are long-running.

I guess the general strategy of locking s.config in every exported method is okay as long as we release the lock before any long-running operation. Not sure if this just an issue for Search, or if it also applies to other methods.

I think it is my mistake to lock the entire func Search. It will block others.
Now I try to only lock s.config in Search. Like this:

s.mu.Lock() index, err := newIndexInfo(s.config, indexName) s.mu.Unlock()

PTAL, thanks @aaronlehmann

aaronlehmann · 2016-10-18T19:02:33Z

LGTM

@dmcgowan @LK4D4 Could you please review as well?

My main concern with this one is getting the locking correct. It's been through a few passes and I think it's correct now (thanks @allencloud for sticking with it). There are some cases where the lock could be held for an extended period, for example isSecureIndex appears to do a hostname lookup. I don't expect this would be a big deal in practice, but I'm curious what other people think.

Signed-off-by: allencloud <allen.sun@daocloud.io>

LK4D4 · 2016-10-20T18:41:18Z

LGTM

thaJeztah · 2016-10-20T21:33:37Z

Yay! Thanks @allencloud!

allencloud · 2016-10-21T02:23:50Z

🤗 Thanks for all of your help and guide.

fsword · 2016-11-07T00:46:45Z

Thanks for the pr, but it cannot do the thing well on docker for Mac. The gui app just has apple&restart button and not reload button here.

thaJeztah · 2016-11-07T08:21:57Z

@fsword this PR will be in docker 1.13, so isn't available yet; also, for enhancements for the docker for mac ui, please use https://github.com/docker/for-mac/issues

GordonTheTurtle added the status/0-triage label Apr 26, 2016

allencloud force-pushed the support-insecure-registry-config-reload branch from d353874 to 95e671e Compare April 26, 2016 15:35

thaJeztah added status/1-design-review and removed status/0-triage labels Apr 26, 2016

allencloud force-pushed the support-insecure-registry-config-reload branch 5 times, most recently from ee2739d to 2a57374 Compare April 26, 2016 16:45

allencloud force-pushed the support-insecure-registry-config-reload branch from 2a57374 to 01c7125 Compare May 6, 2016 08:59

icecrime added status/2-code-review and removed status/1-design-review labels May 11, 2016

allencloud force-pushed the support-insecure-registry-config-reload branch 2 times, most recently from d0acf00 to 4aab6a4 Compare May 13, 2016 02:56

thaJeztah added the impact/changelog label May 13, 2016

thaJeztah added this to the 1.12.0 milestone May 13, 2016

thaJeztah mentioned this pull request May 16, 2016

--insecure-registry should be on "docker pull" #8887

Open

andrask reviewed May 16, 2016
View reviewed changes

Comment thread daemon/daemon_test.go Outdated

Copy link
Copy Markdown

andrask May 16, 2016

Choose a reason for hiding this comment

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

Would be better to check multiple entries as the interface supports it.

allencloud force-pushed the support-insecure-registry-config-reload branch 5 times, most recently from e90b5a7 to 7a792fe Compare August 30, 2016 15:33

aaronlehmann reviewed Sep 16, 2016

View reviewed changes

aaronlehmann mentioned this pull request Sep 16, 2016

[Proposal] a new way to handle none https registry #16578

Closed

thaJeztah mentioned this pull request Sep 28, 2016

[WIP] Add --insecure-registry to docker pull #26886

Closed

LK4D4 suggested changes Oct 3, 2016

View reviewed changes

aaronlehmann reviewed Oct 10, 2016

View reviewed changes

Comment thread docs/reference/commandline/dockerd.md Outdated

Copy link
Copy Markdown

aaronlehmann Oct 10, 2016

Choose a reason for hiding this comment

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

typo: daemom's

aaronlehmann reviewed Oct 10, 2016

View reviewed changes

Comment thread registry/config.go Outdated

Copy link
Copy Markdown

aaronlehmann Oct 10, 2016

Choose a reason for hiding this comment

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

lowercase skip

aaronlehmann reviewed Oct 11, 2016

View reviewed changes

thaJeztah mentioned this pull request Oct 17, 2016

--insecure-registry host:port parameter invalid but accepted #27192

Closed

aaronlehmann reviewed Oct 17, 2016

View reviewed changes

support insecure registry in configuration reload

582803f

Signed-off-by: allencloud <allen.sun@daocloud.io>

Conversation

allencloud commented Apr 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

allencloud commented Apr 30, 2016

thaJeztah commented May 2, 2016

aaronlehmann commented May 2, 2016

icecrime commented May 11, 2016

icecrime commented May 11, 2016

aaronlehmann commented May 12, 2016

yank1 commented May 13, 2016

thaJeztah commented May 13, 2016

allencloud commented May 15, 2016

Choose a reason for hiding this comment

thaJeztah commented May 26, 2016

allencloud commented May 26, 2016

thaJeztah commented May 26, 2016

pikeszfish commented Sep 7, 2016

allencloud commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

allencloud commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Oct 10, 2016

allencloud commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Oct 12, 2016

allencloud commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Oct 18, 2016

LK4D4 commented Oct 20, 2016

thaJeztah commented Oct 20, 2016

allencloud commented Oct 21, 2016

fsword commented Nov 7, 2016

thaJeztah commented Nov 7, 2016

Labels

12 participants

allencloud commented Apr 26, 2016 •

edited

Loading

allencloud Oct 10, 2016 •

edited

Loading

allencloud commented Oct 17, 2016 •

edited

Loading