Add basic network policy for Ingress to Workers - #488
Conversation
0af3ac5 to
5eba34e
Compare
| os.Exit(1) | ||
| } | ||
|
|
||
| if err = (&controllers.NetworkPolicyReconciler{ |
There was a problem hiding this comment.
Should this be optional? seems like a net positive, but not sure if specific networking needs might want this togglable
I asked Bowei Du (@bowei) to close #340 |
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
LGTM -- apart from busybox comment
| Containers: []corev1.Container{ | ||
| { | ||
| Name: "probe", | ||
| Image: "busybox@sha256:1487d0af5f52b4ba31c7e465126ee2123fe3f2305d638e7827681e7cf6c83d5e", |
There was a problem hiding this comment.
I remember there are some issues with busybox license. Can we use a different image for this?
There was a problem hiding this comment.
Which license issue are you referring to?
https://github.com/search?q=repo%3Aagent-substrate%2Fsubstrate%20busybox&type=code: there are several places in the codebase using busybox.
There was a problem hiding this comment.
You can find this in a web search: "Busybox license issues". You can probably replace with "toybox" -- I just remember in some projects, we ended up replacing all instances of Busybox
There was a problem hiding this comment.
You need to rebase
There was a problem hiding this comment.
Rebased the PR.
There was a problem hiding this comment.
I sent out #579 to gate copyleft base images in Dockerfiles.
Referencing busybox docker images is okay, and should not have license issues.
| WithMatchLabels(map[string]string{"kubernetes.io/metadata.name": ateSystemNamespace})). | ||
| WithPodSelector(metav1ac.LabelSelector(). | ||
| WithMatchLabels(map[string]string{"app": atenetRouterAppName})), |
There was a problem hiding this comment.
Bowei Du (@bowei), will this be the official documented way to ensure that an ingress which is used besides the OOTB will pass NetworkPolicy?
Restrict Ingress to Workers and hence Actors to be only from the Router component. This makes the atecontroller responsible for K8s level network policy Note: this is a belts-and-suspenders model as this applies on the Worker level. This will be refined further with Actor-specific policies.
- Add ate.dev/worker-pool label to NetworkPolicy metadata and remove Egress management to allow default egress behavior - Define ateSystemNamespace and atenetRouterAppName constants for Ingress peer selectors - Centralize deterministic NetworkPolicy naming logic (resources.NetworkPolicyName) and prevent trailing hyphens - Add comprehensive E2E test suite for networkpolicy covering control plane reconciliation, positive data plane verification (authorized ingress via atenet-router), and negative data plane verification (unauthorized ingress blocking)
|
/rerun |
|
Looks like the request parking e2e test is failing. |
|
Can we check that it's not related to this change. |
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
code lgtm ; tests are not passing (yet)
|
Bowei Du (@bowei) , a rerun of the presubmit succeeded. I also ran the parking e2e test locally 10+ times, they all succeeded. I think we can treat it a rare flakiess for now. I will investigate it more if this comes back. |
53aac5b
into
agent-substrate:main
This PR includes #340 and several tiny enhancements:
Here is the e2e test result: