Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for certain problems on Synology devices #1068

Closed
wants to merge 1 commit into from
Closed

Fix for certain problems on Synology devices #1068

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions pkg/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,10 @@ func (c Container) runtimeConfig() *dockercontainer.Config {
}
}
for p := range c.containerInfo.HostConfig.PortBindings {
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{}{}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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...

}
}

config.Image = c.ImageName()
Expand Down Expand Up @@ -298,9 +301,5 @@ func (c Container) VerifyConfiguration() error {
return errorInvalidConfig
}

if len(hostConfig.PortBindings) > 0 && containerConfig.ExposedPorts == nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return errorNoExposedPorts
}

return nil
}