From 2a170b61574d382464dac5f544a7e0f27d583d4c Mon Sep 17 00:00:00 2001 From: Nick Santos Date: Mon, 17 Jun 2024 21:39:42 -0400 Subject: [PATCH] registry: tweak how desired image is reconciled the old logic wasn't doing the 3-way merge quite right fixes https://github.com/tilt-dev/ctlptl/issues/344 Signed-off-by: Nick Santos --- pkg/registry/registry.go | 29 +++++++++++--- pkg/registry/registry_test.go | 74 +++++++++++++++++------------------ 2 files changed, 60 insertions(+), 43 deletions(-) diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index a33e528..832f560 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -64,10 +64,6 @@ func FillDefaults(registry *api.Registry) { if registry.Name == "" { registry.Name = "ctlptl-registry" } - - if registry.Image == "" { - registry.Image = DefaultRegistryImageRef - } } type socatController interface { @@ -212,7 +208,11 @@ func (c *Controller) Apply(ctx context.Context, desired *api.Registry) (*api.Reg // If the port has changed, let's delete the registry and recreate it. needsDelete = true } - if !imagesRefsEqual(existing.Status.Image, desired.Image) { + // If the desired image is different + // from the existing image, we need + // to delete the registry and recreate it. + if existing.Status.Image != "" && desired.Image != "" && + !imagesRefsEqual(existing.Status.Image, desired.Image) { needsDelete = true } if existing.Status.State != containerStateRunning { @@ -284,13 +284,15 @@ func (c *Controller) Apply(ctx context.Context, desired *api.Registry) (*api.Reg return nil, err } + image := c.imageConfig(existing, desired) + err = dctr.Run( ctx, c.dockerCLI, desired.Name, &container.Config{ Hostname: desired.Name, - Image: desired.Image, + Image: image, ExposedPorts: exposedPorts, Labels: c.labelConfigs(existing, desired), Env: desired.Env, @@ -377,6 +379,21 @@ func (c *Controller) labelConfigs(existing *api.Registry, desired *api.Registry) return newLabels } +// Compute the image to ContainerCreate() call +func (c *Controller) imageConfig(existing *api.Registry, desired *api.Registry) string { + // Desired image takes precedence. + if desired.Image != "" { + return desired.Image + } + + // Preserve existing image when possible. + if existing.Status.Image != "" { + return existing.Status.Image + } + + return DefaultRegistryImageRef +} + func (c *Controller) maybeCreateForwarder(ctx context.Context, port int) error { if docker.IsLocalHost(c.dockerCLI.Client().DaemonHost()) { return nil diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index 17047da..c3e29d8 100644 --- a/pkg/registry/registry_test.go +++ b/pkg/registry/registry_test.go @@ -27,7 +27,7 @@ func kindRegistry() types.Container { return types.Container{ ID: "a815c0ec15f1f7430bd402e3fffe65026dd692a1a99861a52b3e30ad6e253a08", Names: []string{"/kind-registry"}, - Image: "registry:2", + Image: DefaultRegistryImageRef, ImageID: "sha256:2d4f4b5309b1e41b4f83ae59b44df6d673ef44433c734b14c1c103ebca82c116", Command: "/entrypoint.sh /etc/docker/registry/config.yml", Created: 1603483645, @@ -56,7 +56,7 @@ func kindRegistryLoopback() types.Container { return types.Container{ ID: "d62f2587ff7b03858f144d3cf83c789578a6d6403f8b82a459ab4e317917cd42", Names: []string{"/kind-registry-loopback"}, - Image: "registry:2", + Image: DefaultRegistryImageRef, ImageID: "sha256:2d4f4b5309b1e41b4f83ae59b44df6d673ef44433c734b14c1c103ebca82c116", Command: "/entrypoint.sh /etc/docker/registry/config.yml", Created: 1603483646, @@ -139,7 +139,7 @@ func TestListRegistries(t *testing.T) { ContainerID: "a815c0ec15f1f7430bd402e3fffe65026dd692a1a99861a52b3e30ad6e253a08", State: "running", Labels: map[string]string{"dev.tilt.ctlptl.role": "registry"}, - Image: "registry:2", + Image: DefaultRegistryImageRef, Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=true", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}, }, }, list.Items[0]) @@ -174,7 +174,7 @@ func TestListRegistries(t *testing.T) { Networks: []string{"bridge", "kind"}, ContainerID: "d62f2587ff7b03858f144d3cf83c789578a6d6403f8b82a459ab4e317917cd42", State: "running", - Image: "registry:2", + Image: DefaultRegistryImageRef, Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=true", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}, }, }, list.Items[2]) @@ -202,7 +202,7 @@ func TestGetRegistry(t *testing.T) { ContainerID: "a815c0ec15f1f7430bd402e3fffe65026dd692a1a99861a52b3e30ad6e253a08", State: "running", Labels: map[string]string{"dev.tilt.ctlptl.role": "registry"}, - Image: "registry:2", + Image: DefaultRegistryImageRef, Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=true", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}, }, }, registry) @@ -216,10 +216,6 @@ func TestApplyDeadRegistry(t *testing.T) { deadRegistry.State = "dead" f.docker.containers = []types.Container{deadRegistry} - f.docker.onCreate = func() { - f.docker.containers = []types.Container{kindRegistry()} - } - registry, err := f.c.Apply(context.Background(), &api.Registry{ TypeMeta: typeMeta, Name: "kind-registry", @@ -239,10 +235,6 @@ func TestApplyLabels(t *testing.T) { // because it doesn't have the labels we want. f.docker.containers = []types.Container{kindRegistry()} - f.docker.onCreate = func() { - f.docker.containers = []types.Container{kindRegistry()} - } - registry, err := f.c.Apply(context.Background(), &api.Registry{ TypeMeta: typeMeta, Name: "kind-registry", @@ -258,7 +250,7 @@ func TestApplyLabels(t *testing.T) { "dev.tilt.ctlptl.role": "registry", }, config.Labels) assert.Equal(t, "kind-registry", config.Hostname) - assert.Equal(t, "docker.io/library/registry:2", config.Image) + assert.Equal(t, DefaultRegistryImageRef, config.Image) assert.Equal(t, []string{"REGISTRY_STORAGE_DELETE_ENABLED=true"}, config.Env) } } @@ -272,11 +264,6 @@ func TestPreservePort(t *testing.T) { existingRegistry.Ports[0].PublicPort = 5010 f.docker.containers = []types.Container{existingRegistry} - // Running a command makes the registry come alive! - f.docker.onCreate = func() { - f.docker.containers = []types.Container{kindRegistry()} - } - registry, err := f.c.Apply(context.Background(), &api.Registry{ TypeMeta: typeMeta, Name: "kind-registry", @@ -289,7 +276,7 @@ func TestPreservePort(t *testing.T) { if assert.NotNil(t, config) { assert.Equal(t, map[string]string{"dev.tilt.ctlptl.role": "registry"}, config.Labels) assert.Equal(t, "kind-registry", config.Hostname) - assert.Equal(t, "docker.io/library/registry:2", config.Image) + assert.Equal(t, DefaultRegistryImageRef, config.Image) } } @@ -301,15 +288,11 @@ func TestCustomImage(t *testing.T) { // because it doesn't have the image we want. f.docker.containers = []types.Container{kindRegistry()} - f.docker.onCreate = func() { - f.docker.containers = []types.Container{kindRegistry()} - } - // ensure stable w/o image change _, err := f.c.Apply(context.Background(), &api.Registry{ TypeMeta: typeMeta, Name: "kind-registry", - Image: "registry:2", + Image: DefaultRegistryImageRef, }) if assert.NoError(t, err) { assert.Nil(t, f.docker.lastCreateConfig, "Registry should not have been re-created") @@ -330,6 +313,26 @@ func TestCustomImage(t *testing.T) { assert.Equal(t, "kind-registry", config.Hostname) assert.Equal(t, "fake.tilt.dev/different-registry-image:latest", config.Image) } + + // Apply a config with new labels, + // ensure image is not changed. + registry, err = f.c.Apply(context.Background(), &api.Registry{ + TypeMeta: typeMeta, + Name: "kind-registry", + Labels: map[string]string{"extra-label": "ctlptl"}, + }) + if assert.NoError(t, err) { + assert.Equal(t, "running", registry.Status.State) + } + config = f.docker.lastCreateConfig + if assert.NotNil(t, config) { + assert.Equal(t, map[string]string{ + "dev.tilt.ctlptl.role": "registry", + "extra-label": "ctlptl", + }, config.Labels) + assert.Equal(t, "kind-registry", config.Hostname) + assert.Equal(t, "fake.tilt.dev/different-registry-image:latest", config.Image) + } } func TestCustomEnv(t *testing.T) { @@ -340,15 +343,11 @@ func TestCustomEnv(t *testing.T) { // because it doesn't have the image we want. f.docker.containers = []types.Container{kindRegistry()} - f.docker.onCreate = func() { - f.docker.containers = []types.Container{kindRegistry()} - } - // ensure stable w/o image change _, err := f.c.Apply(context.Background(), &api.Registry{ TypeMeta: typeMeta, Name: "kind-registry", - Image: "registry:2", + Image: DefaultRegistryImageRef, }) if assert.NoError(t, err) { assert.Nil(t, f.docker.lastCreateConfig, "Registry should not have been re-created") @@ -358,7 +357,7 @@ func TestCustomEnv(t *testing.T) { registry, err := f.c.Apply(context.Background(), &api.Registry{ TypeMeta: typeMeta, Name: "kind-registry", - Image: "registry:2", + Image: DefaultRegistryImageRef, Env: []string{"REGISTRY_STORAGE_DELETE_ENABLED=false"}, }) if assert.NoError(t, err) { @@ -368,7 +367,7 @@ func TestCustomEnv(t *testing.T) { if assert.NotNil(t, config) { assert.Equal(t, map[string]string{"dev.tilt.ctlptl.role": "registry"}, config.Labels) assert.Equal(t, "kind-registry", config.Hostname) - assert.Equal(t, "registry:2", config.Image) + assert.Equal(t, DefaultRegistryImageRef, config.Image) assert.Equal(t, []string{"REGISTRY_STORAGE_DELETE_ENABLED=false"}, config.Env) } } @@ -390,7 +389,6 @@ type fakeDocker struct { lastRemovedContainer string lastCreateConfig *container.Config lastCreateHostConfig *container.HostConfig - onCreate func() } type objectNotFoundError struct { @@ -432,7 +430,7 @@ func (d *fakeDocker) ContainerInspect(ctx context.Context, containerID string) ( Cmd: []string{"/etc/docker/registry/config.yml"}, Healthcheck: (*container.HealthConfig)(nil), ArgsEscaped: false, - Image: "docker.io/library/registry:2", + Image: DefaultRegistryImageRef, Volumes: map[string]struct{}{"/var/lib/registry": struct{}{}}, WorkingDir: "", Entrypoint: []string{"/entrypoint.sh"}, @@ -483,9 +481,11 @@ func (d *fakeDocker) ContainerCreate(ctx context.Context, config *container.Conf containerName string) (container.CreateResponse, error) { d.lastCreateConfig = config d.lastCreateHostConfig = hostConfig - if d.onCreate != nil { - d.onCreate() - } + + c := kindRegistry() + c.Image = config.Image + d.containers = []types.Container{c} + return container.CreateResponse{}, nil } func (d *fakeDocker) ContainerStart(ctx context.Context, containerID string,