✨ add NetworkPolicy objects for catalogd and operator-controller #1942
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I did verify that the NetworkPolicy objects have an effect in
|
| - protocol: TCP | ||
| port: 9443 # webhook | ||
| egress: | ||
| - {} # Allows all egress traffic (needed to pull catalog images from arbitrary image registries) |
There was a problem hiding this comment.
Nit: Add space at the end of the file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1942 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 75 75
Lines 6326 6326
=======================================
Hits 4218 4218
Misses 1843 1843
Partials 265 265
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
@joelanford Just a quick heads-up — the network policies won’t be tested at all with the default Kind configuration.
To ensure we properly test the NetworkPolicy resources scaffolded by us, we’ll need to follow the same or a similar approach used in Kubebuilder’s own e2e setup. That means disabling the default CNI and installing Calico manually.
You can see how they handle this here:
-
Kind config (disabling default CNI):
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/kind-config.yaml#L15-L20 -
Calico setup:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/setup.sh#L42-L43
Before moving forward, we should make sure the changes are tested end-to-end. I’d suggest we replicate the setup done there to ensure coverage.
Let me know if you’d like help setting it up!
| control-plane: catalogd-controller-manager | ||
| policyTypes: | ||
| - Ingress | ||
| - Egress |
There was a problem hiding this comment.
I think we just need Ingress, right?
There was a problem hiding this comment.
First I did like you there, and I received this feedback from community: kubernetes-sigs/kubebuilder#3853 (comment)
So, we might need to reconsider.
There was a problem hiding this comment.
We explicitly want to allow all egress traffic both catalogd and operator-controller because they need to be able to communicate with unknown-in-advance image registries. If we are not explicitly granting that access, then another NetworkPolicy that matches our pod that is more restrictive could break us.
There was a problem hiding this comment.
I think we'll need to be more restrictive to make sense.
But I get your point.
We can also enhance it on follow ups. So, 👍
| - protocol: TCP | ||
| port: 8443 # metrics | ||
| egress: | ||
| - {} # Allows all egress traffic (needed to pull bundle images from arbitrary image registries) |
There was a problem hiding this comment.
Just as a bit of context — back when we added this to Kubebuilder, there was a request from the community for us to use labels.
The idea was to allow scenarios where, for example, a Pod wants to scrape metrics. In those cases, having specific labels becomes necessary to permit that traffic through the NetworkPolicy.
You might want to take a look at this example for reference:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/config/network-policy/allow-metrics-traffic.yaml#L19-L24
So, we might want to consider the same approach. Sharing to allow you to think about the pros and cons.
There was a problem hiding this comment.
Given that we are fully open now, I think keeping it simple and slightly more open is better for this first pass. If we find later that we can ratchet down access to metrics and the catalogd webhook port in a general way, we can do that later.
There was a problem hiding this comment.
OK, we can do a follow-up after checking that this config works on the downstream as well.
It is fine.
|
Installing a separate CNI is no longer necessary. See: |
Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
network-policy
branch
from
f75dd52 to
7831977
Compare
|
Should this have a new e2e specific to validating the reduced network access are working as intended? |
This change proves that the NetworkPolicy for catalogd and operator-controller allows scraping metrics from outside the namespace in which catalogd and operator-controller are running. Signed-off-by: Joe Lanford <[email protected]>
Brett and I discussed this a bit offline and couldn't immediately figure out a concrete test to add.
It would be nice to be able to introduce a higher barrier for changing network policy because it is a security-sensitive change. We would not want to accidentally open up network access unnecessarily. From a technical perspective, I'm not sure tests that probing blocked ports would work all that well because we may need to have something actually listening on the blocked port and matched by the network policy selector in order to ever see something other than a network failure (would the client see a difference between "NetworkPolicy blocked the traffic" and "nothing is listening there"?) Perhaps we could have a meta-test like "make sure the NetworkPolicy looks like $this", where "$this" is some sort of annotated list of rules where a justification is provided. If we had that, then:
|
|
/lgtm |
Description
This PR adds
NetworkPolicyobjects, 1 each for operator-controller and catalogd.Both network policies allow:
In addition, catalogd's network policy also allows
Reviewer Checklist