Conversation
Add network policies to restrict ingress traffic to Che server and user workspaces. Policies are controlled via the new `networking.networkPolicies.enabled` field in the CheCluster CR (defaults to false). Che server namespace: creates an allow-from-workspaces policy that permits traffic from workspace namespaces. User namespaces: creates allow-from-che, allow-from-same-namespace, and on OpenShift additionally allow-from-openshift-monitoring, allow-from-openshift-operators, and allow-from-openshift-ingress. All network policy code uses the K8sClient wrapper with proper DiffOpts, owner references for same-namespace objects, and flavor-aware resource naming via defaults.GetCheFlavor(). CRW-10821 Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[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 |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
This PR contains changes to files in directories that are typically not intended to be committed:
Please verify these changes are intentional. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Tests were setting Spec.Networking.NetworkPolicies (Che namespace) instead of Spec.DevEnvironments.Networking.NetworkPolicies (user namespace), causing the controller to never see the feature as enabled. Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/che-ai-assistant ok-pr-review Task completed. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2157 +/- ##
==========================================
+ Coverage 50.12% 52.80% +2.68%
==========================================
Files 101 116 +15
Lines 13234 14813 +1579
==========================================
+ Hits 6633 7822 +1189
- Misses 6134 6459 +325
- Partials 467 532 +65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
General Findings
Egress restriction is intentionally excluded but undocumented
All policies use PolicyTypes: [Ingress] only. This is likely intentional as a first step, but it would be helpful to document in the CRD field comments or PR description that egress restrictions are out of scope. Future maintainers may otherwise assume egress was an oversight.
.claude/rules/ files in this PR
The PR includes multiple .claude/rules/*.md files and a restructured AGENTS.md. If these are intentional, a brief note in the PR description would help reviewers understand why they are included alongside a networking feature.
allow-from-same-namespace enables lateral movement in shared namespaces
This policy allows any pod in the workspace namespace to reach any other pod in the same namespace. For clusters where defaultNamespace.template does not produce per-user-unique namespaces, a compromised workspace container could probe all other workspace pods in the same namespace. This is likely acceptable for the default single-user namespace configuration, but worth documenting in the CRD field comment.
No +kubebuilder:rbac marker for NetworkPolicy resources
The impact review confirms that RBAC is present in the CSV (bundle/next/eclipse-che/manifests/che-operator.clusterserviceversion.yaml, networking.k8s.io/networkpolicies), but it was added manually rather than via a +kubebuilder:rbac marker in the Go source. If RBAC is ever regenerated from markers alone, the NetworkPolicy permissions will be dropped, causing 403 errors at runtime. Consider adding a // +kubebuilder:rbac marker to the server or usernamespace reconciler files.
allow-from-operators naming is inconsistent with OpenShift-prefixed peers
The OpenShift-specific policies are named allow-from-openshift-monitoring and allow-from-openshift-ingress, but the operators policy is allow-from-operators (no openshift- prefix) even though it serves a similar platform-scoped purpose. The API doc comment matches the code name correctly, but the asymmetry may confuse future maintainers. Would it be worth renaming to allow-from-openshift-operators for consistency, or adding a comment explaining the difference?
getNetworkPolicies is called on every reconciliation even when the feature is disabled
When networkPolicies.enabled is false (the default), every user namespace reconciliation still calls getNetworkPolicies() to build the full policy list, then performs a Get+check+Delete for each name. Consider adding an early exit via a single list call with a label selector to skip all this work in the common case where no operator-managed policies exist in the namespace.
| checluster.Spec.DevEnvironments.Networking.NetworkPolicies.Enabled | ||
|
|
||
| if !isNetworkPolicyEnabled { | ||
| for _, policy := range policies { |
There was a problem hiding this comment.
The disable path calls getNetworkPolicies() to enumerate policy names, then deletes each by name. The list includes OpenShift-specific policies only when infrastructure.IsOpenShift() returns true. If the operator created policies on OpenShift and IsOpenShift() returns differently during cleanup (platform migration, environment inconsistency, or detection bug), the OpenShift-specific policies (allow-from-openshift-monitoring, allow-from-openshift-ingress) will never appear in the list and will be orphaned.
Consider using DeleteAllOf with a label selector matching the operator-managed labels for cleanup. This is platform-independent and avoids coupling the disable path to runtime platform detection. The server-side code uses a similar get-check-delete-by-name pattern, but it only manages a single known policy name, so the coupling is less risky there.
| checluster *chev2.CheCluster, | ||
| ) ([]networkingv1.NetworkPolicy, error) { | ||
| operatorNamespace, err := infrastructure.GetOperatorNamespace() | ||
| if err != nil { |
There was a problem hiding this comment.
infrastructure.GetOperatorNamespace() reads from /var/run/secrets/kubernetes.io/serviceaccount/namespace or OPERATOR_NAMESPACE on every call to getNetworkPolicies(). On the disable path, the operator namespace value is never actually used for cleanup (deletion works by name). A transient I/O failure here will prevent the policy list from being built, blocking cleanup entirely even though no namespace value is needed.
Would it be possible to either cache the operator namespace at reconciler startup, or restructure the disable path to avoid calling getNetworkPolicies() altogether (e.g., using DeleteAllOf with a label selector)?
| networkPolicy := &networkingv1.NetworkPolicy{} | ||
| exists, err := r.clientWrapper.GetIgnoreNotFound( | ||
| context.TODO(), | ||
| types.NamespacedName{ |
There was a problem hiding this comment.
All K8s API calls in reconcileNetworkPolicies use context.TODO(). The Reconcile() method already has a live context.Context. Consider threading it through to reconcileNetworkPolicies (and getNetworkPolicies) so that cancellation signals from leader election loss or graceful shutdown propagate to in-flight network policy mutations. This matches the pattern used in other reconcile methods on this controller.
| func IsPartOfEclipseCheResourceAndManagedByOperator(labels map[string]string) bool { | ||
| return labels[constants.KubernetesPartOfLabelKey] == constants.CheEclipseOrg && labels[constants.KubernetesManagedByLabelKey] == GetManagedByLabel() | ||
| func IsPartOfEclipseCheAndManagedByOperator(labels map[string]string, component string) bool { | ||
| return labels[constants.KubernetesPartOfLabelKey] == constants.CheEclipseOrg && |
There was a problem hiding this comment.
The function now additionally checks KubernetesComponentLabelKey == component, which narrows the match compared to the old IsPartOfEclipseCheResourceAndManagedByOperator. Both current callers create resources with GetLabels(component) which sets that label, so in-flight resources should be unaffected. Does it make sense to add a brief comment here documenting that this narrowing is safe because GetLabels() has always set the component label, so any resource previously managed by this operator will already have it?
|
|
||
| var NetworkPolicy = cmp.Options{ | ||
| cmpopts.IgnoreFields(networking.NetworkPolicy{}, "TypeMeta", "ObjectMeta"), | ||
| } |
There was a problem hiding this comment.
cmpopts.IgnoreFields(..., "ObjectMeta") means Sync() compares only the Spec. If someone manually removes the managed-by label from a NetworkPolicy, the operator will not detect the difference and will not restore the label. The cleanup path checks IsPartOfEclipseCheAndManagedByOperator before deleting, so a policy with a stripped label would also be skipped during feature disable. Consider whether labels should be included in the diff comparison, or document this limitation so future maintainers are aware.
| } | ||
|
|
||
| _, cl, r := setup(infrastructure.OpenShiftV4, cheCluster, userProject, userNamespace) | ||
|
|
There was a problem hiding this comment.
TestNetworkPoliciesCreatedWhenEnabled passes infrastructure.OpenShiftV4 to setup(). There is no test verifying that exactly 3 policies (allow-from-<flavor>, allow-from-same-namespace, allow-from-operators) are created on plain Kubernetes, and that the 2 OpenShift-specific policies are absent. Would it be possible to add a parallel test case for infrastructure.Kubernetes?
| err := cl.Get(context.TODO(), client.ObjectKey{Name: name, Namespace: "user-project"}, networkPolicy) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.Equal(t, constants.CheEclipseOrg, networkPolicy.Labels[constants.KubernetesPartOfLabelKey]) |
There was a problem hiding this comment.
The assertions verify that policies exist and have the correct KubernetesPartOfLabelKey label, but do not validate the actual ingress rule specs (PolicyTypes, Spec.Ingress[0].From, namespace selectors). A refactoring that changes the ingress spec would pass these tests undetected. Consider asserting at least the PolicyTypes and the namespace selector MatchLabels for the key policies.
1 participant
Summary
networking.networkPolicies.enabledfield to CheCluster CR (defaults tofalse) to control ingress restriction via network policiesallow-from-workspaces-namespacespolicy permitting traffic from workspace namespacesallow-from-<flavor>,allow-from-same-namespace, and OpenShift-specific policies (allow-from-openshift-monitoring,allow-from-openshift-operators,allow-from-openshift-ingress)K8sClientwrapper withDiffOpts, owner references, and flavor-aware naming viadefaults.GetCheFlavor()CRW-10821
Test plan
networking.networkPolicies.enabled: trueon a CheCluster CR and verify network policies are created in both the Che namespace and user namespacesdevspaces) flavor produces correctly named policies (allow-from-devspacesinstead ofallow-from-che)make test— all unit tests pass