Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: feat(ec2): add instance resource #3697
WIP: feat(ec2): add instance resource #3697
Changes from 12 commits
ca8e12f
ffebb9b
b1d84f1
04ef866
d0c58f1
9caa820
1ae11d1
18c33c9
4e60ca7
d5a0fba
37bd20e
aa4d6d6
7b07fa1
3ede39b
15c51ec
5f634bd
56e0c30
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shouldn't this be a
subnetSelection
?And shouldn't public subnets be the default? What use is a Bastion host in a private subnet?
EDIT: Oh I see, it probably has to do with whether you intend to access the bastion directly using SSH or using Session Manager. Could you at least make that clear in the documentation text? Or better yet, maybe it should be a property to select between the two?
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.
I think there are three scenarios:
So public subnets should not be used imho...
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.
Re use cases, we use all of these and also have use case where we have had to have a baston host, (jump box) in all 3 zones.
When conducting penetration tests we will place a (bastion) host in each zone (public, private, ..), sometime the only way to test a zone get to jump through bastions in each zone using proxy chains.
So in my imho this construct should be able to live in any zone, just default to public.
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.
Also you should be ideally able to place this in an explicit subnet.
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.
Yes, so I still feel we should expose the
subnetSelection
(and have it default to something sane, and I thinkPRIVATE
is the best sane default we can have). It would immediately benefit from future extensions we would add to thesubnetSelection
mechanism (for example, selecting individual subnets such as @slipdexic requested). And it should have explicit documentation on there saying you need to select public subnets if you want to access it from the internet.Would it make sense to have a top-level helper method for a common use case,
bastion.allowSshAccessFrom(peer, peer, ...)
or something? The only thing it would do is encapsulate port 22, but it might be just a little nicer.Also, @hoegertn, could you add a section to the README about the 2 Bastion Host use cases (SSM access and SSH access)?
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.
Sounds good. I will change it accordingly.
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.
Sorry for not seeing this before: I also agree with Elad that this host should probably encapsulate rather than extend
Instance
.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.
Can you elaborate on why? I still think that a bastion host is a special instance and should derive from it, so users have all the options they have with an instance but with some defaults regarding the use case. I thought OOP is one of the benefits of CDK.
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.
We achieve OOP/polymorphism by implementing interfaces. Interfaces declare "a (this) can do the same thing as a (that)", regardless of the implementation. So if you want a Bastion Host to be-an Instance, then declare it like so:
Inheriting from a class brings both the "(this) can do the same as (that)" relationship, but it also couples one implementation to the other, and can have unintended implications for the future extensibility of the classes involved (either a change in the implementation of
Instance
has an unintended side effect in the implementation ofBastionHost
, or we now face the limitation thatBastionHost
can only extend one class).In some situations implementation sharing is a perfectly valid choice (which would usually be made to make it quicker to implement a certain class since you don't have to type so much), but polymorphism through interface implementation is generally a safer choice in the long term, when it's hard to predict future changes.
In this case, I'd rather be safe than sorry later.
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.
Ok, I will change it and see what methods and fields I want to expose from the instance class.