search

inlets / inletsctl

Create inlets servers on the top cloud platforms
https://docs.inlets.dev/
MIT License
463 stars 61 forks source link

Proposed fixes for ec2 provider create with non-default VPC. Closes #… #75

Closed thesurlydev closed 4 years ago

thesurlydev commented 4 years ago

…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:

 ./inletsctl create -f ~/Downloads/access-key --secret-key-file ~/Downloads/secret-access-key -r us-west-2 -p ec2 --vpc-id vpc-f12e6b88 --subnet-id subnet-89ef61d3

By providing the new args vpc-id and subnet-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 ec2 provider will now have the option to include the new vpc-id and subnet-id args. 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:

derek[bot] commented 4 years ago

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. Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

derek[bot] commented 4 years ago

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. Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

thesurlydev commented 4 years ago

@Waterdrips ready for review of feedback items.

I had a question regarding passing parameters that may have nil values now that the new vpc-id and subnet-id parameters are optional. Is it okay to pass those to the SDK requests as-is or should they be excluded if the values are nil?

Waterdrips commented 4 years ago

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?

thesurlydev commented 4 years ago

@Waterdrips For now, I'll just exclude if they're nil.

thesurlydev commented 4 years ago

@Waterdrips Let me know if there's something more to be addressed. Thanks.

Waterdrips commented 4 years ago

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

thesurlydev commented 4 years ago

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

Should be good now. Thanks

alexellis commented 4 years ago

Why?

with changes as-is, users that target the ec2 provider will be required to include the new vpc-id and subnet-id args.

Waterdrips commented 4 years ago

Why?

with changes as-is, users that target the ec2 provider will be required to include the new vpc-id and subnet-id args.

that text is no longer correct @digitalsanctum might be worth re-wording the pr body to match the changes now.

thesurlydev commented 4 years ago

Why?

with changes as-is, users that target the ec2 provider will be required to include the new vpc-id and subnet-id args.

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.

thesurlydev commented 4 years ago

@Waterdrips Is this good to merge now?

Waterdrips commented 4 years ago

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