Fix for certain problems on Synology devices #1068

tammert · 2021-09-04T20:21:10Z

Avoid 'assignment to entry in nil map' when *container.Config.ExposedPorts is not filled

These logs come directly from my Synology device (with the code from this PR running):

time="2021-09-04T20:05:34Z" level=debug msg="Trying to load authentication credentials." container=/homeassistant image="homeassistant/home-assistant:stable"
time="2021-09-04T20:05:34Z" level=debug msg="No credentials for homeassistant found" config_file=/config.json
time="2021-09-04T20:05:34Z" level=debug msg="Got image name: homeassistant/home-assistant:stable"
time="2021-09-04T20:05:34Z" level=debug msg="Checking if pull is needed" container=/homeassistant image="homeassistant/home-assistant:stable"
time="2021-09-04T20:05:34Z" level=debug msg="Building challenge URL" URL="https://index.docker.io/v2/"
time="2021-09-04T20:05:35Z" level=debug msg="Got response to challenge request" header="Bearer realm=\"https://auth.docker.io/token\",service=\"registry.docker.io\"" status="401 Unauthorized"
time="2021-09-04T20:05:35Z" level=debug msg="Checking challenge header content" realm="https://auth.docker.io/token" service=registry.docker.io
time="2021-09-04T20:05:35Z" level=debug msg="Setting scope for auth token" image=homeassistant/home-assistant scope="repository:homeassistant/home-assistant:pull"
time="2021-09-04T20:05:35Z" level=debug msg="No credentials found."
time="2021-09-04T20:05:35Z" level=debug msg="Parsing image ref" host=index.docker.io image=homeassistant/home-assistant normalized="docker.io/homeassistant/home-assistant:stable" tag=stable
time="2021-09-04T20:05:35Z" level=debug msg="Doing a HEAD request to fetch a digest" url="https://index.docker.io/v2/homeassistant/home-assistant/manifests/stable"
time="2021-09-04T20:05:35Z" level=debug msg="Found a remote digest to compare with" remote="sha256:43b39af7c9c63b933ba4ed17c226c97111c39a4cb379e444099292bdeedfbaff"
time="2021-09-04T20:05:35Z" level=debug msg=Comparing local="sha256:8f5fd806376b78183851cd408885bf27e82f7d9de7638c8cfcef838a6e7c31ea" remote="sha256:43b39af7c9c63b933ba4ed17c226c97111c39a4cb379e444099292bdeedfbaff"
time="2021-09-04T20:05:35Z" level=debug msg="Digests did not match, doing a pull."
time="2021-09-04T20:05:35Z" level=debug msg="Pulling image" container=/homeassistant image="homeassistant/home-assistant:stable"
time="2021-09-04T20:06:41Z" level=info msg="Found new homeassistant/home-assistant:stable image (16caae5dfda5)"
.........
time="2021-09-04T20:06:43Z" level=info msg="Stopping /homeassistant (ddc0d725a628) with SIGTERM"
time="2021-09-04T20:06:50Z" level=debug msg="Removing container ddc0d725a628"
time="2021-09-04T20:06:58Z" level=debug msg="This is the watchtower container /tender_banach"
time="2021-09-04T20:06:58Z" level=info msg="Creating /homeassistant"
time="2021-09-04T20:07:00Z" level=debug msg="Starting container /homeassistant (0fdc56784bd2)"
time="2021-09-04T20:07:02Z" level=debug msg="Session done: 4 scanned, 1 updated, 0 failed"

…Ports is not filled

tammert · 2021-09-04T20:22:26Z

pkg/container/container.go

-		config.ExposedPorts[p] = struct{}{}
+		// config.ExposedPorts isn't reliably in sync with HostConfig.PortBindings, so we can't assign directly to map without checking
+		if _, ok := config.ExposedPorts[p]; ok {
+			config.ExposedPorts[p] = struct{}{}


The whole point of this code is to empty out map entries, so we should be able to safely skip that action if the map is nil

I'm not sure that it is... Couldn't we just assign ExposedPorts to a new map if it's nil instead? If we keep part of the config as nil we might run into issues in other parts of the code that expects it to be a map.
I think the point of this code is to explicitly add the portbindings to the exposed ports (but with no explicit mapping config), but I may be wrong here.
Since the docker daemon on Synology doesn't return any exposed ports, it's somewhat natural that it doesn't require them in the create container payload. It might break container creation on non-synology docker though...

tammert · 2021-09-04T20:23:58Z

pkg/container/container.go

@@ -298,9 +301,5 @@ func (c Container) VerifyConfiguration() error {
 		return errorInvalidConfig
 	}

-	if len(hostConfig.PortBindings) > 0 && containerConfig.ExposedPorts == nil {


Containers get recreated with the correct ports for me, so I'm assuming the Docker client uses the ones it finds in hostConfig.PortBindings and doesn't necessarily need those from *container.Config.ExposedPorts

If we did this instead:

if len(hostConfig.PortBindings) > 0 && containerConfig.ExposedPorts == nil { containerConfig.ExposedPorts = make(map[Port]struct{}) }

it would just align the docker responses for synology and non-synology docker daemons.

piksel · 2021-09-05T08:33:06Z

This is essentially what we needed testing. Hopefully this is the only difference on Synology Docker. I don't remember off hand what other images caused the partial config.

tammert · 2021-09-05T08:43:24Z

I was thinking, maybe the Dockerfile for home-assistant omits the EXPOSE instruction, and that might explain the differences between this image and others?

piksel · 2021-09-19T06:55:19Z

That seems like the obvious cause, could you do some investigation on this? Like building a docker image with and without EXPOSE and checking docker container inspect on it when running.
From what I gathered previously on this issue, it wasn't that simple (or it might have been confusion in the communication between the users with the device and the developers).

Also, the tests fail :)

simskij · 2021-10-16T09:32:31Z

Given that we're unable to test this ourselves, we'll have to make due with one pair of eyes on this particular fix. Let me know if you feel it's ready for merging and I'll push it through.

tammert · 2021-10-18T07:42:23Z

@simskij I still want to look at what @piksel mentioned, just haven't found the time yet. I'll add you as reviewers again once I feel confident the comments have been adressed

driesvandamme · 2021-11-09T23:03:09Z

@tammert Do you have any update on this? I'm experiencing the same issue.

piksel

I added a suggestion¹ for what would be the least intrusive change to get this working. Just doing this I think is "safe" enough to release in the next version.

¹ It's not a "proper" suggestion though, since github doesn't allow them on deleted rows for some reason...

piksel · 2021-11-12T11:44:10Z

pkg/container/container.go

@@ -298,9 +301,5 @@ func (c Container) VerifyConfiguration() error {
 		return errorInvalidConfig
 	}

-	if len(hostConfig.PortBindings) > 0 && containerConfig.ExposedPorts == nil {


If we did this instead:

if len(hostConfig.PortBindings) > 0 && containerConfig.ExposedPorts == nil { containerConfig.ExposedPorts = make(map[Port]struct{}) }

it would just align the docker responses for synology and non-synology docker daemons.

driesvandamme · 2021-12-14T12:41:11Z

@zoispag - Could you have a look at the review? Thanks!

driesvandamme · 2021-12-28T12:22:07Z

@tammert - Could you have a look at this? It would be great if the Container would be able to auto-update using Portainer :)
Right now it always has to be done manually with every release because of this.

simskij · 2022-01-22T16:58:59Z

This was resolved in #1183. Closing this one

Avoid 'assignment to entry in nil map' when *container.Config.Exposed…

410b836

…Ports is not filled

tammert requested a review from simskij as a code owner September 4, 2021 20:21

tammert commented Sep 4, 2021

View reviewed changes

tammert requested review from piksel and zoispag September 4, 2021 20:28

simskij removed their request for review September 29, 2021 13:21

piksel reviewed Nov 12, 2021

View reviewed changes

piksel mentioned this pull request Jan 11, 2022

fix: add missing portmap when needed #1183

Merged

simskij closed this Jan 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for certain problems on Synology devices #1068

Fix for certain problems on Synology devices #1068

tammert commented Sep 4, 2021 •

edited

Loading

tammert Sep 4, 2021

piksel Nov 12, 2021

tammert Sep 4, 2021

piksel Nov 12, 2021

piksel commented Sep 5, 2021

tammert commented Sep 5, 2021

piksel commented Sep 19, 2021

simskij commented Oct 16, 2021

tammert commented Oct 18, 2021

driesvandamme commented Nov 9, 2021

piksel left a comment

piksel Nov 12, 2021

driesvandamme commented Dec 14, 2021

driesvandamme commented Dec 28, 2021

simskij commented Jan 22, 2022

Fix for certain problems on Synology devices #1068

Fix for certain problems on Synology devices #1068

Conversation

tammert commented Sep 4, 2021 • edited Loading

tammert Sep 4, 2021

Choose a reason for hiding this comment

piksel Nov 12, 2021

Choose a reason for hiding this comment

tammert Sep 4, 2021

Choose a reason for hiding this comment

piksel Nov 12, 2021

Choose a reason for hiding this comment

piksel commented Sep 5, 2021

tammert commented Sep 5, 2021

piksel commented Sep 19, 2021

simskij commented Oct 16, 2021

tammert commented Oct 18, 2021

driesvandamme commented Nov 9, 2021

piksel left a comment

Choose a reason for hiding this comment

piksel Nov 12, 2021

Choose a reason for hiding this comment

driesvandamme commented Dec 14, 2021

driesvandamme commented Dec 28, 2021

simskij commented Jan 22, 2022

tammert commented Sep 4, 2021 •

edited

Loading