feat: add named ingresses map format to Bifrost Helm chart alongside legacy single ingress - #4312
Merged
Merged
Conversation
Contributor
Author
17 tasks
BearTS
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHelm chart templates now accept two ChangesIngress Format Dual Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Confidence Score: 4/5Safe to merge after fixing the format-detection logic in ingress.yaml and NOTES.txt; the legacy path is otherwise unchanged and the named-ingress rendering is correct for all non-edge cases. Both ingress.yaml and NOTES.txt share a detection loop that misidentifies a legacy ingress as a named-ingress map if its annotations block contains an "enabled" key — the result is zero Ingress resources rendered with no warning. This is a present defect on a real code path, not a theoretical one, though it requires an unusual annotation key to trigger. The fix is a one-liner in each file. helm-charts/bifrost/templates/ingress.yaml and helm-charts/bifrost/templates/NOTES.txt — both contain the same flawed $isLegacy detection block. Important Files Changed
Reviews (11): Last reviewed commit: "chore: multiple ingress profile" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/templates/ingress.yaml`:
- Around line 1-3: Update the chart NOTES output to detect and render both
legacy and named-ingress branches: when .Values.ingress is a map with named
entries, iterate over the map (e.g., range $name, $ing := .Values.ingress) and
for each entry that has enabled: true, iterate its hosts to print access
instructions; still preserve the legacy checks that read .Values.ingress.enabled
and .Values.ingress.hosts so existing single-ingress installs are unchanged.
Ensure the NOTES template mirrors the same conditional logic used in the ingress
template (the branch that handles named ingresses vs the legacy single ingress)
so named ingresses produce the same access output.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 159-203: The schema currently marks the per-ingress "enabled"
property optional, but templates check $ing.enabled in templates/ingress.yaml
and drop entries when it's missing; update the JSON schema for the
additionalProperties object (the per-ingress definition that contains "enabled",
"className", "annotations", "hosts", "tls") to make "enabled" required by adding
a "required": ["enabled"] array alongside its "properties" so every named
ingress entry must include the "enabled" boolean.
In `@helm-charts/bifrost/values.yaml`:
- Around line 90-112: Document that keys under the ingress map (the named
ingress entries) must conform to DNS-1123 label rules because the template uses
the raw map key in metadata.name as "<release>-<key>"; explicitly state allowed
characters (lowercase a–z, 0–9 and hyphen), no underscores or uppercase, must
start/end with alphanumeric, and respect the 63-character label length (and note
combined "<release>-<key>" may need to stay within k8s name limits). Add this
note adjacent to the example ingress block so users validate their keys before
using them.
- Around line 89-91: Update the comment "Named ingresses map (new format) —
replaces the single ingress above." to state this is an additional/alternative
format rather than a replacement so legacy single-ingress shape remains
supported; specifically change the wording around the "Named ingresses map (new
format)" comment that references "<release>-<key>" to clarify it can be used in
addition to the legacy single-ingress configuration and does not remove or
disable the existing single-ingress option in values.yaml.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 9550cf4c-5842-45d8-8117-4bf7c5d548dd
📒 Files selected for processing (3)
helm-charts/bifrost/templates/ingress.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 06:20
2508c71 to
ed594e8
Compare
BearTS
force-pushed
the
06-12-chore_add_mcp_library_url_to_helm
branch
from
June 12, 2026 06:20
468bf30 to
3b69f07
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
helm-charts/bifrost/values.yaml (1)
89-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify that the named-ingress block is an alternative, not a replacement.
This still reads as replacing the legacy ingress, which conflicts with the supported/default legacy path. Please reword it as an alternative format and keep the key-name constraint visible with the example.
Based on the prior review feedback, the current wording still risks confusing users about the chart contract.
Suggested wording
-# Named ingresses map (new format) — replaces the single ingress above. -# Each key becomes a separate Ingress resource named "<release>-<key>". +# Named ingresses map (new format) — alternative to the single ingress above. +# Each key becomes a separate Ingress resource named "<release>-<key>". +# Keys must be DNS-1123-safe.🤖 Prompt for 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. In `@helm-charts/bifrost/values.yaml` around lines 89 - 112, Update the comment describing the named ingresses block to say it is an alternative format to the legacy single ingress (not a replacement) and explicitly preserve the key-name constraint and example; change the first line from "replaces the single ingress above." to language like "alternative to the legacy single ingress (use when you need multiple ingress controllers)" and keep the example keys ("public", "internal") and the note that each key becomes a separate Ingress resource named "<release>-<key>" so the chart contract and naming constraint remain clear; ensure the wording also directs users to use this format only when they need multiple ingress controllers and does not imply the legacy `ingress:` block is removed or unsupported.
🤖 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/templates/ingress.yaml`:
- Line 56: The ingress name currently concatenates {{ include "bifrost.fullname"
$ }} and raw map key $name which can produce invalid/too-long Kubernetes
metadata.name; update the template to sanitize and bound the suffix by: 1)
normalizing $name (lowercase, replace invalid characters with '-', e.g. using
regexReplaceAll "[^a-z0-9-]" "-" and trim leading/trailing '-') and 2)
truncating the sanitized suffix so the final name length <= 63 characters
(compute maxSuffix = 63 - (len of include "bifrost.fullname" $) - 1 for the
hyphen and apply trunc to that length), then build the resource name as {{
include "bifrost.fullname" $ }}-{{ sanitizedAndTruncatedName }}; apply these
changes where name: is set to avoid invalid names.
In `@helm-charts/bifrost/templates/NOTES.txt`:
- Around line 10-20: The NOTES branch currently triggers for named-map shaped
.Values.ingress even when no named ingress is enabled; modify the template to
first detect whether at least one named ingress has enabled: iterate over (keys
.Values.ingress | sortAlpha), inspect each index $.Values.ingress . and its
$ing.enabled flag to set a boolean (e.g., $hasNamedEnabled), then change the
outer condition from else if and .Values.ingress (not (hasKey .Values.ingress
"enabled")) to also require $hasNamedEnabled so this block only renders when at
least one named ingress is enabled, otherwise fall back to the service access
instructions.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 550-554: The schema for the property "mcpLibraryUrl" currently
uses "type": "string" with "format": "uri" while the docs imply empty is
allowed; update the JSON Schema to explicitly express the intended contract:
either allow empty string by replacing the property with a oneOf that accepts {
"const": "" } and { "type": "string", "format": "uri" }, or if empty should be
rejected enforce non-empty URIs by adding "minLength": 1 alongside "format":
"uri"; modify the "mcpLibraryUrl" schema accordingly to remove ambiguity.
---
Duplicate comments:
In `@helm-charts/bifrost/values.yaml`:
- Around line 89-112: Update the comment describing the named ingresses block to
say it is an alternative format to the legacy single ingress (not a replacement)
and explicitly preserve the key-name constraint and example; change the first
line from "replaces the single ingress above." to language like "alternative to
the legacy single ingress (use when you need multiple ingress controllers)" and
keep the example keys ("public", "internal") and the note that each key becomes
a separate Ingress resource named "<release>-<key>" so the chart contract and
naming constraint remain clear; ensure the wording also directs users to use
this format only when they need multiple ingress controllers and does not imply
the legacy `ingress:` block is removed or unsupported.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 7d66cc02-5bac-41ef-803b-9721a79e91cf
📒 Files selected for processing (4)
helm-charts/bifrost/templates/NOTES.txthelm-charts/bifrost/templates/ingress.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (1)
helm-charts/bifrost/values.yaml (1)
89-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify that the named-ingress block is an alternative, not a replacement.
This still reads as replacing the legacy ingress, which conflicts with the supported/default legacy path. Please reword it as an alternative format and keep the key-name constraint visible with the example.
Based on the prior review feedback, the current wording still risks confusing users about the chart contract.
Suggested wording
-# Named ingresses map (new format) — replaces the single ingress above. -# Each key becomes a separate Ingress resource named "<release>-<key>". +# Named ingresses map (new format) — alternative to the single ingress above. +# Each key becomes a separate Ingress resource named "<release>-<key>". +# Keys must be DNS-1123-safe.🤖 Prompt for 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. In `@helm-charts/bifrost/values.yaml` around lines 89 - 112, Update the comment describing the named ingresses block to say it is an alternative format to the legacy single ingress (not a replacement) and explicitly preserve the key-name constraint and example; change the first line from "replaces the single ingress above." to language like "alternative to the legacy single ingress (use when you need multiple ingress controllers)" and keep the example keys ("public", "internal") and the note that each key becomes a separate Ingress resource named "<release>-<key>" so the chart contract and naming constraint remain clear; ensure the wording also directs users to use this format only when they need multiple ingress controllers and does not imply the legacy `ingress:` block is removed or unsupported.
🤖 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/templates/ingress.yaml`:
- Line 56: The ingress name currently concatenates {{ include "bifrost.fullname"
$ }} and raw map key $name which can produce invalid/too-long Kubernetes
metadata.name; update the template to sanitize and bound the suffix by: 1)
normalizing $name (lowercase, replace invalid characters with '-', e.g. using
regexReplaceAll "[^a-z0-9-]" "-" and trim leading/trailing '-') and 2)
truncating the sanitized suffix so the final name length <= 63 characters
(compute maxSuffix = 63 - (len of include "bifrost.fullname" $) - 1 for the
hyphen and apply trunc to that length), then build the resource name as {{
include "bifrost.fullname" $ }}-{{ sanitizedAndTruncatedName }}; apply these
changes where name: is set to avoid invalid names.
In `@helm-charts/bifrost/templates/NOTES.txt`:
- Around line 10-20: The NOTES branch currently triggers for named-map shaped
.Values.ingress even when no named ingress is enabled; modify the template to
first detect whether at least one named ingress has enabled: iterate over (keys
.Values.ingress | sortAlpha), inspect each index $.Values.ingress . and its
$ing.enabled flag to set a boolean (e.g., $hasNamedEnabled), then change the
outer condition from else if and .Values.ingress (not (hasKey .Values.ingress
"enabled")) to also require $hasNamedEnabled so this block only renders when at
least one named ingress is enabled, otherwise fall back to the service access
instructions.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 550-554: The schema for the property "mcpLibraryUrl" currently
uses "type": "string" with "format": "uri" while the docs imply empty is
allowed; update the JSON Schema to explicitly express the intended contract:
either allow empty string by replacing the property with a oneOf that accepts {
"const": "" } and { "type": "string", "format": "uri" }, or if empty should be
rejected enforce non-empty URIs by adding "minLength": 1 alongside "format":
"uri"; modify the "mcpLibraryUrl" schema accordingly to remove ambiguity.
---
Duplicate comments:
In `@helm-charts/bifrost/values.yaml`:
- Around line 89-112: Update the comment describing the named ingresses block to
say it is an alternative format to the legacy single ingress (not a replacement)
and explicitly preserve the key-name constraint and example; change the first
line from "replaces the single ingress above." to language like "alternative to
the legacy single ingress (use when you need multiple ingress controllers)" and
keep the example keys ("public", "internal") and the note that each key becomes
a separate Ingress resource named "<release>-<key>" so the chart contract and
naming constraint remain clear; ensure the wording also directs users to use
this format only when they need multiple ingress controllers and does not imply
the legacy `ingress:` block is removed or unsupported.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 7d66cc02-5bac-41ef-803b-9721a79e91cf
📒 Files selected for processing (4)
helm-charts/bifrost/templates/NOTES.txthelm-charts/bifrost/templates/ingress.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
🛑 Comments failed to post (3)
helm-charts/bifrost/templates/ingress.yaml (1)
56-56:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNamed ingress resource name can become invalid Kubernetes metadata.name.
Using the raw map key in the name can generate invalid/too-long resource names and break deployment for otherwise accepted values. Normalize and bound the suffix defensively in-template.
Suggested fix
- name: {{ include "bifrost.fullname" $ }}-{{ $name }} + {{- $base := include "bifrost.fullname" $ -}} + {{- $suffix := $name | lower | regexReplaceAll "[^a-z0-9-]+" "-" | trimAll "-" -}} + name: {{ printf "%s-%s" $base $suffix | trunc 63 | trimSuffix "-" }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{{- $base := include "bifrost.fullname" $ -}} {{- $suffix := $name | lower | regexReplaceAll "[^a-z0-9-]+" "-" | trimAll "-" -}} name: {{ printf "%s-%s" $base $suffix | trunc 63 | trimSuffix "-" }}🤖 Prompt for 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. In `@helm-charts/bifrost/templates/ingress.yaml` at line 56, The ingress name currently concatenates {{ include "bifrost.fullname" $ }} and raw map key $name which can produce invalid/too-long Kubernetes metadata.name; update the template to sanitize and bound the suffix by: 1) normalizing $name (lowercase, replace invalid characters with '-', e.g. using regexReplaceAll "[^a-z0-9-]" "-" and trim leading/trailing '-') and 2) truncating the sanitized suffix so the final name length <= 63 characters (compute maxSuffix = 63 - (len of include "bifrost.fullname" $) - 1 for the hyphen and apply trunc to that length), then build the resource name as {{ include "bifrost.fullname" $ }}-{{ sanitizedAndTruncatedName }}; apply these changes where name: is set to avoid invalid names.helm-charts/bifrost/templates/NOTES.txt (1)
10-20:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNamed-ingress NOTES branch masks fallback instructions when all named ingresses are disabled.
This branch runs for named-map shape even if no entry is enabled, so users get an empty “Access Bifrost at” section and miss service fallback instructions. Gate this branch on at least one enabled named ingress.
🤖 Prompt for 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. In `@helm-charts/bifrost/templates/NOTES.txt` around lines 10 - 20, The NOTES branch currently triggers for named-map shaped .Values.ingress even when no named ingress is enabled; modify the template to first detect whether at least one named ingress has enabled: iterate over (keys .Values.ingress | sortAlpha), inspect each index $.Values.ingress . and its $ing.enabled flag to set a boolean (e.g., $hasNamedEnabled), then change the outer condition from else if and .Values.ingress (not (hasKey .Values.ingress "enabled")) to also require $hasNamedEnabled so this block only renders when at least one named ingress is enabled, otherwise fall back to the service access instructions.helm-charts/bifrost/values.schema.json (1)
550-554:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify that the current schema rejects empty mcpLibraryUrl and accepts a URI. python - <<'PY' import json, sys from jsonschema import Draft201909Validator schema_path = "helm-charts/bifrost/values.schema.json" with open(schema_path) as f: schema = json.load(f) validator = Draft201909Validator(schema) cases = [ ("empty", {"bifrost": {"framework": {"mcpLibraryUrl": ""}}}), ("valid_uri", {"bifrost": {"framework": {"mcpLibraryUrl": "https://example.com/catalog.json"}}}), ] for name, doc in cases: errs = sorted(validator.iter_errors(doc), key=lambda e: e.path) print(name, "PASS" if not errs else "FAIL") for e in errs: print(" ", list(e.path), "-", e.message) PYRepository: maximhq/bifrost
Length of output: 85
Relaxed URI format vs empty default contract:
helm-charts/bifrost/values.schema.jsonusesbifrost.mcpLibraryUrlastype: "string"withformat: "uri"and docs implying it may be empty, andjsonschema’sFormatCheckeraccepts""forformat: "uri", so the schema doesn’t unambiguously enforce the intended contract—make the allowance explicit (e.g.,oneOfwithconst: ""orminLength: 1if empty should be rejected).🤖 Prompt for 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. In `@helm-charts/bifrost/values.schema.json` around lines 550 - 554, The schema for the property "mcpLibraryUrl" currently uses "type": "string" with "format": "uri" while the docs imply empty is allowed; update the JSON Schema to explicitly express the intended contract: either allow empty string by replacing the property with a oneOf that accepts { "const": "" } and { "type": "string", "format": "uri" }, or if empty should be rejected enforce non-empty URIs by adding "minLength": 1 alongside "format": "uri"; modify the "mcpLibraryUrl" schema accordingly to remove ambiguity.
BearTS
force-pushed
the
06-12-chore_add_mcp_library_url_to_helm
branch
from
June 12, 2026 08:39
3b69f07 to
99c8745
Compare
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 08:39
ed594e8 to
c892195
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
helm-charts/bifrost/values.yaml (1)
90-92: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd DNS-1123 naming constraint to the example comment block.
The schema documents that ingress map keys must conform to DNS-1123 label rules (lowercase letters, digits, and hyphens only; no underscores or uppercase) because the template uses the key directly in the resource name (
<release>-<key>). This constraint should also appear in the values.yaml comments so users editing this file understand the restriction without having to consult the schema.Suggested addition
# Named ingresses map (new format) — replaces the single ingress above. # Each key becomes a separate Ingress resource named "<release>-<key>". + # Keys must be DNS-1123 compliant (lowercase letters, digits, and hyphens only). # Use this when you need multiple ingress controllers (e.g. public + internal).🤖 Prompt for 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. In `@helm-charts/bifrost/values.yaml` around lines 90 - 92, Update the example comment for the ingress map to mention the DNS-1123 label constraint for map keys: state that each key becomes a separate Ingress resource named "<release>-<key>" and that keys must be DNS-1123 labels (lowercase letters, digits and hyphens only; no underscores or uppercase) so they are safe to use directly in the resource name; modify the comment block that currently starts with "# Each key becomes a separate Ingress resource named "<release>-<key>"." to include this rule.Source: Coding guidelines
🤖 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/templates/ingress.yaml`:
- Around line 1-3: The template currently uses hasKey .Values.ingress "enabled"
to pick legacy mode which misclassifies named ingresses; change the detection to
compute explicit flags (e.g., isLegacy = hasKey .Values.ingress "hosts" or
hasKey .Values.ingress "enabled", isNamed = hasKey .Values.ingress "public" or
hasKey .Values.ingress "internal") and branch on those flags so that legacy
logic (the block that expects .Values.ingress.hosts and .Values.ingress.enabled)
only runs when isLegacy is true, and the named-ingress logic runs when isNamed
is true; update conditions around .Values.ingress, hasKey, and the legacy/named
branches accordingly to avoid treating a default ingress.enabled = false as
legacy when public/internal keys are supplied.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 160-162: The schema currently documents DNS-1123 constraints for
the named ingresses map but does not enforce them; add a propertyNames
constraint on the object that uses "additionalProperties" (the named ingresses
map) to require keys match the DNS-1123 label regex (e.g.
^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$) so invalid keys like "Public_API" are
rejected before templates/ingress.yaml (where ingress names are generated) can
produce invalid Kubernetes resource names.
---
Duplicate comments:
In `@helm-charts/bifrost/values.yaml`:
- Around line 90-92: Update the example comment for the ingress map to mention
the DNS-1123 label constraint for map keys: state that each key becomes a
separate Ingress resource named "<release>-<key>" and that keys must be DNS-1123
labels (lowercase letters, digits and hyphens only; no underscores or uppercase)
so they are safe to use directly in the resource name; modify the comment block
that currently starts with "# Each key becomes a separate Ingress resource named
"<release>-<key>"." to include this rule.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 46d16a39-3f05-40c3-90e0-5ca72c3f120b
📒 Files selected for processing (4)
helm-charts/bifrost/templates/NOTES.txthelm-charts/bifrost/templates/ingress.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 08:53
c892195 to
1574229
Compare
BearTS
force-pushed
the
06-12-chore_add_mcp_library_url_to_helm
branch
from
June 12, 2026 08:53
99c8745 to
12a1062
Compare
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 08:59
1574229 to
22acb95
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm-charts/bifrost/values.schema.json (1)
2094-2110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
business_unitunless the mapping uses wildcard passthrough.Removing
business_unitfromrequiredunconditionally makes configs like{ attribute: "groups", value: "finance" }schema-valid even though there is no destination business unit to assign. The relaxation should apply only to thevalue: "*"passthrough case; non-wildcard mappings still need an explicitbusiness_unit.🧩 Suggested pattern to apply to each provider block
- "required": ["attribute", "value"], + "required": ["attribute", "value"], + "allOf": [ + { + "if": { + "properties": { + "value": { + "not": { "const": "*" } + } + }, + "required": ["value"] + }, + "then": { + "required": ["business_unit"] + } + } + ], "additionalProperties": falseAlso applies to: 2241-2255, 2363-2367, 2465-2469, 2577-2581
🤖 Prompt for 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. In `@helm-charts/bifrost/values.schema.json` around lines 2094 - 2110, Update the JSON schema for the mapping object so that business_unit is required for non-wildcard mappings but optional when value == "*": replace the unconditional "required": ["attribute","value"] for the mapping schema with a conditional subschema using "if": {"properties": {"value": {"const": "*"}}}, "then": { "required": ["attribute","value"] }, "else": { "required": ["attribute","value","business_unit"] }; keep additionalProperties:false and existing properties (attribute, value, business_unit, attributeType, attributeValue) intact so configs like {attribute, value: "*"} remain valid but any non-"*" value must include business_unit.
♻️ Duplicate comments (3)
helm-charts/bifrost/values.schema.json (1)
159-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the documented named-ingress key format.
This branch says ingress-map keys must be DNS-1123 label segments, but the schema still accepts keys like
Public_API. Those values then flow intohelm-charts/bifrost/templates/ingress.yamlLine 56 and can render invalid Kubernetes resource names.🧩 Suggested schema fix
{ "type": "object", "description": "Named ingresses map. Each key produces a separate Ingress named <release>-<key>. Keys must be valid DNS-1123 label segments (lowercase alphanumeric and hyphens only, no underscores or uppercase) so the resulting Ingress name is a valid Kubernetes resource name.", + "propertyNames": { + "type": "string", + "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", + "maxLength": 63 + }, "additionalProperties": {Based on learnings:
santhosh-tekuri/jsonschema/v6supports standard Draft 2019-09 keywords, sopropertyNamesis valid here.🤖 Prompt for 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. In `@helm-charts/bifrost/values.schema.json` around lines 159 - 161, The named-ingresses object schema currently allows invalid keys like "Public_API"; add a propertyNames constraint to the object (the schema entry with description "Named ingresses map") that enforces DNS-1123 label segments, e.g. a regex like ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ so keys are lowercase alphanumeric with optional internal hyphens and cannot start/end with a hyphen; update the schema by adding the propertyNames keyword referencing that pattern and keep additionalProperties as-is to validate each ingress value.Source: Learnings
helm-charts/bifrost/templates/ingress.yaml (1)
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegacy-mode detection still masks named ingresses on merged values.
Branching on
hasKey .Values.ingress "enabled"treats any merged ingress map that still carries the legacyenabledfield as single-ingress mode, so named entries never reach the loop at Lines 48-93. This breaks the common values-file overlay path for the new format;helm-charts/bifrost/templates/NOTES.txtLines 3-10 repeat the same predicate and need the same fix.🧩 One way to distinguish the two shapes explicitly
{{- if .Values.ingress }} -{{- if hasKey .Values.ingress "enabled" }} +{{- $namedIngresses := omit .Values.ingress "enabled" "className" "annotations" "hosts" "tls" }} +{{- if gt (len $namedIngresses) 0 }} +{{- /* Named ingresses map (new format) */}} +{{- range (keys $namedIngresses | sortAlpha) }} +{{- $name := . }} +{{- $ing := index $namedIngresses $name }} +{{- if $ing.enabled }} +--- +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: {{ include "bifrost.fullname" $ }}-{{ $name }} + namespace: {{ $.Release.Namespace }} + labels: + {{- include "bifrost.labels" $ | nindent 4 }} + {{- with $ing.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +spec: + {{- if $ing.className }} + ingressClassName: {{ $ing.className }} + {{- end }} + {{- if $ing.tls }} + tls: + {{- range $ing.tls }} + - hosts: + {{- range .hosts }} + - {{ . | quote }} + {{- end }} + secretName: {{ .secretName }} + {{- end }} + {{- end }} + rules: + {{- range $ing.hosts }} + - host: {{ .host | quote }} + http: + paths: + {{- range .paths }} + - path: {{ .path }} + pathType: {{ .pathType }} + backend: + service: + name: {{ include "bifrost.fullname" $ }} + port: + number: {{ $.Values.service.port }} + {{- end }} + {{- end }} +{{- end }} +{{- end }} +{{- else if hasKey .Values.ingress "enabled" }} {{- /* Single ingress (legacy format) */}}Also applies to: 46-50
🤖 Prompt for 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. In `@helm-charts/bifrost/templates/ingress.yaml` around lines 1 - 2, The current predicate treats any ingress map containing an "enabled" key as legacy single-ingress mode and hides named ingresses; update the check to only treat it as legacy when the ingress map contains only the "enabled" key (i.e. detect single-shape vs named-shape explicitly). Replace the `hasKey .Values.ingress "enabled"` check with a compound predicate using hasKey, keys and len (for example: hasKey .Values.ingress "enabled" and eq (len (keys .Values.ingress)) 1) so maps that include named entries alongside "enabled" are handled as the new named-ingress shape; apply the same change in the NOTES.txt predicate as well (use hasKey, keys, len, eq to disambiguate).helm-charts/bifrost/values.yaml (1)
89-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDNS-1123 constraint documentation still missing (flagged in past review, unresolved).
The comment block for the named ingresses example does not document the DNS-1123 naming constraint for map keys. The template renders keys directly into
metadata.nameas<release>-<key>, so users who use invalid characters (underscores, uppercase, etc.) will create broken Ingress resources. This was flagged in a previous review but remains absent from the current code.Suggested fix
# Named ingresses map (new format) — replaces the single ingress above. # Each key becomes a separate Ingress resource named "<release>-<key>". +# Keys must be DNS-1123-safe: lowercase letters, digits, and hyphens only; no underscores or uppercase. # Use this when you need multiple ingress controllers (e.g. public + internal).🤖 Prompt for 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. In `@helm-charts/bifrost/values.yaml` around lines 89 - 91, Update the named ingresses comment to explicitly document the DNS-1123 constraint: state that each map key becomes part of metadata.name as "<release>-<key>" and must be lowercase alphanumeric or '-', start/end with alphanumeric, and not contain underscores or uppercase letters (i.e., conform to DNS-1123 label rules). Reference the "named ingresses map" and the template behavior that renders keys into metadata.name so users know to sanitize keys before using them.
🤖 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.
Outside diff comments:
In `@helm-charts/bifrost/values.schema.json`:
- Around line 2094-2110: Update the JSON schema for the mapping object so that
business_unit is required for non-wildcard mappings but optional when value ==
"*": replace the unconditional "required": ["attribute","value"] for the mapping
schema with a conditional subschema using "if": {"properties": {"value":
{"const": "*"}}}, "then": { "required": ["attribute","value"] }, "else": {
"required": ["attribute","value","business_unit"] }; keep
additionalProperties:false and existing properties (attribute, value,
business_unit, attributeType, attributeValue) intact so configs like {attribute,
value: "*"} remain valid but any non-"*" value must include business_unit.
---
Duplicate comments:
In `@helm-charts/bifrost/templates/ingress.yaml`:
- Around line 1-2: The current predicate treats any ingress map containing an
"enabled" key as legacy single-ingress mode and hides named ingresses; update
the check to only treat it as legacy when the ingress map contains only the
"enabled" key (i.e. detect single-shape vs named-shape explicitly). Replace the
`hasKey .Values.ingress "enabled"` check with a compound predicate using hasKey,
keys and len (for example: hasKey .Values.ingress "enabled" and eq (len (keys
.Values.ingress)) 1) so maps that include named entries alongside "enabled" are
handled as the new named-ingress shape; apply the same change in the NOTES.txt
predicate as well (use hasKey, keys, len, eq to disambiguate).
In `@helm-charts/bifrost/values.schema.json`:
- Around line 159-161: The named-ingresses object schema currently allows
invalid keys like "Public_API"; add a propertyNames constraint to the object
(the schema entry with description "Named ingresses map") that enforces DNS-1123
label segments, e.g. a regex like ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ so keys are
lowercase alphanumeric with optional internal hyphens and cannot start/end with
a hyphen; update the schema by adding the propertyNames keyword referencing that
pattern and keep additionalProperties as-is to validate each ingress value.
In `@helm-charts/bifrost/values.yaml`:
- Around line 89-91: Update the named ingresses comment to explicitly document
the DNS-1123 constraint: state that each map key becomes part of metadata.name
as "<release>-<key>" and must be lowercase alphanumeric or '-', start/end with
alphanumeric, and not contain underscores or uppercase letters (i.e., conform to
DNS-1123 label rules). Reference the "named ingresses map" and the template
behavior that renders keys into metadata.name so users know to sanitize keys
before using them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f92b3e61-55ac-4296-a16d-de4097601ba9
📒 Files selected for processing (4)
helm-charts/bifrost/templates/NOTES.txthelm-charts/bifrost/templates/ingress.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
helm-charts/bifrost/values.yaml (1)
89-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDNS-1123 naming constraint for ingress map keys remains undocumented.
The template renders Ingress resource names as
<release>-<key>, so ingress map keys directly become part of the Kubernetes resource name. Keys must conform to DNS-1123 label rules (lowercase letters, digits, hyphens only; no underscores, uppercase, or leading/trailing non-alphanumeric characters). The previous review flagged this; it still needs to be documented here to prevent user misconfiguration.Suggested addition:
# Named ingresses map (new format) — alternative to the single ingress above. # Each key becomes a separate Ingress resource named "<release>-<key>". +# Keys must conform to DNS-1123 label rules: lowercase letters, digits, hyphens only. +# Example valid keys: public, internal, api-gateway. Invalid: Public_Key, api-PROD. # Use this when you need multiple ingress controllers (e.g. public + internal).🤖 Prompt for 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. In `@helm-charts/bifrost/values.yaml` around lines 89 - 112, Document that keys of the ingress map (the top-level ingress: map entries) are used verbatim in Ingress resource names as "<release>-<key>" and therefore must follow DNS-1123 label rules; update the values.yaml comment block near the ingress example to state the exact constraints (lowercase a–z, digits 0–9 and hyphens only; no underscores or uppercase; must start and end with an alphanumeric character; max 63 chars) and give a brief valid/invalid key example so users know to avoid characters like underscores or leading/trailing hyphens when naming ingress keys.helm-charts/bifrost/values.schema.json (1)
159-161:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the documented DNS-1123 key constraint.
Line 160 says the named ingress keys must be DNS-1123 label segments, but the schema still does not validate that. Invalid keys will pass schema validation and then be concatenated into the resource name in
templates/ingress.yamlLine 56.Suggested fix
{ "type": "object", "description": "Named ingresses map. Each key produces a separate Ingress named <release>-<key>. Keys must be valid DNS-1123 label segments (lowercase alphanumeric and hyphens only, no underscores or uppercase) so the resulting Ingress name is a valid Kubernetes resource name.", + "propertyNames": { + "type": "string", + "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", + "maxLength": 63 + }, "additionalProperties": { "type": "object",🤖 Prompt for 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. In `@helm-charts/bifrost/values.schema.json` around lines 159 - 161, The schema currently documents DNS-1123 key requirements but doesn't enforce them; add a propertyNames constraint on the named ingress object (the same object that currently has "additionalProperties") to validate keys as DNS-1123 label segments (e.g. pattern enforcing lowercase alphanumerics and hyphens, no leading/trailing hyphen, and maxLength 63) so invalid ingress keys fail schema validation before being concatenated into resource names (see the object with "additionalProperties" in values.schema.json and use propertyNames with a pattern and maxLength to implement this).
🤖 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 126-154: The JSON schema currently allows missing ingress
host/path fields that the ingress template (templates/ingress.yaml) renders
unconditionally; update the schema to require "hosts" (array) when ingress is
enabled and make each host object require "host" and "paths", and make each path
object require "path" and "pathType"; also tighten the "tls" array schema (and
mirror the same required changes for the other ingress block referenced around
the second occurrence) so that schema-valid values always provide the fields
templates/ingress.yaml expects ("hosts", "host", "paths", "path", "pathType",
"tls").
- Around line 113-157: The legacy ingress schema currently accepts any object
that contains "enabled", allowing mixed shapes (e.g., ingress: { enabled: false,
public: {...} }) to validate as legacy while templates/ingress.yaml treats
presence of top-level "hosts" as named mode; fix by tightening the legacy
branch: in the ingress legacy object (the object with "properties": {"enabled",
"className", "annotations", "hosts", "tls"} and "required":["enabled"]) add
"additionalProperties": false so only those legacy keys are allowed (i.e., only
enabled, className, annotations, hosts, tls), preventing mixed legacy/named
shapes from validating.
---
Duplicate comments:
In `@helm-charts/bifrost/values.schema.json`:
- Around line 159-161: The schema currently documents DNS-1123 key requirements
but doesn't enforce them; add a propertyNames constraint on the named ingress
object (the same object that currently has "additionalProperties") to validate
keys as DNS-1123 label segments (e.g. pattern enforcing lowercase alphanumerics
and hyphens, no leading/trailing hyphen, and maxLength 63) so invalid ingress
keys fail schema validation before being concatenated into resource names (see
the object with "additionalProperties" in values.schema.json and use
propertyNames with a pattern and maxLength to implement this).
In `@helm-charts/bifrost/values.yaml`:
- Around line 89-112: Document that keys of the ingress map (the top-level
ingress: map entries) are used verbatim in Ingress resource names as
"<release>-<key>" and therefore must follow DNS-1123 label rules; update the
values.yaml comment block near the ingress example to state the exact
constraints (lowercase a–z, digits 0–9 and hyphens only; no underscores or
uppercase; must start and end with an alphanumeric character; max 63 chars) and
give a brief valid/invalid key example so users know to avoid characters like
underscores or leading/trailing hyphens when naming ingress keys.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 4a5d48fd-c4c5-4de6-bf65-d2f8e0d71d84
📒 Files selected for processing (4)
helm-charts/bifrost/templates/NOTES.txthelm-charts/bifrost/templates/ingress.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
2 times, most recently
from
June 12, 2026 09:16
244a7fc to
c022c58
Compare
BearTS
force-pushed
the
06-12-chore_add_mcp_library_url_to_helm
branch
from
June 12, 2026 09:16
12a1062 to
903218f
Compare
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 09:31
c022c58 to
5de5e38
Compare
18 tasks
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 10:22
5de5e38 to
edbf745
Compare
BearTS
force-pushed
the
06-12-chore_add_mcp_library_url_to_helm
branch
from
June 12, 2026 10:22
903218f to
514290f
Compare
BearTS
changed the base branch from
06-12-chore_add_mcp_library_url_to_helm
to
graphite-base/4312
June 12, 2026 10:36
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 10:36
edbf745 to
f204f53
Compare
BearTS
force-pushed
the
graphite-base/4312
branch
from
June 12, 2026 10:36
514290f to
aa43c2e
Compare
BearTS
changed the base branch from
graphite-base/4312
to
06-12-feat_add_server.readbuffersize_in_helm_for_headers
June 12, 2026 10:36
18 tasks
BearTS
force-pushed
the
06-12-feat_add_server.readbuffersize_in_helm_for_headers
branch
from
June 12, 2026 10:42
aa43c2e to
8c35268
Compare
BearTS
force-pushed
the
06-12-chore_multiple_ingress_profile
branch
from
June 12, 2026 10:42
f204f53 to
7822ddb
Compare
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-12-feat_add_server.readbuffersize_in_helm_for_headers
to
graphite-base/4312
June 12, 2026 10:51
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.
Summary
The Bifrost Helm chart's ingress template previously only supported a single
Ingressresource. This PR adds support for a named ingresses map, allowing multipleIngressresources to be created from a singlevalues.yaml— useful when routing through multiple ingress controllers (e.g. a public-facing and an internal controller simultaneously).Changes
ingresscontains a top-levelenabledkey (legacy single-ingress format) or a map of named entries (new format). In the new format, each key produces a separateIngressresource named<release>-<key>.values.schema.jsoningress definition was updated to useoneOf, distinguishing between the legacy object (requiresenabled) and the new named map (must not have a top-levelenabledkey).values.yamlto guide users.values.yamlis preserved unchanged, so existing deployments are unaffected.Type of change
Affected areas
How to test
Breaking changes
The legacy
ingress.enabledformat continues to work without modification.Related issues
Security considerations
No new auth, secrets, or PII surface area introduced. Ingress routing rules follow the same patterns as before.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Documentation