-
Notifications
You must be signed in to change notification settings - Fork 2k
Export elastic ip for tunnel when present. #5820
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
Conversation
|
Whats the status on this? |
|
I run into problems with the tunnel every time I update the version of SST and it's very painful to resolve. I am really hoping this might fix it? 🙏 |
|
@jescalan Did this fix it for you? |
|
Can confirm this fixed the issue for me. @lputnam2000 @fwang |
|
Do we know when this will be merged? |
|
@fwang @lputnam2000 I'm a big fan of SST, by the way —it's made my life so much easier overall! will this PR resolve the issue? Every time I'm using SST with RDS, I need to revert to an old version to connect through the tunnel — it's cumbersome and not the best practice. do you have more info on this tunnel issue and when it might be fixed? |
|
@gkpln3 Small change needed to this PR before we can merge, the change you proposed breaks the tunnel when using managed nat. In this case, the IP has to be the bastion ec2 instance public ip address not the elastic ips - which are still created, as they are associated to the nat gateways. If you're able to make this change I will push to get this merged |
@jamesgibbons92 I've opened a new pull request #6273 with this change and you're suggestion, they work well, sooner this goes in the better end up manually modifying the file every time i need to do a db migration etc. Closed this now since the original author has made the change |
|
@jackwatters45 Done. |
|
Lets merge this? |
| ip: natInstances.apply((instances) => | ||
| instances.length ? elasticIps[0]?.publicIp : bastion.publicIp, | ||
| ), |
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.
- Should this be
self.instancesand included intheall()? I think the static get case is affected. - I do not think this handles the case where NAT Eip's are specified (https://sst.dev/docs/component/aws/vpc/#nat-ip)
- I suggest we add a comment to clarify the managed NAT case (Export elastic ip for tunnel when present. #5820 (comment)) - this is not clear from the code. Actually, more than that - why are the Elastic IPs created in the managed NAT case? And further, this will break the static get case.



Solves #5657