chore: Add NetworkPolicy reconciler - #2169
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Introduces operator-managed NetworkPolicy resources for both the Che namespace and user workspace namespaces on OpenShift. A new `spec.networking.networkPolicy.enabled` field controls the feature (disabled by default). When enabled, the operator creates fine-grained ingress and egress policies: Che namespace policies: - allow-from-same-namespace: Che-to-Che pod traffic - allow-from-workspaces-namespaces-to-openvsx-registry - allow-from-workspaces-namespaces-to-plugin-registry - allow-from-openshift-ingress - allow-from-openshift-monitoring - allow-from-operator: operator pod to Che components - allow-to-everywhere: all egress from Che pods User workspace namespace policies: - allow-from-<che-namespace>: ingress from Che namespace - allow-from-same-namespace: intra-namespace traffic - allow-from-devworkspace-operator: DWO to workspace pods - allow-from-openshift-monitoring - allow-from-openshift-ingress - allow-to-everywhere: all egress from workspace pods Also refactors DwoNamespaceReconciler into a utility function and extends the cache selector to watch DevWorkspace operator pods. Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
| }, | ||
| } | ||
|
|
||
| allowFromOpenShiftIngress := &networkingv1.NetworkPolicy{ |
There was a problem hiding this comment.
nit, I see a lot of repeated NetworkPolicy construction. Maybe we can create a small helper (or a builder for common metadata) to reduce duplication?
There was a problem hiding this comment.
They are slightly different, all NP for eclipse-che namespace has a pod selector:
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
constants.KubernetesPartOfLabelKey: constants.CheEclipseOrg,
},
},
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
tolusha
left a comment
There was a problem hiding this comment.
Review of PR #2169 - Add NetworkPolicy reconciler
Overall the feature is well-structured: disabled by default, OpenShift-only guard, proper owner references for Che-namespace policies, and reasonable test coverage. A few things worth addressing before merge.
User-namespace NetworkPolicies orphaned on CheCluster CR deletion
When the CheCluster CR is deleted, the user namespace controller's Reconcile returns early without cleaning up NetworkPolicies in user namespaces. Cross-namespace owner references are not supported, so GC won't handle these. The Che-namespace policies are fine - owner references cover them. But user-namespace NPs will persist after Che is uninstalled, potentially blocking workspace traffic with no obvious origin (the CR and operator are both gone at that point).
Does it make sense to add cleanup logic when checluster == nil, or at least document this as a known limitation requiring manual cleanup after uninstall?
allow-all-egress permits unrestricted outbound traffic - worth documenting
Both reconcilers create an allow-all-egress policy with an empty egress rule, which permits all outbound traffic. This is the right default for workspaces (git, extensions, etc.), but administrators enabling this feature in a hardened environment may not expect unrestricted egress. Consider noting in the API doc comment that egress is intentionally unrestricted.
| targetNs string, | ||
| deployContext *chetypes.DeployContext, | ||
| ) error { | ||
| policies, err := r.getNetworkPolicies(targetNs, deployContext) |
There was a problem hiding this comment.
getNetworkPolicies is called unconditionally here, before the IsNetworkPoliciesEnabled() guard at line 635. getNetworkPolicies calls GetDevWorkspaceOperatorNamespace, which returns an error when no DWO pod is found. Since network policies are disabled by default, user-namespace reconciliation will fail with a spurious error any time the DWO pod is temporarily unavailable - for example during cluster startup or DWO upgrades.
Consider moving the IsNetworkPoliciesEnabled check to the top of reconcileNetworkPolicies, before calling getNetworkPolicies, so that DWO discovery is skipped entirely when the feature is off.
| // When enabled, the following policies are created: | ||
| // In the Che namespace: | ||
| // - allow-from-same-namespace: allows ingress traffic between Che pods in the same namespace. | ||
| // - allow-from-workspaces-namespaces-to-openvsx-registry: allows ingress traffic from user workspace |
There was a problem hiding this comment.
The comment lists allow-from-workspaces-namespaces-to-openvsx-registry and allow-from-workspaces-namespaces-to-plugin-registry as two separate policies, but networkpolicies.go creates a single policy named allow-from-workspaces that allows ingress from all workspace namespaces to all Che pods. Would it be possible to update this to match the actual policy name?
| type NetworkPolicy struct { | ||
| // Enabled controls whether the operator creates NetworkPolicy resources. | ||
| // +optional | ||
| Enabled *bool `json:"enabled"` |
There was a problem hiding this comment.
Similar bool pointer fields in the API use json:"enabled,omitempty" (e.g., PersistentHomeConfig.Enabled). Consider adding omitempty for consistency:
| Enabled *bool `json:"enabled"` | |
| Enabled *bool `json:"enabled,omitempty"` |
|
|
||
| func init() { | ||
| test.EnableTestMode() | ||
|
|
There was a problem hiding this comment.
The reconciler is guarded by infrastructure.IsOpenShift() at registration time, but the test infrastructure initializes as Kubernetes. The tests work because they call the reconciler directly and bypass that guard, but it would be more consistent to use OpenShiftV4 here - the init_test.go in the usernamespace package does this correctly.
| infrastructure.InitializeForTesting(infrastructure.OpenShiftV4) |
| targetNs string, | ||
| deployContext *chetypes.DeployContext, | ||
| ) ([]*networkingv1.NetworkPolicy, error) { | ||
| devWorkspaceOperatorNamespace, err := devworkspace.GetDevWorkspaceOperatorNamespace(deployContext.ClusterAPI.ClientWrapper) |
There was a problem hiding this comment.
GetDevWorkspaceOperatorNamespace does a pod list on every user-namespace reconciliation. The main controller already resolves and stores DwoNamespace in deployContext.DwoNamespace (see checluster_controller.go), but the user namespace controller builds its own DeployContext without populating that field, then calls GetDevWorkspaceOperatorNamespace again here.
Does it make sense to populate deployContext.DwoNamespace once in the user namespace controller's Reconcile function, and pass it into getNetworkPolicies as a parameter rather than re-discovering it on each call?
| npList := &networkingv1.NetworkPolicyList{} | ||
| err = ctx.ClusterAPI.Client.List(context.TODO(), npList, &client.ListOptions{Namespace: "eclipse-che"}) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, len(allNetworkPolicyNames), len(npList.Items)) |
There was a problem hiding this comment.
The test asserts on policy count and Get success, but never inspects the Spec of any created policy. A bug that creates allow-from-operator with the wrong namespace selector, or allow-from-workspaces with an empty ingress rule, would pass silently.
Would it be possible to add at least one assertion on the spec of a representative policy - for example, verifying that allow-from-operator has the correct NamespaceSelector for the operator namespace?
What does this PR do?
Introduces operator-managed NetworkPolicy resources for both the Che namespace and user workspace namespaces on OpenShift clusters.
A new
spec.networking.networkPolicy.enabledfield controls the feature (disabled by default). When enabled, the operator creates fine-grained ingress and egress NetworkPolicy resources to restrict network traffic to only the flows required by Che components and workspaces.Che namespace policies:
allow-from-same-namespace— allows ingress traffic between Che pods in the same namespaceallow-from-workspaces— allows ingress traffic from user workspace namespacesallow-from-openshift-ingress— allows ingress traffic from the OpenShift ingress namespaceallow-from-openshift-monitoring— allows ingress traffic from the OpenShift monitoring namespaceallow-from-operator— allows ingress traffic from the operator pod to Che componentsallow-all-egress— allows all egress traffic from Che podsUser workspace namespace policies:
allow-from-<che-namespace>— allows ingress traffic from Che namespace podsallow-from-same-namespace— allows ingress traffic between pods in the same namespaceallow-from-devworkspace-operator— allows ingress traffic from the DevWorkspace operatorallow-from-openshift-monitoring— allows ingress traffic from the OpenShift monitoring namespaceallow-from-openshift-ingress— allows ingress traffic from the OpenShift ingress namespaceallow-all-egress— allows all egress traffic from Che podsAdditional changes:
DwoNamespaceReconcilerinto a utility functionGetDevWorkspaceOperatorNamespaceNetworkPolicydiff option,DeleteIgnoreNotFoundk8s client method, and related constantsScreenshot/screencast of this PR
N/A — infrastructure change, no UI impact.
What issues does this PR fix or reference?
CRW-10821
How to test this PR?
OpenShift
or
kubectl patch checluster eclipse-che -n eclipse-che --type merge -p '{"spec":{"networking":{"networkPolicy":{"enabled":true}}}}'Expected: 7 NetworkPolicy resources (allow-from-same-namespace, allow-from-workspaces-namespaces-to-openvsx-registry, allow-from-workspaces-namespaces-to-plugin-registry, allow-from-openshift-ingress, allow-from-openshift-monitoring, allow-from-operator, allow-to-everywhere).
Expected: 6 NetworkPolicy resources (allow-from-eclipse-che, allow-from-same-namespace, allow-from-devworkspace-operator, allow-from-openshift-monitoring, allow-from-openshift-ingress, allow-to-everywhere).
Expected: all Che-managed NetworkPolicy resources are deleted.
Common Test Scenarios
PR Checklist
As the author of this Pull Request I made sure that:
Reviewers
Reviewers, please comment how you tested the PR when approving it.