fix: Helm chart - map perUserHeaderKeys into rendered config.json - #6034
fix: Helm chart - map perUserHeaderKeys into rendered config.json#6034CallumWayve wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Helm chart now accepts ChangesHelm MCP header configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change maps per-user header names into the rendered Helm configuration and adds matching schema and validation coverage; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ValuesSchema
participant HelmTemplate
participant RenderedConfig
participant ValidationScript
HelmValues->>ValuesSchema: Validate perUserHeaderKeys
HelmValues->>HelmTemplate: Provide MCP client configuration
HelmTemplate->>RenderedConfig: Render per_user_header_keys
ValidationScript->>RenderedConfig: Assert auth type and header keys
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm-charts/bifrost/values.schema.json`:
- Around line 6001-6007: Update the schema around perUserHeaderKeys to
conditionally enforce it when authType is "per_user_headers": require the
property, require at least one item, and reject whitespace-only header names
using the schema’s string constraints. Keep these rules scoped to that
authentication type so the field remains optional and ignored for other authType
values.
In `@helm-charts/bifrost/values.yaml`:
- Around line 527-534: The commented HTTP example in the values configuration is
missing the required httpConfig structure. Update the example identified by
“example-per-user-headers-mcp” to define httpConfig with its url field using the
existing HTTP example shape, and remove or replace the standalone
connectionString so it passes schema validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 52cee6f7-5506-4d57-b7be-2a17377d7d84
📒 Files selected for processing (5)
.github/workflows/scripts/validate-helm-config-fields.shhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
The merge-base changed after approval.
|
Hey @CallumWayve I am happy to take a look at this, can you please rebase the PR and ping me |
|
@CallumWayve you might need to rebase this branch 🙇 |
94e1865 to
30b6cab
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@BearTS Rebased now |
The merge-base changed after approval.
244a01d to
ce1b2a6
Compare
30b6cab to
4638e55
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@BearTS Rebased this again now, should be entirely up to date |
The merge-base changed after approval.
MCPClientConfig.PerUserHeaderKeys (per_user_header_keys) is required (non-empty) for authType per_user_headers MCP clients, but the clientConfigs -> config.json template in _helpers.tpl never mapped it, so it silently dropped out of Helm-rendered config regardless of what was set in values.yaml. values.schema.json also never declared the property. Fixes maximhq#6033. Affected packages: - helm-charts/bifrost/values.schema.json - helm-charts/bifrost/templates/_helpers.tpl - helm-charts/bifrost/values.yaml - helm-charts/bifrost/README.md - .github/workflows/scripts/validate-helm-config-fields.sh
…er_headers Address review feedback on the previous commit: - values.schema.json now requires perUserHeaderKeys (non-empty, no whitespace-only entries) via an allOf/if/then keyed on authType, scoped to per_user_headers only — other auth types are unaffected. - values.yaml's per_user_headers example used a bare connectionString with connectionType: http, which the schema requires httpConfig for; fixed to use httpConfig.url like the adjacent http example. Affected packages: - helm-charts/bifrost/values.schema.json - helm-charts/bifrost/values.yaml
4638e55 to
a439196
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Hi @BearTS, this has been rebased again and is ready for review. |
3 participants
Summary
Fixes #6033.
MCPClientConfig.PerUserHeaderKeys(per_user_header_keysintransports/config.schema.json) is required (non-empty) forauthType: per_user_headersMCP clients — it declares the header names each caller must individually supply (e.g.Authorization). It's fully wired through the Go core, config store, and API (credential store, client manager, MCP handlers, reconciliation hash), but the Helm chart never mapped it:_helpers.tpl'sclientConfigs→config.jsonloop explicitly copies each field one at a time, and there was no case forperUserHeaderKeys.values.schema.jsonalso never declared the property. So setting it underbifrost.mcp.clientConfigs[]in Helm values was silently dropped, even thoughauthType: per_user_headersitself renders fine.Changes
helm-charts/bifrost/values.schema.json— addedperUserHeaderKeys(array of strings) tomcpClientConfig, matching the existingallowedExtraHeadersshape.helm-charts/bifrost/templates/_helpers.tpl— mapsperUserHeaderKeys→per_user_header_keysin renderedconfig.json, same pattern as the adjacentallowedExtraHeadersmapping.helm-charts/bifrost/values.yaml— added a commentedper_user_headersexample next to the existingoauth/oauthConfigIdexample.helm-charts/bifrost/README.md— documented under### Upcoming..github/workflows/scripts/validate-helm-config-fields.sh— added a fourth MCP test client (per-user-headers-server) assertingauth_typeandper_user_header_keys[0..1]render correctly.Type of change
Affected areas
How to test
All pass locally, including the new MCP assertions for
per_user_header_keys.Breaking changes
Related issues
Fixes #6033
Security considerations
None — this only affects which header names an admin declares as required; header values continue to be collected and stored per-identity through the existing per-user-headers submission flow, never through this static config.
Checklist
docs/contributing/README.mdand followed the guidelines