-
Notifications
You must be signed in to change notification settings - Fork 609
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
Network policy #9077
base: master
Are you sure you want to change the base?
Network policy #9077
Conversation
Only pod (not service) ports are applicable. Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
Based on the documentation in https://longhorn.io/docs/1.6.2/references/networking/ Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
The `examples/network-policy/` directory is outdated, the chart contains up-to-date network policies. Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
…c port Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
The removed Edit: done in longhorn/website#955 |
I have some questions about the network policies:
|
…c ports Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
…ge-manager according to the documentation The original policies mixed some ingress and egress rules and did not specify port numbers. Signed-off-by: Jakub Klinkovský <1289205+lahwaacz@users.noreply.github.com>
See longhorn/longhorn#9077 for context. Signed-off-by: davidko <dko@suse.com>
See longhorn/longhorn#9077 for context. Signed-off-by: davidko <dko@suse.com>
Assigned to @ChanYiLin to help with this review. cc @derekbit |
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.
These changes might be not sufficient especially when no ingress controller is used. See: #9067 (comment)
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.
You are right, this case is not addressed in this pull request. I was focusing on "simple" improvements that don't depend on any configuration, but the review is taking too long regardless... 😞
Which issue(s) this PR fixes:
Issue #9067
What this PR does / why we need it:
Current network policies are too open, they should be allow only traffic that is specifically needed for Longhorn functionality and deny the rest.
The policies should also be in line with the documentation: https://longhorn.io/docs/1.6.2/references/networking/
Future work
I will go offline in the following days and have a busy next week, but I will definitely revisit this later! Maintainers are invited to push to my branch or split the rest to a separate issue if doing it by parts is preferred.