support registry mirror config reload by allencloud · Pull Request #29650 · moby/moby

allencloud · 2016-12-22T05:50:14Z

Signed-off-by: allencloud [email protected]

fixes #29594

this PR makes docker daemon support reload config registry mirror.

- What I did

add registry-mirror enable reload
fix a bug that duplicated mirrors will appear in the daemon and docker info
add a test case for that

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2016-12-22T18:02:29Z

daemon/daemon.go

(just quickly peeking at the code changes); these whitespace changes are not related to this change, and not everyone like adding too much whitespace; could you remove these?

thaJeztah · 2016-12-22T18:13:18Z

Feature-wise, I'm ok with adding this. I do think that the whole configuration (re)load functionality needs a massive overhaul / refactor though; I think the code is getting quite messy, and a lot of duplication

thaJeztah · 2016-12-22T19:10:19Z

Design-wise we're okay with this

@aaronlehmann do you see any possible issues with live-reloading this list?

thaJeztah · 2016-12-23T09:03:15Z

registry/registry_test.go

Why did you change this test?

since it fails in the test, when validateMirror.
Mirror address must have scheme. In all the test file, mirror has scheme, while this has no.

In addition, I need more investigation.

allencloud · 2016-12-23T12:14:01Z

registry/config.go

Since every mirror in the daemon no matter from flag, config or reload config, they will all be validated in LoadMirrors.
While ValidateMirror twice will lead to error, since the first ValidateMirror will add / at the end of the mirror address, if the new mirror enter the second ValidateMirror with a postfix of /, it will report error.

So I remove this part of ValidateMirror for flag options.

Hm, I think we should fix ValidateMirror(); it basically does two things; validate the value, and "normalise" it to a standard format (to always have a trailing /). It's odd that we don't accept a trailing / as input but actually require it to be set in the output.

This should do the trick;

func ValidateMirror(val string) (string, error) { return "", fmt.Errorf("Unsupported scheme %s", uri.Scheme) } - if uri.Path != "" || uri.RawQuery != "" || uri.Fragment != "" { + if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { return "", fmt.Errorf("Unsupported path/query/fragment at end of the URI") } - return fmt.Sprintf("%s://%s/", uri.Scheme, uri.Host), nil + return strings.TrimSuffix(val, "/") + "/", nil }

The unit test for ValidateMirror() will need to be updated as well, and some values moved from "invalid" to "valid"

Actually, perhaps we should update those error messages a bit, as they are a bit inconsistent, and don't show which mirror/url was invalid;

// ValidateMirror validates an HTTP(S) registry mirror func ValidateMirror(val string) (string, error) { uri, err := url.Parse(val) if err != nil { return "", fmt.Errorf("invalid mirror: %q is not a valid URI", val) } if uri.Scheme != "http" && uri.Scheme != "https" { return "", fmt.Errorf("invalid mirror: unsupported scheme %q in %q", uri.Scheme, uri) } if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { return "", fmt.Errorf("invalid mirror: path, query, or fragment at end of the URI %q", uri) } return strings.TrimSuffix(val, "/") + "/", nil }

diff --git a/registry/config.go b/registry/config.go index 79819de..88b62af 100644 --- a/registry/config.go +++ b/registry/config.go @@ -247,18 +247,15 @@ func isSecureIndex(config *serviceConfig, indexName string) bool { func ValidateMirror(val string) (string, error) { uri, err := url.Parse(val) if err != nil { - return "", fmt.Errorf("%s is not a valid URI", val) + return "", fmt.Errorf("invalid mirror: %q is not a valid URI", val) } - if uri.Scheme != "http" && uri.Scheme != "https" { - return "", fmt.Errorf("Unsupported scheme %s", uri.Scheme) + return "", fmt.Errorf("invalid mirror: unsupported scheme %q in %q", uri.Scheme, uri) } - - if uri.Path != "" || uri.RawQuery != "" || uri.Fragment != "" { - return "", fmt.Errorf("Unsupported path/query/fragment at end of the URI") + if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { + return "", fmt.Errorf("invalid mirror: path, query, or fragment at end of the URI %q", uri) } - - return fmt.Sprintf("%s://%s/", uri.Scheme, uri.Host), nil + return strings.TrimSuffix(val, "/") + "/", nil }

Oh, interesting; it marks URLs containing a username/password (http://user:[email protected]/) as "valid", but the original version silently strips them from the "normalised" URL

This should probably do it;

// ValidateMirror validates an HTTP(S) registry mirror func ValidateMirror(val string) (string, error) { uri, err := url.Parse(val) if err != nil { return "", fmt.Errorf("invalid mirror: %q is not a valid URI", val) } if uri.Scheme != "http" && uri.Scheme != "https" { return "", fmt.Errorf("invalid mirror: unsupported scheme %q in %q", uri.Scheme, uri) } if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { return "", fmt.Errorf("invalid mirror: path, query, or fragment at end of the URI %q", uri) } if uri.User != nil { // strip password from output uri.User = url.UserPassword(uri.User.Username(), "xxxxx") return "", fmt.Errorf("invalid mirror: username/password not allowed in URI %q", uri) } return strings.TrimSuffix(val, "/") + "/", nil }

thaJeztah

@allencloud I left some comments, will have another look after it's updated 😊

thaJeztah · 2016-12-23T14:47:39Z

registry/config.go

I don't really like overwriting the configuration, and reverting if the values are invalid. How about something like this;

// LoadMirrors loads mirrors to config, after removing duplicates. // Returns an error if mirrors contains an invalid mirror. func (config *serviceConfig) LoadMirrors(mirrors []string) error { seen := map[string]struct{}{} unique := []string{} for _, m := range mirrors { m2, err := ValidateMirror(m) if err != nil { return err } if _, exist := seen[m2]; exist { continue } unique = append(unique, m2) seen[m2] = struct{}{} } config.Mirrors = unique return nil }

thaJeztah · 2016-12-23T14:55:51Z

registry/config.go

Hm, I think we should fix ValidateMirror(); it basically does two things; validate the value, and "normalise" it to a standard format (to always have a trailing /). It's odd that we don't accept a trailing / as input but actually require it to be set in the output.

This should do the trick;

func ValidateMirror(val string) (string, error) { return "", fmt.Errorf("Unsupported scheme %s", uri.Scheme) } - if uri.Path != "" || uri.RawQuery != "" || uri.Fragment != "" { + if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { return "", fmt.Errorf("Unsupported path/query/fragment at end of the URI") } - return fmt.Sprintf("%s://%s/", uri.Scheme, uri.Host), nil + return strings.TrimSuffix(val, "/") + "/", nil }

The unit test for ValidateMirror() will need to be updated as well, and some values moved from "invalid" to "valid"

thaJeztah · 2016-12-23T15:02:47Z

registry/config.go

Actually, perhaps we should update those error messages a bit, as they are a bit inconsistent, and don't show which mirror/url was invalid;

// ValidateMirror validates an HTTP(S) registry mirror func ValidateMirror(val string) (string, error) { uri, err := url.Parse(val) if err != nil { return "", fmt.Errorf("invalid mirror: %q is not a valid URI", val) } if uri.Scheme != "http" && uri.Scheme != "https" { return "", fmt.Errorf("invalid mirror: unsupported scheme %q in %q", uri.Scheme, uri) } if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { return "", fmt.Errorf("invalid mirror: path, query, or fragment at end of the URI %q", uri) } return strings.TrimSuffix(val, "/") + "/", nil }

diff --git a/registry/config.go b/registry/config.go index 79819de..88b62af 100644 --- a/registry/config.go +++ b/registry/config.go @@ -247,18 +247,15 @@ func isSecureIndex(config *serviceConfig, indexName string) bool { func ValidateMirror(val string) (string, error) { uri, err := url.Parse(val) if err != nil { - return "", fmt.Errorf("%s is not a valid URI", val) + return "", fmt.Errorf("invalid mirror: %q is not a valid URI", val) } - if uri.Scheme != "http" && uri.Scheme != "https" { - return "", fmt.Errorf("Unsupported scheme %s", uri.Scheme) + return "", fmt.Errorf("invalid mirror: unsupported scheme %q in %q", uri.Scheme, uri) } - - if uri.Path != "" || uri.RawQuery != "" || uri.Fragment != "" { - return "", fmt.Errorf("Unsupported path/query/fragment at end of the URI") + if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { + return "", fmt.Errorf("invalid mirror: path, query, or fragment at end of the URI %q", uri) } - - return fmt.Sprintf("%s://%s/", uri.Scheme, uri.Host), nil + return strings.TrimSuffix(val, "/") + "/", nil }

thaJeztah · 2016-12-23T15:05:09Z

registry/config.go

Oh, interesting; it marks URLs containing a username/password (http://user:[email protected]/) as "valid", but the original version silently strips them from the "normalised" URL

thaJeztah · 2016-12-23T15:07:39Z

registry/config.go

This should probably do it;

// ValidateMirror validates an HTTP(S) registry mirror func ValidateMirror(val string) (string, error) { uri, err := url.Parse(val) if err != nil { return "", fmt.Errorf("invalid mirror: %q is not a valid URI", val) } if uri.Scheme != "http" && uri.Scheme != "https" { return "", fmt.Errorf("invalid mirror: unsupported scheme %q in %q", uri.Scheme, uri) } if (uri.Path != "" && uri.Path != "/") || uri.RawQuery != "" || uri.Fragment != "" { return "", fmt.Errorf("invalid mirror: path, query, or fragment at end of the URI %q", uri) } if uri.User != nil { // strip password from output uri.User = url.UserPassword(uri.User.Username(), "xxxxx") return "", fmt.Errorf("invalid mirror: username/password not allowed in URI %q", uri) } return strings.TrimSuffix(val, "/") + "/", nil }

allencloud · 2016-12-24T04:10:47Z

Thanks a lot. Really helpful comments. I updated this PR. PTAL

thaJeztah · 2016-12-24T15:08:12Z

registry/config.go

note; variable names in Go

This naming part updated. PTAL

thaJeztah

oops, looks like I had a review "pending" here; see my comment

thaJeztah · 2016-12-24T15:10:41Z

registry/config.go

Can you override the original password here, as I did in my example? #29650 (comment)

We don't want passwords to be printed/logged

thaJeztah · 2016-12-27T12:51:01Z

registry/config_test.go

can you add a http://foo:[email protected]/ to the test?

thaJeztah · 2016-12-27T12:57:11Z

daemon/daemon_test.go

I wonder if we should simplify this test, and have a single slice of test-cases, each with "before", "new", and "expected", and a property that indicates if it should fail.

That is great. I will try that. Thanks a lot.

Updated with clearer test. PTAL 😄

aaronlehmann · 2017-01-03T20:00:46Z

daemon/daemon.go

Non-blocking observation: I feel like none of the registry-related settings should be prefixed with "Daemon".

removing is reasonable.

aaronlehmann · 2017-01-03T20:06:41Z

docs/reference/commandline/dockerd.md

Typo: resgitry

aaronlehmann · 2017-01-03T20:07:28Z

registry/config.go

Why not nil (i.e. omit this line)?

done, thanks

aaronlehmann · 2017-01-03T20:11:40Z

Code LGTM

Signed-off-by: allencloud <[email protected]>

thaJeztah

LGTM, thanks!

GordonTheTurtle added the status/0-triage label Dec 22, 2016

allencloud force-pushed the support-registry-mirror-config-reload branch 6 times, most recently from af48291 to 16c8d98 Compare December 22, 2016 11:14

vdemeester added status/1-design-review and removed status/0-triage labels Dec 22, 2016

allencloud force-pushed the support-registry-mirror-config-reload branch from 16c8d98 to 07803d2 Compare December 22, 2016 16:08

thaJeztah reviewed Dec 22, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Dec 22, 2016

allencloud force-pushed the support-registry-mirror-config-reload branch 5 times, most recently from 75a22f8 to d568ce3 Compare December 23, 2016 06:09

thaJeztah reviewed Dec 23, 2016

View reviewed changes

allencloud force-pushed the support-registry-mirror-config-reload branch 4 times, most recently from bf4ae87 to 04efd0d Compare December 23, 2016 12:02

allencloud commented Dec 23, 2016

View reviewed changes

allencloud force-pushed the support-registry-mirror-config-reload branch 2 times, most recently from dcb312a to 621f082 Compare December 23, 2016 13:01

thaJeztah requested changes Dec 23, 2016

View reviewed changes

allencloud force-pushed the support-registry-mirror-config-reload branch from 621f082 to 69cb253 Compare December 23, 2016 17:34

thaJeztah reviewed Dec 24, 2016

View reviewed changes

allencloud force-pushed the support-registry-mirror-config-reload branch 5 times, most recently from 8071ded to 3ad1479 Compare December 26, 2016 15:34

thaJeztah requested changes Dec 27, 2016

View reviewed changes

thaJeztah reviewed Dec 27, 2016

View reviewed changes

allencloud force-pushed the support-registry-mirror-config-reload branch 4 times, most recently from 0222770 to f7d552f Compare January 3, 2017 10:49

aaronlehmann reviewed Jan 3, 2017

View reviewed changes

docs/reference/commandline/dockerd.md Outdated

Copy link

aaronlehmann Jan 3, 2017

Choose a reason for hiding this comment

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

Typo: resgitry

allencloud reacted with thumbs up emoji

aaronlehmann reviewed Jan 3, 2017

View reviewed changes

allencloud force-pushed the support-registry-mirror-config-reload branch 2 times, most recently from 29b2e90 to e90f36d Compare January 4, 2017 02:28

support registry mirror config reload

5b9348c

Signed-off-by: allencloud <[email protected]>

allencloud force-pushed the support-registry-mirror-config-reload branch from e90f36d to 5b9348c Compare January 4, 2017 03:04

thaJeztah approved these changes Jan 4, 2017

View reviewed changes

thaJeztah merged commit d3f30d6 into moby:master Jan 4, 2017

GordonTheTurtle added this to the 1.14.0 milestone Jan 4, 2017

thaJeztah added the impact/changelog label Jan 4, 2017

allencloud deleted the support-registry-mirror-config-reload branch January 4, 2017 14:36

thaJeztah mentioned this pull request Aug 10, 2017

Invalid registry-mirrors in daemon.json silently ignored #34476

Closed

thaJeztah mentioned this pull request Apr 2, 2019

daemon: fix mirrors validation #38991

Merged

Conversation

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

Uh oh!

Uh oh!

thaJeztah Dec 22, 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!

thaJeztah commented Dec 22, 2016

Uh oh!

thaJeztah commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allencloud Dec 23, 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!

allencloud Dec 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 23, 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!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

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!

thaJeztah Dec 23, 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!

Choose a reason for hiding this comment

Uh oh!

allencloud commented Dec 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

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!

aaronlehmann commented Jan 3, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

allencloud commented Dec 22, 2016 •

edited

Loading

thaJeztah Dec 22, 2016 •

edited

Loading

allencloud Dec 23, 2016 •

edited

Loading

allencloud Dec 23, 2016 •

edited

Loading

thaJeztah Dec 23, 2016 •

edited

Loading

thaJeztah Dec 23, 2016 •

edited

Loading