feat: migrate framework imports to llm-d IPP#331
Conversation
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAll plugins ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
github-actions
Bot
added
the
size/L
nirrozenbaum
left a comment
nirrozenbaum
left a comment
There was a problem hiding this comment.
go.mod: local replace directive must be removed before merge
The replace directive points to a local filesystem path:
replace github.com/llm-d/llm-d-inference-payload-processor => ../llm-d-inference-payload-processor
This will break for anyone cloning the repo. The module is already published — remove the replace directive and use a real version in the require block instead:
require (
github.com/llm-d/llm-d-inference-payload-processor v0.0.0-20260609111331-11891e132621
)To fix:
- Delete the
replace github.com/llm-d/llm-d-inference-payload-processor => ../llm-d-inference-payload-processorline fromgo.mod - Run
go get github.com/llm-d/llm-d-inference-payload-processor@latest - Run
go mod tidy - Verify with
go build ./...
|
Fixed — replaced local path with fork URL ( Can't point to upstream main yet because the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
pkg/plugins/model-provider-resolver/plugin.go (1)
177-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent request-path panic when provider refs are empty or non-positive weight.
Line 177 can reach
selectByWeightwith invalidrefs; then Line 216 executesrand.IntN(totalWeight)and panics whenlen(refs)==0ortotalWeight<=0(CWE-20, CWE-248). This is a straightforward denial-of-service trigger via malformed CR spec data on the hot request path.Suggested fix
func (p *ModelProviderResolverPlugin) ProcessRequest(ctx context.Context, cycleState *plugin.CycleState, request *requesthandling.InferenceRequest) error { @@ - ref := selectByWeight(modelInfo.refs) + if len(modelInfo.refs) == 0 { + logger.Error(nil, "no provider refs resolved for external model", "model", modelKey.String()) + return errcommon.Error{Code: errcommon.Internal, Msg: "no provider refs resolved for external model"} + } + ref, err := selectByWeight(modelInfo.refs) + if err != nil { + logger.Error(err, "invalid provider refs for weighted selection", "model", modelKey.String()) + return errcommon.Error{Code: errcommon.Internal, Msg: err.Error()} + } @@ -func selectByWeight(refs []*resolvedProviderRef) *resolvedProviderRef { +func selectByWeight(refs []*resolvedProviderRef) (*resolvedProviderRef, error) { + if len(refs) == 0 { + return nil, fmt.Errorf("no provider refs available") + } if len(refs) == 1 { - return refs[0] + if refs[0].weight <= 0 { + return nil, fmt.Errorf("provider ref weight must be > 0") + } + return refs[0], nil } totalWeight := 0 for _, ref := range refs { + if ref.weight <= 0 { + return nil, fmt.Errorf("provider ref weight must be > 0") + } totalWeight += ref.weight } + if totalWeight <= 0 { + return nil, fmt.Errorf("total provider ref weight must be > 0") + } r := rand.IntN(totalWeight) for _, ref := range refs { r -= ref.weight if r < 0 { - return ref + return ref, nil } } - return refs[len(refs)-1] + return refs[len(refs)-1], nil }As per coding guidelines,
**/*.go: “Validate CR spec fields before using in ConfigMaps/Secrets.”Also applies to: 208-224
🤖 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 `@pkg/plugins/model-provider-resolver/plugin.go` at line 177, The selectByWeight function (around lines 208-224) can panic when receiving empty refs or refs with non-positive total weight. Add validation at line 177 to ensure modelInfo.refs is not empty and contains valid weights before calling selectByWeight. Additionally, add defensive checks inside the selectByWeight function itself (lines 208-224) to safely handle edge cases where len(refs) is zero or totalWeight is non-positive by returning a sensible default or error instead of allowing rand.IntN to panic. This prevents denial-of-service attacks via malformed CR spec data on the request path.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.
Outside diff comments:
In `@pkg/plugins/model-provider-resolver/plugin.go`:
- Line 177: The selectByWeight function (around lines 208-224) can panic when
receiving empty refs or refs with non-positive total weight. Add validation at
line 177 to ensure modelInfo.refs is not empty and contains valid weights before
calling selectByWeight. Additionally, add defensive checks inside the
selectByWeight function itself (lines 208-224) to safely handle edge cases where
len(refs) is zero or totalWeight is non-positive by returning a sensible default
or error instead of allowing rand.IntN to panic. This prevents denial-of-service
attacks via malformed CR spec data on the request path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8b26d207-93ee-4f3d-86e9-ff9b1f306ecc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (24)
cmd/main.gogo.modpkg/plugins/api-translation/plugin.gopkg/plugins/api-translation/plugin_test.gopkg/plugins/api-translation/translator/anthropic/anthropic.gopkg/plugins/api-translation/translator/bedrock/bedrock_openai.gopkg/plugins/api-translation/translator/openai/openai.gopkg/plugins/api-translation/translator/vertex/vertex_openai.gopkg/plugins/apikey-injection/auth-generator/auth_generator.gopkg/plugins/apikey-injection/auth-generator/sigv4_auth_generator.gopkg/plugins/apikey-injection/auth-generator/simple_auth_generator.gopkg/plugins/apikey-injection/plugin.gopkg/plugins/apikey-injection/plugin_test.gopkg/plugins/apikey-injection/reconciler.gopkg/plugins/model-provider-resolver/external_model_reconciler.gopkg/plugins/model-provider-resolver/external_provider_reconciler.gopkg/plugins/model-provider-resolver/plugin.gopkg/plugins/model-provider-resolver/plugin_test.gopkg/plugins/nemo/nemo_guard_base.gopkg/plugins/nemo/request_guard.gopkg/plugins/nemo/request_guard_test.gopkg/plugins/nemo/response_guard.gopkg/plugins/nemo/response_guard_test.gopkg/plugins/plugins.go
|
Plugin configuration: The upstream runner in The expected config format is: apiVersion: llm-d.ai/v1alpha1
kind: PayloadProcessorConfig
plugins:
- type: body-field-to-header
name: model-extractor
parameters:
fieldName: model
headerName: X-Gateway-Model-Name
- type: model-provider-resolver
- type: api-translation
- type: apikey-injection
profiles:
default:
request:
plugins:
- pluginRef: model-extractor
- pluginRef: model-provider-resolver
- pluginRef: api-translation
- pluginRef: apikey-injection
response:
plugins:
- pluginRef: api-translationWhat needs to change in this PR:
|
|
Thanks — you're right about the config format change. Here's where we stand: Fixed so far:
Still needed (Helm chart migration): Options:
Which approach do you prefer? Option 1 is cleaner but means we depend on the IPP chart being published. Option 2 gives us full control. |
|
option 1 please - replace the chart dependency to llm-d-inference-payload-processor |
github-actions
Bot
added
size/XL
|
Your PR is large. Please consider breaking it into multiple PRs. The |
1 similar comment
|
Your PR is large. Please consider breaking it into multiple PRs. The |
| fieldName: model | ||
| headerName: X-Gateway-Model-Name | ||
| - type: model-provider-resolver | ||
| name: model-provider-resolver |
There was a problem hiding this comment.
remove plugin names when the name is identical to type
There was a problem hiding this comment.
do the same also in the e2e customConfig
| istio: | ||
| envoyFilter: | ||
| operation: INSERT_AFTER | ||
| anchorSubFilter: "" |
| response: | ||
| - pluginRef: api-translation | ||
|
|
||
| tracing: |
There was a problem hiding this comment.
remove tracing section
| provider: | ||
| name: istio | ||
|
|
||
| supportedEvents: |
There was a problem hiding this comment.
remove section.
more generally remove sections that are 100% duplication of ipp (no need to repeat)
| require.NoError(t, err) | ||
|
|
||
| actualModel, err := framework.ReadCycleStateKey[string](cs, state.ModelKey) | ||
| actualModel, err := pluginpkg.ReadCycleStateKey[string](cs, state.ModelKey) |
There was a problem hiding this comment.
nit: keep imports consistent. for this you need to change to variable name from plugin to instance or something like it.
|
Your PR is large. Please consider breaking it into multiple PRs. The |
| provider: | ||
| name: istio | ||
| istio: | ||
| envoyFilter: | ||
| operation: INSERT_AFTER |
There was a problem hiding this comment.
need to bring this back (in llm-d provider = none)
| inferenceGateway: | ||
| name: maas-default-gateway |
There was a problem hiding this comment.
need to this back as well
| alias: upstreamBbr | ||
| - name: payload-processor | ||
| # Local path to the IPP chart. Update to an OCI/repo URL once the chart is published. | ||
| repository: file://../../../llm-d-inference-payload-processor/config/charts/payload-processor |
There was a problem hiding this comment.
this is the only concerning point in current PR.
need to push for a pre-release including image of the chart that can be referenced, which should unblock this PR.
Replace local filesystem path in go.mod replace directive with the fork's published commit. Both replace directives (gateway-api-inference- extension pin + IPP fork) are needed until upstream PRs merge: - IPP PR opendatahub-io#169 (ResponseBodyMode types) - ODH PR opendatahub-io#331 (framework migration removes gateway-api-inference-extension)
openshift-ci
Bot
added
the
needs-rebase
|
Your PR is large. Please consider breaking it into multiple PRs. The |
1 similar comment
|
Your PR is large. Please consider breaking it into multiple PRs. The |
16e64b8 to
3c6defc
Compare
github-actions
Bot
added
size/L
…uard - api-translation: configurable via "responseBodyMode" parameter (none/chunked/full, defaults to full) - nemo-response-guard: always BodyFull (needs complete body for content inspection) Depends on opendatahub-io#331 (framework migration)
…uard - api-translation: configurable via "responseBodyMode" parameter (none/chunked/full, defaults to full) - nemo-response-guard: always BodyFull (needs complete body for content inspection) Depends on opendatahub-io#331 (framework migration)
3c6defc to
235f457
Compare
- Replace all gateway-api-inference-extension imports with llm-d-inference-payload-processor - Point go.mod to official release tag v0.1.0-rc.1 - Update Helm chart dependency to oci://ghcr.io/llm-d/charts payload-processor 0.1.0-rc.1 - Rename chart alias from upstreamBbr to upstreamIpp - Update test mocks for new plugin.Handle interface
85d7d2a to
281c7d1
Compare
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/scripts/setup-kind.sh`:
- Around line 140-142: The Helm override keys in the setup-kind.sh script under
lines 140-142 have incorrect paths that don't match the chart values schema. The
overrides currently attempt to set
upstreamIpp.payloadProcessor.inferenceGateway.name,
upstreamIpp.payloadProcessor.provider.name, and
upstreamIpp.payloadProcessor.provider.istio.envoyFilter.operation, but according
to the values.yaml file, provider and inferenceGateway are siblings of
payloadProcessor under upstreamIpp, not children of it. Remove payloadProcessor
from each of these three --set arguments so that inferenceGateway and provider
are direct children of upstreamIpp instead of children of payloadProcessor.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 463bee30-8a3c-4616-9721-bbf9311a853c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.gitignoreDockerfilecmd/main.godeploy/payload-processing/Chart.yamldeploy/payload-processing/templates/rbac.yamldeploy/payload-processing/values.yamlgo.modpkg/plugins/api-translation/plugin.gopkg/plugins/api-translation/plugin_test.gopkg/plugins/api-translation/translator/anthropic/anthropic.gopkg/plugins/api-translation/translator/bedrock/bedrock_openai.gopkg/plugins/api-translation/translator/openai/openai.gopkg/plugins/api-translation/translator/vertex/vertex_openai.gopkg/plugins/apikey-injection/auth-generator/auth_generator.gopkg/plugins/apikey-injection/auth-generator/sigv4_auth_generator.gopkg/plugins/apikey-injection/auth-generator/simple_auth_generator.gopkg/plugins/apikey-injection/auth-generator/simple_auth_generator_test.gopkg/plugins/apikey-injection/plugin.gopkg/plugins/apikey-injection/plugin_test.gopkg/plugins/apikey-injection/reconciler.gopkg/plugins/model-provider-resolver/external_model_reconciler.gopkg/plugins/model-provider-resolver/external_model_reconciler_test.gopkg/plugins/model-provider-resolver/external_provider_reconciler.gopkg/plugins/model-provider-resolver/plugin.gopkg/plugins/model-provider-resolver/plugin_test.gopkg/plugins/nemo/nemo_guard_base.gopkg/plugins/nemo/request_guard.gopkg/plugins/nemo/request_guard_test.gopkg/plugins/nemo/response_guard.gopkg/plugins/nemo/response_guard_test.gopkg/plugins/plugins.gotest/e2e/scripts/e2e-values.yamltest/e2e/scripts/setup-kind.sh
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- pkg/plugins/model-provider-resolver/external_model_reconciler.go
- pkg/plugins/api-translation/translator/bedrock/bedrock_openai.go
- pkg/plugins/model-provider-resolver/external_provider_reconciler.go
- Dockerfile
- pkg/plugins/model-provider-resolver/external_model_reconciler_test.go
- cmd/main.go
- pkg/plugins/nemo/nemo_guard_base.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/plugins/apikey-injection/reconciler.go
- pkg/plugins/api-translation/translator/vertex/vertex_openai.go
- pkg/plugins/api-translation/translator/openai/openai.go
- pkg/plugins/plugins.go
- pkg/plugins/nemo/response_guard_test.go
- pkg/plugins/model-provider-resolver/plugin.go
- pkg/plugins/api-translation/translator/anthropic/anthropic.go
- pkg/plugins/apikey-injection/auth-generator/sigv4_auth_generator.go
- pkg/plugins/apikey-injection/auth-generator/auth_generator.go
- pkg/plugins/api-translation/plugin_test.go
- pkg/plugins/nemo/request_guard_test.go
- pkg/plugins/model-provider-resolver/plugin_test.go
- pkg/plugins/nemo/request_guard.go
- pkg/plugins/apikey-injection/plugin_test.go
- pkg/plugins/api-translation/plugin.go
c412c42 to
6ca95d1
Compare
| fieldName: model | ||
| headerName: X-Gateway-Model-Name | ||
| - type: model-provider-resolver | ||
| name: model-provider-resolver |
There was a problem hiding this comment.
nit: we don't need name in cases the name is identical to type (that's is set by default if no other name was specified)
| name: istio | ||
| istio: | ||
| envoyFilter: | ||
| operation: INSERT_AFTER | ||
| name: none | ||
|
|
||
| inferenceGateway: | ||
| name: maas-default-gateway | ||
| name: inference-gateway |
There was a problem hiding this comment.
revert pls. this is wrong
nirrozenbaum
removed
the
do-not-merge/hold
…test var - Remove plugin name fields in e2e-values.yaml where name == type (defaults) - Rename test variable p → instance for import consistency Signed-off-by: Noy Itzikowitz <nitzikow@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/payload-processing/templates/rbac.yaml (1)
50-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale comment.
Comment still references "bbr" instead of the new payload processor terminology.
📝 Proposed fix
-{{- else }} ## if bbr is set in a single namespace +{{- else }} ## if payload processor is set in a single namespace🤖 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 `@deploy/payload-processing/templates/rbac.yaml` at line 50, The comment at line 50 in the rbac.yaml template file references outdated "bbr" terminology instead of the current "payload processor" naming convention. Locate the comment that reads "## if bbr is set in a single namespace" and update it to use "payload processor" terminology instead of "bbr" to ensure consistency with the current project conventions.
🤖 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 `@deploy/payload-processing/values.yaml`:
- Around line 28-32: The request pipeline in the values.yaml file lacks
documentation of critical execution order dependencies. Add code comments above
or within the request pipeline block documenting that model-provider-resolver
must execute before api-translation and apikey-injection, explaining that
api-translation requires ProviderKey from CycleState (set by
model-provider-resolver) to select the correct translator format, and that
apikey-injection requires AuthTypeKey, CredsRefName, and CredsRefNamespace also
set by model-provider-resolver. Include a note that the current silent failure
when CycleState keys are missing masks ordering violations until the provider
API rejects the malformed request.
---
Outside diff comments:
In `@deploy/payload-processing/templates/rbac.yaml`:
- Line 50: The comment at line 50 in the rbac.yaml template file references
outdated "bbr" terminology instead of the current "payload processor" naming
convention. Locate the comment that reads "## if bbr is set in a single
namespace" and update it to use "payload processor" terminology instead of "bbr"
to ensure consistency with the current project conventions.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 87e3292c-05d8-42ca-9168-045034bcc46e
📒 Files selected for processing (5)
deploy/payload-processing/templates/rbac.yamldeploy/payload-processing/values.yamlpkg/plugins/model-provider-resolver/plugin_test.gotest/e2e/scripts/e2e-values.yamltest/e2e/scripts/setup-kind.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/scripts/setup-kind.sh
- pkg/plugins/model-provider-resolver/plugin_test.go
| request: | ||
| - pluginRef: model-extractor | ||
| - pluginRef: model-provider-resolver | ||
| - pluginRef: api-translation | ||
| - pluginRef: apikey-injection |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Cross-reference plugin execution order in code and documentation
# Find plugin ordering documentation or validation
rg -n -C5 'plugin.*order|execution.*order|pipeline.*order' --type=md --type=go
# Check if any plugin code has ordering dependencies/assertions
rg -n -C3 'model-provider-resolver.*before|apikey.*after|translation.*after' pkg/plugins/Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# First, understand the YAML structure and context
cat deploy/payload-processing/values.yaml | head -50Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1068
🏁 Script executed:
# Find plugin-related code and framework
fd -t f -e go | head -20Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1018
🏁 Script executed:
# Search for how plugins are loaded/executed
rg -n 'request.*pipeline|plugin.*execute|plugin.*chain' -A 3 -B 3Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1460
🏁 Script executed:
# Check plugin package structure
find . -type d -name '*plugin*' 2>/dev/nullRepository: opendatahub-io/ai-gateway-payload-processing
Length of output: 102
🏁 Script executed:
# Find the plugin execution framework code
rg -n 'type.*Profile|plugins.*execute|request.*chain' --type=go -A 5 -B 5Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Check main plugin loading and execution logic
find pkg -name '*.go' -type f | xargs grep -l 'Profile\|pipeline' | head -5Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Look for plugin framework or processor that executes the request pipeline
cat pkg/processor/processor.go 2>/dev/null || find . -name 'processor.go' -o -name 'pipeline.go' | head -3Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Search for code that processes the config/Profile structure
rg -n 'payloadProcessor\|Profile\|pluginRef' --type=go -B 2 -A 2 | head -80Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Find main handler code
rg -n 'http.Handler\|ServeHTTP\|ProcessPayload' --type=go -B 1 -A 5 | head -100Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Look for plugin initialization or chain building
rg -n 'plugin.*chain\|build.*pipeline\|plugins.*\[' --type=go -B 2 -A 3 | head -80Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# List all Go files and search more broadly
ls -la pkg/plugins/*/Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 2649
🏁 Script executed:
# Search in all Go files for plugin execution without type-specific filtering
grep -r "pluginRef\|executePlugin\|request.*plugin" --include="*.go" | head -40Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 213
🏁 Script executed:
# Look at the main.go and see how it initializes the system
cat cmd/main.go | head -100Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1461
🏁 Script executed:
# Find the RegisterPlugins implementation
rg -n 'RegisterPlugins|register.*plugin' --type=go -A 10 | head -100Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 6437
🏁 Script executed:
# Check if there's a plugins package init or registry
find pkg/plugins -name '*.go' | head -20Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1318
🏁 Script executed:
# Look for plugin interface definitions and ordering constraints
rg -n 'type.*Plugin\|interface.*Plugin' --type=go -B 2 -A 8 | head -120Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Check api-translation plugin for dependencies on model-provider-resolver output
cat pkg/plugins/api-translation/plugin.go | head -200Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 8321
🏁 Script executed:
# Check apikey-injection plugin for dependencies
cat pkg/plugins/apikey-injection/plugin.go | head -200Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 6740
🏁 Script executed:
# Search for CycleState or state storage to understand data flow between plugins
rg -n 'CycleState\|state\|Store\|common.state' --type=go pkg/plugins/ -B 2 -A 3 | head -150Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
Document plugin execution order dependencies; add ordering validation.
The request pipeline depends on execution order: model-provider-resolver must run before api-translation and apikey-injection. The api-translation plugin reads ProviderKey from CycleState (set by model-provider-resolver) to select the correct translator. If api-translation runs first, the provider is unknown and translation is skipped, sending OpenAI format to the provider API instead of its native format (e.g., Anthropic Messages API), causing API errors. Similarly, apikey-injection reads AuthTypeKey, CredsRefName, and CredsRefNamespace from CycleState; if it runs before model-provider-resolver, no auth headers are injected, causing 401/403 errors.
Currently, plugins fail silently if required CycleState keys are missing, masking ordering violations until the provider API rejects the request. Add explicit ordering assertions in the plugin framework or document this dependency in code comments.
🤖 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 `@deploy/payload-processing/values.yaml` around lines 28 - 32, The request
pipeline in the values.yaml file lacks documentation of critical execution order
dependencies. Add code comments above or within the request pipeline block
documenting that model-provider-resolver must execute before api-translation and
apikey-injection, explaining that api-translation requires ProviderKey from
CycleState (set by model-provider-resolver) to select the correct translator
format, and that apikey-injection requires AuthTypeKey, CredsRefName, and
CredsRefNamespace also set by model-provider-resolver. Include a note that the
current silent failure when CycleState keys are missing masks ordering
violations until the provider API rejects the malformed request.
Co-authored-by: Nir Rozenbaum <nir.rozenbaum@gmail.com>
openshift-ci
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum, noyitz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@noyitz: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:ai-gateway-group-test-z7t47 |
4 participants
Summary
sigs.k8s.io/gateway-api-inference-extensiontogithub.com/llm-d/llm-d-inference-payload-processorDepends on: llm-d/llm-d-inference-payload-processor#169 (response body mode framework)
Why
The MaaS binary was using the old BBR runner from
gateway-api-inference-extension, which always buffers all response body chunks. The active framework atllm-d/llm-d-inference-payload-processorhas streaming support (per-profile conditional buffering, ChunkProcessor) that only takes effect when the ODH plugins use the IPP types.Import mapping
gateway-api-inference-extension/pkg/bbr/frameworkllm-d-inference-payload-processor/pkg/framework/interface/requesthandlinggateway-api-inference-extension/pkg/epp/framework/interface/pluginllm-d-inference-payload-processor/pkg/framework/interface/plugingateway-api-inference-extension/pkg/common/errorllm-d-inference-payload-processor/pkg/common/errorgateway-api-inference-extension/pkg/common/observability/loggingllm-d-inference-payload-processor/pkg/common/observability/logginggateway-api-inference-extension/cmd/bbr/runnerllm-d-inference-payload-processor/cmd/runnergo.mod
github.com/llm-d/llm-d-inference-payload-processor(local replace directive until version is published)gateway-api-inference-extensionis fully removed from go.modTest plan
go build ./...passesapi-translation,apikey-injection,model-provider-resolver,nemo)legacymigrationtest requires envtest (kubebuilder binaries) — pre-existing, unrelated to this changeSummary by CodeRabbit
Refactor
Chores
upstreamBbrtoupstreamIpp, including RBAC naming and payload-processing chart wiring; refreshed build metadata and Go module dependencies; updated.gitignore.Tests