fix: the logic of the protocol in loadListeners by tao12345666333 · Pull Request #44823 · moby/moby

tao12345666333 · 2023-01-14T17:17:38Z

Signed-off-by: Jintao Zhang zhangjintao9020@gmail.com

- What I did

Here introduced a judgment in 5008409

This will result in the following error.

➜  moby sudo /usr/bin/dockerd -H fd:// --containerd=/run/containerd/containerd.sock                                                                                                                       
INFO[2023-01-15T00:56:52.640211255+08:00] Starting up                                  
WARN[2023-01-15T00:56:52.640525792+08:00] Running experimental build                          
failed to load listeners: bad format fd://, expected PROTO://ADDR

The configuration fd:// is the default configuration for docker.service.
we shouldn't break it
https://github.com/moby/moby/blob/master/contrib/init/systemd/docker.service#LL13C31-L13C36

- How I did it

- How to verify it

- Description for the changelog

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

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>

neersighted

Hmm, good catch, though I'm not convinced this is the right place to carve out this special case. This also definitely requires a regression test to prevent the same mistake again.

neersighted · 2023-01-14T22:06:25Z

Actually, something fishy is going on:

moby/opts/hosts_test.go

Lines 30 to 31 in d62a88e

    
           "fd://":                    "fd://", 
        
           "fd://something":           "fd://something",

We have a test case explicitly for this -- so we need to investigate why that is failing to catch the regression.

tao12345666333 · 2023-01-14T23:50:03Z

Thanks! Even though we have this test case, it doesn't use the parseHost or parseDaemonHost functions at this point in my modification.

In the previous code, it is not judged whether addr is empty.
5008409#diff-eab6997069d2fc9fd3e50c0721e88a363570b9d3ed03facd987373ab1ac9405bL665-L666

I think I should change the function called to parseDaemonHost to clean up a bit here, what do you think?

thaJeztah · 2023-01-14T23:50:07Z

Could it be the subtests here not capturing the variable?

moby/opts/hosts_test.go

Lines 50 to 51 in d62a88e

    
           for value, expectedError := range invalid { 
        
           	t.Run(value, func(t *testing.T) {

tao12345666333 · 2023-01-14T23:54:06Z

no, fd:// is a valid configuration.

thaJeztah · 2023-01-15T00:08:29Z

I'm personally fine with the check added in this PR (this was indeed an oversight of mine), or perhaps remove the check altogether (if we already validate these before this).

That said, we should refactor some of this (I recall I looked at this code as well, when working on #43459); I think there's too much handling happening in loadListeners(); it would be better if we did all the parsing and handling before we reach that code, to allow validating the configuration before we even try to start the daemon, and so that dockerd --validate already returns configuration errors (if we don't validate this specific case already);

moby/cmd/dockerd/daemon.go

Lines 83 to 92 in 803c21f

    
           serverConfig, err := newAPIServerConfig(cli.Config) 
        
           if err != nil { 
        
           	return err 
        
           } 
        
           if opts.Validate { 
        
           	// If config wasn't OK we wouldn't have made it this far. 
        
           	_, _ = fmt.Fprintln(os.Stderr, "configuration OK") 
        
           	return nil 
        
           }

thaJeztah · 2023-01-15T00:09:41Z

no, fd:// is a valid configuration.

Ah, yea, you're right; was wondering for a bit if the test was broken.

thaJeztah · 2023-01-15T00:11:47Z

Looks like fd://something should not be valid though; so some validation is missing in that part;

moby/daemon/listeners/listeners_linux.go

Lines 82 to 90 in 803c21f

    
           // default to all fds just like unix:// and tcp:// 
        
           if addr == "" || addr == "*" { 
        
           	return listeners, nil 
        
           } 
        
           fdNum, err := strconv.Atoi(addr) 
        
           if err != nil { 
        
           	return nil, errors.Errorf("failed to parse systemd fd address: should be a number: %v", addr) 
        
           }

thaJeztah · 2023-01-15T00:21:02Z

Removed the cherry-pick label because it looks like 5008409 (#44381) is only in master

tao12345666333 · 2023-01-15T00:29:53Z

Then I plan to do this:

Modify the current PR, and remove the check on addr. Since the previous code didn't have this check either, here's a quick fix. ~~Or leave this PR unchanged, since we really only need to cover fd for one scenario.~~
Create a new PR to refactor this:
- Replace the processing logic here with parseDaemonHost
- Add validation for fd://something

WDYT?

neersighted · 2023-01-15T02:02:20Z

I'd split the difference: use this PR, but change the implementation to the second, and add e2e testing of this area of code (not just parseDaemonHost).

tao12345666333 · 2023-01-15T02:37:05Z

OK, let me add some e2e test cases, thanks

thaJeztah · 2023-01-18T12:42:53Z

and add e2e testing of this area of code (not just parseDaemonHost).

Agreed; There's also quite a bit more to look at w.r.t. validation in general. Some quick thoughts on some of that;

Make sure we're more consistent in where validation (needs to) happen
Important there is that not all validation has to be a full validation (for better words). What I mean is that validating "format" and "valid input" is sometimes a separate concern; e.g. https://hello.world/blabla/blablabla is validly formatted URL, but the URL may not exist. Same as unix:///no/such/socket being a valid formatted "unix" socket path, but may not exist.
This distinction is sometimes important. For example, it's OK to validate for a valid formatted input in some areas (e.g. could be the docker CLI), but in some areas you don't want to (or are not able to) validate everything.
Not validating everything is fine as long as we catch errors (and ideally "as early as possible")/
Some things will always remain a bit of a "grey" area (e.g. socket paths may be "too long", but the allowed length may depend on how the host is configured. For such cases we must decide (probably on a per-case basis) wether we (dockerd) should validate, or if that's something to delegate to the system itself.

tao12345666333 · 2023-01-20T17:23:28Z

Thanks for your reply, I will do some thinking.

I'll be back from vacation in two days.

neersighted · 2023-03-10T00:48:31Z

Thanks for working on this! @cpuguy83 ended up picking it up in #45132, and addressed the testing concern there.

fix: the logic of the protocol in loadListeners

933105f

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>

neersighted requested changes Jan 14, 2023

View reviewed changes

neersighted added area/cli Client kind/bugfix PR's that fix bugs labels Jan 14, 2023

neersighted added this to the v-next milestone Jan 14, 2023

neersighted added status/2-code-review process/cherry-pick labels Jan 14, 2023

thaJeztah removed the process/cherry-pick label Jan 15, 2023

thaJeztah mentioned this pull request Jan 15, 2023

Replace uses of strings.Split(N) with strings.Cut() #44381

Merged

cpuguy83 mentioned this pull request Mar 9, 2023

Fix listener parsering regression when no addr set #45132

Merged

neersighted closed this in #45132 Mar 10, 2023

Conversation

tao12345666333 commented Jan 14, 2023

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

neersighted commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tao12345666333 commented Jan 14, 2023

Uh oh!

thaJeztah commented Jan 14, 2023

Uh oh!

tao12345666333 commented Jan 14, 2023

Uh oh!

thaJeztah commented Jan 15, 2023

Uh oh!

thaJeztah commented Jan 15, 2023

Uh oh!

thaJeztah commented Jan 15, 2023

Uh oh!

thaJeztah commented Jan 15, 2023

Uh oh!

tao12345666333 commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neersighted commented Jan 15, 2023

Uh oh!

tao12345666333 commented Jan 15, 2023

Uh oh!

thaJeztah commented Jan 18, 2023

Uh oh!

tao12345666333 commented Jan 20, 2023

Uh oh!

neersighted commented Mar 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neersighted commented Jan 14, 2023 •

edited

Loading

tao12345666333 commented Jan 15, 2023 •

edited

Loading