-
Notifications
You must be signed in to change notification settings - Fork 25
ROX-29399: Directional external-IPs #2130
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
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #2130 +/- ##
==========================================
+ Coverage 28.52% 28.82% +0.30%
==========================================
Files 94 96 +2
Lines 5757 5797 +40
Branches 2547 2550 +3
==========================================
+ Hits 1642 1671 +29
- Misses 3393 3408 +15
+ Partials 722 718 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2d149bd
to
d4c351c
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.
Only a partial review, mostly refactorings that would be nice to see
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.
Finished going through the PR, changes look good apart from the pending comment in my previous review.
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.
Hey @ovalenti - I've reviewed your changes - here's some feedback:
- Refactor the large manual test loop in TestExternalIPsConfigChangeEnableEgress into a parameterized TEST_P for clarity and maintainability.
- Extract common logic from the ingress/egress branches in CloseConnectionsOnExternalIPsConfigChange to reduce duplication and simplify predicate construction.
- Simplify ExternalIPsConfig constructor by mapping the proto direction directly to the enum (e.g., with a small helper) to reduce the switch verbosity.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Overall looks good, just a couple more comments on readability, but we should be able to merge soon
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!
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.
Still LGTM, just a minor comment on ordering in an example.
Please explain what manual testing you did in the PR description. |
Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
Co-authored-by: JoukoVirtanen <[email protected]>
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!
Description
Add an attribute to the runtime-configuration to set in which direction the external-IPs are enabled.
The possible options for the new
direction
attribute areINGRESS
,EGRESS
andBOTH
.By default (when the attribute is missing) the behavior is to enable in both directions.
Example:
The related proto changes are in stackrox/stackrox#15373
Checklist
Automated testing
Testing
Preliminary tests in #2140 tend to show that the behavior is as expected.
I did some manual testing and could show that setting
direction: INGRESS
normalized the outgoing connections.