Repository navigation
feat(chart): enable Edge v3.6.0 features and pin to v3.6.0 (chart 0.6.0) - #13
Conversation
…v capture Adds the endpoints resource to the ClusterRole + informer gate (has_endpoints), and exposes configMapCapture and envCapture (hash-by-default value capture with clearText / redactKeyPatterns / captureCap knobs) in values.yaml + config.json.
7025f15 to
446d883
Compare
The chart has never pinned an Edge version: image.tag has been "latest"
since the repo was created, and appVersion sat at a stale 2.1.0 that only
ever reached the app.kubernetes.io/version label. Pin it to the release
whose features the rest of this branch enables.
image.tag is the load-bearing edit — edge-proxy inherits it through
nofire-edge.edgeProxy.image, so one value pins both deployments. The tag
carries the "v" prefix because edge's release workflow computes
${GITHUB_REF#refs/tags/}, which strips refs/tags/ but not the v; the
registry has nofireai/edge:v3.6.0 and no bare 3.6.0. appVersion is set to
the same v-prefixed string so the tag|default .Chart.AppVersion fallback
stays resolvable if anyone clears image.tag.
The chart version bump is mandatory, not cosmetic: the release workflow
triggers only on paths: [Chart.yaml] and skips every publish step when a
release for the current version already exists. Without it this branch
would merge and ship nothing. Minor rather than patch, matching the
repo's feature convention (0.4.0 -> 0.5.0, 0.3.0 -> 0.4.0).
Also pins the production values example, which would otherwise override
the default straight back to a floating tag, and the standalone raw
manifest for consistency.
pullPolicy stays Always so overriding the tag back to latest still works.
stheppi
changed the title
helm-unittest covers ConfigMap shape and render failures. tests/render/run.sh renders each case under tests/render/cases, checks config.json with jq, and with EDGE_SRC set also loads it with the Edge's own config loader (built via go build -overlay, so the Edge checkout is never written to). tests/ and ci/ are excluded from the packaged chart. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
The template dropped config.kube.namespaceFilter, so namespace scoping set in values never reached the Edge. Write it whenever the map is non-empty. Before: cases ns-filter-allow/deny/none FAIL: .kube.namespaceFilter missing Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Mirror the Edge's validation (config.go:708-725) so a bad filter fails at helm install time instead of crashlooping the pod. Before: cases ns-filter-fail-* FAIL: render succeeded, expected failure Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Written whenever set, including "0s"; an explicit null counts as unset (Helm keeps the nil for keys not in the chart defaults, so it is checked). Before: cases ttl-1h, ttl-zero FAIL: .netobs.edgeExistenceTtl missing Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Written when set, including [] (which replaces the Edge default list); an explicit null counts as unset. Before: cases exclude-ns, exclude-ns-empty FAIL: .netobs.excludeNamespaces missing Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
services.address alone failed helm template (nil pointer on the missing
tls/compression/handshake maps) and the old fallbacks (empty durations,
unset maxConns) would have been rejected by the Edge anyway. Scalars are
now emitted only when set; services-full guards the previously supported
full config.
Before: case services-address-only FAIL: render failed: nil pointer evaluating interface {}.enabled
Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
The objects were rebuilt with ""/false/-1 fallbacks for every field, so a
partial config overrode Edge defaults with invalid zero values. Now only the
keys the user set are written, and an object is omitted when none are set
(never null); the Edge merges the partial object into its defaults.
Before: cases services-tls-enabled, services-handshake-timeout,
services-empty-objects FAIL: .services.tls == {"enabled":true} was false (all fields filled)
Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Covers compression.enabled=false, compression.level=0, workers=0 and empty
cipherSuites. No template change: the pickSet-based rendering from the
previous commit already handles them.
Against the pre-fix template: compression.enabled=false rendered
{enabled:false,type:"",level:-1}; level: 0 was rewritten to -1 by
`default -1`; falsy-scalars compression rendered null.
Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
…es defaults Add commented config.kube.namespaceFilter, netobs.edgeExistenceTtl and netobs.excludeNamespaces examples (noting that excludeNamespaces replaces the Edge default). Correct the config.services comments to the Edge v3.6.0 defaults. The values-examples render case uncomments the examples and checks they render and pass the Edge's config loader. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Installs the chart on a throwaway kind cluster and checks the rendered ConfigMap, that the Edge accepts it and rolls out, that its startup log shows the namespace filter, that an upgrade to deny changes checksum/config and rolls the pod, and that an invalid filter fails at render. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Values given as strings (--set-string, quoted YAML) rendered as JSON strings and crashed the Edge's decoder, e.g. workers "8". Each key is now coerced per type (int64, bool, string, list) with a clear failure for values that cannot be, and objects are built without the float64 fromJson round-trip. An empty services.address is treated as unset (Edge default :6000) as before, and non-map tls/compression/handshake fail with a clear message. Before: services-quoted-scalars FAIL (workers rendered "8"), services-empty-address FAIL (address "" written, disabling DNSTap), services-fail-tls-not-map / -tls-false FAIL (raw Go template error) Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
The namespaceFilter guard accepted names the Edge rejects (uppercase, underscores, non-strings, empty, over 63 chars), which crashlooped the pod. Each entry is now checked as a DNS-1123 label. Wrong-typed namespaceFilter, excludeNamespaces and edgeExistenceTtl fail with a clear message instead of a Go template error or a bad config, and null mode/namespaces count as unset. Expect-fail patterns and unittest patterns now match their own guard's wording so a test cannot pass on the wrong guard. run.sh normalises a relative EDGE_SRC. Before: ns-filter-fail-uppercase/-nonstring/-empty-entry/-too-long FAIL (render succeeded), ns-filter-null-fields FAIL (nulls rendered), exclude-ns-fail-string and ttl-fail-number FAIL (render succeeded) Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
This fixes code introduced by PR #13, not by this change. helm upgrade --reuse-values from 0.5.2 reuses 0.5.2's values, which have no config.configMapCapture or config.envCapture maps, so the template hit a nil pointer. Default-values output is byte-identical to before. run.sh gains a replace-values marker: the case values.yaml replaces the chart defaults, which is what --reuse-values does. Before: case reuse-values-0.5.2 FAIL: nil pointer evaluating .Values.config.configMapCapture.clearText Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
After helm upgrade, pick the pod by its checksum/config annotation instead of reading logs from every pod of the release, so a leftover old pod cannot satisfy the deny assertion. Select the deployment by chart name too, and list yq in the dependencies. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
excludeNamespaces [1, 2] rendered and the Edge then failed to unmarshal a number into a string. Entries are now type-checked (strings only). Names are deliberately not validated further: the Edge accepts e.g. "Prod_1" here, which exclude-ns-uppercase documents. Before: case exclude-ns-fail-number FAIL (render succeeded, expected failure); unittest 'fails on a non-string excludeNamespaces entry' failed Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
"not-a-duration" rendered and the Edge rejected it at startup. The value is now matched against Go duration syntax (sign, fractions, compound 1h30m, ns/us/µs/ms/s/m/h, and plain "0") and fails the render otherwise. The ttl-ok-* cases pass the Edge loader, including "-1h" which it accepts. Before: ttl-fail-invalid and ttl-fail-missing-unit FAIL (render succeeded); two unittest cases failed Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
namespaceFilter {node: allow} (a typo for mode) rendered nothing and silently
left every namespace in scope. Any key other than mode and namespaces now
fails the render and names the key.
Before: cases ns-filter-fail-unknown-key and -valid-mode FAIL (render
succeeded); unittest 'fails on an unknown namespaceFilter key' failed
Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
run.sh merged stderr into the manifest it parsed, so a helm warning on a successful render (e.g. a symlink in the chart) broke yq. stdout and stderr are now captured separately; stderr is matched only for expect-fail cases and printed on failure. replace-values cases copy only chart files (no .git, node_modules or tests). harness_test.sh covers this with a symlink in a temp copy of the chart and runs in CI, which now also triggers on push to main. Before: harness_test.sh FAIL: yq 'bad file -: mapping values are not allowed' (helm's 'found symbolic link in path' warning parsed as YAML) Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
fix(chart): render namespace scoping and netobs keys into the Edge config
|
@claude-fleet review |
|
Review task queued. Task: aed92319-0134-45ce-8dae-8792746d8353 |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: two should_fix findings. The chart work itself is solid — the typedPick helper, the render-case harness and the fail-at-render guards are a real improvement over 0.5.2, and the revert check confirms the new cases are actually tied to their fixes.
Verified:
endpointsis in both gates:values.yaml:197(config.kube.resources) and the core-group rule intemplates/rbac.yaml:10.- The pin resolves in both deployments.
helm template t . --set edgeProxy.enabled=truerendersnofireai/edge:v3.6.0andnofireai/edge-proxy:v3.6.0; with--set image.tag=""thetag | default .Chart.AppVersionfallback intemplates/_helpers.tpl:68and:122still yieldsv3.6.0, so thev-prefixedappVersionclaim holds. - No
latestremains invalues.yaml,examples/,manifests.yamlorREADME.md. Chart.yamlis required for a publish:.github/workflows/release.ymlis gated onpaths: ['Chart.yaml'].- The
--reuse-valuesnil-pointer fix attemplates/configmap.yaml:113and:124is real, andtests/render/cases/reuse-values-0.5.2/reproduces it with a full 0.5.2 values file. .helmignore:34-36keepstests/andci/out of the packaged chart.
Checks run:
tests/render/run.sh→53 passed, 0 failedtests/render/harness_test.sh→ PASShelm lint .→ 0 failed (only the pre-existing "icon is recommended")- Revert check: deleted the unknown-key guard at
templates/configmap.yaml:31-35, rerantests/render/run.sh ns-filter-fail-unknown-key→ bothns-filter-fail-unknown-keyandns-filter-fail-unknown-key-valid-modefail withrender succeeded, expected failure. Restored;git status --porcelainclean.
Smaller things:
manifests.yaml:155— theresourcesarray has a trailing comma before], so the embeddedconfig.jsonis not parseable (jqrejects it). Pre-existing, from the initial upload, but this PR edits the file.- The description covers commits
446d883and7e8108eonly. The other 20 — the render harness, the CI workflow,namespaceFilter,excludeNamespaces,edgeExistenceTtl, theservicestype coercion — are not mentioned, so a reader of the PR body will not know this branch changes howconfig.servicesscalars are typed. values.yaml:299-322repeats theclearText/redactKeyPatterns/captureCapexplanation nearly verbatim for both capture blocks. Two copies to keep in sync; acceptable now, worth folding into one if a third capture block appears.
Not checked:
- tests/e2e/kind.sh — not executed; needs docker and a kind cluster, and the review workspace has no container runtime.
- tests/render/edgecheck/main.go and the EDGE_SRC path of run.sh — no Edge checkout available, so the rendered configs were not run through config.LoadConfigFromReader.
- helm unittest tests/configmap_test.yaml — the helm-unittest plugin is not installed in this workspace ('unknown command "unittest"'). Assertions were read, not executed.
- Whether the Edge recognises "endpoints" as a valid informer name in config.kube.resources, and the semantics of captureCap 0 — both live in the Edge source, which is not reachable here.
- The values.yaml comments asserting Edge defaults (DNSTap :6000, netobs excludeNamespaces [kube-system, monitoring], edgeExistenceTtl 2h, compression gzip) — these are claims about Edge code outside this repo.
- Docker Hub tag existence for nofireai/edge:v3.6.0 and nofireai/edge-proxy:v3.6.0 — no registry access from this workspace.
- The ~110 tests/render/cases/* data files were exercised as a suite rather than read individually; I read defaults, values-examples and reuse-values-0.5.2.
Findings outside the diff:
- examples/production-values.yaml:53 (should_fix): The documented production example overrides
config.kube.resourceswith its own list that has noendpointsentry, which leaves the headline feature of this PR inert for anyone who follows the example.
Failure: helm install -f examples/production-values.yaml grants the ClusterRole endpoints verbs (templates/rbac.yaml:10) but Helm replaces lists rather than merging them, so the informer list stays at the 15 entries in examples/production-values.yaml:54-68. getEndpointsPresence never runs, has_endpoints stays nil, and brain never flags selector/reachability faults — the exact state the PR describes as "safe, but inert".
Evidence: examples/production-values.yaml:53-68 vs values.yaml:194-209, which has endpoints at :197.
Fix: add - "endpoints" to the example list.
- manifests.yaml:80 (nit):
manifests.yamlis pinned tov3.6.0but neither of its twoendpointsgates is opened, so the standalone manifest ships the v3.6.0 image with the v3.6.0 feature off.
Failure: applying manifests.yaml gives a ClusterRole without endpoints (manifests.yaml:80) and a config.json resources list without it (manifests.yaml:141-155), so has_endpoints stays nil exactly as before the pin.
The PR body notes this file is outside the chart render path; if it is kept in sync enough to pin the tag, the two feature gates belong with it. If it is not meant to be kept in sync, say so in a comment at the top of the file.
Task aed92319-0134-45ce-8dae-8792746d8353 on pmoust-mbp, model claude-opus-5, effort medium.
| {{- $cmc := .Values.config.configMapCapture | default dict }} | ||
| "configMapCapture": { | ||
| "clearText": {{ $cmc.clearText | default false }}, | ||
| "captureCap": {{ $cmc.captureCap | default 4096 }}, |
There was a problem hiding this comment.
should_fix: configMapCapture and envCapture are the only config blocks that skip nofire-edge.typedPick, so a string-typed scalar renders straight into config.json unquoted and produces a file the Edge cannot parse.
Failure: config.envCapture.captureCap: "4k" in a values file renders "captureCap": 4k, — helm install succeeds and the Edge crashloops on an unparseable config. Same for --set-string config.configMapCapture.clearText=yes → "clearText": yes,. I confirmed both are rejected by jq -e .. Contrast config.services.workers: "4k", which this PR made fail the render with config.services.workers must be an integer (got 4k) — commit 1a124b1 fixed exactly this class of bug and these two blocks were missed.
Second defect on the same lines: | default 4096 discards an explicit captureCap: 0. helm template t . --set config.configMapCapture.captureCap=0 renders "captureCap": 4096. The typedPick contract ("a key is kept when present, even if falsy", templates/_helpers.tpl:144) is the behavior you want here too.
Fix: route both blocks through nofire-edge.typedPick with (dict "clearText" "bool" "captureCap" "int" "redactKeyPatterns" "strlist") and emit the resulting dict with toJson, as nofire-edge.servicesJson already does. That also replaces the raw range can't iterate over foo you currently get from --set-string config.configMapCapture.redactKeyPatterns=foo, and the can't evaluate field clearText in type int64 from a scalar configMapCapture.
There was a problem hiding this comment.
Fixed in ca9cb0f. Both blocks now go through typedPick (bool/int/strlist) in a new nofire-edge.captureJson. Unknown keys and invalid redactKeyPatterns regexes also fail the render. The Edge only logs and skips a bad pattern, which with clearText: true would send the value in clear text. captureCap: 0 now renders as 0, although that changes nothing at runtime: the Edge maps <= 0 to 4096 (informers/configmaps.go:166, pods.go:491). Moving captureCap to typedPick showed that typedPick rejected whole numbers from values files (1048576 → 1.048576e+06). That's fixed in e119c23.
| name: CI | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
nit: The PR description says "this repo has no PR-triggered CI — release.yml fires on push-to-main and workflow_dispatch only, so no checks will report here", but this diff adds a workflow with on: pull_request.
The description is the stale side: .github/workflows/ci.yaml:4 triggers on every pull request and runs helm lint, helm unittest, tests/render/run.sh and tests/render/harness_test.sh. Update the Testing section so a reviewer does not skip checks that will in fact report.
There was a problem hiding this comment.
Right, the description was the stale side. The Testing section is updated: CI runs on every PR.
A number read from a values file is a float64, and toString prints 1048576 as 1.048576e+06, so the integer check rejected it. Accept whole-number floats directly; fractions and strings still go through the regex check. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
The capture blocks were rendered by hand, so a string captureCap such as 4k produced invalid config.json and a mistyped key was silently ignored. The Edge skips an invalid redactKeyPatterns regex with only a warning, which with clearText sends the value in the clear. Render both blocks through typedPick, reject unknown keys and bad regexes at install, and keep an explicit captureCap of 0. The output is now compact JSON, so checksum/config changes on upgrade. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Helm replaces lists, so the example's kube.resources dropped the endpoints informer that the chart default enables. Cover the example and the default ClusterRole with unit tests. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
The standalone manifest pinned Edge v3.6.0 but left the endpoints gates closed: no ClusterRole rule and no informer entry. Its resources list named "services", which is not an Edge informer (k8services is), and a trailing comma made the embedded config.json invalid JSON. Add tests/manifests/check.sh and run it in CI. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
Install with explicit configMapCapture and envCapture values (captureCap 0, and 1048576 as a whole-number float) and check the rendered ConfigMap, the Edge's startup log, and that its service account can list endpoints. A non-integer captureCap must be rejected at render. Signed-off-by: stheppi <stefan.bocutiu@gmail.com>
|
2 participants
Activates the merged Edge v3.6.0 features that are inert without chart changes, then pins the chart to that release and publishes it as chart 0.6.0.
has_endpoints (edge#112 / brain#1118)
endpointstoconfig.kube.resources(informer gate —config.jsonoverrides the binary's compiled default).endpoints(get/list/watch) to the ClusterRole intemplates/rbac.yaml(RBAC gate).Without both,
getEndpointsPresenceis RBAC-denied →has_endpointsstaysnil→ brain treats it as unknown and never flags selector/reachability faults (safe, but inert).ConfigMap capture (edge#117 / brain#1151)
config.configMapCapture(clearText/redactKeyPatterns/captureCap) so the hash-by-default + opt-in clear-text policy is configurable.Pin to Edge v3.6.0 (added after initial review)
values.yaml—image.tag: "latest"→"v3.6.0". This is the load-bearing edit; edge-proxy inherits it vianofire-edge.edgeProxy.image, so one value pins both deployments.Chart.yaml—version: 0.5.2→0.6.0,appVersion: "2.1.0"→"v3.6.0".examples/production-values.yaml— pinned; it previously overrode the default straight back to a floating tag.manifests.yaml— pinned for consistency. Note this is the standalone raw manifest, outside the chart render path, so it isn't part of the pin proper.Three things worth a reviewer's attention:
The chart has never pinned a version.
image.taghas been"latest"since the repo was created, andappVersionsat at a stale2.1.0that only ever reached theapp.kubernetes.io/versionlabel. So this is floating → pinned, a behavior change for every consumer, not a routine bump. Anyone who relied onlatestto auto-track Edge releases now needs a chart upgrade per release — that's the intent, but it's a change.The tag carries a
v.edge/.github/workflows/release.ymlcomputesversion=${GITHUB_REF#refs/tags/}, which stripsrefs/tags/but not thev. The registry hasnofireai/edge:v3.6.0andnofireai/edge-proxy:v3.6.0(both published 2026-08-10); there is no bare3.6.0tag in either repo.appVersionis set to the samev-prefixed string so thetag | default .Chart.AppVersionfallback stays resolvable if anyone clearsimage.tag— this deviates from the bare-semver form used inc580052, deliberately.The
Chart.yamlbump is mandatory, not cosmetic. The release workflow triggers only onpaths: ['Chart.yaml']and skips every publish step when a release for the current version already exists. As originally scoped, this PR touched noChart.yamland so would have merged and shipped nothing.0.6.0is free (gh release listshows onlynofire-edge-0.5.2); minor rather than patch matches the repo's feature convention (0.4.0 → 0.5.0, 0.3.0 → 0.4.0).pullPolicystaysAlwaysso overriding the tag back tolateststill behaves.Also in this branch
Beyond the two feature commits, the branch carries the work that came out of review:
tests/render/run.shrenders each case intests/render/cases/and checks it with jq. A case can also expect the render to fail..github/workflows/ci.yamlruns lint, helm-unittest, the render cases, the harness self-test andtests/manifests/check.shon every PR.tests/e2e/kind.shinstalls the chart into a kind cluster.config.json:kube.namespaceFilter,netobs.excludeNamespacesandnetobs.edgeExistenceTtl. Each fails the render on bad input: unknown keys, invalid namespace names, a non-duration TTL.config.servicestype coercion. This changes behavior. Scalars are now coerced to the types the Edge expects, so--set-stringvalues work. A value of the wrong type fails the render instead of producing aconfig.jsonthe Edge can't parse. Partialtls/compression/handshakeobjects render, and falsy values are kept.maxConns: 1048576failed withgot 1.048576e+06.configMapCapture/envCaptureare type-checked. Unknown keys and invalidredactKeyPatternsregexes fail the render. The Edge only logs and skips a bad pattern, and withclearText: truethat would send the value in clear text.captureCap: 0is no longer rewritten to 4096; the Edge treats<= 0as 4096 anyway.--reuse-valuesfrom 0.5.2 no longer hits a nil pointer on the capture blocks.examples/production-values.yamlnow includesendpoints. Helm replaces lists rather than merging them, so the example was leaving the feature off.manifests.yamlopens bothendpointsgates and replaces the unknown informer nameserviceswithk8services. It also drops a trailing comma that made its embeddedconfig.jsoninvalid JSON.Upgrade note: the capture blocks now render as compact JSON. Their content is unchanged, but
checksum/configchanges, so Edge pods restart on upgrade to 0.6.0.Known gaps, not addressed here:
values.yamland the production example still list"services", which isn't an Edge informer name. The Edge ignores it, so it does no harm.servicesJsonstill drops the example'snameLabels,versionLabelsandprovider, as it did onmain.Testing
CI runs on every PR. At
8b79007,gitleaksandtestboth pass.helm lint .: 0 failed. The only message is the existing "icon is recommended" info.helm unittest .: 23 of 23 pass.tests/render/run.sh: 66 of 66 pass. WithEDGE_SRCpointing at an Edge v3.6.0 checkout, all 66 rendered configs also load through the Edge's own config loader.tests/render/harness_test.shandtests/manifests/check.shpass.tests/e2e/kind.shpasses on a local kind cluster. The Edge rolls out, its startup log showsendpointsin its resource list, and its service account can list endpoints. The capture blocks reach the ConfigMap as set, andcaptureCap=4kis rejected at render.helm template t . --set edgeProxy.enabled=truerendersnofireai/edge:v3.6.0andnofireai/edge-proxy:v3.6.0. With--set image.tag="", it falls back toappVersionand still rendersv3.6.0.On merge,
release.ymlshould create the releasenofire-edge-0.6.0and updategh-pages/index.yaml.Refs NOFireAI/engineering#799, NOFireAI/engineering#811
Refs NOFireAI/edge@v3.6.0 · changelog entry in NOFireAI/docs#30