From bde922093bb600f44379f9a88fd8cd70388412ef Mon Sep 17 00:00:00 2001 From: Penghao He Date: Fri, 28 Jul 2023 13:50:20 -0700 Subject: [PATCH 1/2] docs: update efs and fix fmt bug for additional rule hc (#5136) Related to #5127 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License. --- site/content/docs/developing/storage.en.md | 12 ++++++++---- site/content/docs/include/http-additionalrules.en.md | 2 +- site/content/docs/include/http-additionalrules.ja.md | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/site/content/docs/developing/storage.en.md b/site/content/docs/developing/storage.en.md index 9386c47fade..d9e107b4e1e 100644 --- a/site/content/docs/developing/storage.en.md +++ b/site/content/docs/developing/storage.en.md @@ -77,9 +77,7 @@ storage: read_only: false ``` -This manifest will result in an EFS volume being created at the environment level, with an Access Point and dedicated directory at the path `/frontend` in the EFS filesystem created specifically for your service. Your container will be able to access this directory and all its subdirectories at the `/var/efs` path in its own filesystem. The `/frontend` directory and EFS filesystem will persist until you delete your environment. - -The use of an access point for each service ensures that no two services can access each other's data unless you specifically intend for them to do so by specifying the full advanced configuration. You can read more in [Advanced Use Cases](#advanced-use-cases). +This manifest will result in an EFS volume being created at the environment level, with an Access Point and dedicated directory at the path `/frontend` in the EFS filesystem created specifically for your service. Your container will be able to access this directory and all its subdirectories at the `/var/efs` path in its own filesystem. The `/frontend` directory and EFS filesystem will persist until you delete your environment. The use of an access point for each service ensures that no two services can access each other's data. You can also customize the UID and GID used for the access point by specifying the `uid` and `gid` fields in advanced EFS configuration. If you do not specify a UID or GID, Copilot picks a pseudorandom UID and GID for the access point based on the [CRC32 checksum](https://stackoverflow.com/a/14210379/5890422) of the service's name. @@ -126,8 +124,11 @@ This may not be suitable for workloads which depend on the correct data being pr ###### Using `copilot svc exec` For workloads where data must be present prior to your task containers coming up, we recommend using a placeholder container first. -For example, deploy your service with the following values in the manifest: +For example, deploy your `frontend` service with the following values in the manifest: ```yaml +name: frontend +type: Load Balanced Web Service + image: location: amazon/amazon-ecs-sample exec: true @@ -152,6 +153,9 @@ This will open an interactive shell from which you can add packages like `curl` When you have populated the directory, modify your manifest to remove the `exec` directive and update the `build` field to your desired Docker build config or image location. ```yaml +name: frontend +type: Load Balanced Web Service + image: build: ./Dockerfile storage: diff --git a/site/content/docs/include/http-additionalrules.en.md b/site/content/docs/include/http-additionalrules.en.md index 914da65ad2d..89e0afa5932 100644 --- a/site/content/docs/include/http-additionalrules.en.md +++ b/site/content/docs/include/http-additionalrules.en.md @@ -1,7 +1,7 @@ http.additional_rules.`path` String Requests to this path will be forwarded to your service. Each listener rule should listen on a unique path. - {% include 'http-additionalrules-healthcheck.en.md' %} +{% include 'http-additionalrules-healthcheck.en.md' %} http.additional_rules.`deregistration_delay` Duration The amount of time to wait for targets to drain connections during deregistration. The default is 60s. Setting this to a larger value gives targets more time to gracefully drain connections, but increases the time required for new deployments. Range 0s-3600s. diff --git a/site/content/docs/include/http-additionalrules.ja.md b/site/content/docs/include/http-additionalrules.ja.md index 03b30edca00..3c2a57e757e 100644 --- a/site/content/docs/include/http-additionalrules.ja.md +++ b/site/content/docs/include/http-additionalrules.ja.md @@ -2,7 +2,7 @@ 指定したパスに対するリクエストが、Service に転送されます。各リスナールールはユニークなパスで 公開している必要があります。 - {% include 'http-additionalrules-healthcheck.ja.md' %} +{% include 'http-additionalrules-healthcheck.ja.md' %} http.additional_rules.`deregistration_delay` Duration 登録解除時に、ターゲットがコネクションをドレイニングするのを待つ時間です。デフォルト値は 60 秒です。大きな値に設定すると、安全にコネクションをドレイニングするのに長い時間を使える様になりますが、新しいデプロイに必要な時間が増加します。設定可能な範囲は、0 秒 - 3600 秒です。 From fc2dc09107c23ce749d1d266a276848eea29216f Mon Sep 17 00:00:00 2001 From: Penghao He Date: Fri, 28 Jul 2023 14:43:39 -0700 Subject: [PATCH 2/2] fix(ecs): filter out non-active ECS services (#5125) Applies similar fix as https://github.com/aws/copilot-cli/pull/5062 to ecs service. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License. --- internal/pkg/aws/ecs/ecs.go | 42 +++++++++++-- internal/pkg/aws/ecs/ecs_test.go | 89 +++++++++++++++++++++++++--- internal/pkg/aws/ecs/service.go | 65 +++++++++++++------- internal/pkg/aws/ecs/service_test.go | 49 +++++++++++++++ internal/pkg/cli/task_run.go | 11 +--- internal/pkg/ecs/ecs.go | 37 ++++++------ internal/pkg/ecs/ecs_test.go | 89 ++++++++++++++++++---------- internal/pkg/ecs/mocks/mock_ecs.go | 19 ++++++ internal/pkg/term/progress/ecs.go | 9 ++- 9 files changed, 315 insertions(+), 95 deletions(-) diff --git a/internal/pkg/aws/ecs/ecs.go b/internal/pkg/aws/ecs/ecs.go index 7911def5db9..8f07c5c550a 100644 --- a/internal/pkg/aws/ecs/ecs.go +++ b/internal/pkg/aws/ecs/ecs.go @@ -19,7 +19,7 @@ import ( ) const ( - clusterStatusActive = "ACTIVE" + statusActive = "ACTIVE" waitServiceStablePollingInterval = 15 * time.Second waitServiceStableMaxTry = 80 stableServiceDeploymentNum = 1 @@ -301,7 +301,7 @@ func (e *ECS) DefaultCluster() (string, error) { // NOTE: right now at most 1 default cluster is possible, so cluster[0] must be the default cluster cluster := resp.Clusters[0] - if aws.StringValue(cluster.Status) != clusterStatusActive { + if aws.StringValue(cluster.Status) != statusActive { return "", ErrNoDefaultCluster } @@ -319,7 +319,7 @@ func (e *ECS) HasDefaultCluster() (bool, error) { return true, nil } -// ActiveClusters returns the subset of arns that have an ACTIVE status. +// ActiveClusters returns the subset of cluster arns that have an ACTIVE status. func (e *ECS) ActiveClusters(arns ...string) ([]string, error) { resp, err := e.client.DescribeClusters(&ecs.DescribeClustersInput{ Clusters: aws.StringSlice(arns), @@ -333,13 +333,47 @@ func (e *ECS) ActiveClusters(arns ...string) ([]string, error) { var active []string for _, cluster := range resp.Clusters { - if aws.StringValue(cluster.Status) == clusterStatusActive { + if aws.StringValue(cluster.Status) == statusActive { active = append(active, aws.StringValue(cluster.ClusterArn)) } } return active, nil +} +// ActiveServices returns the subset of service arns that have an ACTIVE status. +// Note that all services should be in the same cluster. +func (e *ECS) ActiveServices(serviceARNs ...string) ([]string, error) { + var prevSvcArn *ServiceArn + for _, arn := range serviceARNs { + svcArn, err := ParseServiceArn(arn) + if err != nil { + return nil, err + } + if prevSvcArn != nil && prevSvcArn.clusterName != svcArn.clusterName { + return nil, fmt.Errorf("service %q and service %q should be in the same cluster", prevSvcArn.String(), svcArn.String()) + } + prevSvcArn = svcArn + } + resp, err := e.client.DescribeServices(&ecs.DescribeServicesInput{ + Cluster: aws.String(prevSvcArn.ClusterArn()), + Services: aws.StringSlice(serviceARNs), + }) + switch { + case err != nil: + return nil, fmt.Errorf("describe services: %w", err) + case len(resp.Failures) > 0: + return nil, fmt.Errorf("describe services: %s", resp.Failures[0].GoString()) + } + + var active []string + for _, svc := range resp.Services { + if aws.StringValue(svc.Status) == statusActive { + active = append(active, aws.StringValue(svc.ServiceArn)) + } + } + + return active, nil } // RunTask runs a number of tasks with the task definition and network configurations in a cluster, and returns after diff --git a/internal/pkg/aws/ecs/ecs_test.go b/internal/pkg/aws/ecs/ecs_test.go index d34f0387bab..05debf8aec7 100644 --- a/internal/pkg/aws/ecs/ecs_test.go +++ b/internal/pkg/aws/ecs/ecs_test.go @@ -732,12 +732,12 @@ func TestECS_DefaultCluster(t *testing.T) { { ClusterArn: aws.String("arn:aws:ecs:us-east-1:0123456:cluster/cluster1"), ClusterName: aws.String("cluster1"), - Status: aws.String(clusterStatusActive), + Status: aws.String(statusActive), }, { ClusterArn: aws.String("arn:aws:ecs:us-east-1:0123456:cluster/cluster2"), ClusterName: aws.String("cluster2"), - Status: aws.String(clusterStatusActive), + Status: aws.String(statusActive), }, }, }, nil) @@ -822,7 +822,7 @@ func TestECS_HasDefaultCluster(t *testing.T) { Clusters: []*ecs.Cluster{ { ClusterArn: aws.String("cluster"), - Status: aws.String(clusterStatusActive), + Status: aws.String(statusActive), }, }, }, nil) @@ -857,12 +857,14 @@ func TestECS_HasDefaultCluster(t *testing.T) { func TestECS_ActiveClusters(t *testing.T) { testCases := map[string]struct { + inArns []string mockECSClient func(m *mocks.Mockapi) wantedError error wantedClusters []string }{ "describe clusters returns error": { + inArns: []string{"arn1"}, mockECSClient: func(m *mocks.Mockapi) { m.EXPECT(). DescribeClusters(gomock.Any()). @@ -871,14 +873,17 @@ func TestECS_ActiveClusters(t *testing.T) { wantedError: fmt.Errorf("describe clusters: some error"), }, "ignore inactive cluster": { + inArns: []string{"arn1", "arn2"}, mockECSClient: func(m *mocks.Mockapi) { m.EXPECT(). - DescribeClusters(gomock.Any()). + DescribeClusters(&ecs.DescribeClustersInput{ + Clusters: aws.StringSlice([]string{"arn1", "arn2"}), + }). Return(&ecs.DescribeClustersOutput{ Clusters: []*ecs.Cluster{ { ClusterArn: aws.String("cluster1"), - Status: aws.String(clusterStatusActive), + Status: aws.String(statusActive), }, { ClusterArn: aws.String("cluster2"), @@ -886,7 +891,7 @@ func TestECS_ActiveClusters(t *testing.T) { }, { ClusterArn: aws.String("cluster3"), - Status: aws.String(clusterStatusActive), + Status: aws.String(statusActive), }, { ClusterArn: aws.String("cluster4"), @@ -913,7 +918,7 @@ func TestECS_ActiveClusters(t *testing.T) { ecs := ECS{ client: mockECSClient, } - clusters, err := ecs.ActiveClusters("arn1", "arn2") + clusters, err := ecs.ActiveClusters(tc.inArns...) if tc.wantedError != nil { require.EqualError(t, tc.wantedError, err.Error()) } else { @@ -923,6 +928,76 @@ func TestECS_ActiveClusters(t *testing.T) { } } +func TestECS_ActiveServices(t *testing.T) { + testCases := map[string]struct { + inArns []string + mockECSClient func(m *mocks.Mockapi) + + wantedError error + wantedServices []string + }{ + "error if services are not in the same cluster": { + inArns: []string{"arn:aws:ecs:us-west-2:1234567890:service/cluster1/svc1", "arn:aws:ecs:us-west-2:1234567890:service/cluster2/svc2"}, + mockECSClient: func(m *mocks.Mockapi) { + m.EXPECT(). + DescribeServices(gomock.Any()).Times(0) + }, + wantedError: fmt.Errorf(`service "arn:aws:ecs:us-west-2:1234567890:service/cluster1/svc1" and service "arn:aws:ecs:us-west-2:1234567890:service/cluster2/svc2" should be in the same cluster`), + }, + "describe services returns error": { + inArns: []string{"arn:aws:ecs:us-west-2:1234567890:service/cluster1/svc1"}, + mockECSClient: func(m *mocks.Mockapi) { + m.EXPECT(). + DescribeServices(gomock.Any()). + Return(nil, fmt.Errorf("some error")) + }, + wantedError: fmt.Errorf("describe services: some error"), + }, + "ignore inactive service": { + inArns: []string{"arn:aws:ecs:us-west-2:1234567890:service/cluster1/svc1", "arn:aws:ecs:us-west-2:1234567890:service/cluster1/svc2"}, + mockECSClient: func(m *mocks.Mockapi) { + m.EXPECT(). + DescribeServices(gomock.Any()). + Return(&ecs.DescribeServicesOutput{ + Services: []*ecs.Service{ + { + ServiceArn: aws.String("service1"), + Status: aws.String(statusActive), + }, + { + ServiceArn: aws.String("service2"), + Status: aws.String("random"), + }, + }, + }, nil) + }, + wantedServices: []string{ + "service1", + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockECSClient := mocks.NewMockapi(ctrl) + tc.mockECSClient(mockECSClient) + + ecs := ECS{ + client: mockECSClient, + } + services, err := ecs.ActiveServices(tc.inArns...) + if tc.wantedError != nil { + require.EqualError(t, tc.wantedError, err.Error()) + } else { + require.Equal(t, tc.wantedServices, services) + } + }) + } +} + func TestECS_RunTask(t *testing.T) { type input struct { cluster string diff --git a/internal/pkg/aws/ecs/service.go b/internal/pkg/aws/ecs/service.go index 616fbeb096e..ecab627aad5 100644 --- a/internal/pkg/aws/ecs/service.go +++ b/internal/pkg/aws/ecs/service.go @@ -117,38 +117,63 @@ func (s *Service) TargetGroups() []string { } // ServiceArn is the arn of an ECS service. -type ServiceArn string +type ServiceArn struct { + accountID string + partition string + region string + name string + clusterName string +} -// ClusterName returns the cluster name. +// ParseServiceArn parses an arn into ServiceArn. // For example: arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB -// will return my-project-test-Cluster-9F7Y0RLP60R7 -func (s *ServiceArn) ClusterName() (string, error) { - serviceArn := string(*s) - parsedArn, err := arn.Parse(serviceArn) +func ParseServiceArn(s string) (*ServiceArn, error) { + parsedArn, err := arn.Parse(s) if err != nil { - return "", err + return nil, err + } + if parsedArn.Service != EndpointsID { + return nil, fmt.Errorf("expected an ECS arn, but got %q", s) } resources := strings.Split(parsedArn.Resource, "/") if len(resources) != 3 { - return "", fmt.Errorf("cannot parse resource for ARN %s", serviceArn) + return nil, fmt.Errorf("cannot parse resource for ARN %q", s) + } + if resources[0] != "service" { + return nil, fmt.Errorf("expect an ECS service: got %q", s) } - return resources[1], nil + return &ServiceArn{ + accountID: parsedArn.AccountID, + partition: parsedArn.Partition, + region: parsedArn.Region, + name: resources[2], + clusterName: resources[1], + }, nil +} + +func (s *ServiceArn) String() string { + return fmt.Sprintf("arn:%s:%s:%s:%s:service/%s/%s", s.partition, EndpointsID, s.region, s.accountID, s.clusterName, s.name) +} + +// ClusterArn returns the cluster arn. +// For example: arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB +// will return arn:aws:ecs:us-west-2:1234567890:cluster/my-project-test-Cluster-9F7Y0RLP60R7 +func (s *ServiceArn) ClusterArn() string { + return fmt.Sprintf("arn:%s:%s:%s:%s:cluster/%s", s.partition, EndpointsID, s.region, s.accountID, s.clusterName) } // ServiceName returns the service name. // For example: arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB // will return my-project-test-myService-JSOH5GYBFAIB -func (s *ServiceArn) ServiceName() (string, error) { - serviceArn := string(*s) - parsedArn, err := arn.Parse(serviceArn) - if err != nil { - return "", err - } - resources := strings.Split(parsedArn.Resource, "/") - if len(resources) != 3 { - return "", fmt.Errorf("cannot parse resource for ARN %s", serviceArn) - } - return resources[2], nil +func (s *ServiceArn) ServiceName() string { + return s.name +} + +// ClusterName returns the cluster name. +// For example: arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB +// will return my-project-test-Cluster-9F7Y0RLP60R7 +func (s *ServiceArn) ClusterName() string { + return s.clusterName } // NetworkConfiguration holds service's NetworkConfiguration. diff --git a/internal/pkg/aws/ecs/service_test.go b/internal/pkg/aws/ecs/service_test.go index 8fb370d0109..cb6eef49de1 100644 --- a/internal/pkg/aws/ecs/service_test.go +++ b/internal/pkg/aws/ecs/service_test.go @@ -4,6 +4,7 @@ package ecs import ( + "fmt" "testing" "time" @@ -178,3 +179,51 @@ func TestService_ServiceConnectAliases(t *testing.T) { }) } } + +func TestParseServiceArn(t *testing.T) { + tests := map[string]struct { + inArnStr string + + wantedError error + wantedArn ServiceArn + }{ + "error if invalid arn": { + inArnStr: "random string", + wantedError: fmt.Errorf("arn: invalid prefix"), + }, + "error if non ecs arn": { + inArnStr: "arn:aws:acm:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB", + wantedError: fmt.Errorf(`expected an ECS arn, but got "arn:aws:acm:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB"`), + }, + "error if invalid resource": { + inArnStr: "arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7", + wantedError: fmt.Errorf(`cannot parse resource for ARN "arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7"`), + }, + "error if invalid resource type": { + inArnStr: "arn:aws:ecs:us-west-2:1234567890:task/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB", + wantedError: fmt.Errorf(`expect an ECS service: got "arn:aws:ecs:us-west-2:1234567890:task/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB"`), + }, + "success": { + inArnStr: "arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB", + wantedArn: ServiceArn{ + accountID: "1234567890", + partition: "aws", + region: "us-west-2", + name: "my-project-test-myService-JSOH5GYBFAIB", + clusterName: "my-project-test-Cluster-9F7Y0RLP60R7", + }, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + // WHEN + arn, err := ParseServiceArn(tc.inArnStr) + if tc.wantedError != nil { + require.EqualError(t, tc.wantedError, err.Error()) + } else { + require.Equal(t, tc.wantedArn, *arn) + } + }) + } +} diff --git a/internal/pkg/cli/task_run.go b/internal/pkg/cli/task_run.go index a4a8100ac35..55692cdffef 100644 --- a/internal/pkg/cli/task_run.go +++ b/internal/pkg/cli/task_run.go @@ -829,16 +829,11 @@ func (o *runTaskOpts) runTaskCommand() (cliStringer, error) { } func (o *runTaskOpts) parseARN() (string, string, error) { - svcARN := awsecs.ServiceArn(o.generateCommandTarget) - clusterName, err := svcARN.ClusterName() + svcARN, err := awsecs.ParseServiceArn(o.generateCommandTarget) if err != nil { - return "", "", fmt.Errorf("extract cluster name from arn %s: %w", svcARN, err) + return "", "", fmt.Errorf("parse service arn %s: %w", o.generateCommandTarget, err) } - serviceName, err := svcARN.ServiceName() - if err != nil { - return "", "", fmt.Errorf("extract service name from arn %s: %w", svcARN, err) - } - return clusterName, serviceName, nil + return svcARN.ClusterName(), svcARN.ServiceName(), nil } func (o *runTaskOpts) runTaskCommandFromECSService(sess *session.Session, clusterName, serviceName string) (cliStringer, error) { diff --git a/internal/pkg/ecs/ecs.go b/internal/pkg/ecs/ecs.go index e3c4f71075a..8a5c7673bec 100644 --- a/internal/pkg/ecs/ecs.go +++ b/internal/pkg/ecs/ecs.go @@ -48,6 +48,7 @@ type ecsClient interface { UpdateService(clusterName, serviceName string, opts ...ecs.UpdateServiceOpts) error DescribeTasks(cluster string, taskARNs []string) ([]*ecs.Task, error) ActiveClusters(arns ...string) ([]string, error) + ActiveServices(serviceARNs ...string) ([]string, error) } type stepFunctionsClient interface { @@ -305,18 +306,11 @@ func (c Client) NetworkConfiguration(app, env, svc string) (*ecs.NetworkConfigur if err != nil { return nil, err } - arn, err := c.serviceARN(app, env, svc) if err != nil { return nil, err } - - svcName, err := arn.ServiceName() - if err != nil { - return nil, fmt.Errorf("extract service name from arn %s: %w", *arn, err) - } - - return c.ecsClient.NetworkConfiguration(clusterARN, svcName) + return c.ecsClient.NetworkConfiguration(clusterARN, arn.ServiceName()) } // NetworkConfigurationForJob returns the network configuration of the job. @@ -482,15 +476,7 @@ func (c Client) fetchAndParseServiceARN(app, env, svc string) (cluster, service if err != nil { return "", "", err } - clusterName, err := svcARN.ClusterName() - if err != nil { - return "", "", fmt.Errorf("get cluster name: %w", err) - } - serviceName, err := svcARN.ServiceName() - if err != nil { - return "", "", fmt.Errorf("get service name: %w", err) - } - return clusterName, serviceName, nil + return svcARN.ClusterName(), svcARN.ServiceName(), nil } func (c Client) serviceARN(app, env, svc string) (*ecs.ServiceArn, error) { @@ -506,11 +492,22 @@ func (c Client) serviceARN(app, env, svc string) (*ecs.ServiceArn, error) { if len(services) == 0 { return nil, fmt.Errorf("no ECS service found with tags %s", tags.String()) } - if len(services) > 1 { + arns := make([]string, len(services)) + for i := range services { + arns[i] = services[i].ARN + } + active, err := c.ecsClient.ActiveServices(arns...) + if err != nil { + return nil, fmt.Errorf("check if services are active: %w", err) + } + if len(active) > 1 { return nil, fmt.Errorf("more than one ECS service with tags %s", tags.String()) } - serviceArn := ecs.ServiceArn(services[0].ARN) - return &serviceArn, nil + serviceARN, err := ecs.ParseServiceArn(active[0]) + if err != nil { + return nil, fmt.Errorf("parse service arn: %w", err) + } + return serviceARN, nil } type tags map[string]string diff --git a/internal/pkg/ecs/ecs_test.go b/internal/pkg/ecs/ecs_test.go index 025335f0c9d..3bb5bd76170 100644 --- a/internal/pkg/ecs/ecs_test.go +++ b/internal/pkg/ecs/ecs_test.go @@ -132,9 +132,11 @@ func TestClient_ClusterARN(t *testing.T) { func TestClient_serviceARN(t *testing.T) { const ( - mockApp = "mockApp" - mockEnv = "mockEnv" - mockSvc = "mockSvc" + mockApp = "mockApp" + mockEnv = "mockEnv" + mockSvc = "mockSvc" + mockSvcARN1 = "arn:aws:ecs:us-west-2:1234567890:service/mockCluster/mockService1" + mockSvcARN2 = "arn:aws:ecs:us-west-2:1234567890:service/mockCluster/mockService2" ) getRgInput := map[string]string{ deploy.AppTagKey: mockApp, @@ -146,8 +148,8 @@ func TestClient_serviceARN(t *testing.T) { tests := map[string]struct { setupMocks func(mocks clientMocks) - wantedError error - wantedService string + wantedError error + wantedSvcArn string }{ "errors if fail to get resources by tags": { setupMocks: func(m clientMocks) { @@ -172,22 +174,24 @@ func TestClient_serviceARN(t *testing.T) { gomock.InOrder( m.resourceGetter.EXPECT().GetResourcesByTags(serviceResourceType, getRgInput). Return([]*resourcegroups.Resource{ - {ARN: "mockARN1"}, {ARN: "mockARN2"}, + {ARN: mockSvcARN1}, {ARN: mockSvcARN2}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN1, mockSvcARN2).Return([]string{mockSvcARN1, mockSvcARN2}, nil), ) }, wantedError: fmt.Errorf(`more than one ECS service with tags "copilot-application"="mockApp","copilot-environment"="mockEnv","copilot-service"="mockSvc"`), }, - "success": { + "success with only one active svc": { setupMocks: func(m clientMocks) { gomock.InOrder( m.resourceGetter.EXPECT().GetResourcesByTags(serviceResourceType, getRgInput). Return([]*resourcegroups.Resource{ - {ARN: "mockARN"}, + {ARN: mockSvcARN1}, {ARN: mockSvcARN2}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN1, mockSvcARN2).Return([]string{mockSvcARN1}, nil), ) }, - wantedService: "mockARN", + wantedSvcArn: mockSvcARN1, }, } @@ -197,15 +201,16 @@ func TestClient_serviceARN(t *testing.T) { defer ctrl.Finish() // GIVEN - mockRgGetter := mocks.NewMockresourceGetter(ctrl) mocks := clientMocks{ - resourceGetter: mockRgGetter, + resourceGetter: mocks.NewMockresourceGetter(ctrl), + ecsClient: mocks.NewMockecsClient(ctrl), } test.setupMocks(mocks) client := Client{ - rgGetter: mockRgGetter, + rgGetter: mocks.resourceGetter, + ecsClient: mocks.ecsClient, } // WHEN @@ -216,7 +221,7 @@ func TestClient_serviceARN(t *testing.T) { require.EqualError(t, err, test.wantedError.Error()) } else { require.NoError(t, err) - require.Equal(t, string(*get), test.wantedService) + require.Equal(t, get.String(), test.wantedSvcArn) } }) } @@ -244,17 +249,6 @@ func TestClient_DescribeService(t *testing.T) { wantedError error wanted *ServiceDesc }{ - "return error if failed to get cluster name": { - setupMocks: func(m clientMocks) { - gomock.InOrder( - m.resourceGetter.EXPECT().GetResourcesByTags(serviceResourceType, getRgInput). - Return([]*resourcegroups.Resource{ - {ARN: badSvcARN}, - }, nil), - ) - }, - wantedError: fmt.Errorf("get cluster name: arn: invalid prefix"), - }, "return error if failed to get service tasks": { setupMocks: func(m clientMocks) { gomock.InOrder( @@ -262,6 +256,7 @@ func TestClient_DescribeService(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().ServiceRunningTasks(mockCluster, mockService).Return(nil, errors.New("some error")), ) }, @@ -274,6 +269,7 @@ func TestClient_DescribeService(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().ServiceRunningTasks(mockCluster, mockService).Return([]*ecs.Task{ {TaskArn: aws.String("mockTaskARN")}, }, nil), @@ -289,6 +285,7 @@ func TestClient_DescribeService(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().ServiceRunningTasks(mockCluster, mockService).Return([]*ecs.Task{ {TaskArn: aws.String("mockTaskARN")}, }, nil), @@ -366,6 +363,27 @@ func TestClient_Service(t *testing.T) { wantedError error wanted *ecs.Service }{ + "error if fail to get resources by tag": { + setupMocks: func(m clientMocks) { + gomock.InOrder( + m.resourceGetter.EXPECT().GetResourcesByTags(serviceResourceType, getRgInput). + Return(nil, mockError), + ) + }, + wantedError: fmt.Errorf(`get ECS service with tags "copilot-application"="mockApp","copilot-environment"="mockEnv","copilot-service"="mockSvc": some error`), + }, + "error if fail to filter for active services": { + setupMocks: func(m clientMocks) { + gomock.InOrder( + m.resourceGetter.EXPECT().GetResourcesByTags(serviceResourceType, getRgInput). + Return([]*resourcegroups.Resource{ + {ARN: mockSvcARN}, + }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return(nil, mockError), + ) + }, + wantedError: fmt.Errorf("check if services are active: some error"), + }, "error if fail to describe ECS service": { setupMocks: func(m clientMocks) { gomock.InOrder( @@ -373,6 +391,7 @@ func TestClient_Service(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().Service(mockCluster, mockService).Return(nil, mockError), ) }, @@ -385,6 +404,7 @@ func TestClient_Service(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().Service(mockCluster, mockService).Return(nil, mockError), ) }, @@ -397,6 +417,7 @@ func TestClient_Service(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().Service(mockCluster, mockService).Return(&ecs.Service{}, nil), ) }, @@ -468,6 +489,7 @@ func TestClient_LastUpdatedAt(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().Service(mockCluster, mockService).Return(&ecs.Service{ Deployments: []*awsecs.Deployment{ { @@ -545,6 +567,7 @@ func TestClient_ForceUpdateService(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().UpdateService(mockCluster, mockService, gomock.Any()).Return(errors.New("some error")), ) }, @@ -557,6 +580,7 @@ func TestClient_ForceUpdateService(t *testing.T) { Return([]*resourcegroups.Resource{ {ARN: mockSvcARN}, }, nil), + m.ecsClient.EXPECT().ActiveServices(mockSvcARN).Return([]string{mockSvcARN}, nil), m.ecsClient.EXPECT().UpdateService(mockCluster, mockService, gomock.Any()).Return(nil), ) }, @@ -1206,9 +1230,11 @@ func TestServiceDescriber_TaskDefinition(t *testing.T) { func Test_NetworkConfiguration(t *testing.T) { const ( - testApp = "phonetool" - testSvc = "svc" - testEnv = "test" + testApp = "phonetool" + testSvc = "svc" + testEnv = "test" + mockSvcArn = "arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB" + mockClusterArn = "arn:aws:ecs:us-west-2:1234567890:cluster/my-project-test-Cluster-9F7Y0RLP60R7" ) getRgInput := map[string]string{ deploy.AppTagKey: testApp, @@ -1256,17 +1282,18 @@ func Test_NetworkConfiguration(t *testing.T) { gomock.InOrder( m.resourceGetter.EXPECT().GetResourcesByTags(clusterResourceType, getRgInput). Return([]*resourcegroups.Resource{ - {ARN: "cluster-1"}, + {ARN: mockClusterArn}, }, nil), - m.ecsClient.EXPECT().ActiveClusters("cluster-1").Return([]string{"cluster-1"}, nil), + m.ecsClient.EXPECT().ActiveClusters(mockClusterArn).Return([]string{mockClusterArn}, nil), m.resourceGetter.EXPECT().GetResourcesByTags(serviceResourceType, map[string]string{ deploy.AppTagKey: testApp, deploy.EnvTagKey: testEnv, deploy.ServiceTagKey: testSvc, }).Return([]*resourcegroups.Resource{ - {ARN: "arn:aws:ecs:us-west-2:1234567890:service/my-project-test-Cluster-9F7Y0RLP60R7/my-project-test-myService-JSOH5GYBFAIB"}, + {ARN: mockSvcArn}, }, nil), - m.ecsClient.EXPECT().NetworkConfiguration("cluster-1", "my-project-test-myService-JSOH5GYBFAIB").Return(&ecs.NetworkConfiguration{ + m.ecsClient.EXPECT().ActiveServices(mockSvcArn).Return([]string{mockSvcArn}, nil), + m.ecsClient.EXPECT().NetworkConfiguration(mockClusterArn, "my-project-test-myService-JSOH5GYBFAIB").Return(&ecs.NetworkConfiguration{ AssignPublicIp: "1.2.3.4", SecurityGroups: []string{"sg-1", "sg-2"}, Subnets: []string{"sn-1", "sn-2"}, diff --git a/internal/pkg/ecs/mocks/mock_ecs.go b/internal/pkg/ecs/mocks/mock_ecs.go index a5b8f0df55f..f5c0e3c500a 100644 --- a/internal/pkg/ecs/mocks/mock_ecs.go +++ b/internal/pkg/ecs/mocks/mock_ecs.go @@ -92,6 +92,25 @@ func (mr *MockecsClientMockRecorder) ActiveClusters(arns ...interface{}) *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ActiveClusters", reflect.TypeOf((*MockecsClient)(nil).ActiveClusters), arns...) } +// ActiveServices mocks base method. +func (m *MockecsClient) ActiveServices(serviceARNs ...string) ([]string, error) { + m.ctrl.T.Helper() + varargs := []interface{}{} + for _, a := range serviceARNs { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "ActiveServices", varargs...) + ret0, _ := ret[0].([]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ActiveServices indicates an expected call of ActiveServices. +func (mr *MockecsClientMockRecorder) ActiveServices(serviceARNs ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ActiveServices", reflect.TypeOf((*MockecsClient)(nil).ActiveServices), serviceARNs...) +} + // DefaultCluster mocks base method. func (m *MockecsClient) DefaultCluster() (string, error) { m.ctrl.T.Helper() diff --git a/internal/pkg/term/progress/ecs.go b/internal/pkg/term/progress/ecs.go index 065a7760128..be2c9447c6f 100644 --- a/internal/pkg/term/progress/ecs.go +++ b/internal/pkg/term/progress/ecs.go @@ -6,11 +6,12 @@ package progress import ( "bytes" "fmt" - "github.com/aws/copilot-cli/internal/pkg/aws/cloudwatch" "io" "strconv" "sync" + "github.com/aws/copilot-cli/internal/pkg/aws/cloudwatch" + "github.com/aws/copilot-cli/internal/pkg/aws/ecs" "github.com/aws/copilot-cli/internal/pkg/stream" "github.com/aws/copilot-cli/internal/pkg/term/color" @@ -192,9 +193,7 @@ func reverseStrings(arr []string) []string { // parseServiceARN returns the cluster name and service name from a service ARN. func parseServiceARN(arn string) (cluster, service string) { - parsed := ecs.ServiceArn(arn) + parsed, _ := ecs.ParseServiceArn(arn) // Errors can't happen on valid ARNs. - cluster, _ = parsed.ClusterName() - service, _ = parsed.ServiceName() - return cluster, service + return parsed.ClusterName(), parsed.ServiceName() }