-
Notifications
You must be signed in to change notification settings - Fork 30
fix: allow CORS wildcard for Rust server and refactor v1-config to helper template #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
db0c8fd
fix: allow CORS wildcard for Rust server and refactor v1-config to he…
tazarov e5548aa
docs: update README for CORS wildcard support and extraConfig
tazarov 6c10f3f
fix: address PR feedback - remove dead code, show stderr, warn about …
tazarov 5306b34
fix: add input validation guards, fix docs, and remove ISSUE_CONTEXT.md
tazarov bed9102
fix: correct README WARNING and clarify pre-1.0 test scope
tazarov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -120,6 +120,46 @@ Get the chroma api version | |||||||||||||||||
| {{- end }} | ||||||||||||||||||
| {{- end }} | ||||||||||||||||||
|
|
||||||||||||||||||
| {{/* | ||||||||||||||||||
| Build the server config dict for the v1-config ConfigMap. | ||||||||||||||||||
| */}} | ||||||||||||||||||
| {{- define "chromadb.serverConfig" -}} | ||||||||||||||||||
| {{- $port := .Values.chromadb.serverHttpPort | int -}} | ||||||||||||||||||
| {{- if le $port 0 -}} | ||||||||||||||||||
| {{- fail (printf "chromadb.serverHttpPort must be a positive integer, got: %v" .Values.chromadb.serverHttpPort) -}} | ||||||||||||||||||
| {{- end -}} | ||||||||||||||||||
| {{- $maxPayload := .Values.chromadb.maxPayloadSizeBytes | int64 -}} | ||||||||||||||||||
| {{- if le $maxPayload 0 -}} | ||||||||||||||||||
| {{- fail (printf "chromadb.maxPayloadSizeBytes must be a positive integer, got: %v" .Values.chromadb.maxPayloadSizeBytes) -}} | ||||||||||||||||||
| {{- end -}} | ||||||||||||||||||
| {{- $config := dict -}} | ||||||||||||||||||
| {{- $_ := set $config "port" $port -}} | ||||||||||||||||||
| {{- $_ := set $config "listen_address" .Values.chromadb.serverHost -}} | ||||||||||||||||||
| {{- $_ := set $config "max_payload_size_bytes" $maxPayload -}} | ||||||||||||||||||
| {{- $_ := set $config "persist_path" .Values.chromadb.persistDirectory -}} | ||||||||||||||||||
| {{- $_ := set $config "allow_reset" .Values.chromadb.allowReset -}} | ||||||||||||||||||
| {{- if .Values.chromadb.corsAllowOrigins -}} | ||||||||||||||||||
| {{- $_ := set $config "cors_allow_origins" .Values.chromadb.corsAllowOrigins -}} | ||||||||||||||||||
| {{- end -}} | ||||||||||||||||||
| {{- if .Values.chromadb.telemetry.enabled -}} | ||||||||||||||||||
| {{- if not .Values.chromadb.telemetry.endpoint -}} | ||||||||||||||||||
| {{- fail "chromadb.telemetry.endpoint must be set when chromadb.telemetry.enabled is true" -}} | ||||||||||||||||||
| {{- end -}} | ||||||||||||||||||
| {{- $otel := dict "service_name" .Values.chromadb.telemetry.serviceName "endpoint" .Values.chromadb.telemetry.endpoint -}} | ||||||||||||||||||
| {{- $_ := set $config "open_telemetry" $otel -}} | ||||||||||||||||||
| {{- end -}} | ||||||||||||||||||
| {{- with .Values.chromadb.extraConfig -}} | ||||||||||||||||||
|
||||||||||||||||||
| {{- with .Values.chromadb.extraConfig -}} | |
| {{- with .Values.chromadb.extraConfig -}} | |
| {{- $disallowedKeys := list "auth" "authentication" "auth_token" "jwt" "security" -}} | |
| {{- range $k := $disallowedKeys -}} | |
| {{- if hasKey . $k -}} | |
| {{- fail (printf "chromadb.extraConfig cannot set '%s' because it is security-sensitive; use chromadb.auth.* Helm values instead" $k) -}} | |
| {{- end -}} | |
| {{- end -}} |
Copilot
AI
Feb 17, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic for extraConfig.port has a type conversion issue. Line 154 converts the config value to int, but if a user provides a string value in extraConfig (e.g., "9999"), the comparison ne (get $config "port" | int) ($port) may behave unexpectedly. The left side converts the potentially overridden value to int, while $port is already an int. This works, but the error message on line 155 uses (get $config "port") without the int conversion, which could display a string in the error message while comparing against an int. For consistency, either use (get $config "port" | int) in the error message or ensure type consistency throughout.
Suggested change
| {{- fail (printf "extraConfig.port (%v) conflicts with chromadb.serverHttpPort (%v) — update serverHttpPort instead" (get $config "port") $.Values.chromadb.serverHttpPort) -}} | |
| {{- fail (printf "extraConfig.port (%v) conflicts with chromadb.serverHttpPort (%v) — update serverHttpPort instead" (get $config "port" | int) $.Values.chromadb.serverHttpPort) -}} |
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,135 @@ | ||||||||||||||||
| #!/usr/bin/env bash | ||||||||||||||||
| set -euo pipefail | ||||||||||||||||
|
|
||||||||||||||||
| CHART_DIR="$(cd "$(dirname "$0")/../charts/chromadb-chart" && pwd)" | ||||||||||||||||
| PASS=0 | ||||||||||||||||
| FAIL=0 | ||||||||||||||||
|
|
||||||||||||||||
| assert_config_key() { | ||||||||||||||||
| local desc="$1" yaml="$2" key="$3" expected="$4" | ||||||||||||||||
| actual=$(echo "$yaml" | yq eval ".$key" -) | ||||||||||||||||
| if [ "$actual" = "$expected" ]; then | ||||||||||||||||
| echo " PASS: $desc" | ||||||||||||||||
| PASS=$((PASS+1)) | ||||||||||||||||
| else | ||||||||||||||||
| echo " FAIL: $desc (expected '$expected', got '$actual')" | ||||||||||||||||
| FAIL=$((FAIL+1)) | ||||||||||||||||
| fi | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| assert_config_key_missing() { | ||||||||||||||||
| local desc="$1" yaml="$2" key="$3" | ||||||||||||||||
| actual=$(echo "$yaml" | yq eval ".$key" -) | ||||||||||||||||
| if [ "$actual" = "null" ]; then | ||||||||||||||||
| echo " PASS: $desc" | ||||||||||||||||
| PASS=$((PASS+1)) | ||||||||||||||||
| else | ||||||||||||||||
| echo " FAIL: $desc (expected key '$key' to be absent, got '$actual')" | ||||||||||||||||
| FAIL=$((FAIL+1)) | ||||||||||||||||
| fi | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| get_v1_config() { | ||||||||||||||||
| local output | ||||||||||||||||
| output=$(helm template test "$CHART_DIR" "$@" 2>&1) || { | ||||||||||||||||
| echo "TEMPLATE_ERROR: $output" >&2 | ||||||||||||||||
| return 1 | ||||||||||||||||
| } | ||||||||||||||||
| echo "$output" | yq eval 'select(.metadata.name == "v1-config") | .data["config.yaml"]' - | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| assert_template_fails() { | ||||||||||||||||
| local desc="$1"; shift | ||||||||||||||||
| local output | ||||||||||||||||
| if output=$(helm template test "$CHART_DIR" "$@" 2>&1); then | ||||||||||||||||
| echo " FAIL: $desc (expected template to fail, but it succeeded)" | ||||||||||||||||
| FAIL=$((FAIL+1)) | ||||||||||||||||
| else | ||||||||||||||||
| echo " PASS: $desc" | ||||||||||||||||
| PASS=$((PASS+1)) | ||||||||||||||||
| fi | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| # --- Test suite --- | ||||||||||||||||
|
|
||||||||||||||||
| echo "=== v1-config template tests ===" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "1. Default values" | ||||||||||||||||
| config=$(get_v1_config) | ||||||||||||||||
| assert_config_key "port defaults to 8000" "$config" "port" "8000" | ||||||||||||||||
| assert_config_key "listen_address defaults to 0.0.0.0" "$config" "listen_address" "0.0.0.0" | ||||||||||||||||
| assert_config_key "max_payload_size_bytes defaults to 41943040" "$config" "max_payload_size_bytes" "41943040" | ||||||||||||||||
| assert_config_key "persist_path defaults to /data" "$config" "persist_path" "/data" | ||||||||||||||||
| assert_config_key "allow_reset defaults to false" "$config" "allow_reset" "false" | ||||||||||||||||
| assert_config_key_missing "cors_allow_origins absent when empty" "$config" "cors_allow_origins" | ||||||||||||||||
| assert_config_key_missing "open_telemetry absent when disabled" "$config" "open_telemetry" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "2. CORS wildcard (should work)" | ||||||||||||||||
| config=$(get_v1_config --set 'chromadb.corsAllowOrigins={*}') | ||||||||||||||||
| assert_config_key "cors_allow_origins contains wildcard" "$config" "cors_allow_origins[0]" "*" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "3. CORS multiple origins" | ||||||||||||||||
| config=$(get_v1_config --set 'chromadb.corsAllowOrigins={https://a.com,https://b.com}') | ||||||||||||||||
| assert_config_key "first origin" "$config" "cors_allow_origins[0]" "https://a.com" | ||||||||||||||||
| assert_config_key "second origin" "$config" "cors_allow_origins[1]" "https://b.com" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "4. OpenTelemetry enabled" | ||||||||||||||||
| config=$(get_v1_config \ | ||||||||||||||||
| --set 'chromadb.telemetry.enabled=true' \ | ||||||||||||||||
| --set 'chromadb.telemetry.endpoint=http://otel:4317' \ | ||||||||||||||||
| --set 'chromadb.telemetry.serviceName=my-chroma') | ||||||||||||||||
| assert_config_key "otel endpoint" "$config" "open_telemetry.endpoint" "http://otel:4317" | ||||||||||||||||
| assert_config_key "otel service_name" "$config" "open_telemetry.service_name" "my-chroma" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "5. Custom server settings" | ||||||||||||||||
| config=$(get_v1_config \ | ||||||||||||||||
| --set 'chromadb.serverHttpPort=9000' \ | ||||||||||||||||
| --set 'chromadb.serverHost=127.0.0.1' \ | ||||||||||||||||
| --set 'chromadb.allowReset=true' \ | ||||||||||||||||
| --set 'chromadb.persistDirectory=/mnt/data' \ | ||||||||||||||||
| --set 'chromadb.maxPayloadSizeBytes=52428800') | ||||||||||||||||
| assert_config_key "custom port" "$config" "port" "9000" | ||||||||||||||||
| assert_config_key "custom listen_address" "$config" "listen_address" "127.0.0.1" | ||||||||||||||||
| assert_config_key "allow_reset true" "$config" "allow_reset" "true" | ||||||||||||||||
| assert_config_key "custom persist_path" "$config" "persist_path" "/mnt/data" | ||||||||||||||||
| assert_config_key "custom max_payload_size_bytes" "$config" "max_payload_size_bytes" "52428800" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "6. extraConfig merge" | ||||||||||||||||
| config=$(get_v1_config \ | ||||||||||||||||
| --set 'chromadb.extraConfig.circuit_breaker.requests=500' \ | ||||||||||||||||
| --set 'chromadb.extraConfig.sqlite_filename=custom.db') | ||||||||||||||||
| assert_config_key "circuit_breaker.requests from extraConfig" "$config" "circuit_breaker.requests" "500" | ||||||||||||||||
| assert_config_key "sqlite_filename from extraConfig" "$config" "sqlite_filename" "custom.db" | ||||||||||||||||
| assert_config_key "port still present after merge" "$config" "port" "8000" | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "7. extraConfig override of port fails" | ||||||||||||||||
| assert_template_fails "extraConfig.port override rejected" \ | ||||||||||||||||
| --set 'chromadb.extraConfig.port=9999' | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "8. extraConfig override of listen_address fails" | ||||||||||||||||
| assert_template_fails "extraConfig.listen_address override rejected" \ | ||||||||||||||||
| --set 'chromadb.extraConfig.listen_address=127.0.0.1' | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
| echo "9. telemetry enabled without endpoint fails" | ||||||||||||||||
| assert_template_fails "telemetry.enabled without endpoint rejected" \ | ||||||||||||||||
| --set 'chromadb.telemetry.enabled=true' | ||||||||||||||||
|
|
||||||||||||||||
| echo "" | ||||||||||||||||
|
||||||||||||||||
| echo "" | |
| echo "" | |
| # NOTE: v1-config is only mounted and used for apiVersion >= 1.0.0. | |
| # For apiVersion < 1.0.0 (e.g. 0.6.3), this test only validates that the | |
| # Helm template renders the expected CORS wildcard in the ConfigMap; it | |
| # does NOT verify actual runtime CORS behavior for pre-1.0.0, which uses | |
| # a different configuration mechanism. |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation for maxPayloadSizeBytes at line 132 checks if the value is <= 0, but the conversion to int64 happens before the check. If the user provides a non-numeric value, the
int64function may return 0, which would then fail the validation with a potentially confusing error message saying "got: ". Consider adding explicit type checking or updating the error message to clarify that the value must be numeric.