fix: variable namespaces for networkpolicies#3342
Conversation
|
Welcome to the Kubeflow Manifests Repository Thanks for opening your first PR. Your contribution means a lot to the Kubeflow community. Before making more PRs: Community Resources:
Thanks again for helping to improve Kubeflow. |
ec1e9f0 to
6c7b0c9
Compare
|
Thank you for the PR. Please check out all the comments such as #3319 (comment) "everything outside of the kubeflow namespaces so cert-manager, knative-serving etc. We should directly move in this PR to the respective folders/overlays in /common" lets aim for a long-term solution that is better than just creating empty namespaces. I will also do a dummy istio change to trigger more tests. For example we can rename common/cert-manager/kubeflow-issuer/base to common/cert-manager/overlay/kubeflow and add the cert-manager networkpolicy in that folder |
a820ebf to
e60201f
Compare
@juliusvonkohout updated the PR to address the feedback
|
juliusvonkohout
left a comment
There was a problem hiding this comment.
Thank you, I added comments.
|
/ok-to-test |
@juliusvonkohout I have addressed the feedback , renamed all network policy files as requested and updated their references throughout the codebase. looking at your suggestion #3342 additionally i refactored the namespace creation into a proper Kustomize base (common/namespaces) to provide a declarative, long-term solution instead of relying on the imperative bash loop, ensuring all target namespaces exist before policies are applied. |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
2db6d1a to
b7a6a0c
Compare
@juliusvonkohout done |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
@juliusvonkohout Added |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
|
@juliusvonkohout moved the remaining networkpolicies in |
|
Thank you, that looks quite good already. Please address the last comments and then maybe a final review will do it. |
Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
@juliusvonkohout addressed all the comments |
Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
|
Thank you |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout 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 |
|
If you are interested in a follow up PR: https://github.com/kubeflow/manifests/blob/master/common/cert-manager/README.md update logic should be moved to /scripts as for all other components. and you can also check which scripts in /scripts have to be run since there are newer upstream releases and raise corresponding PRs. |
* fix: variable namespaces for networkpolicies Remove namespace override in kustomization.yaml to allow NetworkPolicies to use their self-defined namespaces. Also update multi_tenancy_install.sh to create required namespaces before applying network policies, ensuring the installation succeeds. Supersedes kubeflow#3319 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * fix: restore netpol namespace & refactor overlays Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * fix: update multi-tenancy script to include new overlays Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * fix Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * refactor Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * fix Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * format yaml disable modification of spec.selector Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * refactor: move networkpolicy files to canonical paths Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * refactor: wire subfolder kustomizations and remove dead overlays Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * test: align cert-manager install and trivy scan paths Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * Update tests/cert_manager_install.sh Co-authored-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> Signed-off-by: hippie-danish <133037056+danish9039@users.noreply.github.com> * Update tests/trivy_scan.py Co-authored-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> Signed-off-by: hippie-danish <133037056+danish9039@users.noreply.github.com> * Update cert-manager installation script to use base Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * test: fix multitenancy wait and lint Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * Update tests/cert_manager_install.sh Co-authored-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> Signed-off-by: hippie-danish <133037056+danish9039@users.noreply.github.com> * Update tests/trainer_install.sh Co-authored-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> Signed-off-by: hippie-danish <133037056+danish9039@users.noreply.github.com> * Apply suggestion from @juliusvonkohout Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * Apply suggestion from @juliusvonkohout Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * Apply suggestion from @juliusvonkohout Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * Update multi_tenancy_install.sh Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * Apply suggestion from @juliusvonkohout Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * Comment out default-allow-same-namespace.yaml Comment out default network policy and note future changes. Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * Apply suggestions from code review Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> * test: enable istio-system default-allow-same-namespace only Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * test(netpol): add istiod control-plane allow policy only Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * test(netpol): add istiod webhook apiserver policy only Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * lint Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * test(netpol): add oauth2-proxy ingressgateway allow policy only Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: allow oauth2-proxy and istio to reach dex Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: allow apiserver to reach knative webhook Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: allow apiserver to reach net-istio webhook Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: re-enable same-namespace allow for cert-manager Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: re-enable same-namespace allow for knative-serving Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: allow knative-serving to reach istio gateways Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * netpol: allow istio gateways to reach knative activator Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * move kubeflow network policy files into kubeflow namespace folder Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * rewire kubeflow namespace to apply moved network policies Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * addressed comments Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> * move readme and owners Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com> * cleanup Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com> --------- Signed-off-by: danish9039 <danishsiddiqui040@gmail.com> Signed-off-by: hippie-danish <133037056+danish9039@users.noreply.github.com> Signed-off-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com> Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com> Co-authored-by: Julius von Kohout <45896133+juliusvonkohout@users.noreply.github.com>
✏️ Summary of Changes
This PR fixes an issue #3319 where NetworkPolicies for various core components were incorrectly being applied to the
kubeflownamespace. This was caused by a hardcodednamespace: kubeflowin the base kustomization.yaml.Additionally, this PR updates the
tests/multi_tenancy_install.shscript to proactively create the required target namespaces before applying the policies, ensuring the installation succeeds even if components are installed in a pick-and-choose manner.The Problem
flowchart TB subgraph "Before Fix (Broken)" K[kustomization.yaml<br>namespace: kubeflow] --> |Forces ALL resources| KF[kubeflow namespace] P1[cert-manager-webhook.yaml<br>namespace: cert-manager] --> K P2[default-allow-auth.yaml<br>namespace: auth] --> K P3[istio-policy.yaml<br>namespace: istio-system] --> K K --> |All end up in| KF KF --> |Contains policies for| WRONG[❌ Wrong namespaces!] endThe Solution
flowchart TB subgraph "After Fix (Correct)" K2[kustomization.yaml<br>NO namespace override] --> |Respects each file|SPLIT SPLIT{Each policy goes<br>to its own namespace} SPLIT --> CM[cert-manager namespace] SPLIT --> AU[auth namespace] SPLIT --> IS[istio-system namespace] SPLIT --> KS[knative-serving namespace] SPLIT --> KF2[kubeflow namespace] SPLIT --> KFS[kubeflow-system namespace] CM --> |✅ Protects| CMP[cert-manager pods] AU --> |✅ Protects| AUP[auth pods] IS --> |✅ Protects| ISP[istio pods] endVerification
kustomize build common/networkpolicies/basegenerates policies with correct namespaces.shellcheck tests/multi_tenancy_install.shon the modified script to ensure robustness.Continuing work by @juhyeon-cha with addition of namespace creation in test scripts.
📦 Dependencies
🐛 Related Issues
✅ Contributor Checklist