Add support for several ECS backends #1913
Conversation
ldez
added
kind/enhancement
provider/ecs/cluster.go
Outdated
|
|
||
| //Set adds strings elem into the the parser | ||
| //it splits str on , and ; | ||
| func (ns *Clusters) Set(str string) error { |
provider/ecs/cluster.go
Outdated
| // Clusters holds ecs clusters name | ||
| type Clusters []string | ||
|
|
||
| //Set adds strings elem into the the parser |
There was a problem hiding this comment.
could you add space between // and the comment. Same for others.
ldez
changed the title
|
@ldez ECS provider use AWS API. Do you want that I mock AWS API to be able to do some integration tests ? |
|
Maybe you can use https://github.com/localstack/localstack? |
|
I have already watch localstack. but currently they not provide ecs and ec2 endpoints. I will try to work on this new feature on localstack when i will have time |
provider/ecs/ecs.go
Outdated
| input.NextToken = result.NextToken | ||
| result, _ = client.ecs.ListClusters(input) | ||
| } | ||
| clustersArn = append(clustersArn, result.ClusterArns...) |
There was a problem hiding this comment.
In the way to avoid repetition and possible problem if result == nil, what is your mind about the suggestion described below?
for {
if result, _ := client.ecs.ListClusters(input); result != nil {
clustersArn = append(clustersArn, result.ClusterArns...)
input.NextToken = result.NextToken
if result.NextToken == nil {
break
}
} else {
break
}
}There was a problem hiding this comment.
I think that result can never be nil. But I like the suggestion, this is simplifying the code.
I will do the change
provider/ecs/ecs.go
Outdated
| clusters = append(clusters, *carns) | ||
| } | ||
| } else if p.Cluster != "" { | ||
| clusters = Clusters{p.Cluster} |
There was a problem hiding this comment.
Could you add a log.warn to inform the user that he uses a deprecated option? And also add a // TODO to remind us to remove this in the future?
provider/ecs/ecs.go
Outdated
| if p.AutoDiscoverClusters { | ||
| input := &ecs.ListClustersInput{} | ||
| for { | ||
| if result, _ := client.ecs.ListClusters(input); result != nil { |
There was a problem hiding this comment.
You should not discard the returned error here
traefiker
force-pushed
the
ecs-multiple-backends
branch
from
0f6fd23
to
c8e0cf2
Compare
Description
The goal of this PR is to add support for several ECS backends.
Two new fields has been added for ECS provider:
Clusters: used to define explicit list of clusters
AutoDiscoverClusters: used to enable auto discover clusters functionality. If this property is set to true, Clusters property will be ignored. Træfik ECS policy need to be updated with "ecs:ListClusters", "ecs:DescribeClusters"
Fixes #1570
To avoid breaking change ECS provider accept old field Cluster. If AutoDiscoverClusters is set Cluster field will be ignored.