Repository navigation
Replies: 3 comments
Review: does the proposed API force a standard on its users?This comment covers two things: an adversarial review of the proposal above, and one review comment raised separately, which is that the baggage key must not be a constant. The question that matters most is whether bedrock forces wire-level conventions on the organisations that use it. FIT isn't an open standard, and bedrock is a foundation for teams that build their own frameworks. Why this mattersAn organisation's edge decides which requests are under test. It is often an API gateway such as Envoy, Zuul, Kong or a cloud load balancer, and a different team usually owns it. In a large organisation, the teams that own application services and the teams that own infrastructure such as edge gateways are not the same people. Infrastructure changes also get much harder scrutiny than application changes. So any wire convention bedrock forces turns an application team's choice of library into a change request for the infrastructure team. The organisation should be able to standardize on its own baggage keys and values, and bedrock should work with them. VerdictYes, the proposal forces a standard, and the baggage key is only one part of it. The proposal says the package "assumes nothing about the architecture" and "imposes no naming scheme". Neither is true. The only way to write or evaluate a scenario is through
An Envoy, Kong or ALB edge would have to produce all of these. Making the key configurable is necessary, but on its own it isn't enough. The edge team would still have to ship bedrock's grammar, an HMAC implementation and a shared secret, which is a bigger change request than picking a key name. Only two things genuinely need to be fixed:
Nothing on the wire has to be fixed. Forced conventions
The other side of the argument. Within one organisation, every service has to agree on a format, and a ready-made signed format is useful. Most teams don't want to design an HMAC envelope. The flaw is only that bedrock's format is the sole option, so bedrock chooses instead of the organisation. The original reasoning contradicts itself. The proposal rejects ID-based designs because they "assume an architecture". Forbidding them is also an architecture choice. With a Bugs in the original prototypeEach of these is shown by a test in the collapsible section Tests against the original prototype below.
Revised corepackage fit
type Fault struct{ Delay time.Duration; Abort bool } // Wait, Inject and ErrInjected unchanged
type Rule struct{ Point string; Fault Fault } // no character set
type Scenario struct{ Rules []Rule } // Expires removed
func (Scenario) Match(point string) (Rule, bool)
// Codec owns every wire convention: the baggage key or keys, the grammar,
// the version, the size limit and trust.
type Codec interface {
Encode(b baggage.Baggage, s Scenario) (baggage.Baggage, error)
Decode(b baggage.Baggage) (s Scenario, ok bool, err error)
Strip(b baggage.Baggage) baggage.Baggage
}
func ContextWithScenario(ctx context.Context, c Codec, s Scenario) (context.Context, error)
func ScenarioFromContext(ctx context.Context, c Codec) (Scenario, bool, error)
func ContextWithoutScenario(ctx context.Context, c Codec) context.Context
type Evaluation struct{ Point string; Rule Rule; Matched bool; Fault Fault; Err error }
type Evaluator struct { // every field is required
Codec Codec
MaxDelay time.Duration // clamps longer delays instead of rejecting them
Observe func(context.Context, Evaluation) // also receives scenarios the Codec rejected
}
func (Evaluator) Eval(ctx context.Context, point string) Fault // a rejected scenario injects nothingErrors are no longer returned from The revised prototype's
Recommendations beyond the review
PlacementWith the convention-setting parts removed, "assumes no transport and no topology" is finally true, so the case for bedrock is stronger than before. Because There is an alternative. If Revised prototype
|
Follow-up: which calls happen at startup and which per request?This applies to the revised API in the review comment above. The per-call costs below come from reading the code. They have not been measured yet. In bedrock's model, "startup" means inside
Gaps this exposes in the revised API
|
|
Implemented in #583, #584 and #585 and released as |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Note
The API and wire format in this post are superseded by the review comment below. This draft forces wire conventions on its users: the
fitbaggage key constant, the value grammar, the HMAC envelope, the required expiry and the character set for injection point names. The revision moves all of them into a caller-suppliedCodec, so the organisation's edge can keep its own baggage keys and formats. The FIT background below still applies. The rest of this post is unchanged and kept for context.Summary
Add a small package for request-scoped failure injection, modelled on Netflix's FIT (Failure Injection Testing). It uses OpenTelemetry baggage as the failure context.
The package does only two things:
context.Contextand reads it back;Moving the OTel context between services is the caller's job, using their own propagators. The package assumes nothing about the architecture: no gateway, no control plane, no transport and no naming scheme. Later, humus could provide protocol clients (HTTP, gRPC, Kafka) that standardize how injection points use it.
Background: how FIT works
From the 2014 post by Kolton Andrus, Naresh Gopalani and Ben Schmaus:
Two later papers point the same way:
Proposed API
Usage at the edge and at an injection point:
Design decisions
Faultspares its points. That covers both of FIT's scenario shapes: "fail X" and "fail everything except X".*. The package imposes no naming scheme; protocol clients choose one.Faultis onlyDelayplusAbort, FIT's two fault types. Making an abort look real is the injection point's job, as it was in FIT. An HTTP client might return a 503 or a transport error, and a gRPC client might returncodes.Unavailable. A client callsWaitand checksAbortto do this itself;Injectis the generic fallback.baggage.ContextWithBaggage,baggage.FromContext) rather than Inject/Extract. In OTel, Inject and Extract are what propagators do to carriers, and that is the part this package leaves to the caller. "Inject" also already means something else in a fault-injection package.fit.evaluationevent to the current span. It is modelled on OTel'sfeature_flag.evaluationevent. A scenario with no rules injects nothing, yet the request's trace still lists every injection point it reached. This gives FIT's tracing-based discovery of injection points for free.trace.SpanFromContextrather than a global tracer provider, and the clock isInjector.Now.Injectormissing a required field is a programming error, so it panics.Trust
OTel baggage has no integrity check, and services commonly accept baggage from any caller. For example, humus's REST server registers the W3C baggage propagator (
rest/rest.go#L256-L261) and wraps every handler inotelhttp.NewHandler(rest/rest.go#L286-L288). With an unauthenticated format, any external caller could sendbaggage: fit=…*=delay:…and tie up goroutines and connections at every hop. Envoy's fault filter docs give the same warning about itsx-envoy-fault-*headers. So the proposal adds four safeguards:Keys[0]signs and any key verifies.Expires. OTel auto-instrumentation forwards baggage to third-party APIs, so scenarios will leak, and a leaked scenario has to stop working.MaxDelay. Even a validly signed scenario can only cost a bounded amount.ContextWithoutScenario. Call it wherever requests cross a trust boundary, in either direction.All of this is about 80 lines of standard-library crypto.
Wire format
version~key id~expiry (unix seconds)~rules~mac. A rule is a point pattern, optionally followed by=abort,=delay:<ms>or=delay:<ms>+abort.Exampletest below checks this.baggage.Parsereturns empty baggage for a header over 8192 bytes, so an oversized scenario would wipe every other baggage entry downstream.Where it should live: bedrock or humus?
The proposal is to make it a nested module in the bedrock repository,
github.com/z5labs/bedrock/fit, with its owngo.mod.runtime/oteland the Renovate rules for OTel (see story(otel): inline OpenTelemetry setup in humus humus#623). An OTel package in the root module would undo that in three ways:fit/go.modrequires onlygo.opentelemetry.io/otelandgo.opentelemetry.io/otel/trace. It has three costs:go build/vet/test ./...in.github/workflows/build.yamldoes not descend into nested modules.fit/vX.Y.Ztags).fitimports only the OTel API".humus/fitrather than loosening bedrock's standard-library-only rule.What humus would build on top
grpc:<rpc method>,http:<server.address>,kafka:<messaging.destination.name>.Injectorfrom config (keys andMaxDelay) through bedrock'sEnvironment.Open questions
+abort:throttled.fitreads asfit.Faultandfit.Scenario;faultwould stutter asfault.Fault.Prototype
A working prototype is below.
go vet,go test -raceandgolangci-lintwith bedrock's.golangci.yamlall pass.go.opentelemetry.io/otel,go.opentelemetry.io/otel/traceandgithub.com/cespare/xxhash/v2.go.modfit.goinjector.gofit_test.goReferences
All reactions