Proposed fixes for ec2 provider create with non-default VPC. Closes #… #75
Conversation
|
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide. |
|
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide. |
|
@Waterdrips ready for review of feedback items. I had a question regarding passing parameters that may have |
|
Id think that excluding them if they are nil (actually only setting if not nil), which matches the current version might be best. You could even do an explicit check for the error code you reported initially and prompt a user to use the flags if the same error is returned? |
|
@Waterdrips For now, I'll just exclude if they're nil. |
|
@Waterdrips Let me know if there's something more to be addressed. Thanks. |
|
So iv tested both default vpc (no options) and custom VPC (flags) - Both worked like a charm. If we can get the couple of bits tidyd up and the 4 or so commits squashed into 1 commit with a good commit message (using something like this as a guide: https://chris.beams.io/posts/git-commit/) we should be good to go :) Thanks for your work on this @digitalsanctum |
The create command failed when a default VPC was not present for the ec2 provider. When the failure occurred there was also a side effect of dangling security groups that got created but not cleaned up in the event of failure. Two additional arguments were added, vpc-id and subnet-id, to mitigate the failure while also giving users the option to choose a specific VPC and subnet if more than one VPC exists. New validation checks that if one of these new arguments are specified then the other must be as well. Additional logic was added so that in the event of ec2 instance creation failure any security groups created will get deleted. Signed-off-by: Shane Witbeck <[email protected]>
Should be good now. Thanks |
|
Why?
|
that text is no longer correct @digitalsanctum might be worth re-wording the pr body to match the changes now. |
Done. Thanks for catching that. |
|
@Waterdrips Is this good to merge now? |
|
Alex needs to sign off and merge - its on the list (the openfaas 2020 roadmap) here: https://trello.com/c/GGzIvan7/142-inletsctl-inlets-operator-with-ec2-and-non-default-vpc |
…73 and #74.
Description
These changes are proposed fixes for issues #73 and #74. If maintainers are happy with proposed changes I'll commit to updating documentation or whatever other changes are prudent. I should note that I'm a golang novice so feedback on the provide code changes is welcome!
How Has This Been Tested?
Tested with this PR's code changes via:
By providing the new args
vpc-idandsubnet-id, I was able to successfully create the exit node and accompanying security group.How are existing users impacted? What migration steps/scripts do we need?
With these changes users that target the
ec2provider will now have the option to include the newvpc-idandsubnet-idargs. By providing these args users with no default VPC will no longer face an error during exit node creation. They also allow users to have more control over the placement of the exit node.Checklist:
I have:
git commit -s