Skip to content

Commit

Permalink
Fix AWS ListDeployedDatabaseServices when there's no ECS Cluster (#50843
Browse files Browse the repository at this point in the history
)

Calling the AWS API `ecs:ListServices` with a non-existent ECS Cluster
name will return a 400 w/ ClusterNotFoundException.

The existing code was not handling that error and a raw error was
returned.

This PR changes the logic to ensure that case is handled and that the
ListDeployedDatabaseServices returns an empty list.

An alternative would be to call the ListClusters beforehand, but that
would increase the number of API calls we do to external services.
  • Loading branch information
marcoandredinis authored Jan 9, 2025
1 parent 3fc749a commit 4b2a0da
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 3 deletions.
9 changes: 9 additions & 0 deletions lib/cloud/aws/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"strings"

awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http"
ecstypes "github.com/aws/aws-sdk-go-v2/service/ecs/types"
iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/iam"
Expand Down Expand Up @@ -55,6 +56,10 @@ func ConvertRequestFailureErrorV2(err error) error {
return err
}

var (
ecsClusterNotFoundException *ecstypes.ClusterNotFoundException
)

func convertRequestFailureErrorFromStatusCode(statusCode int, requestErr error) error {
switch statusCode {
case http.StatusForbidden:
Expand All @@ -69,6 +74,10 @@ func convertRequestFailureErrorFromStatusCode(statusCode int, requestErr error)
if strings.Contains(requestErr.Error(), redshiftserverless.ErrCodeAccessDeniedException) {
return trace.AccessDenied(requestErr.Error())
}

if strings.Contains(requestErr.Error(), ecsClusterNotFoundException.ErrorCode()) {
return trace.NotFound(requestErr.Error())
}
}

return requestErr // Return unmodified.
Expand Down
12 changes: 12 additions & 0 deletions lib/cloud/aws/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,18 @@ func TestConvertRequestFailureError(t *testing.T) {
},
wantIsError: trace.IsNotFound,
},
{
name: "v2 sdk error for ecs ClusterNotFoundException",
inputError: &awshttp.ResponseError{
ResponseError: &smithyhttp.ResponseError{
Response: &smithyhttp.Response{Response: &http.Response{
StatusCode: http.StatusBadRequest,
}},
Err: trace.Errorf("ClusterNotFoundException"),
},
},
wantIsError: trace.IsNotFound,
},
}

for _, test := range tests {
Expand Down
6 changes: 6 additions & 0 deletions lib/integrations/awsoidc/listdeployeddatabaseservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
ecstypes "github.com/aws/aws-sdk-go-v2/service/ecs/types"
"github.com/gravitational/trace"

awslib "github.com/gravitational/teleport/lib/cloud/aws"
"github.com/gravitational/teleport/lib/integrations/awsoidc/tags"
)

Expand Down Expand Up @@ -139,6 +140,11 @@ func ListDeployedDatabaseServices(ctx context.Context, clt ListDeployedDatabaseS

listServicesOutput, err := clt.ListServices(ctx, listServicesInput)
if err != nil {
convertedError := awslib.ConvertRequestFailureErrorV2(err)
if trace.IsNotFound(convertedError) {
return &ListDeployedDatabaseServicesResponse{}, nil
}

return nil, trace.Wrap(err)
}

Expand Down
25 changes: 22 additions & 3 deletions lib/integrations/awsoidc/listdeployeddatabaseservice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,11 @@ type mockListECSClient struct {
}

func (m *mockListECSClient) ListServices(ctx context.Context, params *ecs.ListServicesInput, optFns ...func(*ecs.Options)) (*ecs.ListServicesOutput, error) {
ret := &ecs.ListServicesOutput{}
if aws.ToString(params.Cluster) != m.clusterName {
return ret, nil
if aws.ToString(params.Cluster) != m.clusterName || len(m.services) == 0 {
return nil, trace.NotFound("ECS Cluster not found")
}

ret := &ecs.ListServicesOutput{}
requestedPage := 1

totalEndpoints := len(m.services)
Expand Down Expand Up @@ -348,6 +348,25 @@ func TestListDeployedDatabaseServices(t *testing.T) {
},
errCheck: require.NoError,
},
{
name: "returns empty list when the ECS Cluster does not exist",
req: ListDeployedDatabaseServicesRequest{
Integration: "my-integration",
TeleportClusterName: "my-cluster",
Region: "us-east-1",
},
mockClient: func() *mockListECSClient {
ret := &mockListECSClient{
pageSize: 10,
}
return ret
},
respCheck: func(t *testing.T, resp *ListDeployedDatabaseServicesResponse) {
require.Empty(t, resp.DeployedDatabaseServices, "expected 0 services")
require.Empty(t, resp.NextToken, "expected an empty NextToken")
},
errCheck: require.NoError,
},
} {
t.Run(tt.name, func(t *testing.T) {
resp, err := ListDeployedDatabaseServices(ctx, tt.mockClient(), tt.req)
Expand Down

0 comments on commit 4b2a0da

Please sign in to comment.