-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/aws ecs support #197
Conversation
Hey @fdr2 Thanks for taking this up! We'll get this reviewed and merged in soon. Afterwards we'll have to update go-discover in the consul repo, so this won't be reflected in the main branch until the next patch release. The team did want to caution though that we haven't evaluated the effects of running consul servers inside of ECS ( aside from a single node cluster for dev/testing ) . So for the moment we only officially support using servers outside of ECS. There was a couple of factors that went into that decision -- one of them being some performance concerns around the EFS that ECS uses under the hood. Regardless, we're still cool with this as an experimental feature so please report back on the main consul repo sometime & let us know how this is working for you 👍 😄 |
Thanks @Amier3 I didn't mean to go against the grain 😅 It seemed like a common topic and I really enjoyed digging through go-discover to implement it. If you happen to know any way I can benchmark consul or what the particular performance concerns may be on ecs-efs I would be interesting in digging in deeper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
I made several comments, so let me know if you have any questions. Overall, it looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few minor things I noticed. Thanks for addressing all my previous comments, though! Great job on this!
Thanks @fdr2! To enable use of this in Consul, the next step is to open a PR to bump the library version in the consul repo. |
* Add service argument to aws provider, where service can be either ec2 or ecs (defaulting to ec2) * Add optional ecs_cluster and ecs_family arguments
* Add service argument to aws provider, where service can be either ec2 or ecs (defaulting to ec2) * Add optional ecs_cluster and ecs_family arguments
* Add service argument to aws provider, where service can be either ec2 or ecs (defaulting to ec2) * Add optional ecs_cluster and ecs_family arguments
author Jeff Widman <jeff@jeffwidman.com> 1621366525 -0700 committer Tiago Peczenyj <tpeczenyj@weborama.com> 1703171978 +0100 Update tencent version Tencent pulled the previous tag of their API and switched to `v1` starting here: https://github.com/TencentCloud/tencentcloud-sdk-go#%E9%80%9A%E8%BF%87go-get%E5%AE%89%E8%A3%85%E6%8E%A8%E8%8D%90 As a result, currently `go get -d` is complaining in downstream repos, breaking Dependabot (among other things). So this fixes that. Fix hashicorp#172. Update CI badge to CircleCI The old badge was travis, and we have not used travis in some time. update dependency fix incompatibity issue hashicorp#183 Revert "fix incompatibity issue hashicorp#183" This reverts commit 986207c. Revert "update dependency" This reverts commit bc42ce5. update dependency github.com/denverdino/aliyungo fix incompatibity issue hashicorp#183 go mod tidy Add AWS SessionToken Support (hashicorp#189) * add support for SessionToken in AWS credential resolver * add AWS AssumeRole configuration to AWS tests Fix "Config options" links Apply suggestions from code review Co-authored-by: Blake Covarrubias <blake.covarrubias@gmail.com> deps: bump Azure/go-autorest to remove transitive jwt-go dependency with known vulnerability (hashicorp#174) update dependency github.com/denverdino/aliyungo Add AWS SessionToken Support (hashicorp#189) * add support for SessionToken in AWS credential resolver * add AWS AssumeRole configuration to AWS tests Fix "Config options" links Apply suggestions from code review Co-authored-by: Blake Covarrubias <blake.covarrubias@gmail.com> Add AWS endpoint definition as an option (hashicorp#194) Signed-off-by: obeyler <beyler_olivier@yahoo.fr> Feature/aws ecs support (hashicorp#197) * Add service argument to aws provider, where service can be either ec2 or ecs (defaulting to ec2) * Add optional ecs_cluster and ecs_family arguments Update tencentcloud-sdk-go modules Dependencies are available as individual go modules now. [COMPLIANCE] Update MPL 2.0 LICENSE Adding workflow .github/actions/acctest/action.yml .github/actions/tf-install/action.yml .github/workflows/acceptance.yml SHA-pin all 3rd-party actions Restrict workflow permissions Add actionslint Add dependabot Add CODEOWNERS Remove CircleCI configuration Remove unused local actions Update migration enable scleway acceptance test add success job add success job fix copy pasta Result of tsccr-helper -pin-all-workflows . Fix CVEs Need at least go 1.16 for the golang.org/x/net fixes Update github action to use go 1.16 Some dependencies needed 1.17 Set miekg/dns to 1.1.50 Result of tsccr-helper -log-level=info -pin-all-workflows . Update README.md Update README.md Update README.md Update README.md update vendor github.com/denverdino/aliyungo
Description
Add ECS support to AWS Go-Discover plugin.
Fixes: 6802
Revives/Simplifies PR #54
PR Checklist