From 6b74716490acbb01acc7d8637f41fcc3f4383ec1 Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Fri, 3 Jul 2020 10:03:46 +0200 Subject: [PATCH 1/5] Specific error message when user tries to remove one container from an ACI compose application, mentioning the name of the compose application and that user should use compose down --- azure/backend.go | 6 +++++- azure/backend_test.go | 9 +++++++++ example/backend.go | 3 ++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/azure/backend.go b/azure/backend.go index cd3a4654e..5d1f9213d 100644 --- a/azure/backend.go +++ b/azure/backend.go @@ -258,7 +258,11 @@ func (cs *aciContainerService) Logs(ctx context.Context, containerName string, r } func (cs *aciContainerService) Delete(ctx context.Context, containerID string, _ bool) error { - cg, err := deleteACIContainerGroup(ctx, cs.ctx, containerID) + groupName, containerName := getGroupAndContainerName(containerID) + if groupName != containerID { + return errors.New(fmt.Sprintf(`cannot delete service "%s" from compose app "%s", you must delete the entire compose app with docker compose down`, containerName, groupName)) + } + cg, err := deleteACIContainerGroup(ctx, cs.ctx, groupName) if err != nil { return err } diff --git a/azure/backend_test.go b/azure/backend_test.go index 6f50678df..7b17f704f 100644 --- a/azure/backend_test.go +++ b/azure/backend_test.go @@ -17,6 +17,7 @@ package azure import ( + "context" "testing" "github.com/stretchr/testify/suite" @@ -42,6 +43,14 @@ func (suite *BackendSuiteTest) TestGetContainerName() { Expect(container).To(Equal("service1")) } +func (suite *BackendSuiteTest) TestErrorMessageDeletingContainerFromComposeApplication() { + service := aciContainerService{} + err := service.Delete(context.TODO(), "compose-app_service1", false) + + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(Equal("cannot delete service \"service1\" from compose app \"compose-app\", you must delete the entire compose app with docker compose down")) +} + func TestBackendSuite(t *testing.T) { RegisterTestingT(t) suite.Run(t, new(BackendSuiteTest)) diff --git a/example/backend.go b/example/backend.go index 076b4087f..c5da8bf1e 100644 --- a/example/backend.go +++ b/example/backend.go @@ -22,9 +22,10 @@ import ( "context" "errors" "fmt" - "github.com/compose-spec/compose-go/cli" "io" + "github.com/compose-spec/compose-go/cli" + "github.com/docker/api/context/cloud" "github.com/docker/api/backend" From 76cb73c5c27cff805bf54f508cddc1c6fe2052e4 Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Fri, 3 Jul 2020 10:24:06 +0200 Subject: [PATCH 2/5] =?UTF-8?q?ACI=20:=20allow=20`docker=20compose=20down?= =?UTF-8?q?=20=E2=80=94project-name=20xxx`=20without=20requiring=20the=20c?= =?UTF-8?q?ompose=20file?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- azure/backend.go | 13 ++++++++++--- tests/aci-e2e/e2e-aci_test.go | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/azure/backend.go b/azure/backend.go index 5d1f9213d..6bb9f9200 100644 --- a/azure/backend.go +++ b/azure/backend.go @@ -319,9 +319,16 @@ func (cs *aciComposeService) Up(ctx context.Context, opts cli.ProjectOptions) er } func (cs *aciComposeService) Down(ctx context.Context, opts cli.ProjectOptions) error { - project, err := cli.ProjectFromOptions(&opts) - if err != nil { - return err + var project types.Project + + if opts.Name != "" { + project = types.Project{Name:opts.Name} + } else { + fullProject, err := cli.ProjectFromOptions(&opts) + if err != nil { + return err + } + project = *fullProject } logrus.Debugf("Down on project with name %q\n", project.Name) diff --git a/tests/aci-e2e/e2e-aci_test.go b/tests/aci-e2e/e2e-aci_test.go index e09ff74c6..41687fbb6 100644 --- a/tests/aci-e2e/e2e-aci_test.go +++ b/tests/aci-e2e/e2e-aci_test.go @@ -297,7 +297,7 @@ func (s *E2eACISuite) TestACIBackend() { }) s.T().Run("shutdown compose app", func(t *testing.T) { - s.NewDockerCommand("compose", "down", "-f", composeFile, "--project-name", "acidemo").ExecOrDie() + s.NewDockerCommand("compose", "down", "--project-name", "acidemo").ExecOrDie() }) s.T().Run("switches back to default context", func(t *testing.T) { From 568dc6de2387f7c5d4993386433475c065b0ce0a Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Fri, 3 Jul 2020 10:53:50 +0200 Subject: [PATCH 3/5] Do not allow ACI single containers with name that includes separator used for compose services (this would breaks docker logs, docker exec, docker rm using ) --- azure/backend.go | 15 +++++++++++---- azure/backend_test.go | 10 ++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/azure/backend.go b/azure/backend.go index 6bb9f9200..34c5bdf4b 100644 --- a/azure/backend.go +++ b/azure/backend.go @@ -42,7 +42,10 @@ import ( "github.com/docker/api/errdefs" ) -const singleContainerName = "single--container--aci" +const ( + singleContainerName = "single--container--aci" + composeContainerSeparator = "_" +) // ErrNoSuchContainer is returned when the mentioned container does not exist var ErrNoSuchContainer = errors.New("no such container") @@ -135,7 +138,7 @@ func (cs *aciContainerService) List(ctx context.Context, _ bool) ([]containers.C if *container.Name == singleContainerName { containerID = *containerGroup.Name } else { - containerID = *containerGroup.Name + "_" + *container.Name + containerID = *containerGroup.Name + composeContainerSeparator + *container.Name } status := "Unknown" if container.InstanceView != nil && container.InstanceView.CurrentState != nil { @@ -155,6 +158,10 @@ func (cs *aciContainerService) List(ctx context.Context, _ bool) ([]containers.C } func (cs *aciContainerService) Run(ctx context.Context, r containers.ContainerConfig) error { + if strings.Contains(r.ID, composeContainerSeparator) { + return errors.New(fmt.Sprintf(`invalid container name. ACI container name cannot include "%s"`, composeContainerSeparator)) + } + var ports []types.ServicePortConfig for _, p := range r.Ports { ports = append(ports, types.ServicePortConfig{ @@ -204,7 +211,7 @@ func (cs *aciContainerService) Stop(ctx context.Context, containerName string, t } func getGroupAndContainerName(containerID string) (groupName string, containerName string) { - tokens := strings.Split(containerID, "_") + tokens := strings.Split(containerID, composeContainerSeparator) groupName = tokens[0] if len(tokens) > 1 { containerName = tokens[len(tokens)-1] @@ -322,7 +329,7 @@ func (cs *aciComposeService) Down(ctx context.Context, opts cli.ProjectOptions) var project types.Project if opts.Name != "" { - project = types.Project{Name:opts.Name} + project = types.Project{Name: opts.Name} } else { fullProject, err := cli.ProjectFromOptions(&opts) if err != nil { diff --git a/azure/backend_test.go b/azure/backend_test.go index 7b17f704f..c70904894 100644 --- a/azure/backend_test.go +++ b/azure/backend_test.go @@ -20,6 +20,8 @@ import ( "context" "testing" + "github.com/docker/api/containers" + "github.com/stretchr/testify/suite" . "github.com/onsi/gomega" @@ -51,6 +53,14 @@ func (suite *BackendSuiteTest) TestErrorMessageDeletingContainerFromComposeAppli Expect(err.Error()).To(Equal("cannot delete service \"service1\" from compose app \"compose-app\", you must delete the entire compose app with docker compose down")) } +func (suite *BackendSuiteTest) TestErrorMessageRunSingleContainerNameWithComposeSeparator() { + service := aciContainerService{} + err := service.Run(context.TODO(), containers.ContainerConfig{ID: "container_name"}) + + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(Equal("invalid container name. ACI container name cannot include \"_\"")) +} + func TestBackendSuite(t *testing.T) { RegisterTestingT(t) suite.Run(t, new(BackendSuiteTest)) From 0dcab4004d4a5040ceabf7c5705d46fb21834f3a Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Fri, 3 Jul 2020 15:39:44 +0200 Subject: [PATCH 4/5] Use %q instead of \"%s\" Co-authored-by: Djordje Lukic --- azure/backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure/backend.go b/azure/backend.go index 34c5bdf4b..22484f15a 100644 --- a/azure/backend.go +++ b/azure/backend.go @@ -159,7 +159,7 @@ func (cs *aciContainerService) List(ctx context.Context, _ bool) ([]containers.C func (cs *aciContainerService) Run(ctx context.Context, r containers.ContainerConfig) error { if strings.Contains(r.ID, composeContainerSeparator) { - return errors.New(fmt.Sprintf(`invalid container name. ACI container name cannot include "%s"`, composeContainerSeparator)) + return errors.New(fmt.Sprintf("invalid container name. ACI container name cannot include %q", composeContainerSeparator)) } var ports []types.ServicePortConfig From da3c3dab1c730daca0d0b69318e6197956ccf254 Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Fri, 3 Jul 2020 15:40:25 +0200 Subject: [PATCH 5/5] @gtardif @rumpl Use %q instead of \"%s\" Co-authored-by: Djordje Lukic --- azure/backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure/backend.go b/azure/backend.go index 22484f15a..25ff9809e 100644 --- a/azure/backend.go +++ b/azure/backend.go @@ -267,7 +267,7 @@ func (cs *aciContainerService) Logs(ctx context.Context, containerName string, r func (cs *aciContainerService) Delete(ctx context.Context, containerID string, _ bool) error { groupName, containerName := getGroupAndContainerName(containerID) if groupName != containerID { - return errors.New(fmt.Sprintf(`cannot delete service "%s" from compose app "%s", you must delete the entire compose app with docker compose down`, containerName, groupName)) + return errors.New(fmt.Sprintf("cannot delete service %q from compose app %q, you must delete the entire compose app with docker compose down", containerName, groupName)) } cg, err := deleteACIContainerGroup(ctx, cs.ctx, groupName) if err != nil {