-
Notifications
You must be signed in to change notification settings - Fork 41
feat: add NetworkPolicy for the dashboard #159
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 NetworkPolicy for the dashboard #159
Conversation
333a6d3 to
7c91ac1
Compare
| - service-account.yaml | ||
| - service.yaml | ||
| - configmap.yaml | ||
| - network-policy.yaml |
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.
If we are starting to move network policies to within components (from kubeflow/manifests where these definitions reside today) - what should we do about the https://github.com/kubeflow/manifests/blob/master/common/networkpolicies/base/default-allow-same-namespace.yaml file?
Seems like we should a networkpolicy to allow traffic within the kubeflow namespace also defined somewhere in this repo (?)
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.
This is a very good question!
My proposal is that every repo should own the NetworkPolicies of its components, and the kubeflow/manifests should own resources that should live in the kubeflow namespace, and don't target/configure a workload that is owned by another repo.
But this should have a dedicated issue (I'm trying to create one, but GH doesn't allow me to create it!)

So for this PR, I'd suggest we only copy the ones that are specific to components of this repo. And continue the discussion about "generic" resources in kubeflow to kubeflow/manifests, as this can be generalised to other resources.
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.
Slightly orthogonal, we could also create a NetworkPolicy that allows only the dashboard to talk to access-management, so that we don't rely at all in the NetworkPolicy in the kubeflow namespace.
But was thinking of not introducing a new functionality outside of the overall one yet, as I would treat it as a dedicated feature. But if you feel strongly otherwise let me know @andyatmiami @juliusvonkohout
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 am comfortable with that response - appreciate the follow up!
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.
yeah, for now focusing on "feature parity" I think is good ... so we can deal with adding new functionality once the general release process has been vetted (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.
Created also kubeflow/manifests#3261 to track the discussion for the policies
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.
"Slightly orthogonal, we could also create a NetworkPolicy that allows only the dashboard to talk to access-management, so that we don't rely at all in the NetworkPolicy in the kubeflow namespace." Yes that should be done and you also need to add tests to verify that the networkpolicy blocks it.
Signed-off-by: Kimonas Sotirchos <[email protected]>
7c91ac1 to
904eff7
Compare
|
/ok-to-test |
|
|
||
| # test the NetworkPolicy, by ensuring other Pods timeout talking to the dashboard | ||
| OUTPUT=$(kubectl run \ | ||
| netshoot-test --rm -i \ | ||
| --restart=Never \ | ||
| --image nicolaka/netshoot \ | ||
| -- curl -s dashboard.kubeflow.svc --connect-timeout 5 \ | ||
| 2>&1) | ||
| echo $OUTPUT | grep "Connection timed out after" |
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.
@andyatmiami I've also added a small test to ensure other Pods can't reach Access Management.
One realisation I'm having is that this test_service.sh file seems to keep aggregating tests for each component. I would be more in favour of each repo to own its testing code, and potentially split the test code to scripts in the component folders. We don't have to do this now, but mentioned this to get some feedback
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.
yeah, i think that would be a great idea..
test_service.sh is (imho) more impediment than helper...
i'd be in favor of:
- breaking it up into respective components
- further breaking it up to not have N different pieces of functionality in one script (i.e. a "command" in
test-servicemeans we have N "scripts" in one uber script - and i'm not sure that is really helpful
if there is truly generic and/or cross-cutting functionality - that can be its own script to run standalone or source into a give component script - but i think it would make more sense to split the test code for sure - and myself would be cool doing it aggressively 😈
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.
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.
(Fixed the commit of integration tests, should be ready for a pass)
Signed-off-by: Kimonas Sotirchos <[email protected]>
0e33fb3 to
90afa57
Compare
andyatmiami
left a 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.
/lgtm
Appreciate this contribution to getting us closer to a release @kimwnasptd !
Verified the checks running on the PR are now sufficient in validating the changes
Also manually verified this additional check invocation demonstrates the necessary behavior of the NetworkPolicy
Original behavior:
$ kubectl run netshoot-test --rm -i --restart=Never --image nicolaka/netshoot -- curl dashboard.kubeflow.svc --connect-timeout 5
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 19 100 19 0 0 2079 0 --:--:-- --:--:-- --:--:-- 2375
RBAC: access deniedpod "netshoot-test" deleted
With fix:
$ kubectl run \
netshoot-test --rm -i \
--restart=Never \
--image nicolaka/netshoot \
-- curl dashboard.kubeflow.svc --connect-timeout 5
If you don't see a command prompt, try pressing enter.
0 0 0 0 0 0 0 0 --:--:-- 0:00:05 --:--:-- 0
curl: (28) Connection timed out after 5003 milliseconds
pod "netshoot-test" deleted
pod default/netshoot-test terminated (Error)
ℹ️ I also confirmed similar verification of the dashboard-angular NetworkPolicy but excluding output for sake of brevity.
|
Thanks for the thorough review yet again @andyatmiami! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kimwnasptd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes #155