From 12467712a19ff5b99319b3b55cc798afb3da5ffa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sat, 24 Apr 2021 18:29:05 +0200 Subject: [PATCH] feat: check container config before update (#925) * feat: check container config before restart * fix: only skip when hostconfig and config differ * fix: update test mocks to not fail tests * test: add verify config tests --- cmd/root.go | 7 ++- internal/actions/mocks/container.go | 7 ++- internal/actions/update.go | 19 ++++-- pkg/container/container.go | 29 +++++++++ pkg/container/container_test.go | 95 +++++++++++++++++++++++++++-- pkg/container/errors.go | 7 +++ 6 files changed, 151 insertions(+), 13 deletions(-) create mode 100644 pkg/container/errors.go diff --git a/cmd/root.go b/cmd/root.go index 6f84727..707c4fd 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -269,6 +269,9 @@ func writeStartupMessage(c *cobra.Command, sched time.Time, filtering string) { } log.Info("Watchtower ", version, "\n", notifs, "\n", filtering, "\n", schedMessage) + if log.IsLevelEnabled(log.TraceLevel) { + log.Warn("trace level enabled: log will include sensitive information as credentials and tokens") + } } } @@ -330,8 +333,10 @@ func runUpdatesWithNotifications(filter t.Filter) *metrics.Metric { } metricResults, err := actions.Update(client, updateParams) if err != nil { - log.Println(err) + log.Error(err) } notifier.SendNotification() + log.Debugf("Session done: %v scanned, %v updated, %v failed", + metricResults.Scanned, metricResults.Updated, metricResults.Failed) return metricResults } diff --git a/internal/actions/mocks/container.go b/internal/actions/mocks/container.go index a4ded79..0c0ee94 100644 --- a/internal/actions/mocks/container.go +++ b/internal/actions/mocks/container.go @@ -16,10 +16,13 @@ func CreateMockContainer(id string, name string, image string, created time.Time Image: image, Name: name, Created: created.String(), + HostConfig: &container2.HostConfig{ + PortBindings: map[nat.Port][]nat.PortBinding{}, + }, }, Config: &container2.Config{ - Image: image, - Labels: make(map[string]string), + Image: image, + Labels: make(map[string]string), ExposedPorts: map[nat.Port]struct{}{}, }, } diff --git a/internal/actions/update.go b/internal/actions/update.go index 06bb345..66a28f1 100644 --- a/internal/actions/update.go +++ b/internal/actions/update.go @@ -1,7 +1,6 @@ package actions import ( - "errors" "github.com/containrrr/watchtower/internal/util" "github.com/containrrr/watchtower/pkg/container" "github.com/containrrr/watchtower/pkg/lifecycle" @@ -33,11 +32,23 @@ func Update(client container.Client, params types.UpdateParams) (*metrics2.Metri for i, targetContainer := range containers { stale, err := client.IsContainerStale(targetContainer) - if stale && !params.NoRestart && !params.MonitorOnly && !targetContainer.IsMonitorOnly() && !targetContainer.HasImageInfo() { - err = errors.New("no available image info") + shouldUpdate := stale && !params.NoRestart && !params.MonitorOnly && !targetContainer.IsMonitorOnly() + if err == nil && shouldUpdate { + // Check to make sure we have all the necessary information for recreating the container + err = targetContainer.VerifyConfiguration() + // If the image information is incomplete and trace logging is enabled, log it for further diagnosis + if err != nil && log.IsLevelEnabled(log.TraceLevel) { + imageInfo := targetContainer.ImageInfo() + log.Tracef("Image info: %#v", imageInfo) + log.Tracef("Container info: %#v", targetContainer.ContainerInfo()) + if imageInfo != nil { + log.Tracef("Image config: %#v", imageInfo.Config) + } + } } + if err != nil { - log.Infof("Unable to update container %q: %v. Proceeding to next.", containers[i].Name(), err) + log.Infof("Unable to update container %q: %v. Proceeding to next.", targetContainer.Name(), err) stale = false staleCheckFailed++ metric.Failed++ diff --git a/pkg/container/container.go b/pkg/container/container.go index 7631b5e..92abec2 100644 --- a/pkg/container/container.go +++ b/pkg/container/container.go @@ -258,3 +258,32 @@ func (c Container) HasImageInfo() bool { func (c Container) ImageInfo() *types.ImageInspect { return c.imageInfo } + +// VerifyConfiguration checks the container and image configurations for nil references to make sure +// that the container can be recreated once deleted +func (c Container) VerifyConfiguration() error { + if c.imageInfo == nil { + return errorNoImageInfo + } + + containerInfo := c.ContainerInfo() + if containerInfo == nil { + return errorInvalidConfig + } + + containerConfig := containerInfo.Config + if containerConfig == nil { + return errorInvalidConfig + } + + hostConfig := containerInfo.HostConfig + if hostConfig == nil { + return errorInvalidConfig + } + + if len(hostConfig.PortBindings) > 0 && containerConfig.ExposedPorts == nil { + return errorNoExposedPorts + } + + return nil +} diff --git a/pkg/container/container_test.go b/pkg/container/container_test.go index 8ddeb8b..8f22044 100644 --- a/pkg/container/container_test.go +++ b/pkg/container/container_test.go @@ -6,6 +6,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" cli "github.com/docker/docker/client" + "github.com/docker/go-connections/nat" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -32,14 +33,14 @@ var _ = Describe("the container", func() { containerKnown := *mockContainerWithImageName("docker.io/prefix/imagename:latest") When("warn on head failure is set to \"always\"", func() { - c := NewClient(false, false, false, false, false, "always") + c := newClientNoAPI(false, false, false, false, false, "always") It("should always return true", func() { Expect(c.WarnOnHeadPullFailed(containerUnknown)).To(BeTrue()) Expect(c.WarnOnHeadPullFailed(containerKnown)).To(BeTrue()) }) }) When("warn on head failure is set to \"auto\"", func() { - c := NewClient(false, false, false, false, false, "auto") + c := newClientNoAPI(false, false, false, false, false, "auto") It("should always return true", func() { Expect(c.WarnOnHeadPullFailed(containerUnknown)).To(BeFalse()) }) @@ -48,7 +49,7 @@ var _ = Describe("the container", func() { }) }) When("warn on head failure is set to \"never\"", func() { - c := NewClient(false, false, false, false, false, "never") + c := newClientNoAPI(false, false, false, false, false, "never") It("should never return true", func() { Expect(c.WarnOnHeadPullFailed(containerUnknown)).To(BeFalse()) Expect(c.WarnOnHeadPullFailed(containerKnown)).To(BeFalse()) @@ -130,6 +131,63 @@ var _ = Describe("the container", func() { }) }) }) + Describe("VerifyConfiguration", func() { + When("verifying a container with no image info", func() { + It("should return an error", func() { + c := mockContainerWithPortBindings() + c.imageInfo = nil + err := c.VerifyConfiguration() + Expect(err).To(Equal(errorNoImageInfo)) + }) + }) + When("verifying a container with no container info", func() { + It("should return an error", func() { + c := mockContainerWithPortBindings() + c.containerInfo = nil + err := c.VerifyConfiguration() + Expect(err).To(Equal(errorInvalidConfig)) + }) + }) + When("verifying a container with no config", func() { + It("should return an error", func() { + c := mockContainerWithPortBindings() + c.containerInfo.Config = nil + err := c.VerifyConfiguration() + Expect(err).To(Equal(errorInvalidConfig)) + }) + }) + When("verifying a container with no host config", func() { + It("should return an error", func() { + c := mockContainerWithPortBindings() + c.containerInfo.HostConfig = nil + err := c.VerifyConfiguration() + Expect(err).To(Equal(errorInvalidConfig)) + }) + }) + When("verifying a container with no port bindings", func() { + It("should not return an error", func() { + c := mockContainerWithPortBindings() + err := c.VerifyConfiguration() + Expect(err).ToNot(HaveOccurred()) + }) + }) + When("verifying a container with port bindings, but no exposed ports", func() { + It("should return an error", func() { + c := mockContainerWithPortBindings("80/tcp") + c.containerInfo.Config.ExposedPorts = nil + err := c.VerifyConfiguration() + Expect(err).To(Equal(errorNoExposedPorts)) + }) + }) + When("verifying a container with port bindings and exposed ports is non-nil", func() { + It("should return an error", func() { + c := mockContainerWithPortBindings("80/tcp") + c.containerInfo.Config.ExposedPorts = map[nat.Port]struct{}{"80/tcp": {}} + err := c.VerifyConfiguration() + Expect(err).ToNot(HaveOccurred()) + }) + }) + }) When("asked for metadata", func() { var c *Container BeforeEach(func() { @@ -281,10 +339,23 @@ var _ = Describe("the container", func() { }) }) +func mockContainerWithPortBindings(portBindingSources ...string) *Container { + mockContainer := mockContainerWithLabels(nil) + mockContainer.imageInfo = &types.ImageInspect{} + hostConfig := &container.HostConfig{ + PortBindings: nat.PortMap{}, + } + for _, pbs := range portBindingSources { + hostConfig.PortBindings[nat.Port(pbs)] = []nat.PortBinding{} + } + mockContainer.containerInfo.HostConfig = hostConfig + return mockContainer +} + func mockContainerWithImageName(name string) *Container { - container := mockContainerWithLabels(nil) - container.containerInfo.Config.Image = name - return container + mockContainer := mockContainerWithLabels(nil) + mockContainer.containerInfo.Config.Image = name + return mockContainer } func mockContainerWithLinks(links []string) *Container { @@ -317,3 +388,15 @@ func mockContainerWithLabels(labels map[string]string) *Container { } return NewContainer(&content, nil) } + +func newClientNoAPI(pullImages, includeStopped, reviveStopped, removeVolumes, includeRestarting bool, warnOnHeadFailed string) Client { + return dockerClient{ + api: nil, + pullImages: pullImages, + removeVolumes: removeVolumes, + includeStopped: includeStopped, + reviveStopped: reviveStopped, + includeRestarting: includeRestarting, + warnOnHeadFailed: warnOnHeadFailed, + } +} diff --git a/pkg/container/errors.go b/pkg/container/errors.go new file mode 100644 index 0000000..b927220 --- /dev/null +++ b/pkg/container/errors.go @@ -0,0 +1,7 @@ +package container + +import "errors" + +var errorNoImageInfo = errors.New("no available image info") +var errorNoExposedPorts = errors.New("exposed ports does not match port bindings") +var errorInvalidConfig = errors.New("container configuration missing or invalid")