-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Add AssociatePublicIpAddress to EC2NodeClass #5437
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
55d57b2
to
8f79f9d
Compare
Pull Request Test Coverage Report for Build 7820524726
💛 - Coveralls |
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.
/karpenter snapshot
Snapshot successfully published to
|
60c994f
to
e20cb23
Compare
5e380ae
to
a7ab06e
Compare
Signed-off-by: Mahmoud Gaballah <[email protected]>
Signed-off-by: Mahmoud Gaballah <[email protected]>
Signed-off-by: Mahmoud Gaballah <[email protected]>
aefdf44
to
7d63122
Compare
Signed-off-by: Mahmoud Gaballah <[email protected]>
7d63122
to
cb7a7dd
Compare
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.
/karpenter snapshot
Snapshot successfully published to
|
Signed-off-by: Mahmoud Gaballah <[email protected]>
Other than this piece of feedback this looks good to me! I'm definitely open to discussion on that point but I think it's reasonable to enable users to still disable public IPs in public subnets for EFA instances if they want. This is relevant for instance types with a single network card and thus a single EFA. |
thanks, would it be possible to add this piece of requirements iteratively? I do not think we are compromising the UX of developers with this PR, so we can consider this as the next step. I am just afraid this is taking more and more time and we are in need of this feature, so incremental improvements would help here |
Sure, we can include this in a follow-up PR. I do think it's important but it is a gap in the original EFA implementation. I'll take one more review pass since it's been a couple weeks since I've last looked and then this should be good to go. |
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.
/karpenter snapshot
Snapshot successfully published to
|
Drops EFA tests from E2E suite, simplifies since we're only testing private subnets, and drops EIP association checks (not assigning EIPs)
ca4199a
to
d7267aa
Compare
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.
/karpenter snapshot
Snapshot successfully published to
|
@jmdeal |
Since I made a couple of small updates I can't (or at least shouldn't) approve, I've reached out to other members of the Karpenter team to give it a quick review. It looks good to me, this should be able to go in before the next release. |
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.
LGTM 🚀
Fixes #2026
Description
Add an AssignPublicIpAddress field to the
nodeclass
How was this change tested?
unit and integration tests were created;
Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.