Feat/unified configuration - #3789
Open
marvin-hansen wants to merge 54 commits into
Open
Conversation
Adds the add-unified-config-and-secret-managers change proposal and the environment variable audit it is built on. Specs only -- no implementation. The audit (openspec/notes/env-var-inventory.md) measured the current state from source: .bazelrc forwards 45 environment variable names while the integration-test code reads 58. Twenty-six are read but never forwarded and silently take a default, including the TLS-affecting PGSSLTARGETNAME and SRQL_TEST_DATABASE_SSLMODE. Thirteen are forwarded but unread. One setting has three spellings that can never meet: .bazelrc forwards NATS_CA_FILE, Go reads NATS_CACERTFILE, the Elixir e2e test uses NATS_TEST_CERT_DIR. And config/test.exs reads fourteen alias pairs on adjacent lines. The failure mode is silence: nearly every reader has a default, so a missing value produces a working connection with the wrong settings rather than an error. The proposal moves the contract into git and makes each layer mechanically checkable: - a protobuf schema plus one text-format instance per environment as the committed ground truth, using explicit presence and *_UNSPECIFIED enum sentinels so a missing value cannot decode to a type default - validation as a committed rule set over a closed predicate vocabulary, each rule carrying an evaluation phase and a stable violation code - native managers in Rust, Go and Elixir -- no FFI or NIF, since protobuf codegen already provides the cross-language contract - conformance vectors asserting violation identity, plus property-based tests over the predicate laws - least privilege by construction: config files are Bazel targets declared as data, and components declare the logical secret names they may request - SERVICERADAR_ENV as the single boundary variable, selecting both the config file and the secret provider Three claims in the audit were refuted by adversarial review before landing, and the corrections are recorded in the note: NATS_KEY_B64 is pinned by //:buildbuddy_cache_proxy_config_test, SERVICERADAR_TEST_ADMIN_URL is live under bazel run, and TEST_CNPG_PASSWORD is likewise pinned. Nothing in the inventory is safe to delete on the strength of a read-scan alone. openspec validate --strict passes: 10 requirements, 27 scenarios. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Records the last phase-1 decisions on the unified-configuration proposal. Phase 1 is now complete; nothing blocks phase 2. Committed .textproto is the authored ground truth; a protoc build action compiles each instance to a binary message and every manager loads the binary. This matters because no implementation can parse text format: only Go reads textproto natively, prost does not, and Elixir's :protobuf has no parser at any version (0.17.0 added Protobuf.Text, encode-only). Moving the parse into a build action removes the requirement from all three at once. A round-trip test pins each generated binary to its committed source. Also records, with reasoning, the two rejected alternatives: declaring configuration programmatically in Rust (turns config review into code review, blocks non-Rust authors of onprem instances, and lets a builder read the environment), and Starlark-generated instances (held in reserve as the escape hatch if overlays later justify computation). Verified against a real Mix release: runtime.exs can call loaded modules but no application is started; Application.ensure_all_started there is a trap because Config.Provider restarts the VM, so anything started runs twice against pre-runtime config; Application.app_dir/2 works at boot and __DIR__ does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…ings Implements phases 2-3 of add-unified-config-and-secret-managers, and adds the rules_elixir/rules_erlang consolidation specification that the work produced. SCHEMA (config/proto/) config.proto defines EnvironmentConfig over database, NATS and core settings, including the CORE_* family that Decision 11 requires in scope. Two rules govern every field, and both exist to prevent the silent defaulting this replaces: every field uses proto3 explicit presence, and every enum reserves 0 as an *_UNSPECIFIED sentinel that validation rejects. Without them a missing port decodes to 0 and a missing TLS mode to whatever is = 0 -- the same disease relocated into the decoder. The connecting role and the owning role are separate config fields, so nothing needs to recover an identity by parsing a credential-bearing DSN. RULE SET (config/rules/) rules.proto expresses validation as data over a closed predicate vocabulary. The predicate is a oneof rather than an enum plus a separate params message, so a predicate and its parameters cannot disagree. ruleset.textproto carries 37 rules over every schema field; fixtures.textproto carries 10 conformance cases that double as negative fixtures, covering each predicate kind plus cascading, violation ordering, and both sides of scope. config/SEMANTICS.md is normative: the verdict type, per-predicate totality, cascading (an absent required field yields exactly one violation), exhaustive evaluation, a total order on (field_path, code), and phase/scope gating. It also states plainly what a verified engine cannot do -- detect that the rule set forgot a rule. BUILD (config/defs.bzl) environment_config, rule_set and fixture_set compile each committed .textproto to binary via protoc. No implementation parses text format: only Go reads it natively, prost does not, and Elixir's :protobuf has no parser at any version (0.17.0 added Protobuf.Text, encode-only). Doing the parse once in a declared, cached action removes the requirement from all three languages. Verified on RBE, positive and negative: ci.textproto compiles to a 314-byte binary; an unknown field, a wrong type for port, and an invalid enum value each fail the build, naming the offending token. The rule set likewise rejects an unknown predicate and an unknown enum in scope. BINDINGS (config/proto_bindings/) Rust: crate serviceradar-config-schema (prost), registered in the workspace and Cargo.lock. Three tests pass on RBE, including one asserting that unset fields are absent rather than defaulted -- explicit presence surviving codegen is a tested property, not an assumption about a generator. Go and Elixir: checked-in bindings. 26 Elixir fields carry proto3_optional, so presence survives there too. Note: the generators for the checked-in Go and Elixir bindings are deliberately not included. Makefile targets would violate the "everything is a Bazel target" rule, and the Bazel build already generates Go from source via //config/proto:configpb rather than reading the checked-in copy. The Elixir .pb.ex therefore have no drift guard until elixir_proto_library exists; see the consolidation note. CONSOLIDATION SPEC (openspec/notes/rules-elixir-consolidation.md) Writing the Elixir bindings hit a gap -- rules_elixir has no proto rule -- and the investigation into why produced a specification for consolidating the ~4034 lines of de facto Elixir rules in build/ and the ~250-file hex dependency model in third_party/hex into rules_elixir, measured against both rulesets at HEAD. Four findings drive it: elixir_app hardcodes priv = [] and hdrs = [], which drops NIFs and generated output at the provider boundary; rules_erlang's hex resolver is dead code that has never run, while third_party/hex demonstrably works across 269 packages; build/ contains three stacked generations whose newest already produces ErlangAppInfo per package; and rules_erlang fetches OTP with curl inside a build action and extracts to an absolute /tmp path outside the sandbox -- the same bug class this consolidation exists to remove. The note also records the invariants that look like junk and are not, chiefly the Application.compile_env/Config.Provider boot check: drop it and you get a ruleset that builds perfectly and produces releases that refuse to start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…lidator The 37 rules in //config/rules were a document. //config/validator is a Rust implementation of //config/SEMANTICS.md that makes them executable, and a Bazel test that runs them over every committed instance. Engine notes. Only Required treats absence as a violation, so one missing value yields one violation rather than a cascade. Scope gates on kinds/except_kinds. Violations are totally ordered by (field_path, code), which is what will let the Go and Elixir engines be compared against this one rather than merely all rejecting. Field access is an explicit match rather than reflection: the schema is closed and small, and the payoff is that a rule naming a field the schema lacks is a hard error instead of a silently skipped rule. It validates the compiled binaries. That is what a runtime loads; validating the .textproto sources would be checking a different artifact. localhost is its own kind because it is the one the rule set exempts from verified TLS and a secure core transport -- a dev machine has no cluster DNS, no fixture certificate and no SPIFFE agent. saas carries no `instance` field; only on-prem is multi-tenant. Also settles the last open phase-2 question. EnvironmentConfig describes the deployed system; anything describing the build or the test harness stays a Bazel target attribute or a runfiles lookup. SRQL_FIXTURE_ROOT is a fixture-directory override that the harness already resolves through runfiles and that nothing sets, so it gets no schema field. Verified: three tests, all confirmed to run. The committed instances pass; a deliberately broken one is rejected with the expected codes; an absent database.port reports DATABASE_PORT_REQUIRED alone. End-to-end, setting saas.textproto to TLS_MODE_DISABLE turns the test red and restoring it turns it green -- a validator that only ever passes cannot be distinguished from one that never runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
relocating it. A value comes from the configuration system or from a declared build input; an ad-hoc environment override in a test can repoint an input to something the build graph does not know about, which is what makes a green result meaningless. Fixture data resolves through runfiles. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…-4 file checks
ENVIRONMENT_KIND_DEMO (5) with config/environments/demo.textproto. A kind rather
than a saas instance: demo has its own topology and its own admission policy, and
every rule scoped `except_kinds: LOCALHOST` must reach it. It carries saas's
posture -- VERIFY_FULL, mTLS, pool_size 25 -- because it is a real deployment
reachable over the internet. Hostnames are demo's own; copying saas's namespace
verbatim would have committed a known-wrong value as ground truth.
DgraphConfig (host, port, tls_mode) for rust/dgraph-client, with a SEPARATE
DgraphTlsMode enum. Not a reuse of TlsMode: the Dgraph client has no verify-full,
so a shared enum would let an instance state TLS_MODE_VERIFY_FULL for Dgraph -- a
setting that reads as the strongest option and that no client can honour. One host
rather than a repeated endpoint list, because the Kubernetes alpha Service already
load-balances. ACL username, password, api key, bearer token and namespace are
omitted: the last four are SecretManager's, and the schema does not carry fields no
instance sets. Seven rules mirror the database TLS posture; the outside-localhost
rule admits VERIFY_CA alone.
credential_shape_test rejects credential-shaped values in committed instances. Two
choices are load-bearing. It scans the canonical text of the COMPILED BINARY, not
the source: that covers every field that is present, including ones no checker knows
by name, and it does not flag the word "password" in the comment warning against
them. And it matches on SHAPE -- URL userinfo, `password=`-style connection
parameters, PEM material, whole-value base64 runs -- rather than on a list of bad
field names, which would only catch what someone already thought of. A
false-positive control asserts that SPIFFE IDs, service DNS names and
`platform, ag_catalog` survive it, because a shape check that blocks legitimate
values gets disabled by whoever it blocks. Verified end to end on RBE: a DSN with an
embedded password planted in demo.textproto fails the test naming
`demo: database.host`.
round_trip_test compares each committed .textproto against the canonical text of the
binary built from it, flattened to path=value pairs. Structural rather than
byte-for-byte because the committed file carries comments and blank lines no encoder
emits, and neither prost nor Elixir's :protobuf can parse text format. Its weight
comes from the two sides being read from different places -- source tree vs. decoded
artifact -- so it becomes load-bearing once the binary is copied into a release
artifact's priv/. Within one build the two cannot diverge, so an in-test negative
control is what proves the comparison works.
Two defects found while doing the above.
//config/proto_bindings/go/BUILD.bazel claimed `make verify-proto-go-config` was the
drift guard for the checked-in Go bindings. That target was never written, so the Go
bindings had no drift check at all while the Elixir ones did. Adds
//config/proto:config_go and binding_drift_go_{config,rules}_test.
config/validator did not compile under standalone cargo, pre-existing: it used
regex_automata::meta while the workspace declares default-features = false. It built
under Bazel only because feature unification let another crate supply the feature --
the trap AGENTS.md documents under "cargo check -p <crate> must pass standalone".
Fixed by naming ["meta", "syntax", "unicode-case"] at the consuming crate. No
Cargo.lock or crate-mirror churn.
Verified: bazel test --config=remote --nocache_test_results //config/... -> 8/8 pass.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Decision 6: a target declares the section it needs, so a database test's runfiles PHYSICALLY DO NOT CONTAIN the NATS configuration. Enforced by the sandbox rather than by discipline, and visible at the target definition instead of inferred from a global union of forwarded environment variables. Under remote execution the property is absolute -- only declared inputs reach the executor. Four sections per instance (database, nats, core, dgraph), e.g. //config/environments:ci_database, cut by //config/tools:extract_section. A tool rather than a protoc invocation because text format cannot be sliced: the section boundary is structure, and recovering it from the source text would be a parser pretending to be a grep. An absent section is an error rather than an empty file -- zero bytes decode to a message whose every field is absent, which validation would then report as a dozen missing values instead of as the one thing that is actually wrong. section_privilege_test declares exactly one section and asserts both halves of the claim, because Decision 6 is a statement about runfiles and is only true if something checks the runfiles. Negative control verified on RBE: granting the target ci_binpb and ci_nats makes both absence assertions fail, each naming the reachable file, while the positive assertion still passes -- so the absence checks are detecting absence rather than a broken lookup. Note: the _SECTIONS loop in config/defs.bzl and the config/tools workspace member in Cargo.toml were reverted during the commit of 16baa78 and are re-applied here. Verified: bazel test --config=remote --nocache_test_results //config/... -> 9/9 pass. cargo check -p serviceradar-config-tools passes standalone. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Decision 6: a target declares the section it needs, so a database test's runfiles PHYSICALLY DO NOT CONTAIN the NATS configuration. Enforced by the sandbox rather than by discipline, and visible at the target definition instead of inferred from a global union of forwarded environment variables. Under remote execution the property is absolute -- only declared inputs reach the executor. Four sections per instance (database, nats, core, dgraph), e.g. //config/environments:ci_database, cut by //config/tools:extract_section. A tool rather than a protoc invocation because text format cannot be sliced: the section boundary is structure, and recovering it from the source text would be a parser pretending to be a grep. An absent section is an error rather than an empty file -- zero bytes decode to a message whose every field is absent, which validation would then report as a dozen missing values instead of as the one thing that is actually wrong. section_privilege_test declares exactly one section and asserts both halves of the claim, because Decision 6 is a statement about runfiles and is only true if something checks the runfiles. Negative control verified on RBE: granting the target ci_binpb and ci_nats makes both absence assertions fail, each naming the reachable file, while the positive assertion still passes -- so the absence checks are detecting absence rather than a broken lookup. Note: the _SECTIONS loop in config/defs.bzl and the config/tools workspace member in Cargo.toml were reverted during the commit of 16baa78 and are re-applied here. Verified: bazel test --config=remote --nocache_test_results //config/... -> 9/9 pass. cargo check -p serviceradar-config-tools passes standalone. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
config/environments/onprem/untd.textproto, in its own Bazel package so each deployment's targets mirror its file path and the instance set grows without touching a shared BUILD file. Its coordinates are the ServiceRadar chart's DEFAULTS rather than invented topology: an on-prem install runs the same chart, so `cnpg-rw` in namespace `serviceradar` is what a default install produces. Anything a given deployment overrides in its values file must be overridden here too. This is Decision 10 in effect -- a customer instance is validated by this repository's build, not by a pipeline this repo does not control. Negative control verified on RBE: weakening its database.tls_mode to DISABLE fails file_phase_test with `onprem/untd.textproto has 1 violation(s): database.tls_mode DATABASE_TLS_MODE_VERIFIED_OUTSIDE_LOCALHOST`, so a customer file is held to the same cross-environment invariants as ci and saas. INSTANCE_FORMAT is unchanged: the identifier is lowercase, matching the committed rule and the file path it appears in. INSTANCES now lives once in src/utils_tests.rs and is shared by file_phase_test, round_trip_test and credential_shape_test. It was duplicated in file_phase_test.rs, which is how an instance gets added to one check and not the others. Verified: bazel test --config=remote --nocache_test_results //config/... -> 9/9 pass. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…tegrity requirement A dedicated configuration server is out of scope and a follow-up specification. This records the constraints that keep it reachable, because each is free now and expensive later. The one that would have blocked it is a bootstrap cycle: a server authenticates its clients, authenticating needs a credential, credentials come from SecretManager, and SecretManager needs configuration to know its provider. Broken by requiring the config source to be reachable with platform-provided workload identity alone -- a Kubernetes service account token, a SPIFFE SVID, or a platform-mounted client certificate. ServiceRadar already deploys with SPIFFE and mTLS, so that identity exists before any ServiceRadar configuration is read. That also corrects the open call from the previous commit. Requiring a #sha256= digest pin on https: would foreclose a server that renders per deployment, since no client-known digest can exist for it. The requirement is now the PROPERTY -- a remotely fetched artifact's integrity must be verifiable against something the client knew before fetching -- satisfied by a digest pin for a static artifact or a signature against a pinned public key for anything dynamic. Three further constraints: the selector stays a URI rather than a path so a new source is a new scheme; schemes are a closed set with an unknown one a startup error, but the loader is shaped as a resolver keyed by scheme so adding one is adding an entry; and the loader returns the instance together with its provenance from the first version, because explain has nowhere to report endpoint, fetch time or verified identity if the result is a bare EnvironmentConfig. Hot reload is explicitly not promised. Decision 9 requires the first resolution to be synchronous and complete before the application tree starts, so any server must support a fetch at boot; push or lease is an addition to that, never a replacement. Client-side validation needed nothing extra -- a server is not trusted to have validated what it serves, which is what makes it safe to plug in rather than a new thing to trust. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…least one rule Without this the rule set is complete only by memory. A field added to the schema and not to the rule set is reported by nothing: validate evaluates the rules it has, so an unconstrained field passes silently, which reads exactly like being valid. This session supplied the case -- four dgraph.* fields were added by hand, and nothing would have caught a missing rule for any of them. The field list comes from the schema's own descriptor (//config/proto:config_descriptor_set), recursed to leaves, rather than from a list in the test. A hand-kept list is one more thing to forget to update, and forgetting is the failure being caught. Two further assertions cover the blind spots of the obvious one. A rule naming a field the schema lacks is dead -- it can never fire, so the constraint it expresses is absent -- and the descriptor walk is checked to reach nested sections and to stop at leaves, because a walk returning only top-level fields would pass the coverage check while constraining nothing nested. Negative control verified on RBE: adding dgraph.forgotten_field to the schema without a rule fails with `schema fields with no rule -- they are unvalidated, which reads exactly like valid: ["dgraph.forgotten_field"]`. prost-types is named as a dependency of config/validator rather than inherited through Bazel's feature unification, so cargo check -p serviceradar-config-validator still passes standalone. Verified: bazel test --config=remote --nocache_test_results //config/... -> 10/10 pass. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…s integrity ForbiddenIf: a field must be ABSENT when another field holds one of the given values. It closes the half of `instance` that had no predicate. The schema said "required if and only if the kind is ONPREM", but only the required half was expressible, so nothing stopped ci.textproto naming an instance -- a file describing something it is not. Its trigger is a SET where RequiredIf takes a single value, and that asymmetry is the point. The forbidding side enumerates the COMPLEMENT of what is permitted, so a per-value rule is exactly how a newly added enum value becomes permitted by omission -- and the coverage meta-rule cannot catch that, because the field still has rules. So the set comes with a guard: a RequiredIf/ForbiddenIf pair keyed on the same enum must cover every value of it. Both negative controls verified on RBE. Naming an instance on ci yields INSTANCE_FORBIDDEN_OUTSIDE_ONPREM. Adding a sixth kind without extending the trigger set fails naming the uncovered value. Two fixtures, not one. The forbidding case, and a permitting case on onprem: a trigger set that accidentally included ONPREM would reject every on-prem instance, and only a positive case catches that. Vocabulary is now nine predicates. SEMANTICS.md carries the verdict row, Elixir and Go bindings are regenerated and their drift guards pass. Artifact integrity over https: is deferred to the configuration-server specification rather than dropped, with the exposure recorded: without verification against something the client knew beforehand, whoever controls the endpoint or its DNS controls the deployment's database host and TLS mode. It cannot be settled here because the mechanism depends on what serves the artifact -- a digest pin cannot exist for anything rendered per client, and a signature presumes a signing key the server design has not introduced. file: carries no such exposure and is the recommended source until then. Verified: bazel test --config=remote --nocache_test_results //config/... -> 10/10 pass. cargo check -p serviceradar-config-validator passes standalone. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
… feat/unified-configuration
…a fixture The fixture file was inert data. Nothing evaluated it, so it was neither the negative fixture set nor the conformance vector set it claimed to be -- just a document. //config/validator:vector_test runs it, and found two real defects on its first run. Every pre-existing fixture was stale: the dgraph section was added to the schema and the rule set without updating them, so all ten silently emitted three phantom violations. And 35 of 45 rules had no fixture at all -- the invariant the rule set file states in its own header and that nothing enforced. A rule no case exercises is indistinguishable from a rule that does nothing. 47 fixtures now, covering all 45 rules. Generated SHAPE, authored INTENT: each case is one mutation of a valid baseline, with expected violations written by hand from what the rule is for. Deriving them by running the validator would have been faster and worthless, because a fixture whose expectations come from the implementation cannot test that implementation. All 35 hand-written expectations matched the engine on the first run. Comparison is the ORDERED SEQUENCE of (code, field_path), not accept/reject and not a set. Three implementations can reject the same input for three different reasons and a bare rejection assertion stays green. Two cases deliberately name two violations, because one field carries several rules and the full sequence is what pins the reason. Two further checks: every rule must be violated by some fixture, and every expected code must name a rule that exists -- a fixture naming an undefined code is dead weight that reads like coverage. Negative control verified on RBE, testing the SEMANTICS.md section 8 property rather than assuming it: silently widening DATABASE_POOL_SIZE_RANGE from min 1 to min 0 reds database_pool_size_zero with `expected [...] actual []`. Verified: bazel test --config=remote --nocache_test_results //config/... -> 11/11. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Eight proptest properties plus eight hand-written cases. Both layers earn their place: the properties state each law as a universally quantified claim and shrink to a minimal counterexample on failure, while the hand-written cases pin the specific points three implementations most easily diverge on. Running the negative control found a real defect in the first generator. Breaking the engine to make IntRange exclusive at its upper bound was caught by the hand-written edge case but NOT by prop_int_range_is_inclusive_containment, which drew min, span and v independently over 400k values and so reached v == max only by luck. The property was passing vacuously with respect to the boundary it claims to prove. Fixed by drawing the value relative to the bounds; the same control now fails and shrinks to min = 0, span = 0, pick = 0 -- the degenerate range [0,0] probed at its own edge. Sampling a boundary is not testing it. proptest is added to [workspace.dependencies] and as a dev-dependency of config/validator, with normal_dev = True on the Bazel target. Six archives vendored via //third_party/crate_mirror:sync; Cargo.lock and MODULE.bazel.lock updated. bazel build //rust/... is unaffected. make test selection confirmed by query: no target under //config/... carries manual, integration_test or acceptance_test, so all twelve are selected. Verified: bazel test --config=remote --nocache_test_results //config/... -> 12/12. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Eight proptest properties plus eight hand-written cases. Both layers earn their place: the properties state each law as a universally quantified claim and shrink to a minimal counterexample on failure, while the hand-written cases pin the specific points three implementations most easily diverge on. Running the negative control found a real defect in the first generator. Breaking the engine to make IntRange exclusive at its upper bound was caught by the hand-written edge case but NOT by prop_int_range_is_inclusive_containment, which drew min, span and v independently over 400k values and so reached v == max only by luck. The property was passing vacuously with respect to the boundary it claims to prove. Fixed by drawing the value relative to the bounds; the same control now fails and shrinks to min = 0, span = 0, pick = 0 -- the degenerate range [0,0] probed at its own edge. Sampling a boundary is not testing it. proptest is added to [workspace.dependencies] and as a dev-dependency of config/validator, with normal_dev = True on the Bazel target. Six archives vendored via //third_party/crate_mirror:sync; Cargo.lock and MODULE.bazel.lock updated. bazel build //rust/... is unaffected. make test selection confirmed by query: no target under //config/... carries manual, integration_test or acceptance_test, so all twelve are selected. Verified: bazel test --config=remote --nocache_test_results //config/... -> 12/12. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Three implementations now, verified against one another by the committed vectors
rather than by three hand-maintained suites. This is the precondition for
Decision 12, which requires every implementation to validate at load.
Go (//config/go/validator) reproduces all 47 fixtures as an ordered
(code, field_path) sequence and revalidates all five committed instances, plus
seven rapid properties. Two negative controls on RBE, catching different things:
making IntRange exclusive at its upper bound was caught by the property and shrank
to `range [0,0] at 0`, the identical minimal counterexample proptest produced in
Rust, but did NOT red the vectors because no committed fixture uses a
max-boundary value; disabling NonEmpty instead failed ten named fixture subtests.
That asymmetry is why both layers exist.
Elixir (config/elixir) carries the engine and seven StreamData properties. All
three trees draw range values relative to the bounds rather than independently,
carrying forward the defect the Rust control exposed: an independent draw reaches
v == max only by luck, so the property passes against an off-by-one engine.
The Elixir vector harness is written but BLOCKED, and excluded from the glob with
the diagnosis inline rather than left red or tagged manual. rules_elixir
private/ex_unit_test.bzl:51 stages every test input with `src = s.path`, which for
a generated file is bazel-out/<cfg>/bin/... and does not resolve from the test's
working directory; source files work only because path == short_path for them.
Fix is `src = s.short_path`, against pinned commit 832a95b4.
Both engines depend on the checked-in bindings rather than the generated ones,
because a Go source file can only name one import path and `go test` must resolve
it too. binding_drift_go_{config,rules}_test is what makes that safe.
pgregory.net/rapid added to go.mod and registered by `bazel mod tidy` as
net_pgregory_rapid.
Verified: bazel test --config=remote --nocache_test_results //config/... -> 14/14.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
The tree had four conventions at once: config/validator (Rust, no language dir),
config/go/validator (language first), config/elixir (language, no component) and
config/manager_config/rust (component, then language). Only the last is coherent,
so it now applies everywhere.
proto/, environments/, rules/ data and schema -- no language
proto_bindings/{rust,go,elixir} generated bindings
validator/{rust,go,elixir} the rule engine
manager_config/{rust,go,elixir} ConfigManager
manager_secret/{rust,go,elixir} SecretManager
tools/{rust} build tooling
Component first, language second, because the implementations of ONE component
change together -- a shape change lands in all three or the shared conformance
vectors go red -- while two different components in the same language do not.
Grouping by language would scatter the things that must agree across three places,
which is the arrangement Decision 10 rejected for the tree as a whole. tools/ has
one language today and still carries the directory: a rule with an exception is a
rule nobody applies.
Mechanical fallout, all of it load-bearing: crate manifests re-point one level
deeper; the Rust test runfiles fallback gains a path segment; the Go importpath
becomes .../config/validator/go and its imports are now explicitly aliased,
because a path ending in /go no longer matches the package name and inference
should not carry that; defs.bzl's Label() for the section extractor moves; and the
labels in tasks.md and design.md follow.
One real trap on the way through. //config/validator was valid shorthand only
because the package basename matched the target name; one level down it resolves
to :rust, which does not exist. Bazel dropped both manager tests from the build
and still reported "Executed 15 out of 15 tests: 15 tests pass" -- green over a
shrunken target set. Confirmed the fix by enumerating targets with
`bazel query 'kind(".*test", //config/...)'` (18) rather than trusting the summary.
Verified: bazel test --config=remote --nocache_test_results //config/... -> 17/17.
cargo check passes for all three config crates; go build and go vet clean.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…y by mutation
cargo test ran ZERO tests in every config crate. The files under src/*_test.rs were
never declared in lib.rs, so cargo never compiled them; they existed only through
Bazel's crate_root. `cargo check --lib` passing said nothing about them, and the
signal -- "running 0 tests" -- was in the output twice and went unread.
Both crates now follow the repository's Rust layout: src/{types,errors,traits}/ with
one type per file and each hand-written trait impl in its own file, shared fixtures
at src/utils_tests/ where Bazel can reach them, and tests/ exercising the public API
as a consumer sees it with every module registered from one entry point. Dispatch is
static: ReadSource is a generic bound, not dyn.
autotests = false with one declared [[test]] per crate. Cargo otherwise discovers
each tests/*.rs as its own binary, double-compiling what lib_tests.rs registers and
letting a file run under cargo that Bazel's single crate_root never sees.
Bazel is authoritative. The validator's suite reads artifacts protoc compiles during
the build, and this repository removes Bazel's convenience symlinks, so cargo has no
path to them. Those carry #[ignore] naming the reason, so cargo lists them as skipped
rather than quietly covering less, and the Bazel target passes --include-ignored. The
manager needed no such gap: its rule set is built in code, because those tests are
about what the manager does with a rule set rather than the committed one's contents.
section_privilege_test stays a separate target in both builds. Folded into the
omnibus target it received every fixture and failed correctly -- it asserts that
declaring one config section does not grant the others, which only holds if its
inputs are declared alone.
Adds the Go ConfigManager mirroring the Rust one, with the same 26-test surface.
Every safety property is verified by MUTATION, not asserted. Removing the identity
cross-check, skipping validation, accepting an instance on a single-instance kind,
and defaulting an unset variable each fail exactly the tests that name them, in both
languages, with no mutation uncaught.
Verified: bazel test --config=remote --nocache_test_results //config/... -> 12/12.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…N assembly
ConfigManager lands in Go and Elixir mirroring Rust: one variable, source derived
from the kind, identity cross-check, loading that validates. Where the three
disagree one of them is a bug, since the same SERVICERADAR_ENV must behave
identically whichever service reads it.
SecretManager enforces two properties with types rather than discipline. A Secret
cannot be printed: Debug and Display redact, the value is reachable only through
expose(), and redaction is proven to survive nesting in Option, Vec, tuples and
Result -- {:?} reaches secrets through tracing spans, unwrap panics and error
chains, none of which look like printing a password at the call site. And refusal
precedes resolution: an undeclared name is refused identically whether or not the
store holds it, because answering even to say "not found" reveals whether a secret
the component may not have exists.
Empty is not a secret. A provider returning an empty string has failed to resolve
one, and treating it as a value is how a component connects with a blank password.
DSN assembly returns a redacting type. The DSN is not a schema field precisely
because it embeds a password, and that reasoning does not stop at the schema:
a bare String would undo the redaction SecretManager provides. sslmode comes from
the typed TLS mode rather than string concatenation, which is how sslmode went
missing and tokio-postgres fell back to Prefer; userinfo is percent-encoded,
because a password containing @ or : would truncate the host or the role and
produce a DSN that parses into something else rather than failing.
Ten mutations, none uncaught: removing the identity check, skipping validation,
accepting an instance on a single-instance kind, defaulting an unset variable,
printing a Secret from Debug or Display, accepting an empty secret, removing the
manifest check, printing a Dsn, dropping sslmode, and skipping percent-encoding
each fail exactly the tests that name them.
One test was rewritten mid-flight: asserting database_url returns None for an
unspecified TLS mode was testing dead code, because validation rejects that
instance before a manager can exist. It now asserts the load fails.
Still open in this phase: SecretManager in Go and Elixir, and the explain command.
Verified: bazel test --config=remote --nocache_test_results //config/... -> 16/16.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…n, and rename config/validator becomes config/manager_validator, so every component in the tree reads component/language. The crate's relative dependency path (../../validator/rust) contains no "config/" and was missed by the path rewrite; cargo caught it. SecretManager lands in Go and Elixir. Elixir needed a different mechanism, and a test found out why: a custom Inspect implementation is not enough on the BEAM, because a struct IS a map and inspect(term, structs: false) renders it as one, printing every field and bypassing the protocol -- as does anything that walks the term, including a crash report. The value is therefore held in a closure rather than a field, verified against structs: false, Map.from_struct/1 and term_to_binary/1. Go needed GoString as well as String, since %#v prints the struct literal. Three languages, three different leak vectors. explain reports where every value came from, including values compiled into the release: "it was built in" is an answer, and omitting it leaves open the question this system exists to close. Enums report by name rather than number. Secrets are absent rather than redacted -- Explanation has no way to reach a secret value at all, which is stronger than remembering to mask one, and a masked value beside a name is one formatting change away from an unmasked one. Seventeen mutations across the phase, none uncaught. The harness itself was wrong twice and is fixed: backups keyed by full path rather than basename, which had destroyed a file when two modules were both named mod.rs; and a missing test log now reports a failed build rather than being read as "not caught". Verified: bazel test --config=remote --nocache_test_results //config/... -> 19/19. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Every Rust target was unbuildable on macOS. Three faults stacked, each hidden behind the one before it. The sysroot archive could not be extracted. osx.bzl builds its include list as both System/Library/Frameworks/<F>.framework/* and System/Library/PrivateFrameworks/<F>.framework/* for EVERY requested framework, unconditionally, and all six of its defaults are public frameworks. The joseluisq/macosx-sdks repackage this repository pinned carries no PrivateFrameworks tree, so six include patterns matched nothing and bsdtar exits 1 on an unmatched include pattern. The pin is now the genuine Apple CLT SDK package hermetic-llvm itself consumes, where archive and include list are known to agree. That exposed the second: `ld: framework 'System' not found`. System is absent from _DEFAULT_FRAMEWORKS and the driver emits -framework System for every executable, so libraries compiled and only binaries failed. Which exposed the third: osx.frameworks REPLACES the default list rather than extending it, so naming System alone dropped the other six and the link failed on Foundation instead. All seven are now listed. `Failed to open property list SDKSettings.plist` is emitted throughout and is NOT fatal -- verified both ways: supplying a plist did not fix the link, and the final build succeeds with it absent. It reads like the headline error and is a dead end. The comment on the pin claimed it overrode a default for faster download. There is no default: @llvm's extension fails with "Missing osx.from_archive(...)" without it. It was load-bearing while reading as an optimisation, and now says so. The cost is a slower first fetch, since a full Apple SDK package replaces a trimmed tarball. llvm stays at 0.8.17: v0.8.18's osx.bzl is identical in this area, so the bump buys nothing here and carries a rules_rust update. Verified: bazel build --config=cache_only //rust/... -> 11,272 actions, clean, binaries linking under darwin-sandbox. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…uild input The integration lifecycle is six separate Bazel invocations that share no process, so each must independently derive the same disposable database name while two concurrent runs must not. That identity came from GITHUB_RUN_ID and GITHUB_RUN_ATTEMPT, read separately by Rust and Elixir, each formatting the name itself and each falling back to the constant "sr_core_test_local" when the variables were absent. Three things were wrong with that. The names belonged to a CI vendor that was never the source: BuildBuddy does not set them, so buildbuddy.yaml synthesised entropy locally and laundered it through cksum into a fake numeric id purely to satisfy a digits-only validator. The format was implemented twice, with a comment admitting the two "must agree exactly or the suite runs against a database nothing provisioned". And the constant fallback meant two runs against one fixture silently shared a database, with each teardown dropping the other's data. Replace all of it with --//build:run_id, materialised by //build:run_id_file and read from runfiles by both languages. The caller mints one id and repeats the flag on every invocation, so the value arrives on the command line -- the build's declared input -- rather than as ambient process environment. There is no default: a missing id now stops the run with the whole six-step sequence in the error, instead of substituting a name two runs can share. Bazel cannot mint this itself. A genrule running uuidgen has no inputs, so its key never changes: measured across three invocations it returned one value, and with the remote cache that constant would be shared org-wide. Forcing re-execution gives a different value per invocation, so the six steps disagree. Neither is "stable within a run, different between runs", because a run spanning six invocations is not a concept Bazel models -- it belongs to the caller. Cache impact is confined by construction: the rule reading the flag is a leaf with no dependencies, and consumers take its output as data, which changes an action's inputs but not its configuration. Flipping the id re-executes 2 of 4318 actions and recompiles nothing; only the analysis cache is discarded, and the flag is constant across a run. Also fixes an unrelated live failure in the same path. database_url_named appended libpq's &sslsni=1&host=<name> whenever tls_server_name was set, which every verify-full environment sets. tokio-postgres has no sslsni arm and rejected the whole connection string; it also reads a query-string host as an additional host to dial, so the verification name would have become a silent fallback endpoint. The name is a typed field the TLS connector already receives. Fix drift found while hoisting the fixture inputs: provision_db declared the rule set without the committed instances, and the per-shard variants declared neither, so both would have failed the same way the sweep did. One FIXTURE_DATA list now. - //build:run_id + //build:run_id_file (build/run_id.bzl) - database_name() reads runfiles; validation split into a pure, tested function (7 new tests, all four guards mutation-verified) - integration_env.exs reads the same file with the same guards, failing closed - drop the GITHUB_RUN_ID/ATTEMPT --test_env passthrough from .bazelrc - thread the flag through all 6 buildbuddy, 7 forgejo and 8 skill invocations - add build/run_id.bzl to the integration workflow's paths filter Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…test.py was deleted env-var-inventory.md §4.1 presented that test as the repository's existing .bazelrc drift guard, and three verdicts in §9 rested on it pinning NATS_KEY_B64 and TEST_CNPG_PASSWORD. It is deleted, so those pins are gone: both names now have no reader and no pin, which moves two "refuted" verdicts to partly restored. §8's first recommendation asked to extend that guard, which is no longer possible. Two facts about it are worth carrying forward rather than losing with the file. It had no py_test target, so make test never ran it despite §4.1 claiming it did; and its setUp read .github/workflows/elixir-integration-sr-core.yml, deleted in 8ce61b5, so every test in it errored rather than asserted. A guard that is not a build target is not a guard -- which is why its replacement is now specified as a Bazel test target asserting both directions. The same correction applies to this change's own proposal.md and tasks.md, which told the reader the test "must be updated, not deleted" and that removing the --test_env lines without it would turn make test red. Removing them is now silent, which is worse. Makefile: the comment explaining why BAZEL_CACHE_PROXY_CONFIG is left empty cited that test as the thing enforcing every named --config exists in .bazelrc. Nothing enforces it now; a stale value fails at invocation instead. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…ookup
Two implementations of one security decision drift, and these two already
had. srql's build_tls_connector read its CA from a FILE PATH and supported
client certificates; integration-db's tls_connector_for took PEM content and
refused client auth outright. Both are now rust/srql/src/tls.rs, which takes
content only -- a path is meaningful just on the host that resolves it, and
SecretManager yields content because a secret that must be a file on disk
cannot be a Kubernetes secret, a Docker secret and a developer's directory at
once. Two guards neither had: an empty CA bundle now fails loudly rather than
producing a root store that verifies nothing, and half a client identity is an
error rather than a silent fall back to anonymous against an mTLS server.
Six hand-rolled runfiles lookups are replaced by the Bazel ecosystem's
reference implementations. They were not merely untidy. The Rust ones read
only TEST_SRCDIR/RUNFILES_DIR, so they found nothing under `bazel run` --
prepare_template failed on exactly that -- and located files with Path::exists,
which cannot work in manifest mode. They also guessed the repository directory:
integration-db and manager_validator tried "_main" then "serviceradar", and the
srql harness tried the runfiles root, $TEST_WORKSPACE, "__main" and "__main__",
none of which is the canonical Bzlmod name. @rules_rust//rust/runfiles consults
the repo mapping the build emitted; it is published to crates.io, so cargo and
Bazel share one source.
Elixir needed no library. rules_elixir's ex_unit_test copies srcs and data to
${TEST_TMPDIR}/<workspace-relative path>, which is the rule's contract and
needs no repository name at all. Reaching into TEST_SRCDIR worked only because
the runfiles tree happens to carry the same files.
prepare_template no longer writes $GITHUB_OUTPUT -- a GitHub Actions concept
BuildBuddy does not set, faked in the workflow with a temp file that was then
grepped back. The caller branches on the line the binary prints, which keeps
the skip that saves 28.8-45.6s of BEAM startup on a branch adding no migrations.
Well-known database secret names move out of the fixture crate into
manager_config, with client_cert and client_key added. Two components
resolving "the database password" must ask for the same name, and while the
constants were private to integration-db they could not.
- rust/srql/src/tls.rs: postgres_connector + PgRustlsConnect, both callers on it
- deletes build_tls_connector, build_client_config, load_client_certs,
load_client_key and a duplicate crypto-provider install
- runfiles crate added to [workspace.dependencies]
- provision_db_test needs no change: SERVICERADAR_TEST_DB_SHARDS is declared in
the target's env, not ambient
7a remains open on rust/srql/src/config.rs, which is blocked on how a SERVICE
obtains the RuleSet (every consumer reads it from runfiles; a container has
none), and on the DSN/TLS half of the srql harness. Both are recorded in
tasks.md with the options.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
`load` took a `&RuleSet`, which put the rule set in the dependency graph of every caller. That shape was wrong twice over. It claimed the rules vary, when they vary by nothing -- not by environment, not by component, not by deployment. And it leaked an implementation concern of the config layer into the API of every consumer: a service had to obtain the rules before it could obtain its configuration, and the only mechanism available was runfiles, which a container does not have. That is precisely what kept rust/srql from using ConfigManager at all. Validation is unchanged -- there is still no entry point that returns an unvalidated value. The rules are now an internal dependency of the config layer, embedded at build time, in all three implementations: Rust `include_bytes!`, Go `go:embed`, Elixir a compile-time module attribute with `@external_resource`. Embedded, NOT shipped beside the instance in the mount. The instance is read from a mount at runtime -- untrusted, and the thing being verified. Reading the rules from that same mount would let whatever supplied a bad instance supply the rules that bless it, which proves nothing. The asymmetry is the whole value of the check: trusted rules, untrusted instance. The .binpb is committed because `include_bytes!` and `go:embed` need a file `cargo` and `go test` can reach, and `go:embed` cannot reach outside its own package -- so there is one copy per language, exactly like the generated protobuf bindings, and guarded the same way. Three diff_tests compare each copy against protoc's output for config/rules/ruleset.textproto. The BuiltIn instances (localhost, ci) are embedded for the same reason. `Source::BuiltIn` means "compiled into the release", so a binary that cannot produce them cannot run in those environments. The mounted kinds are deliberately absent: those genuinely vary per deployment, which is why they are mounted, and embedding them would let a release disagree with the platform about what `demo` means. No test may inject a rule set. A manager test that validates against invented rules is not testing the system -- it tests against rules no deployment has, and it lets the valid fixture drift from what the committed rules require. The synthetic rule sets in all three languages are deleted; the fixtures now satisfy the real 45 committed rules. The Rust fixture had only ever been checked against 2 invented ones. `rust/srql/src/config.rs` loses all five PGSSL* reads. `DatabaseTls::resolve` takes the posture and verification name from the committed instance and the three PEMs from SecretManager, so no combination of variables can describe a server the process is not talking to. AppConfig carries PEM CONTENT, not paths. `integration_tests/srql/tests/support/harness.rs` goes 1117 -> 691 lines with no schema-covered env reads left. The DSNs are assembled from typed fields instead of mined for the owner and database name, which retires parse_fixture_pg_config, normalize_sslmode_for_tokio_postgres, normalize_fixture_pg_connection_string, normalize_postgres_url, parse_host_port, quote_pg_keyword_value, percent_decode, decode_hex_digit, read_env_value, FixtureRootCert and TemporaryCaCert. Every one of those existed to recover something the schema states, or to turn PEM content into a file because a consumer wanted a path. TLS resolves through srql's own DatabaseTls, so the harness verifies the fixture by the code path the service uses rather than a second implementation. That surfaced a live bug: the harness parses its DSN with tokio-postgres, and the assembled DSN carries sslmode=verify-full, which tokio-postgres rejects outright. Both parses now go through integration-db's shared parse_pg_config, which strips the mode rather than rewriting it. Same defect class as the sslsni fix, one layer over. //config/rules:ruleset_binpb is gone from integration-db's FIXTURE_DATA -- the fixture targets no longer declare the rule set at all. Verified: bazel build and test exit 0 (18 pass, 7 skipped as macOS-incompatible Elixir); cargo srql 467, manager_config 38, secret 21, validator 16, integration-db 20; go ok. Each drift guard mutation-tested -- one appended byte fails it, restoring passes. Note: cargo passed while Bazel failed partway through. srql's BUILD names first-party deps explicitly and `cargo_only = True` excludes them, so the three manager deps had to be added by hand, exactly as rust/README_RUST.md warns. The Elixir manager change is parse-verified only: those tests are SKIPPED on darwin (Linux-only OpenSSL sysroot), so it needs a CI run before it is trusted. Also restores the `### 7b` header, which an earlier tasks.md edit had eaten, filing eleven Elixir items under 7a. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
… it does when unset
Phase 7 was scoped from `openspec/notes/env-var-inventory.md`, which measured the
schema-covered readers. Two passes over the Elixir tree produced two different
counts, so this measures both trees properly and writes down a per-name
destination -- the stated goal is zero environment reads, and that cannot be
planned against a count.
Adds `elixir-inventory.md` (676 names, 1173 read sites) and `go-inventory.md`
(173 names, 246 read sites), each with the full inventory, a substitution plan,
and a migration plan. Notes the measured scope in tasks.md at phase 7 and 7d.
Why the earlier counts were wrong: a scan for `get_env("` misses two read forms.
`"NAME" |> System.get_env("default")` puts the name BEFORE the call, so the
DEFAULT is read as the name; and thirteen project helpers take the name as an
argument. Four secrets were reachable only through a helper, `SECRET_KEY_BASE`
among them -- a grep-based exit criterion would have reported them as migrated
while they were not.
The finding that matters is section 3.1 of the Elixir document. 350 names are
set by nothing in this repository. When absent, six raise; the other 340 change
behaviour and say nothing. `nil` is not one behaviour but three, all silent: a
feature switches itself off (CLUSTER_GOSSIP_SECRET, so gossip clustering is
simply not configured), a credential goes absent rather than wrong
(AGENT_GATEWAY_NATS_USER passes `user: nil` into the NATS connection), or an
empty value substitutes for a real one (SERVICERADAR_HOSTED_CLUSTER_CONTRACT
decodes to `%{}`). Whole subsystems boot this way: the cold tier reads 31 names
of which 25 are nil, and 33 of the never-set names are credentials, none of
which raises. Explicit presence plus a validator that rejects a missing field
turns 301 of those into a boot-time error naming the field.
Similarity is checked two ways. A value read as `get_env(A) || get_env(B)` proves
A and B are one value: 63 such pairs in Elixir, so 676 names carry 622 distinct
values. Lexical similarity misses most of them -- GH_TOKEN = GITHUB_TOKEN,
SPIFFE_ENDPOINT_SOCKET = SPIFFE_WORKLOAD_API_SOCKET, and a three-way secret
alias across the edge crypto/recording integrity/onboarding encryption keys.
Go is roughly a quarter the size and better guarded: 20 of 107 never-set names
fail loudly, and only two reads are unresolvable against Elixir's 91. Three Go
findings change 7d. `go/pkg/config/env_loader.go` derives variable names at
runtime from JSON struct tags and accepts a whole config document through
SERVICERADAR_CONFIG_JSON, so no scan can enumerate it -- it is dead in
deployment (every chart sets CONFIG_SOURCE=file) and live in code, and should be
deleted rather than migrated. `NATS_CREDSFILE`, read by two shipped services, is
a spelling nothing sets; Helm and Elixir use `NATS_CREDS_FILE`. And zero of the
169 literal-name reads use `os.LookupEnv`, so no Go code can distinguish an
unset variable from an empty one -- the sole exception is the
`SERVICERADAR_ENV` read this change introduced.
Both documents propose the same partitions, because a single embedded config
message is a global rebuild trigger: the managers read their artifact at compile
time, so one message means one config change invalidates every consumer. The
measurement says the split is natural -- 88 of 108 Elixir consumer files and 45
of 62 Go ones read exactly one partition, with the four `runtime.exs` files the
only hubs.
No code changes; measurement and plan only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_secret/elixir/test/manager_test.exs | View secret |
| 36296917 | Triggered | Generic Password | 7e62ade | config/manager_secret/rust/tests/types/manifest_tests.rs | View secret |
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_secret/elixir/test/manager_test.exs | View secret |
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_secret/go/manager_test.go | View secret |
| 36296916 | Triggered | Generic Password | 3239e92 | config/manager_secret/go/manager_test.go | View secret |
| 36296917 | Triggered | Generic Password | 7e62ade | config/manager_secret/rust/tests/types/manifest_tests.rs | View secret |
| 36296916 | Triggered | Generic Password | 3239e92 | config/manager_secret/elixir/test/manager_test.exs | View secret |
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_secret/elixir/test/manager_test.exs | View secret |
| 36296918 | Triggered | PostgreSQL Credentials | 0511311 | rust/integration-db/src/lib.rs | View secret |
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_secret/go/manager_test.go | View secret |
| 36296917 | Triggered | Generic Password | 7e62ade | config/manager_secret/rust/tests/types/secret_manager_tests.rs | View secret |
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_config/rust/tests/types/explain_tests.rs | View secret |
| 36296917 | Triggered | Generic Password | 7e62ade | config/manager_secret/rust/tests/types/secret_manager_tests.rs | View secret |
| 36296917 | Triggered | Generic Password | 7e62ade | config/manager_secret/rust/tests/types/secret_manager_tests.rs | View secret |
| 36296917 | Triggered | Generic Password | 3239e92 | config/manager_secret/elixir/test/manager_test.exs | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…es Hex mix hex.audit started failing today because Hex published EEF-CVE-2026-43971 (cowlib Link-header smuggling) against every cowlib release from 2.9.0 through 2.19.0. The existing .deps_audit_ignore files already covered 43966/43969 and the gun GHSA alias, so the quality gate failed only on the new id. There is still no Hex release that closes the range. Upstream has a git fix at ninenines/cowlib@89da27ee. This is not a missing-dotfile issue from the Forgejo-to-GitHub move -- the ignore files were already on the branch. Signed-off-by: Michael Freeman <mfreeman451@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
… from the alpine CDN. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
on BB executor and workflow. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…the CA
Four failures in a row on the fixture lifecycle, each hiding the next. The last
one was the design: SecretManager resolved `database.password` through
FileProvider(/etc/serviceradar/secrets), which nothing mounts on a BuildBuddy
runner -- or anywhere else.
Nothing mounts it in production either. //helm/serviceradar supplies every
credential through `valueFrom.secretKeyRef` -- 38 of them, CNPG_PASSWORD among
them -- which is a Kubernetes Secret projected as an ENVIRONMENT VARIABLE, not as
a volume. `grep -rn "mountPath: /etc/serviceradar/secrets" helm/ gitops/ docker/`
returns nothing. So the only provider all three languages shipped with matched no
deployment that exists, and CI was not a special case: it was the first
environment to actually ask.
EnvProvider resolves a logical name from SERVICERADAR_SECRET_<NAME>, a mechanical
transform of the manifest rather than a mapping table -- a caller that knows the
name can compute the variable, so a --test_env list is derived instead of
curated. The bridge it replaces forwarded 43 names, 38 of which nothing set.
EnvironmentProvider::for_kind selects it for every kind except `localhost`, which
keeps the file store because a developer machine has nothing injecting variables
into a test action. Selection now sits behind one constructor: three call sites
wrote `FileProvider::for_kind` themselves, which made the docstring claiming "a
component never names a provider" false.
An empty variable is absent, not an empty credential. A set-but-blank secret
authenticates as nobody and produces a confusing error from the server instead of
a clear one from here.
Two schema fields, because the fixture needs two things the schema could not say.
`database.admin_role`: the suite connects as the application role, which
deliberately lacks CREATEDB, so provisioning cannot use `connecting_role`.
Deriving it produced an admin DSN naming a role with no rights, which PostgreSQL
reports as a permission error and therefore reads like a missing GRANT. Its
password is a separate logical secret, `database.admin_password`.
`database.ca_bundle_url`: configuration, not a secret. A CA bundle is what a
client needs BEFORE it can authenticate anything, so it cannot itself be
authenticated material. A URL rather than the PEM, because a cert-manager issuer
rotates: any copy -- a CI secret, an instance file -- is correct until the next
rotation and then silently is not, which is what a stored
SRQL_TEST_DATABASE_CA_CERT did, expiring on a date nobody was watching. The PEM
now travels nowhere; every client reads the current bundle and no human rotates
anything.
While verifying the roles against the fixture rather than the instance file:
ci.textproto named `srql_test` for both connecting_role and owning_role, and no
such role exists. `pg_roles` has `srql` (the application role) and `srql_hydra`
(superuser). This would have been the next failure after the secret one, reported
as `role "srql_test" does not exist`. The same class of error is already
documented in integration-db's `database_owner()`, where an earlier port
hardcoded `serviceradar` -- a name the fixture has never had.
Four of this repository's own guards caught mistakes in this change, and are
worth naming because each one paid for itself here:
* every_schema_field_carries_at_least_one_rule -- two new fields, no validation
* every_rule_is_violated_by_at_least_one_fixture -- two new rules nothing would
notice breaking
* the per-language validator field registry -- unknown field path
* instance_drift_ci_test -- a stale embedded ci.binpb
The new transform is covered by tests that were mutation-checked: inverting the
separator mapping in EnvProvider::variable_for fails both name tests.
Also in this commit, from the same CI run:
* buildbuddy.yaml forwards the two secrets by name. BuildBuddy injects them into
the runner shell and --test_env passes the value through without it reaching a
log or a disk.
* `bazel run //third_party/crate_mirror:sync` after adding ureq to
integration-db. ureq was already vendored; the sync closed an unrelated gap,
runfiles-0.3.0, which was in the lock with no archive -- the mirror is a
fallback, so that had been degrading rather than failing.
* tasks.md records what 7b now depends on: EnvProvider exists only in Rust, and
Go and Elixir need it before either can resolve a secret in CI or in cluster.
Verified: `bazel test -c opt --config=ci //... --test_tag_filters=-integration_test,
-acceptance_test` -> 168/168 pass. The integration path still needs the fixture,
so provisioning is exercised by the next CI run, not here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…I through the internet
Two CI failures, one cause, and it was never DNS.
BuildBuddy REJECTs every RFC1918 destination from an action's network namespace.
server/util/networking/networking.go builds, per container network:
for allow in task_allowed_private_ips: -I FORWARD/INPUT -d <allow> -j ACCEPT
for r in {10/8, 172.16/12, 192.168/16, 169.254/16}: -I FORWARD/INPUT -d <r> -j REJECT
with the allowlist inserted above the rejects. It is SSRF protection. REJECT
rather than DROP is why every symptom was an instant "Connection refused" instead
of a timeout -- it reads like a dead service rather than a firewall.
That single rule explains the whole sequence. 10.43.x ClusterIPs, 10.42.x pods,
10.0.2.x nodes and the 192.168.6.x LAN pool were all rejected; 23.138.124.18 was
not, because it is a public address that merely happens to live on our own L2.
So every green run before this reached the fixture and the CA over publicly
routable addresses, and the failures began the moment the endpoint moved into
config as srql-fixture-rw.srql-fixtures.svc.cluster.local.
The fix is the supported knob, on BOTH fleets:
task_allowed_private_ips: [10.42.0.0/16, 10.43.0.0/16]
oci.dns: "10.43.0.10"
The pod CIDR is the non-obvious half. kube-proxy DNATs a ClusterIP to a pod IP in
nat PREROUTING, which runs BEFORE the filter FORWARD chain, so by the time these
rules match, the destination is already 10.42.x.x -- allowing only the service
CIDR leaves every ClusterIP connection rejected at its rewritten address.
Deliberately not 10.0.0.0/8, which would also expose the node subnet; 169.254/16
stays blocked, that is cloud metadata.
executor.oci.dns defaults to "8.8.8.8" and the runtime WRITES a resolv.conf
holding that one nameserver into every action container, then bind-mounts it over
/etc/resolv.conf (ociruntime.go:642 and :1340) -- which is why cluster.local came
back NXDOMAIN from a resolver that answered perfectly well. One CoreDNS address
serves both worlds: the Corefile ends in `forward . /etc/resolv.conf`, so it is
authoritative for cluster.local and forwards everything else.
WHAT I TRIED FIRST AND WAS WRONG ABOUT, recorded because the next person will
reach for both:
* `oci.dns: ""` alone. It bind-mounts the pod's resolv.conf, whose nameservers
the namespace could not reach while RFC1918 was blocked -- so it replaced one
failing lookup with EVERY lookup failing, public names included. Deployed and
reverted within minutes. With the allowlist in place it would work; the
explicit address is kept because that file also lists an IPv6 upstream this
namespace has no route to.
* `dockerNetwork: "host"`. Accepted by GetEffectiveDockerNetwork, ignored by the
OCI runtime, which reduces the property to `networkEnabled: mode != "off"` and
then always calls CreateContainerNetwork. Under `oci`, "", "bridge" and "host"
are the same thing. Host networking exists only for podman/docker isolation.
With the cluster reachable, ci.textproto names in-cluster endpoints for the
database, its TLS server name, and the CA bundle. Nothing on the CI path leaves
the cluster now -- which also matters because the published
srql-fixture-ca.serviceradar.cloud endpoint has been switched off and answers
404. buildbuddy_setup_fixture_env.sh follows, and drops the public fallback
rather than keeping a dead URL that would only add a misleading failure line.
Also here, from the same investigation:
* The build fleet's filecache goes 150 -> 100 GB, with the node arithmetic in
both files updated to match (worst case ~380 GB of 589 GB when a node also
carries the workflow fleet's 50 GB).
* The kubectl branch of the CA resolver now reports WHAT kubectl said. "no" and
"cannot reach the API server" have different fixes -- a RoleBinding versus a
route -- and `>/dev/null 2>&1` collapsed them into one indistinguishable line.
Verified: //config/... and //rust/integration-db/... 27/27 pass. The in-cluster
path was proven end to end against the live fixture from an action container, with
a deliberately wrong password, so the only remaining error is
`FATAL: password authentication failed for user "srql_hydra"` -- everything before
it (cluster DNS, ClusterIP routing, CA fetch, verify-full) succeeded.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…pile the integration targets
Two changes that the same CI step was hiding from each other.
MIGRATE_TEMPLATE
test/db/template_env.exs read SRQL_TEST_DATABASE_URL -- a whole DSN carrying
host, port, database, user, password and sslmode in one string -- and rewrote its
path to point at the template. That made a BuildBuddy secret the source of an
ENDPOINT: the fixture's address lived outside this repository, so it could not be
reviewed, could not differ per environment without a second secret, and had to be
parsed back apart to change any one component of it.
Now SERVICERADAR_ENV names an environment, //config/environments/<kind>.textproto
declares the coordinates as typed fields, and SecretManager resolves the one thing
that is actually secret. The database name is SUBSTITUTED into an assembled DSN
rather than rewritten into a parsed one.
The resolution lives in a new test/db/fixture_config.exs rather than in
template_env.exs, because test/db/integration_env.exs needs exactly the same thing
when the suite converts, and a second copy of "how to reach the fixture" is how the
two drift. It is loaded with `-r` for the same reason template_env.exs is: the
values must be in the environment before config/test.exs builds the Repo settings,
which is earlier than any module in lib/ exists.
System.put_env stays, and is not an oversight. It is a handoff INSIDE one OS
process to a Config script that runs before our code can hand it a value any other
way. What changed is where the values come from.
The instance is a DECLARED BUILD INPUT (//config/environments:{ci,localhost}_binpb
in `data`), so the configuration under test is the artifact the build just produced
and validated -- the same contract //rust/integration-db has, rather than a file
found on the ambient filesystem.
Supporting pieces: ServiceradarSecret.Names mirrors //config/manager_config/rust
secrets.rs, so both languages ask for the same logical name -- which matters
because that name is what EnvProvider.variable_for/1 turns into a variable, and a
component that spells it differently silently reads one nothing sets.
Verified against the live fixture from an action container with a deliberately
wrong password:
FATAL 28P01 (invalid_password) password authentication failed for user "srql"
Everything before authentication therefore succeeded: SERVICERADAR_ENV parsed, the
compiled instance located in runfiles and loaded, the DSN assembled with
connecting_role and the template database, the CA fetched from the in-cluster
bundle over cluster DNS, and TLS negotiated. Only the password was wrong, on
purpose.
THE COMPILE ERROR IT WAS HIDING
//integration_tests/srql failed the database sweep with
error[E0432]: unresolved import `serviceradar_integration_db`
no external crate `serviceradar_integration_db`
which reads like a missing Cargo dependency and is a missing BUILD dep:
tests/support/harness.rs resolves the fixture through //rust/integration-db, and
`all_crate_deps(cargo_only = True)` reports only third-party crates -- deliberately
-- so a first-party edge has to be named.
It reached CI because NOTHING COMPILED IT. These targets are `integration_test`-
tagged plus requires_shared_fixture(), and the earlier `bazel build //...` runs
without --//build:enable_integration_tests, so they are incompatible and a wildcard
SKIPS rather than builds them. The error could only appear in the last step, after
a fixture had been provisioned.
So //buildbuddy.yaml now passes that flag to the build step. It starts no database
and runs no test -- it only makes those targets compatible, so they are actually
compiled, and a missing dep fails in minute two instead of minute twenty. Checked
across the whole repo: `bazel build --//build:enable_integration_tests //...` is
exit 0 with no other target in this state.
Verified: `bazel test -c opt --config=ci //... --test_tag_filters=-integration_test,
-acceptance_test` -> 170/170 pass.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
The workflow changed configuration-affecting options at nearly every step, and
Bazel discards the analysis cache for the whole server whenever they change. With
~80k targets that is the dominant fixed cost of a run, and it was being paid four
times.
THE EXPENSIVE ONE was a missing `-c opt`. The database lifecycle's $FLAGS had none,
so its final `bazel test $FLAGS //...` did not merely re-analyse the repo -- it
RECOMPILED it, in fastbuild, sharing no action with the opt build two steps
earlier. A second full build of everything, to run the integration tests.
The rest is flag hygiene. Every invocation that touches //... now carries the same
`--//build:enable_integration_tests`. It runs nothing new anywhere: the unit sweep
still excludes integration_test by tag, and the lifecycle targets are `manual`, so
no wildcard reaches them. It is there so the value never toggles, because a
Starlark build setting is part of the configuration and flipping it between two
otherwise identical invocations is a full discard. The same reasoning adds both
settings to the `buildbuddy_setup_fixture_env` run, which reads neither.
Ordering carries the remainder. The go race step sets
`--@io_bazel_rules_go//go/config:{pure,race}` -- that IS the point of the step, so
it cannot share a configuration with anything else and its discard is unavoidable.
It used to sit between the unit sweep and the database lifecycle, where it cost two
discards instead of one; it now runs last. `//:images` moves ahead of the lifecycle
so the once-per-run `--//build:run_id` change is the final switch rather than one
in the middle.
Remaining, and deliberately not addressed here:
* `--//build:run_id` changes every run by construction, so it discards once per
run. Removing that would mean sourcing the id from workspace status
(`ctx.version_file`), which is designed for exactly this -- values that change
per build without invalidating analysis. That is a redesign of //build:run_id,
not a workflow edit, and //build/run_id.bzl's reasoning about it being a leaf
covers ACTION keys, not the analysis cache.
* The go race configuration could get its own `--output_base` so the main
server's analysis survives it entirely, trading disk and a first-run fetch.
Verified with CI's exact flags: build //... incl. integration targets exit 0; unit
sweep 170/170; //:images exit 0; go race 52/52.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
config/ had SEMANTICS.md, which specifies the validation engine formally, and
nothing that answered "what is this and how do I use it". The tree holds a schema,
five committed instances, a rule set, conformance fixtures and three
implementations each of three components; the entry point for someone meeting it
was a BUILD file comment.
This is the missing half: the one-variable contract, the layout and why it is one
tree, the compile pipeline from .textproto to the per-section artifacts, what
`load` always does, usage in all three languages for both managers, the
secret-name-to-variable transform, an inventory of what each guard prevents, and
recipes for adding a field, an environment or a secret.
Written against the source rather than from memory, which caught four things I
would otherwise have documented wrongly:
* file_phase_tests asserts every committed INSTANCE satisfies every file-phase
rule -- "the gate that makes the rule set real" -- not predicate semantics.
* Only Elixir and Go have committed bindings to refresh. Rust's come from
proto_bindings/rust/build.rs at build time, which is why there are four
binding_drift_* tests and none for Rust.
* All three languages resolve a field path through a hand-written match, so
adding a schema field means teaching three of them; an unhandled path is
`unknown field path`.
* predicate_law_tests asserts each predicate obeys its algebraic law and is
total.
Section 10 records the gaps rather than describing the intended end state as
though it were current: Go has no EnvProvider, so a Go service cannot resolve a
secret from the environment; nothing mounts /etc/serviceradar/environment.binpb,
so the deployed kinds have no instance to load; and adoption is partial. A README
that overstates readiness is worse than none, because the next person builds on
it.
Every Bazel target named here was checked to resolve, and the file is ASCII only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…guration # Conflicts: # buildbuddy_setup_fixture_env.sh # elixir/datasvc/.deps_audit_ignore # elixir/serviceradar_agent_gateway/.deps_audit_ignore # elixir/serviceradar_core/.deps_audit_ignore # elixir/serviceradar_core_elx/.deps_audit_ignore # elixir/web-ng/.deps_audit_ignore # elixir/web-ng/mix.exs # k8s/srql-fixtures/ca-bundle.yaml
--disk_cache went out when the caches were moved off the source tree and was never added back at a correct path. It returns on build:remote_base, machine-local under ~/.cache and capped, so a locally executed action that already ran is not re-run. Disabled on CI. `local_cache_size_bytes` in //k8s/buildbuddy is the executor's filecache; --disk_cache is the client's, and on CI the client runs inside the runner container on that same node disk, under root_directory. It would duplicate the executor cache outside the disk budget, and hold almost nothing anyway since --remote_download_minimal leaves remotely executed outputs remote. Also restores the rule about not putting caches under the source tree, which the distdir line still cited after it was deleted. Verified from a clean state: --config=ci creates no cache directory, --config=remote does, and a build succeeds under each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
--disk_cache, --repository_cache and --repo_contents_cache move to build:remote_base under ~/.cache/serviceradar-bazel. HOME is /root in a BuildBuddy action container (measured on an RBE executor), so the same path is valid on the runner and on a workstation, and --config=ci can no longer name a directory only the pods have -- it previously failed with "could not acquire lock on repo contents cache" anywhere else. Only the GC cap stays per profile. CI persistence comes from a new hostPath, /mnt/buildbuddy/bazel-caches, bind- mounted into action containers at /root/.cache/serviceradar-bazel, so the caches outlive pod replacement. Deliberately not under cache-volume: BuildBuddy's filecache assumes it owns local_cache_directory and evicts against its own accounting. Without the mount the paths still resolve inside the runner, so this is safe to land ahead of the helm upgrade -- it just does not persist yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
--disk_cache, --repository_cache and --repo_contents_cache sit on build:remote_base under ~/.cache/serviceradar-bazel. HOME is /root in an action container, so the same paths resolve on a runner and a workstation; ci and remote differ only in the GC cap, so interchanging them cannot name a missing path. Both fleets mount a dedicated hostPath at /root in action containers, separate from cache-volume: BuildBuddy's filecache assumes it owns local_cache_directory and evicts against its own accounting. Paths are distinct per fleet, matching the existing cache / cache-workflows split, and hostPath is per node so each executor keeps its own. Sharing one directory between replicas on a node is fine: Bazel documents the repo contents cache as holding fetched repo directories shareable across workspaces. Keying it per pod was rejected -- the only identity the chart exposes is K8S_POD_UID, which changes on restart and would discard the cache on the event it exists to survive. Deployed: build fleet rev 54, workflow fleet rev 14, both rolled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
--disk_cache, --repository_cache and --repo_contents_cache sit on build:remote_base under ~/.cache/serviceradar-bazel. HOME is /root in an action container, so the same paths resolve on a runner and a workstation; ci and remote differ only in the GC cap, so interchanging them cannot name a missing path. Both fleets mount a dedicated hostPath at /root in action containers, separate from cache-volume: BuildBuddy's filecache assumes it owns local_cache_directory and evicts against its own accounting. Paths are distinct per fleet, matching the existing cache / cache-workflows split, and hostPath is per node so each executor keeps its own. Sharing one directory between replicas on a node is fine: Bazel documents the repo contents cache as holding fetched repo directories shareable across workspaces. Keying it per pod was rejected -- the only identity the chart exposes is K8S_POD_UID, which changes on restart and would discard the cache on the event it exists to survive. Deployed: build fleet rev 54, workflow fleet rev 14, both rolled out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
…general build. Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Run two still re-fetched every external repo, and /bazel-caches on the workflow
node was 0 bytes despite the mount and the cache paths both being live.
The runner image sets no ENV HOME, so ci_runner.go ensureHomeDir redirects HOME to
{workdir}/.home, which is destroyed with the runner. Bazel wrote --disk_cache,
--repository_cache and --repo_contents_cache there every run and threw them away.
ensureHomeDir returns early when HOME is already set, so setting it in the action
env is enough to make ~ resolve to the mounted volume.
Ruled out first: the config was live (330e207 landed 06:08 UTC, run at 06:20),
and oci.mounts is not gated -- the authentication check is in
initPersistentVolumes, a different feature.
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
Two independent causes.
The Elixir shards failed at random because bootstrap_app_role! did check-then-
create on a CLUSTER-WIDE role: concurrent bootstraps both saw it absent, both ran
CREATE ROLE, and one raised 42710. It now converges on ALTER via the module's own
duplicate_ddl_error?/1. Production code, not test-only.
The three //integration_tests/srql targets were green while asserting nothing.
SRQL_IT_DATA carried neither the run id nor the compiled instance, so
Fixture::from_env() failed and the harness took its "no fixture, skip" path. With
the data declared they run, which surfaced the rest:
* all three reset database.database -- the shared fixture. Each now derives a
disposable per-target database and calls assert_disposable, the guard
//rust/integration-db already owned and this bypassed.
* the "admin" DSN resolved connecting_role, which ci.textproto denies CREATEDB.
* install_required_extensions used a bare parse, so sslmode=verify-full was
rejected rather than stripped.
* four tests share one binary and reset one database on parallel libtest
threads; RUST_TEST_THREADS=1 serializes them.
Removed what turned those into "flake": the 3x seed retry (it only ever helped
when another target was resetting underneath it) and unwrap_or(false) on the AGE
probe (it downgraded every AGE-gated assertion when the check itself broke).
The bootstrap scratch database was named outside the sr_core_test_ prefix that
sweep_stale collects, so the leak backstop its comment promised did not exist.
Verified: full unit gate 170/170; //rust/integration-db + //config 27/27 including
a new case pinning assert_disposable("srql_fixture"); all integration targets
compile under --//build:enable_integration_tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marvin Hansen <marvin.hansen@gmail.com>
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
2 participants
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.
Describe your changes
Feat/unified configuration
Issue ticket number and link
Code checklist before requesting a review