feat(frankgateway): observability + OpenBao follow-ups for the traffic-class split (IN-2511) - #401
Open
jimleitch01 wants to merge 10 commits into
Conversation
…-2511) Two observability gaps that the per-traffic-class demos depend on. JSON structured access logs (frankgateway.accessLog.jsonFormat, default off), picked up from feat/frankgateway-json-access-logs which was never merged anywhere. The Alloy-to-Loki pipeline currently regex-parses the combined format; with this, queries filter on fields directly. The template hunk moved into the config helper, since the config template is now a range over instances. Both the access-log line and the prometheus http_status labels now carry instance_name, so one Loki query or one PromQL selector can separate inway from outway from internal traffic — without it the split produces three gateways whose telemetry is indistinguishable. instance_name is emitted only when the gateway is actually split: adding it to the single-instance default would change the label set of every existing series for no information gain. The default render remains byte-identical to feature/podiumd-4.8.4.
The dashboard was carried per-environment in the deploy repo, so every municipality enabling the gateway had to copy the JSON. It belongs next to the ServiceMonitor that produces the metrics it reads. Adds a "Traffic class" variable filtering on instance_name, so one dashboard serves all three gateways once the split is enabled. Its All value is a ".*" regex, which also matches series carrying no instance_name label at all, so the dashboard is unchanged on single-instance deployments. The log panel now parses the JSON access-log format and prefixes each line with the instance, status, method and URI, which is only readable because of the structured logging added alongside this. A dashboardProviders entry accompanies the ConfigMap — without one the grafana subchart never mounts it. Set grafana.dashboardsConfigMaps.frankgateway to "" to skip the dashboard where the gateway is not deployed.
…N-2511) openbao-apikey-function.lua hard-coded the BAG field and header and carried a comment telling you which two lines to edit per route. With a third call site arriving (the ESB consumer key for IN-2543) that copy-editing stops being reasonable, so path, field and header become arguments. Also fixes a real mismatch: the function read a kv-v1 mount named "apisix", while the chart's OpenBao config Job provisions kv-v2 at openbao.configuration.kvPath (default "secret"). The read path and response shape differ between the two engine versions, so anyone following the chart defaults got a silent miss. It now defaults to kv-v2 to match the chart, with kv-v1 still selectable via OPENBAO_KV because the jim00 rig was built against a hand-made kv-v1 mount and has not been migrated. Address and mount are environment-driven rather than hard-coded, and a missing OPENBAO_TOKEN is now reported instead of silently producing an unauthenticated request. Error logging deliberately omits the response body, which can echo the requested secret path into the access log. Fail-open behaviour is kept and now stated: on any error the header is simply not set and the upstream rejects the call. A gateway that 500s on an OpenBao blip would turn a secret-store hiccup into an outage.
Adds frankgateway.instances.<key>.routes.extra — routes keyed by id, rendered into the same seeding ConfigMap as the shipped files. The chart can only ship routes that are the same everywhere. An inway route has to match the environment's own public hostname, so it is per-gemeente by nature and there was nowhere to put it. jim00 worked around that by maintaining its own copy of the seeding script and its own route directory, which is exactly the kind of local patching IN-2511 exists to eliminate. A values entry overrides a shipped file of the same id, so an environment can also adjust a chart route without forking the file. The example in values.yaml points out that internal routes want pass_host: rewrite with upstream_host set to the application's public FQDN: ZGW APIs emit absolute self-referencing URLs, and callers cannot resolve cluster-local ones. Getting that wrong produces 403s on writes that look unrelated to the routing change. Default render remains byte-identical to feature/podiumd-4.8.4.
…_status (IN-2511) The traffic-class label was being added through the APISIX prometheus plugin's `metrics.http_status.extra_labels`. That hook only exists for http_status, so apisix_http_latency_bucket, apisix_bandwidth, the nginx connection gauges and `up` were all left without it. A dashboard filtering by class would render the request-rate and status panels correctly and show empty latency, bandwidth and connection panels — the failure mode being blank graphs rather than an error, which reads as "no traffic" instead of "wrong query". Stamp it at scrape time with a ServiceMonitor relabeling instead: one rule covers every series the target exposes, including `up`. The plugin extra_label is removed so there is a single mechanism rather than two that have to agree. Unchanged: the label is still only emitted for split instances, so the pre-split default instance's series keep their existing label set. Verified by re-rendering the chart with default values — byte-identical to feature/podiumd-4.8.4 after normalising the per-render random values. The dashboard's remaining unfiltered panels (NGINX connections, scrape target up) and the Loki logs panel now honour the Traffic class variable too, which they could not do before this change. 🤖 Generated by Claude Code with help from Jimbo
A snapshot version built from a long branch name pushes chart-name+version past the 63-character label limit. The stock helm scaffold truncates and then trims only a trailing "-", so a cut landing on a "." leaves an invalid label value and the entire release fails at admission: Job.batch "openbao-db-schema" is invalid: metadata.labels: Invalid value: "podiumd-4.8.4-feature-IN-2511-frankgateway-chart-followups-4.8.": a valid label must ... start and end with an alphanumeric character The error names whichever object was applied first — here a pre-upgrade hook Job — so it reads as a problem with that Job rather than with the version string, which is what makes it worth fixing rather than working around by shortening branch names. Strip every trailing non-alphanumeric after the truncation instead. Note regexReplaceAll takes (regex, input, replacement), so the input cannot be piped into it: `X | regexReplaceAll "pat" ""` binds X to the REPLACEMENT and runs against an empty input, silently emptying every chart label. Bound to a variable and passed positionally. Default render verified byte-identical to feature/podiumd-4.8.4; rendering with the long snapshot version now yields a 62-character label ending in an alphanumeric. 🤖 Generated by Claude Code with help from Jimbo
The previous commit fixed podiumd.chart, but the release still failed with the same invalid-label error from a different object: Job.batch "create-required-catalogi-job" is invalid: metadata.labels: Invalid value: "podiumd-4.8.4-feature-IN-2511-frankgateway-chart-followups-4.8." create-required-catalogi.yaml and create-required-objecttypen.yaml are podiumd templates, but they label their objects with the openzaak and objecttypen subcharts' label helpers. Those helpers read `.Chart`, which when called from a podiumd template is PODIUMD's chart — so they build the label from podiumd's long version while carrying the same stock truncation bug. Fixing podiumd.chart could never reach them. Redefine both helper names in the parent chart. Helm template definitions are global and the parent is loaded after its dependencies, so these replace the subchart versions everywhere, including inside the subcharts' own templates. That is safe: the only behavioural difference is stripping a trailing "." or "_" in addition to "-", which can only happen when the truncation actually cuts something off, and the subcharts' own versions are far too short for that. Verified two ways. The default render is byte-identical to feature/podiumd-4.8.4. Rendering with the long snapshot version and auditing EVERY distinct helm.sh/chart value in the output — rather than just the object that happened to fail first — shows all 24 are within 63 characters and end in an alphanumeric. 🤖 Generated by Claude Code with help from Jimbo
…mespace Enabling the inway on jim00 took inbound OpenZaak down. The chart default for networkPolicies.ingressNamespace is `nginx-gateway`, which is where NGINX Gateway Fabric 2.x runs its CONTROL plane. The pod that actually opens connections to the inway is the per-Gateway DATA plane, which runs in the Gateway's own namespace (`ingress-basic` there). The two are easy to confuse because only the control plane carries the product name, and the failure gives no clue: the policy fails closed, the packet never arrives, and neither side logs anything — so it presents as a TLS or connectivity fault rather than a policy one. The chart cannot infer the Gateway's namespace, so the value stays as it is and is now documented as something to set per environment, with the command to check it. Also recorded on the template that these policies render for every classified instance as soon as networkPolicies.enabled is true. An environment that already has it on ships a policy together with any newly enabled instance — so "add the instance" and "restrict the instance" stop being separable steps unless that is planned for. Comments only; default render verified byte-identical. 🤖 Generated by Claude Code with help from Jimbo
…taught The traffic-class document described the design; this adds what went wrong putting it into service on jim00, because none of it is inferable from the values file and each item cost real debugging time. Corrects the diagram: the NGF pod that connects to an inway is the per-Gateway DATA plane, which runs in the Gateway's namespace. Only the control plane lives in ns nginx-gateway. The old label named the control-plane namespace, which is precisely the confusion that caused an outage. New sections: - Anything the deploy tooling does "to the gateway" must be done per instance. Four scripts assumed one gateway, and every failure surfaced somewhere other than the change that caused it — tabulated with the symptom for each, since the symptom is the part that misleads. - networkPolicies.ingressNamespace is the ingress data plane's namespace, and fails closed with no log line on either side when wrong. - Enabling networkPolicies is sticky: a later instance ships a policy with it, which removes the ability to verify an instance first and restrict it second unless planned for. - An ingress controller may not notice a retargeted ExternalName, so a rollback relying on that can appear to succeed while changing nothing. - ZGW URL identity: pass_host preserves the host but not the scheme, four attempted header fixes were all discarded by frank-gateway:104, and since those URLs are stored the safer approach is to keep the URL and change what the hostname resolves to. - The split is not a latency optimisation — measured under 1ms, sometimes negative at p99. The mermaid diagram could not be render-validated locally (mermaid-cli needs Chromium, absent here); the edit is a single node label and its quoting was checked by hand. Default chart render verified byte-identical. 🤖 Generated by Claude Code with help from Jimbo
…host collisions Dashboards are for debugging and testing, so turning them on and off should be one line and should not be able to go quietly wrong. The mechanism already worked; this makes it discoverable and adds the guard it was missing. Verified behaviour, now documented in values.yaml and the traffic-class doc: - `frankgateway.dashboard.enabled: false` turns every instance's dashboard off in one line; a per-instance value overrides it in either direction. With it off, nothing renders — no dashboard, oauth2-proxy, shim, ingress or Keycloak client. - `dashboard.enabled: true` with `auth.enabled: false` renders the dashboard alone. No hostname, no certificate SAN, no Keycloak client — which is what normally makes enabling one mid-investigation slow. Reached by port-forward on 9000. Documented as a debugging mode only: without oauth2-proxy in front, nothing but the Admin API key stands between the cluster and the GUI. - Nothing depends on a dashboard. The route-seeding Job uses the Admin API directly, so a class without a GUI is still fully managed and observable. New guard: two enabled instances may no longer share a dashboard hostname. `hostname` lives in the shared block, so an instance that does not override it INHERITS it — silently rendering two oauth2-proxy Deployments and two ingress routes claiming one host. Whichever the ingress resolves last wins, so one dashboard shows another class's routes and the split looks broken in a way that points at etcd rather than at values. The chart's own ci/lint-values.yaml hits this the moment a second instance is enabled, which is how it was found. The required-hostname failure now also names the debugging alternative and the port-forward command, instead of only stating what is missing. Corrected one inaccuracy while documenting: turning a dashboard off does NOT remove the `frankgateway-dashboard-oidc-secret` key, which is always emitted because the realm import Job references it unconditionally. Per-instance keys do disappear with their client. Harmless either way, but worth stating correctly. Default render verified byte-identical; ci/split-values.yaml still renders. 🤖 Generated by Claude Code with help from Jimbo
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #400. Contributes upstream the things jim00 was carrying deploy-side, plus the observability the per-traffic-class demos depend on.
JSON access logs
Picked up from
feat/frankgateway-json-access-logs(b1c5036), which was never merged anywhere. The Alloy-to-Loki pipeline currently regex-parses the combined format; with this, queries filter on fields. The template hunk moved into the config helper, since the config template is now a range over instances.Per-instance telemetry
Both the access-log line and the prometheus
http_statuslabels now carryinstance_name. Without it the split produces three gateways whose metrics and logs are indistinguishable — one PromQL selector or Loki query can now separate inway from outway from internal.Emitted only when the gateway is actually split: adding the label to the single-instance default would change the label set of every existing series for no information gain.
Grafana dashboard shipped in the chart
Was carried per-environment in the deploy repo, so every municipality enabling the gateway had to copy the JSON. Now lives next to the ServiceMonitor that produces its metrics.
Adds a "Traffic class" variable filtering on
instance_name; its All value is.*, which also matches series carrying no such label, so the dashboard is unchanged on single-instance deployments. The log panel parses the JSON format and prefixes each line with instance/status/method/URI.A
dashboardProvidersentry accompanies the ConfigMap — without one the grafana subchart never mounts it.OpenBao secret fetch — generalised, and a real bug fixed
openbao-apikey-function.luahard-coded the BAG field and header, with a comment telling you which two lines to edit per route. A third call site is arriving (the ESB consumer key, IN-2543), so path, field and header became arguments.It also fixes a genuine mismatch: the function read a kv-v1 mount named
apisix, while the chart's OpenBao config Job provisions kv-v2 atopenbao.configuration.kvPath(defaultsecret). The read path and the response shape differ between engine versions, so anyone following the chart defaults got a silent miss. Now defaults to kv-v2 to match the chart, with kv-v1 still selectable viaOPENBAO_KVsince the jim00 rig was built against a hand-made kv-v1 mount.Additionally: address and mount are environment-driven; a missing
OPENBAO_TOKENis reported rather than silently producing an unauthenticated request; error logging omits the response body, which can echo the requested secret path into the access log. Fail-open behaviour is kept and now stated — a gateway that 500s on an OpenBao blip would turn a secret-store hiccup into an outage.Verification
feature/podiumd-4.8.4after every commit.instance_namein both the prometheus labels and the access-log format, per instance.helm lintclean on bothpodiumdandmonitoring-logging; the dashboard ConfigMap and its provider both render.luac -p.🤖 Generated by Claude Code with help from Jimbo