Skip to content

feat: support in-cluster network policies generation - #214

Merged
GrigoryPervakov merged 4 commits into
mainfrom
network_policies
Aug 25, 2026
Merged

feat: support in-cluster network policies generation#214
GrigoryPervakov merged 4 commits into
mainfrom
network_policies

Conversation

@scanhex12

@scanhex12 scanhex12 commented Jun 2, 2026

Copy link
Copy Markdown
Member

What

Managed NetworkPolicy support for ClickHouseCluster and KeeperCluster.

  • spec.networkPolicy.policy: Enabled | Disabled (default Disabled) on both CRDs
  • ClickHouse policy covers cluster-internal traffic only: interserver (9009) and management (9001) between replicas, management (9001/9002) from operator pods (label clickhouse.com/role: operator, any namespace)
  • Keeper policy: Raft (9234) between replicas; client and control ports (2181/2281/9123) from the operator and from every ClickHouseCluster referencing the keeper — references are the admission contract, cross-namespace included
  • Client connections and metrics scraping are deliberately not covered: users grant access with their own additive policies (see the updated network-policies guide)
  • Deployment-level gate ENABLE_NETWORK_POLICY env / controller.networkPolicyManagement.enabled chart value, mirroring ENABLE_PDB

Why

Make ClickHouse and Keeper work in NetworkPolicy-enforced environments out of the box. The operator knows the internal traffic matrix — peers, TLS-dependent ports, its own identity — and keeps the policies correct as clusters change, which a static example manifest cannot.

Survey of other operators: only Strimzi manages NetworkPolicies; DB operators ship docs at best. Client-access management was considered and deferred — user approaches vary too much to guess (peer lists vs label opt-in), and additive user policies compose cleanly with the managed ones.

Test plan

  • Unit specs for policy templates (shapes, referencing-cluster peers, determinism)
  • e2e: object management — creation, shape, reference admission, disable cascades
  • NetworkPolicy enforcement testing (kube-network-policies in CI, default-deny + blocked-port probes) lands in a follow-up PR

Based on the original draft by @scanhex12, reworked to match the internal Services traffic architecture (#286/#287).


🤖 This description is AI-written

@scanhex12
scanhex12 marked this pull request as draft June 3, 2026 07:48

@GrigoryPervakov GrigoryPervakov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-generated review (Claude, posted on Grigorii's request). Treat as triage input; the maintainer will decide what holds.

Overall shape looks correct — native-first, per-CR enable, ingress-only, per-component object, owner-ref + delete-on-disable. Four concerns before merge.

Bugs

1. TLS ports break the policy. networkpolicy.go hardcodes 9000 (PortNative) and 8123 (PortHTTP). When spec.settings.tls.enabled is true, buildProtocols in internal/controller/clickhouse/templates.go:579 drops 9000/8123 entirely and exposes 9440 (PortNativeSecure) and 8443 (PortHTTPSecure). A TLS-enabled cluster with networkPolicy.enabled: true gets a policy that opens dead ports and silently blocks the live ones.

2. spec.additionalPorts are silently blocked. This is the documented extension surface for MySQL/Postgres/gRPC and custom listeners — docs/guides/configuration.mdx:428, validated at internal/webhook/v1alpha1/clickhousecluster_webhook.go:132, wired into pods and the headless Service at internal/controller/clickhouse/templates.go:38 and :507. The PR ignores cr.Spec.AdditionalPorts in networkpolicy.go. A user with `additionalPorts: [{port: 9004, name: mysql}]` who enables `networkPolicy` loses MySQL connectivity with no warning.

Both 1 and 2 share a root cause: `networkpolicy.go` re-derives the port set independently instead of asking the same source `templates.go` does. One helper returning "all client-facing TCP ports for this CR" (TLS-aware, `additionalPorts`-aware), called from both the Service template and the policy builder, fixes both. Adding a new port type later is then one line in one place.

Design

3. `Backend string` is dead API surface. The field accepts any string, validates nothing, is read nowhere. `// TODO: add Cilium` is a fine internal comment but not a reason to ship an open-ended string on the CRD. Either drop it until CNP support actually lands, or make it `+kubebuilder:validation:Enum=kubernetes` with one value today. API additions on `networkPolicy` are hard to retract; see Strimzi proposal 028 for the kind of rationale these surfaces eventually need.

4. Prometheus is silently blocked on enable. With `enabled: true` and `monitoringPeers` nil, port 9363 has no ingress rule and scraping breaks. Default `monitoringPeers` to "open to all" (Strimzi's choice for metrics ports), or document loudly in both the field godoc and the user guide that enabling the policy without setting `monitoringPeers` will drop metrics.

Scope

No Keeper policy. CH → Keeper on `keeper.PortNative` (2181) and Raft `keeper.PortInterserver` (9234) stay open. If the intent is "ClickHouse-side only, Keeper in a follow-up", say so in the PR title/body and file an issue. Otherwise this is half a feature.

Minor

  • Self-ingress rule conflates `PortInterserver` + `PortNative` into one rule. Interserver replication (9009) and intra-cluster distributed-query (9000) are different concerns; two rules document intent better.
  • `clusterNamespace` peer (`kubernetes.io/metadata.name: cr.Namespace`) allows any pod in the operand namespace to hit client ports. If the intent was "operator can reach the pod", narrow to operator-labelled pods (Strimzi pattern) — the operator already talks through the headless Service.
  • e2e test uses `Replicas: new(int32(1))` in two places — `new()` takes a type, not an expression. CI should catch it; flagging in case it doesn't.
  • Positive e2e check (`nc -z` from a `role=allowed` pod) passes regardless of policy enforcement — kindnet doesn't enforce NetworkPolicies. Negative cases live behind `NP_ENFORCING_CNI` env with no CI job setting it. Test name `manages and enforces the cluster NetworkPolicy` overpromises — under default CI it's structural only.

@GrigoryPervakov GrigoryPervakov changed the title feat: support simple network policies (without cilium) feat: support in-cluster network policies generation Aug 25, 2026
@GrigoryPervakov
GrigoryPervakov marked this pull request as ready for review August 25, 2026 17:38
@GrigoryPervakov
GrigoryPervakov merged commit 9f5550e into main Aug 25, 2026
27 checks passed
@GrigoryPervakov
GrigoryPervakov deleted the network_policies branch August 25, 2026 17:38
@mintlify

mintlify Bot commented Aug 25, 2026

Copy link
Copy Markdown

Docs PR opened: ClickHouse/ClickHouse#116372

Two Kubernetes operator pages now document managed per-cluster NetworkPolicies, their traffic rules, and the new networkPolicy API fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants