Add RunConfig seam for injected middleware configs#5495
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5495 +/- ##
==========================================
+ Coverage 69.64% 69.65% +0.01%
==========================================
Files 642 644 +2
Lines 65397 65556 +159
==========================================
+ Hits 45543 45664 +121
- Misses 16514 16542 +28
- Partials 3340 3350 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-agent review summary
Verdict: no blocking issues. This is a small, purely additive seam (~45 lines of production code; the rest is tests and regenerated swagger) that lets external-auth handlers inject pre-built middleware configs into a RunConfig so they survive PopulateMiddlewareConfigs. The approach is sound, all six acceptance criteria of stacklok-enterprise-platform#1666 are met, the splice position is verified correct (after auth, after the egress group, immediately before recovery), serialization round-trips, and the upstream OBO-agnostic boundary holds — no enterprise symbol appears in production code; only tests reference obo.MiddlewareType.
Verified correct
PopulateMiddlewareConfigsis idempotent — it overwritesconfig.MiddlewareConfigswith a freshly-built local slice (middleware.go:242), so re-running it never duplicates the injected entry. (An agent flagged a duplication risk here; on inspection it does not exist.)- Backwards/forwards compatible:
omitempty; older readers ignore the unknown field, newer readers treat a missing field as a no-op splice. - Pointer-variadic option signature matches sibling options (
WithAuthzConfig,WithTokenExchangeConfig,WithAWSStsConfig). - Tests are load-bearing: deleting the splice fails via
indexOfMiddleware'st.Fatalf; mis-positioning fails via themws[0]==auth/oboIdx==len-2assertions; a real JSON+YAML round-trip exercises the proxyrunner read path.
Non-blocking suggestions (4 inline comments, all LOW)
- Document that the splice is operator-path-only —
WithMiddlewareFromFlags(CLI path) ignores the field. - The carrier persists redundantly in the serialized ConfigMap (injected entry appears in both
middleware_configsandadditional_middleware_configs); consider clearing it after the splice. - Add a test pinning the idempotency invariant the design relies on.
- Document that injected
Typevalues must not shadow typed-field middleware types (no validation today).
Out of scope / not actionable here: recovery being innermost (so it doesn't wrap outer middleware) is pre-existing on main and applies to all middleware equally; this PR's comment only claims "final say on the outbound request," which is accurate.
4 specialist reviewers (general quality, security, Go correctness, test coverage). Codex cross-review skipped (CLI not installed).
🤖 Generated with Claude Code
Addresses #5495 review comments: - LOW pkg/runner/config_builder.go (3397799561): note the injected configs are consumed only by PopulateMiddlewareConfigs (operator path); the CLI flag path ignores AdditionalMiddlewareConfigs - LOW pkg/runner/middleware.go (3397799567): document that the carrier is intentionally not cleared — idempotent overwrite plus serialized fallback - LOW pkg/runner/middleware_test.go (3397799570): add a subtest pinning the re-population idempotency invariant (injected type appears exactly once) - LOW pkg/runner/config.go (3397799572): document the no-shadowing constraint on injected Type values Regenerated the server swagger spec for the updated field doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses #5495 review comment: - pkg/runner/config_builder.go (3399013843): document that injected middleware Parameters are serialized verbatim into the operator's plaintext ConfigMap, so handlers must reference secrets by env-var name and resolve them at runtime rather than embedding raw credentials — mirroring the typed token-exchange middleware's ClientSecret convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The operator's external-auth handlers (e.g. the enterprise OBO handler) build a complete types.MiddlewareConfig and need a way to hand it to the RunConfig the proxyrunner reads. Upstream had no such path: the builder exposed no append-style middleware option, and PopulateMiddlewareConfigs rebuilds MiddlewareConfigs from typed fields, overwriting anything a handler set. The runtime factory for these types is already registered, so the only missing piece is an operator-to-RunConfig carrier. Add a generic, OBO-agnostic seam in pkg/runner so a handler reached via *[]RunConfigBuilderOption can inject pre-built configs that survive population: - RunConfig.AdditionalMiddlewareConfigs: JSON/YAML-serializable carrier for handler-supplied middleware configs. - WithAdditionalMiddlewareConfigs: builder option with append semantics (multiple calls/args additive, nil entries skipped). - PopulateMiddlewareConfigs splices the carrier into the backend-egress group — after auth and header-forward, before recovery — so an injected egress middleware has the final say on the outbound request. Upstream carries the configs verbatim and never inspects their parameters; the type identity is supplied by the caller. No OBO or enterprise symbol is introduced. Regenerated the server swagger spec for the new field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses #5495 review comments: - LOW pkg/runner/config_builder.go (3397799561): note the injected configs are consumed only by PopulateMiddlewareConfigs (operator path); the CLI flag path ignores AdditionalMiddlewareConfigs - LOW pkg/runner/middleware.go (3397799567): document that the carrier is intentionally not cleared — idempotent overwrite plus serialized fallback - LOW pkg/runner/middleware_test.go (3397799570): add a subtest pinning the re-population idempotency invariant (injected type appears exactly once) - LOW pkg/runner/config.go (3397799572): document the no-shadowing constraint on injected Type values Regenerated the server swagger spec for the updated field doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses #5495 review comment: - pkg/runner/config_builder.go (3399013843): document that injected middleware Parameters are serialized verbatim into the operator's plaintext ConfigMap, so handlers must reference secrets by env-var name and resolve them at runtime rather than embedding raw credentials — mirroring the typed token-exchange middleware's ClientSecret convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
The operator's external-auth handlers (for example the enterprise OBO handler) build a complete
types.MiddlewareConfig, but upstream had no way to hand that config to theRunConfigthe proxyrunner reads. The builder exposed no append-style middleware option, andPopulateMiddlewareConfigsrebuildsMiddlewareConfigspurely from typedRunConfigfields, overwriting anything a handler had set. Since the runtime factory for these middleware types is already registered upstream, the only missing piece was an operator-to-RunConfig carrier.This PR adds a generic, OBO-agnostic seam in
pkg/runnerso a handler reached via*[]RunConfigBuilderOptioncan inject pre-built middleware configs that survive population. It is an upstream prerequisite for the enterprise on-behalf-of (OBO) operator handler work.RunConfig; populated configs were rebuilt from typed fields and silently discarded handler-supplied entries.RunConfig.AdditionalMiddlewareConfigscarrier field, aWithAdditionalMiddlewareConfigsbuilder option with append semantics, and a splice inPopulateMiddlewareConfigsthat places injected configs in the backend-egress group (after auth and header-forward, before recovery).The carrier is fully generic: upstream moves the configs verbatim and never inspects their parameters; the middleware type identity is supplied by the caller via
types.MiddlewareConfig.Type. No OBO or enterprise symbol is introduced upstream.Type of change
Test plan
task test)task test-e2e)task lint-fix)New unit tests cover the seam end to end:
TestWithAdditionalMiddlewareConfigs— append semantics across multiple calls and multiple arguments, order preservation, nil-entry skipping, and empty-call leaving the slice nil.TestPopulateMiddlewareConfigs_AdditionalMiddlewareConfigs— injected configs survive population and land after auth and header-forward and immediately before recovery, with opaque parameters carried verbatim and a no-injection case leaving the chain unchanged.TestPopulateMiddlewareConfigs_AdditionalMiddlewareConfigs_RoundTrip— full operator path (NewOperatorRunConfigBuilder+PopulateMiddlewareConfigs) confirming an injected config survives JSON and YAML serialization and dispatches to a registered factory.task testpassed (full unit suite, exit 0).task lint-fixreported the changed files clean; the only flagged issue (cmd/thv/app/upgrade.go:204gosec G115) is pre-existing onorigin/mainand outside this diff.API Compatibility
This PR does not touch the operator
v1beta1API surface. The new field lives onpkg/runner.RunConfig(the proxyrunner config), not on a CRD.Changes
pkg/runner/config.goAdditionalMiddlewareConfigs []types.MiddlewareConfigcarrier field (JSON/YAML-serializable).pkg/runner/config_builder.goWithAdditionalMiddlewareConfigs(...*types.MiddlewareConfig)builder option with additive append semantics, skipping nil entries.pkg/runner/middleware.goPopulateMiddlewareConfigs— after auth and header-forward, before recovery.pkg/runner/config_builder_test.goWithAdditionalMiddlewareConfigs.pkg/runner/middleware_test.godocs/server/docs.go,docs/server/swagger.json,docs/server/swagger.yamlDoes this introduce a user-facing change?
No. This adds an internal builder option and
RunConfigfield for handler-supplied middleware configs. No CLI surface or default behavior changes; the new field isomitemptyand the splice is a no-op when nothing is injected.Special notes for reviewers
Generated with Claude Code