mirrors/seaweedfs
1
0
Fork 0
mirror of https://github.com/seaweedfs/seaweedfs.git synced 2026-08-16 04:06:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

helm: optional NetworkPolicy per component (#10479)

Browse Source 
* helm: optional NetworkPolicy per component

In a namespace with a default-deny policy the chart cannot be installed:
the components never reach each other, and the post-install bucket hook
waits on the master and filer until it gives up.

networkPolicy.enabled renders one policy per component, selecting its
pods by the standard app.kubernetes.io labels and admitting the other
pods of the release on the ports that component listens on. The port
lists come from the same values as the containerPorts, and CI asserts
the two agree. Restricting egress is a second opt-in with extraEgress
for the filer store and notification sinks, which the chart cannot
know about.

Closes #10421

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm: say "changed" instead of "retuned" in the policy comments

codespell reads "retuned" as a misspelling of "returned" and fails the
spelling job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm: refuse empty port and DNS peer lists instead of widening

In a NetworkPolicy an empty ports list means every port and a missing peer
selector means every pod, so `kubeApiServer.ports: []` silently opened the
API server CIDRs on all ports, and nulling a DNS selector rendered
`podSelector: null`, which is every pod in kube-system. Both now fail the
render, and the DNS rule emits only the selectors that are set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm: gate the bucket hook Job and its policy on one helper

Both were deriving the same condition from the same values, kept in step by
a comment. seaweedfs.bucketHookEnabled makes it one definition, so adding an
S3 mode cannot leave the Job running without its policy - which under
default-deny means the hook hangs. CI pins the pairing across the eleven
modes that decide it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm: say that an egress default-deny needs both toggles

networkPolicy.enabled on its own only covers a default-deny that restricts
ingress. Where Egress is in its policyTypes as well, which is the usual
baseline, the components still cannot resolve DNS and egress.enabled is
required too. Both values and the README said the first half of that.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm: quote the label values the policies emit

The same int-coercion the hook templates were fixed for, in the file this PR
adds: unquoted, a release named 123 renders app.kubernetes.io/instance as a
YAML integer in the metadata, the podSelector and both peer selectors, and
the API server rejects the object - a policy that silently never applies.
CI now renders the chart as release "123" and fails if any policy label
comes out as a non-string.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm: gate the resize hook policy on the same lookup as its Job

The policy rendered whenever the hook was enabled, which is the default, so
every release carrying networkPolicy got one - as a pre-install hook that
Helm never collects, left in the namespace after uninstall. It also made
egress.kubeApiServer.cidrs mandatory for every release, since the policy
claims the API server, for a Job that only runs on an upgrade that grows a
PVC.

Move the command computation the Job is gated on into a helper and read it
from both.

* helm: drop the API server rule from the admin policy

No seaweedfs binary talks to the Kubernetes API - there is no client-go in
go.mod - so the rule granted admin an egress path it never uses, and forced
anyone running admin with egress on to name an API server address for it.
The pod-RW ClusterRole the comment was reasoning from is a leftover from a
migration and is not read by any component.

* helm: reject a networkPolicy.components key that names no component

The component names are not guessable - objectstorage-provisioner,
seaweedfs-all-in-one, volume-<name> - and a typo silently dropped the rules
it was carrying. Check against every component the chart can produce, not
the enabled ones, so a values file shared across releases can still hold
overrides for a component this one leaves off.

* helm: name the all-in-one policy after the workload

componentName prefixes the release fullname onto the suffix it is given, and
the component label already starts with seaweedfs-, so the policy came out as
<release>-seaweedfs-seaweedfs-all-in-one.

* helm ci: assert the denied probe failed rather than that it did not succeed

kubectl run's "pod/x created" was captured alongside the pod log, so an
empty log would still not match exit=0 and the denial would pass without
anything having been tested.

* helm: document what turning the network policies on costs

Three things the values did not say: a Prometheus outside the release stops
scraping and nothing reports it, the resize hook's policy is a hook resource
that uninstall leaves behind, and the DNS selectors are wrong on OpenShift.

* helm: spell out the managed distributions codespell reads as a typo

codespell has AKS in its dictionary as a misspelling of ASK, so the DNS
selector note failed the spelling job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* helm ci: read the probe verdict from the marker line, not the whole log

The default-deny step decides both probes from the pod log, but kubectl logs
returns stderr as well, and busybox wget reports "download timed out" there
even under -q. The denied probe therefore produced two lines beginning with
wget:, which matches neither exit=0 nor exit=*, so a correct denial landed in
the arm meant for a probe that produced no result at all and failed the job.

The earlier form hid this behind a catch-all that treated anything without
exit=0 as a denial; tightening that assertion made the stray line fatal
without narrowing the input it reads.

Pick the marker line out instead. The trailing || true is required: the step
runs under bash -e, so a grep that matches nothing would abort it rather than
reach the arm that reports an empty result, which is the case that assertion
exists to catch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Sebastian Preisner <preisner@puzzle-itc.de>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Lu <chris.lu@gmail.com>
This commit is contained in:
Sebastian
2026-07-30 12:04:30 -07:00
committed by GitHub
co-authored by Claude Opus 5 Sebastian Preisner Chris Lu
parent 78ed665557
commit 09da5f634f
7 changed files with 941 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/workflows/helm_ci.yml
+418
View File
@@ -822,6 +822,314 @@ jobs:
PYEOF
echo "Hook Job label tests passed"
echo ""
echo "=== Testing NetworkPolicy rendering ==="
# The policies are only exercised for real by the networkpolicy-install
# job below. These assertions cover what template rendering can see:
# that the flag stays off by default, that every deployed component has
# exactly one policy, that each policy allows every port its workload
# declares, and that egress stays a separate opt-in.
python3 - "$CHART_DIR" <<'PYEOF'
import subprocess, sys, yaml
chart = sys.argv[1]
def render(values, json_values=None):
args = ["helm", "template", "test", chart]
for k, v in values.items():
args += ["--set", f"{k}={v}"]
for k, v in (json_values or {}).items():
args += ["--set-json", f"{k}={v}"]
return subprocess.check_output(args, text=True, stderr=subprocess.STDOUT)
def expect_failure(values, needle, label, json_values=None):
try:
render(values, json_values)
except subprocess.CalledProcessError as e:
if needle not in (e.output or ""):
failed.append(f"{label}: unexpected error: {(e.output or '')[:200]}")
else:
print(label)
return
failed.append(f"{label}: render should have failed")
def docs(manifest):
return [d for d in yaml.safe_load_all(manifest) if d]
def component(labels):
return labels.get("app.kubernetes.io/component")
def policies(manifest):
out = {}
for d in docs(manifest):
if d.get("kind") != "NetworkPolicy":
continue
out[component(d["spec"]["podSelector"]["matchLabels"])] = d
return out
def workloads(manifest, kinds=("Deployment", "StatefulSet")):
out = {}
for d in docs(manifest):
if d.get("kind") in kinds:
out[component(d["spec"]["template"]["metadata"]["labels"])] = d
return out
def allowed_ports(policy):
ports = set()
for rule in policy["spec"].get("ingress") or []:
for p in rule.get("ports") or []:
ports.add(p["port"])
return ports
def container_ports(workload):
ports = set()
for c in workload["spec"]["template"]["spec"].get("containers", []):
for p in c.get("ports") or []:
ports.add(p["containerPort"])
return ports
EVERYTHING = {
"s3.enabled": "true",
"sftp.enabled": "true",
"admin.enabled": "true",
"worker.enabled": "true",
"cosi.enabled": "true",
"s3.createBuckets[0].name": "b",
"volumes.ssd.port": "8081",
"global.seaweedfs.monitoring.enabled": "true",
}
failed = []
# Off by default: the chart has never shipped a NetworkPolicy and must not
# start now, or every existing release in a default-deny namespace changes
# behaviour on the next upgrade.
if policies(render(EVERYTHING)):
failed.append("networkPolicy.enabled unset: policies rendered anyway")
else:
print("networkPolicy off by default: no policies rendered")
on = dict(EVERYTHING, **{"networkPolicy.enabled": "true"})
out = render(on)
pols = policies(out)
wls = workloads(out)
# Every workload gets exactly one policy, and every policy has a workload.
# A component with no policy is wide open under default-deny; a policy with
# no component is dead weight that hides a renamed label.
for comp in wls:
if comp not in pols:
failed.append(f"{comp}: workload has no NetworkPolicy")
for comp in pols:
# The bucket hook Job is covered too, and a Job is not a workload here.
if comp not in wls and comp != "bucket-hook":
failed.append(f"{comp}: NetworkPolicy selects a component that is not deployed")
# The ports a component listens on come from the same values as its
# containerPorts, so the two must agree. This is what catches a port added to
# a workload and forgotten in the policy - the failure mode that only shows up
# once someone turns the flag on.
for comp, wl in wls.items():
if comp not in pols:
continue
declared = container_ports(wl)
allowed = allowed_ports(pols[comp])
missing = sorted(declared - allowed)
if missing:
failed.append(f"{comp}: listens on {missing} but its policy does not allow it")
if not failed:
print("every workload has a policy covering all of its containerPorts")
# The bucket hook Job and its policy are gated on one shared helper, and
# this pins that down: a Job without its policy hangs under default-deny,
# a policy without its Job is noise. The combinations cover the modes that
# decide it, including the ones where a bucket list is ignored.
hook_cases = {
"nothing": {},
"s3 without buckets": {"s3.enabled": "true"},
"s3 with buckets": {"s3.enabled": "true", "s3.createBuckets[0].name": "b"},
"filer.s3 without buckets": {"filer.s3.enabled": "true"},
"filer.s3 with buckets": {"filer.s3.enabled": "true",
"filer.s3.createBuckets[0].name": "b"},
"s3 gateway, buckets on filer.s3": {"s3.enabled": "true",
"filer.s3.createBuckets[0].name": "b"},
"allInOne without buckets": {"allInOne.enabled": "true",
"allInOne.s3.enabled": "true"},
"allInOne with buckets": {"allInOne.enabled": "true",
"allInOne.s3.enabled": "true",
"allInOne.s3.createBuckets[0].name": "b"},
# allInOne reads only its own bucket list, so s3.createBuckets is
# not enough to produce the Job.
"allInOne, buckets on s3": {"allInOne.enabled": "true",
"allInOne.s3.enabled": "true",
"s3.createBuckets[0].name": "b"},
"master off": {"master.enabled": "false", "s3.enabled": "true",
"s3.createBuckets[0].name": "b"},
"buckets but no S3 endpoint": {"s3.createBuckets[0].name": "b"},
}
for label, values in hook_cases.items():
out = render(dict(values, **{"networkPolicy.enabled": "true"}))
job = any(d.get("kind") == "Job"
and d["metadata"]["name"].endswith("-bucket-hook")
for d in docs(out))
policy = "bucket-hook" in policies(out)
if job != policy:
failed.append(f"{label}: bucket hook Job={job} but its policy={policy}; "
"the two are gated on seaweedfs.bucketHookEnabled and "
"have to appear together")
if not failed:
print(f"bucket hook Job and policy agree across {len(hook_cases)} modes")
# Egress is its own opt-in: with it off the policies must not constrain
# outbound traffic at all, or enabling networkPolicy alone would cut the filer
# off from its store.
for comp, p in pols.items():
if "Egress" in p["spec"]["policyTypes"]:
failed.append(f"{comp}: Egress in policyTypes while networkPolicy.egress.enabled is false")
if not any("Egress" in p["spec"]["policyTypes"] for p in pols.values()):
print("egress off by default: policies are ingress-only")
# Components that need the API server must not silently lose it: rendering
# fails with a pointer to the value instead.
expect_failure(dict(on, **{"networkPolicy.egress.enabled": "true"}),
"kubeApiServer.cidrs is empty",
"empty kubeApiServer.cidrs fails the render with a pointer to the value")
egress_on = dict(on, **{
"networkPolicy.egress.enabled": "true",
"networkPolicy.egress.kubeApiServer.cidrs[0]": "10.96.0.1/32",
})
# An empty port or peer list is "everything" in a NetworkPolicy, not
# "nothing", so the two places that could be emptied have to be refused
# rather than quietly widened.
expect_failure(egress_on, "kubeApiServer.ports is empty",
"empty kubeApiServer.ports is refused instead of allowing every port",
json_values={"networkPolicy.egress.kubeApiServer.ports": "[]"})
expect_failure(egress_on, "allowDNS is on but both",
"DNS with no selector is refused instead of allowing every pod",
json_values={"networkPolicy.egress.dnsNamespaceSelector": "null",
"networkPolicy.egress.dnsPodSelector": "null"})
# Dropping one of the two DNS selectors must leave the key out, not
# render it as null - null namespaceSelector means this namespace and
# null podSelector means every pod in the peer namespace.
for dropped in ("dnsNamespaceSelector", "dnsPodSelector"):
out = render(egress_on, {f"networkPolicy.egress.{dropped}": "null"})
nulls = [(c, k) for c, p in policies(out).items()
for r in p["spec"]["egress"]
for t in r.get("to") or []
for k, v in t.items() if v is None]
nulls += [(c, "ports") for c, p in policies(out).items()
for r in p["spec"]["egress"] if "ports" in r and r["ports"] is None]
if nulls:
failed.append(f"{dropped}=null: rendered null keys {sorted(set(nulls))}")
else:
print(f"{dropped}=null: the key is left out rather than rendered as null")
# Helm allows a release named "123", and an unquoted label value then
# renders as a YAML integer. Label values are strings in the API, so the
# whole object gets rejected - a policy that never applies, silently.
# Every label the policies emit, in metadata and in every selector, has
# to survive that release name as a string.
numeric = subprocess.check_output(
["helm", "template", "123", chart,
"--set", "networkPolicy.enabled=true",
"--set", "networkPolicy.egress.enabled=true",
"--set", "networkPolicy.egress.kubeApiServer.cidrs[0]=10.96.0.1/32",
"--set", "s3.enabled=true", "--set", "s3.createBuckets[0].name=b"],
text=True, stderr=subprocess.STDOUT)
def label_maps(policy):
yield "metadata", policy["metadata"].get("labels", {})
yield "podSelector", policy["spec"]["podSelector"]["matchLabels"]
for direction in ("ingress", "egress"):
for i, rule in enumerate(policy["spec"].get(direction) or []):
for peer in rule.get("to") or rule.get("from") or []:
for key in ("podSelector", "namespaceSelector"):
sel = peer.get(key) or {}
yield f"{direction}[{i}].{key}", sel.get("matchLabels", {})
coerced = [(comp, where, k, v)
for comp, p in policies(numeric).items()
for where, labels in label_maps(p)
for k, v in labels.items() if not isinstance(v, str)]
if coerced:
failed.append(f"release name 123: label values are not strings, so the API "
f"server rejects the policy: {coerced}")
else:
print("a numeric release name keeps every policy label a string")
out = render(egress_on)
pols = policies(out)
# DNS for everyone: every component addresses its peers by service name.
for comp, p in pols.items():
dns = [r for r in p["spec"]["egress"]
if {x["port"] for x in r.get("ports") or []} == {53}]
if not dns:
failed.append(f"{comp}: egress on but no DNS rule")
if not failed:
print("every policy allows DNS when egress is on")
# The API server rule goes only to what talks to it, which is the COSI
# sidecar and, on an upgrade that grows a PVC, the resize hook. No
# seaweedfs binary does - there is no client-go in go.mod - so admin
# must not be in here: it would force everyone running admin to name an
# API server address for a connection that is never made.
apiserver = {c for c, p in pols.items()
if any("ipBlock" in t for r in p["spec"]["egress"] for t in r.get("to") or [])}
expected = {"objectstorage-provisioner"}
if apiserver != expected:
failed.append(f"API server egress granted to {sorted(apiserver)}, expected {sorted(expected)}")
else:
print(f"API server egress limited to {sorted(expected)}")
# Which means egress on its own must render for a release that runs
# neither COSI nor a resize: no component of it reaches the API server,
# so nothing may demand a CIDR for one.
for label, values in {"defaults": {}, "admin": {"admin.enabled": "true"}}.items():
try:
render(dict(values, **{"networkPolicy.enabled": "true",
"networkPolicy.egress.enabled": "true"}))
print(f"egress on {label} renders without kubeApiServer.cidrs")
except subprocess.CalledProcessError as e:
failed.append(f"{label}: egress needs kubeApiServer.cidrs but nothing "
f"in the release talks to the API server: {(e.output or '')[:200]}")
# The resize hook's policy is gated on the same lookup as its Job, so
# neither is ever in a rendered set - a policy on its own would be an
# orphaned hook resource on every install, since Helm does not collect
# those. The hook annotations it carries when the lookup does hit are
# only reachable against a live cluster.
if "volume-resize-hook" in pols:
failed.append("volume-resize-hook policy rendered without its Job; the two "
"are gated on seaweedfs.volumeResizeHookCommands and have to "
"appear together")
else:
print("volume-resize-hook policy tracks its Job rather than rendering always")
# A components key that names nothing reads as "these rules are applied"
# and silently does not apply them.
expect_failure(dict(on, **{"networkPolicy.components.filerr.extraIngress[0].ports[0].port": "1"}),
'"filerr" is not a component of this chart',
"a misspelled networkPolicy.components key fails the render")
# The bucket hook is post-install, so the release manifest is already applied;
# its policy must be a plain resource that uninstall cleans up.
if "helm.sh/hook" in (pols["bucket-hook"]["metadata"].get("annotations") or {}):
failed.append("bucket-hook policy is a hook resource; post-install runs after the manifest is applied")
else:
print("bucket-hook policy is a plain release resource")
if failed:
print("\nFAIL:", file=sys.stderr)
for f in failed:
print(f" - {f}", file=sys.stderr)
sys.exit(1)
PYEOF
echo "NetworkPolicy rendering tests passed"
echo "All template rendering tests passed!"
- name: Create kind cluster
@@ -874,3 +1182,113 @@ jobs:
kubectl delete namespace "$NS"
echo "SFTP host key lifecycle tests passed"
- name: Verify install into a default-deny namespace
run: |
set -e
CHART_DIR="k8s/charts/seaweedfs"
NS="netpol"
# kind enforces NetworkPolicy out of the box since v0.24 (kindnetd
# runs sigs.k8s.io/kube-network-policies), so the cluster created for
# chart-testing above is enough and no extra CNI is needed. The two
# probes at the end fail loudly if that ever stops being true, rather
# than letting this pass vacuously.
kubectl create namespace "$NS"
kubectl apply -n "$NS" -f - <<'EOF'
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: default-deny
spec:
podSelector: {}
policyTypes: [Ingress, Egress]
EOF
# The backend address, not the kubernetes service ClusterIP: kube-proxy
# rewrites the destination, so an ipBlock has to name the real endpoint.
APISERVER=$(kubectl get endpointslice kubernetes \
-o jsonpath='{.endpoints[0].addresses[0]}' 2>/dev/null || true)
if [ -z "$APISERVER" ]; then
APISERVER=$(kubectl get endpoints kubernetes -o jsonpath='{.subsets[0].addresses[0].ip}')
fi
echo "kube-apiserver at $APISERVER"
echo "=== install with the policies on ==="
# Without them this hangs: the components cannot resolve or reach each
# other, and the post-install bucket hook waits on master and filer
# until it gives up. --wait covers the components, and helm fails the
# release if the hook Job does not finish, so a clean install is the
# assertion.
helm install np $CHART_DIR -n "$NS" --wait --timeout 8m \
--set s3.enabled=true \
--set s3.createBuckets[0].name=testbucket \
--set networkPolicy.enabled=true \
--set networkPolicy.egress.enabled=true \
--set networkPolicy.egress.kubeApiServer.cidrs[0]="$APISERVER/32"
echo "release came up and the bucket hook finished under default-deny"
FILER_IP=$(kubectl get pod -n "$NS" -l app.kubernetes.io/component=filer \
-o jsonpath='{.items[0].status.podIP}')
echo "filer pod at $FILER_IP"
# Both probes get their own all-egress policy, so the namespace-wide
# default-deny is not what decides the outcome: the only thing left in
# the way is the filer's own policy, which admits release pods only.
# Probing the pod IP keeps DNS out of it.
kubectl apply -n "$NS" -f - <<'EOF'
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: probe-egress
spec:
podSelector:
matchExpressions:
- key: probe
operator: Exists
policyTypes: [Egress]
egress:
- {}
EOF
# Only the marker line, so a probe that never produced one cannot be
# mistaken for a verdict: "pod/x created" on stdout would read as
# "not exit=0", which is how the denied case passes. busybox wget
# reports a timeout on stderr even under -q and kubectl logs returns
# both streams, so the marker has to be picked out rather than taken
# as the whole log. Empty output stays empty - the callers fail on it.
probe() {
local name=$1
local labels=$2
kubectl run "$name" -n "$NS" --image=busybox:1.36 --restart=Never \
--labels="probe=$name,$labels" --command -- \
sh -c "wget -T 5 -q -O /dev/null http://$FILER_IP:8888/; echo exit=\$?" >/dev/null
kubectl wait -n "$NS" --for=jsonpath='{.status.phase}'=Succeeded \
"pod/$name" --timeout=120s >/dev/null
kubectl logs -n "$NS" "$name" | grep '^exit=' || true
}
# A pod carrying the release labels is what the filer's policy allows.
# This has to succeed, otherwise the policies are blocking traffic they
# are supposed to permit - or nothing is enforced and the next probe
# would be meaningless.
ALLOWED=$(probe probe-allowed "app.kubernetes.io/name=seaweedfs,app.kubernetes.io/instance=np")
echo "labelled probe: $ALLOWED"
case "$ALLOWED" in
exit=0) echo "a pod with the release labels reaches the filer";;
*) echo "FAIL: the filer policy rejects a pod carrying the release labels"; exit 1;;
esac
# The same probe without those labels must not get through. Assert the
# wget failure rather than the absence of a success, so an empty log
# fails the job instead of reading as a denial.
DENIED=$(probe probe-denied "role=outsider")
echo "unlabelled probe: $DENIED"
case "$DENIED" in
exit=0) echo "FAIL: a pod outside the release reached the filer; the policy over-allows"; exit 1;;
exit=*) echo "the filer policy refuses a pod outside the release";;
*) echo "FAIL: the probe produced no result, so nothing was tested: '$DENIED'"; exit 1;;
esac
kubectl delete namespace "$NS" --wait=false
echo "default-deny namespace tests passed"