⚠️ Rename llmprovider to gateway - #100
Conversation
📝 WalkthroughWalkthroughThe PR replaces multi-model ChangesGateway migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: plugin(logcheck): plugin "logcheck" not found Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
internal/controller/agent_controller_test.go (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Gatewayterminology in the test fixture.
gatewayNamestill contains a provider-based value, andproviderstores aGateway. Rename these identifiers and related fixture strings to usegatewayconsistently. As per coding guidelines, use the terms defined inCONTEXT.md.Also applies to: 60-77
🤖 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 `@internal/controller/agent_controller_test.go` around lines 43 - 44, Update the test fixture identifiers and related strings around agent_controller tests to use Gateway terminology consistently: rename gatewayName and the provider variable that stores a Gateway, along with their provider-based fixture values, while preserving the existing test behavior and using the terms defined in CONTEXT.md.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 `@CONTEXT.md`:
- Around line 28-35: Update the Agent, AgentRun, and relationship sections in
CONTEXT.md to reference Gateway CR names and the controller’s Gateway
Ready-condition validation, not OpenShell Gateway Services. Clearly describe
OpenShell Gateway Services as the future replacement while preserving the
current Gateway CRD contract, so configuration examples and terminology
consistently require Gateway resources.
In `@hack/run-e2e.sh`:
- Around line 46-51: Update the kubectl wait expression in the Gateway readiness
check around the e2e-gateway command to select the status condition whose type
is Ready rather than assuming status.conditions[0] is Ready. Preserve the
existing timeout, success message, failure handling, and diagnostic output.
In `@internal/controller/agentrun_controller.go`:
- Around line 471-479: After fetching the Gateway in the agent-run flow,
validate its current Ready condition before appending the LLM environment
variables or creating the Sandbox. Use the fetched Gateway’s status conditions
and reject or retry when ConditionTypeReady is not true; only continue with
KONVEYOR_LLM_ENDPOINT and KONVEYOR_LLM_MODEL for a currently Ready Gateway.
In `@internal/controller/gateway_controller.go`:
- Around line 155-162: Before setting the current-generation Gateway status to
Ready=False with Reason "Verifying" in both affected branches, clear
gateway.Status.ConnectionVerified and persist that status change before
verification begins. Update the related flow around patchStatus so the CRD and
clients cannot retain the prior generation’s verification result.
- Around line 145-165: Update the verification-job handling in the Gateway
reconciler around createVerificationJob to discover and delete Jobs belonging to
prior Gateway generations before creating the current-generation Job. Ensure
completion events for stale Jobs cannot leave them orphaned, while preserving
the existing current-job lookup and status behavior; add a regression test
covering a Gateway update while verification is queued or running.
- Around line 317-321: Update GatewayReconciler.SetupWithManager to watch
credential Secrets and enqueue the referencing Gateways on Secret changes. Add
the required credential-Secret field index and use a Secret watch with a mapping
based on that index, preserving the existing Gateway and Job watches.
- Around line 169-177: The gateway verification flow around isJobSucceeded must
not mark ConnectionVerified true for unauthenticated or rejected requests. Make
the probe validate credentials for every supported credentialRef type, treat
401/403 as verification failures, and ensure omitted credentialRef.key does not
pass authentication verification; alternatively, report endpoint reachability
separately from credential verification.
- Line 224: Constrain the verification Job’s outbound access before assigning
gateway.Spec.Endpoint to the LLM_ENDPOINT EnvVar in the Gateway reconciliation
flow. Update the logic around verifyJobEndpoint and the Job creation path to
allow only externally reachable Gateway destinations, or apply a default-deny
egress policy, while preserving endpoint verification for valid destinations.
---
Nitpick comments:
In `@internal/controller/agent_controller_test.go`:
- Around line 43-44: Update the test fixture identifiers and related strings
around agent_controller tests to use Gateway terminology consistently: rename
gatewayName and the provider variable that stores a Gateway, along with their
provider-based fixture values, while preserving the existing test behavior and
using the terms defined in CONTEXT.md.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c53fc25-c496-460b-a5aa-15c5cdf4130b
📒 Files selected for processing (42)
AGENTS.mdCONTEXT.mdPROJECTREADME.mdapi/v1alpha1/agent_types.goapi/v1alpha1/agentrun_types.goapi/v1alpha1/agentworkflowrun_types.goapi/v1alpha1/gateway_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/zz_generated.deepcopy.gochanges/unreleased/rename-llmprovider-to-gateway.yamlcmd/main.goconfig/crd/bases/konveyor.io_agentruns.yamlconfig/crd/bases/konveyor.io_agents.yamlconfig/crd/bases/konveyor.io_agentworkflowruns.yamlconfig/crd/bases/konveyor.io_gateways.yamlconfig/crd/kustomization.yamlconfig/rbac/role.yamldocs/slides/demo-2026-07-16.mdhack/e2e/resources.yamlhack/harness-test/resources.yamlhack/harness-test/workflow-resources.yamlhack/run-e2e.shhack/setup-e2e.shharness/README.mdharness/internal/config/config.goharness/internal/config/config_test.goharness/internal/goose/lifecycle.goimages/README.mdimages/agentic-controller-agent/Containerfileimages/agentic-controller-agent/entrypoint.shinternal/controller/agent_controller.gointernal/controller/agent_controller_test.gointernal/controller/agentrun_controller.gointernal/controller/agentrun_controller_test.gointernal/controller/agentworkflow_controller_test.gointernal/controller/agentworkflowrun_controller.gointernal/controller/agentworkflowrun_controller_test.gointernal/controller/crd_validation_test.gointernal/controller/gateway_controller.gointernal/controller/gateway_controller_test.gointernal/controller/suite_test.go
| // unauthenticated — reachability still verifies (2xx-4xx passes). | ||
| env := []corev1.EnvVar{{Name: "LLM_ENDPOINT", Value: provider.Spec.Endpoint}} | ||
| if provider.Spec.CredentialRef.Key != "" { | ||
| env := []corev1.EnvVar{{Name: "LLM_ENDPOINT", Value: gateway.Spec.Endpoint}} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(gateway_controller\.go|rbac|role|rolebinding|networkpolicy|networkpolicies|manifests|config)/|gateway|.*\.yaml$' || true
echo "== gateway_controller relevant sections =="
if [ -f internal/controller/gateway_controller.go ]; then
wc -l internal/controller/gateway_controller.go
sed -n '1,80p' internal/controller/gateway_controller.go
sed -n '200,285p' internal/controller/gateway_controller.go
sed -n '285,400p' internal/controller/gateway_controller.go
fi
echo "== rbac/search endpoint writers =="
rg -n --hidden -S --glob '*.{go,yaml,yml,md}' 'Gateway|gateway|create.*gateway|update.*gateway|Endpoint|LLM_ENDPOINT|NetworkPolicy|NetworkPolicies|egress|ingress' . || true
echo "== job pod/security policy references =="
rg -n --hidden -S --glob '*.{go,yaml,yml}' 'SecurityContext|runAsNonRoot|NetworkPolicy|egress|AllowDNS|kubectl|namespace|metadata|169\.254|localhost|service account' . || trueRepository: konveyor/agentic-controller
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== network policy files =="
fd -a . config/network-policy -t f | while read -r f; do
echo "--- $f"
sed -n '1,200p' "$f"
done
echo "== Gateway CRD relevant block =="
sed -n '1,100p' config/crd/bases/konveyor.io_gateways.yaml
sed -n '21,80p' api/v1alpha1/gateway_types.go || true
echo "== RBAC relevant block =="
sed -n '1,130p' config/rbac/role.yaml
echo "== Job egress/security references exact files =="
rg -n --hidden -S --glob '*.{go,yaml,yml}' 'NetworkPolicy|NetworkPolicies|egress|allow-metrics-traffic|SecurityContext|runAsNonRoot|ServiceAccount|GatewayReconciler|gateways:' .Repository: konveyor/agentic-controller
Length of output: 10921
SSRF (CWE-918): Server-Side Request Forgery (SSRF)
Reachability: Internal
Reachability path
● Entry
internal/controller/suite_test.go:131
GatewayReconciler
│
▼
● Sink
internal/controller/gateway_controller.go
Constrain verification Job egress for Gateway endpoints.
Gateways can create and update config/crd/bases/konveyor.io_gateways.yaml, and verifyJobEndpoint accepts any non-empty string. That value is passed into a Job that calls "$LLM_ENDPOINT/v1/models". Curl quoting prevents shell injection, but it does not block requests to in-cluster services, control-plane addresses, or metadata endpoints. Restrict egress for these Jobs to externally reachable Gateway destinations, or deny Job egress by default.
🤖 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 `@internal/controller/gateway_controller.go` at line 224, Constrain the
verification Job’s outbound access before assigning gateway.Spec.Endpoint to the
LLM_ENDPOINT EnvVar in the Gateway reconciliation flow. Update the logic around
verifyJobEndpoint and the Job creation path to allow only externally reachable
Gateway destinations, or apply a default-deny egress policy, while preserving
endpoint verification for valid destinations.
| func (r *GatewayReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&konveyoriov1alpha1.LLMProvider{}). | ||
| For(&konveyoriov1alpha1.Gateway{}). | ||
| Owns(&batchv1.Job{}). | ||
| Named("llmprovider"). | ||
| Named("gateway"). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Watch credential Secrets so Gateway readiness tracks rotations.
Line 62 grants Secret watch permission, but this controller watches only Gateways and Jobs. A client Get does not subscribe to Secret changes. If a referenced Secret is deleted, rotated, or changed, the Gateway can remain Ready and verified until another event occurs. Add a credential-Secret field index and a Secret watch that maps changes to referencing Gateways.
🤖 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 `@internal/controller/gateway_controller.go` around lines 317 - 321, Update
GatewayReconciler.SetupWithManager to watch credential Secrets and enqueue the
referencing Gateways on Secret changes. Add the required credential-Secret field
index and use a Secret watch with a mapping based on that index, preserving the
existing Gateway and Job watches.
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
Clean, faithful implementation of #79 — the validateGateway delegation to Agent readiness, the preserved keyless/envFrom credential path, and dropping discoveredModels outright all look right. One gap I think is blocking (inline at buildEnvVars), one behavior question (inline at validateGateway), one script nit.
Two housekeeping items: DCO is failing (both commits missing sign-off), and the changelog fragment could note upgrade ordering — the new controller stops emitting KONVEYOR_MODEL_PRIMARY_*, so older harness/agent images fail against a newer controller (the harness fallback only covers the reverse direction). Agent images should upgrade with or before the controller.
Reshape the LLMProvider CRD to match the OpenShell gateway interaction
model (one gateway = one provider/model combination) so the eventual
flip to real OpenShell Gateways is seamless.
Changes:
- Rename LLMProvider CRD to Gateway (kind, types, controller, tests)
- Flatten Models[] to singular Model field (one CR = one model)
- Rename Agent.spec.providers to Agent.spec.gateways (AgentGatewayRef)
- Replace AgentRun.spec.models (role/provider/model triples) with
AgentRun.spec.gateway (single string)
- Replace AgentWorkflowRun.spec.models with .spec.gateway
- Simplify buildEnvVars: KONVEYOR_MODEL_{ROLE}_* env vars replaced by
KONVEYOR_LLM_ENDPOINT, KONVEYOR_LLM_MODEL, KONVEYOR_LLM_API_KEY
- Simplify validateModels -> validateGateway (membership check only)
- Remove DiscoveredModels from status (no longer needed with single model)
- Clean up stale AgentPlaybook/AgentPlaybookRun entries in PROJECT file
- Update CONTEXT.md, sample resources, all tests
Closes konveyor#79
Signed-off-by: David Zager <david.j.zager@gmail.com>
be12f54 to
0744851
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/agentworkflowrun_controller_test.go (1)
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert Gateway propagation for stage-b.
Line 168 verifies
Gatewayonly on stage-a. A later reconciliation creates stage-b. If that path dropsGateway, this test can still pass because it sets stage-b status directly. AssertstageBRun.Spec.Gateway == gwName.Proposed test addition
Expect(k8sClient.Get(ctx, types.NamespacedName{ Name: stageBRunName, Namespace: testNamespace, }, &stageBRun)).To(Succeed()) Expect(stageBRun.Spec.Instructions).To(Equal("Do stage B")) +Expect(stageBRun.Spec.Gateway).To(Equal(gwName)) Expect(stageBRun.Labels).To(HaveKeyWithValue(labelStage, "stage-b"))🤖 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 `@internal/controller/agentworkflowrun_controller_test.go` at line 168, Add an assertion for stage-b’s Gateway propagation in the reconciliation test after stage-b is retrieved or created, verifying stageBRun.Spec.Gateway equals gwName. Keep the existing stage-a assertion unchanged and ensure the assertion checks the reconciled stage-b spec before status is set directly.
🤖 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 `@hack/harness-test/resources.yaml`:
- Line 14: Update the GatewaySpec endpoint value in resources.yaml to use the
complete HTTPS URL, including the https:// scheme, so verification requests
target the secure transport.
In `@internal/controller/gateway_controller.go`:
- Around line 248-257: Update createVerificationJob so credential references are
mounted only after confirming the Gateway writer is authorized to read the
referenced Secret; reject or fail closed when access cannot be verified.
Validate or restrict gateway.Spec.Endpoint before creating the Job, allowing
credentials only for approved verification destinations. Preserve the existing
no-credential path when CredentialRef.Key is empty.
---
Nitpick comments:
In `@internal/controller/agentworkflowrun_controller_test.go`:
- Line 168: Add an assertion for stage-b’s Gateway propagation in the
reconciliation test after stage-b is retrieved or created, verifying
stageBRun.Spec.Gateway equals gwName. Keep the existing stage-a assertion
unchanged and ensure the assertion checks the reconciled stage-b spec before
status is set directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90846e17-5c43-4ff7-8ae3-db96fb2c67e4
⛔ Files ignored due to path filters (1)
api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
AGENTS.mdCONTEXT.mdMakefilePROJECTREADME.mdapi/go.modapi/v1alpha1/agent_types.goapi/v1alpha1/agentrun_types.goapi/v1alpha1/agentworkflowrun_types.goapi/v1alpha1/gateway_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/zz_generated.deepcopy.gochanges/unreleased/rename-llmprovider-to-gateway.yamlcmd/main.goconfig/crd/bases/konveyor.io_agentruns.yamlconfig/crd/bases/konveyor.io_agents.yamlconfig/crd/bases/konveyor.io_agentworkflowruns.yamlconfig/crd/bases/konveyor.io_gateways.yamlconfig/crd/kustomization.yamlconfig/rbac/role.yamldocs/slides/demo-2026-07-16.mdgo.modhack/e2e/resources.yamlhack/harness-test/resources.yamlhack/harness-test/workflow-resources.yamlhack/run-e2e.shhack/setup-e2e.shharness/README.mdharness/internal/config/config.goharness/internal/config/config_test.goharness/internal/goose/lifecycle.goimages/README.mdimages/agentic-controller-agent/Containerfileimages/agentic-controller-agent/entrypoint.shinternal/controller/agent_controller.gointernal/controller/agent_controller_test.gointernal/controller/agentrun_controller.gointernal/controller/agentrun_controller_test.gointernal/controller/agentworkflow_controller_test.gointernal/controller/agentworkflowrun_controller.gointernal/controller/agentworkflowrun_controller_test.gointernal/controller/crd_validation_test.gointernal/controller/gateway_controller.gointernal/controller/gateway_controller_test.gointernal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (35)
- internal/controller/agentworkflowrun_controller.go
- cmd/main.go
- harness/internal/goose/lifecycle.go
- images/agentic-controller-agent/entrypoint.sh
- README.md
- changes/unreleased/rename-llmprovider-to-gateway.yaml
- docs/slides/demo-2026-07-16.md
- harness/README.md
- api/v1alpha1/agentworkflowrun_types.go
- hack/run-e2e.sh
- api/v1alpha1/groupversion_info.go
- hack/e2e/resources.yaml
- internal/controller/agent_controller.go
- api/v1alpha1/agentrun_types.go
- config/crd/bases/konveyor.io_agents.yaml
- images/README.md
- internal/controller/suite_test.go
- config/crd/bases/konveyor.io_agentruns.yaml
- AGENTS.md
- images/agentic-controller-agent/Containerfile
- config/crd/kustomization.yaml
- harness/internal/config/config_test.go
- hack/harness-test/workflow-resources.yaml
- internal/controller/crd_validation_test.go
- internal/controller/agentrun_controller.go
- internal/controller/gateway_controller_test.go
- api/v1alpha1/gateway_types.go
- config/rbac/role.yaml
- config/crd/bases/konveyor.io_agentworkflowruns.yaml
- config/crd/bases/konveyor.io_gateways.yaml
- api/v1alpha1/agent_types.go
- hack/setup-e2e.sh
- PROJECT
- harness/internal/config/config.go
- api/v1alpha1/zz_generated.deepcopy.go
| name: gcp-vertex-ai | ||
| spec: | ||
| provider: gcp-vertex-ai | ||
| endpoint: "global-aiplatform.googleapis.com" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files relevant =="
git ls-files | rg '(^|/)(gateway_controller\.go|resources\.yaml)$|hack/harness-test/resources\.yaml$' || true
echo "== target yaml =="
if [ -f hack/harness-test/resources.yaml ]; then
cat -n hack/harness-test/resources.yaml
fi
echo "== gateway controller references =="
rg -n "LLM_ENDPOINT|GatewaySpec|Endpoint|curl \"$|curl \"\\$LLM_ENDPOINT|v1/models|Accept-Header|reconcile" -S --glob '*.go' . || true
echo "== all endpoint occurrences =="
rg -n 'endpoint:' -S . || trueRepository: konveyor/agentic-controller
Length of output: 979
🌐 Web query:
POSIX curl URL with no scheme defaults to http documentation
💡 Result:
Yes, when a URL provided to curl lacks a scheme (such as http:// or ftp://), curl defaults to using HTTP, but it employs a heuristic "guessing" mechanism based on the hostname prefix before falling back to this default [1][2][3]. If the URL does not start with a recognized scheme, curl examines the first part of the hostname (the subdomain) [4][5]. If the hostname begins with specific prefixes, it assumes a corresponding protocol [6][7]: - ftp. means FTP - dict. means DICT - ldap. means LDAP - imap. means IMAP - smtp. means SMTP - pop3. means POP3 For any hostname that does not match these specific prefixes, curl defaults to HTTP [4][5]. This behavior is a convenience feature mimicking early web browsers and is not defined by standard URL specifications [6][7]. Users can override this behavior in the following ways: - Command-line: Use the --proto-default option to set a different default protocol [4][5]. - libcurl: Use the CURLOPT_DEFAULT_PROTOCOL option in code to specify the protocol to be used when no scheme is present [8][9][10]. When a default protocol is explicitly set, curl bypasses the hostname-based guessing heuristic [8][9][10].
Citations:
- 1: https://curl.se/docs/manpage.html
- 2: https://man7.org/linux/man-pages/man1/curl.1.html
- 3: https://manpages.debian.org/bookworm/curl/curl.1.en.html
- 4: https://everything.curl.dev/cmdline/urls/scheme.html
- 5: https://ec.haxx.se/cmdline/urls/scheme.html
- 6: https://curl.se/docs/url-syntax.html
- 7: https://github.com/curl/curl/blob/master/docs/URL-SYNTAX.md
- 8: https://curl.se/libcurl/c/CURLOPT_DEFAULT_PROTOCOL.html
- 9: https://curl.se/libcurl/c/CURLOPT_URL.html
- 10: https://github.com/curl/curl/blob/master/docs/libcurl/opts/CURLOPT_URL.md
🏁 Script executed:
#!/bin/bash
set -euo u
echo "== gateway_controller.go outline/size =="
wc -l internal/controller/gateway_controller.go
ast-grep outline internal/controller/gateway_controller.go --view expanded || true
echo "== gateway_controller.go relevant sections =="
rg -n 'Endpoint|Endpoint|llm|curl|models|2xx|4xx|TLS|Transport|DefaultTransport|scheme|URL' -S internal/controller/gateway_controller.go
echo "== full gateway_controller.go around endpoint and curl =="
python3 - <<'PY'
from pathlib import Path
p=Path('internal/controller/gateway_controller.go')
lines=p.read_text().splitlines()
for name in ['func (r *GatewayReconciler):', 'Endpoint', 'curl', 'GetBaseURL', 'v1/models', 'models']:
print(f'-- {name} --')
for i,l in enumerate(lines,1):
if name.lower() in l.lower():
start=max(1,i-25); end=min(len(lines),i+35)
for j in range(start,end+1):
print(f'{j:4}: {lines[j-1]}')
print()
PYRepository: konveyor/agentic-controller
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -u
echo "== gateway_controller.go outline/size =="
wc -l internal/controller/gateway_controller.go
ast-grep outline internal/controller/gateway_controller.go --view expanded || true
echo "== gateway_controller.go relevant terms =="
rg -n 'Endpoint|endpoint|LLM_ENDPOINT|curl|models|2xx|4xx|TLS|Transport|DefaultTransport|scheme|URL' -S internal/controller/gateway_controller.go
echo "== gateway_controller.go around endpoint and curl =="
python3 - <<'PY'
from pathlib import Path
p=Path('internal/controller/gateway_controller.go')
lines=p.read_text().splitlines()
terms = ['Endpoint', 'endpoint', 'LLM_ENDPOINT', 'curl', 'v1/models', 'models', 'models', 'func (r *GatewayReconciler):']
seen=set()
for name in terms:
print(f'-- {name} --')
for i,l in enumerate(lines,1):
if name.lower() in l.lower():
start=max(1,i-25); end=min(len(lines),i+45)
for j in range(start,end+1):
if j not in seen:
print(f'{j:4}: {lines[j-1]}')
seen.add(j)
print()
PYRepository: konveyor/agentic-controller
Length of output: 11620
Use a complete HTTPS URL for spec.endpoint.
GatewaySpec.Endpoint is used as the base URL for the verification curl "$LLM_ENDPOINT/v1/models" request. A scheme-less value makes curl default to HTTP, so this Gateway can verify the wrong transport and report ready from global-aiplatform.googleapis.com:80 instead of HTTPS.
Proposed fix
- endpoint: "global-aiplatform.googleapis.com"
+ endpoint: "https://global-aiplatform.googleapis.com"📝 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.
| endpoint: "global-aiplatform.googleapis.com" | |
| endpoint: "https://global-aiplatform.googleapis.com" |
🤖 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 `@hack/harness-test/resources.yaml` at line 14, Update the GatewaySpec endpoint
value in resources.yaml to use the complete HTTPS URL, including the https://
scheme, so verification requests target the secure transport.
Source: MCP tools
| env := []corev1.EnvVar{{Name: "LLM_ENDPOINT", Value: gateway.Spec.Endpoint}} | ||
| if gateway.Spec.CredentialRef.Key != "" { | ||
| env = append(env, corev1.EnvVar{ | ||
| Name: "LLM_API_KEY", | ||
| ValueFrom: &corev1.EnvVarSource{ | ||
| SecretKeyRef: &corev1.SecretKeySelector{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: provider.Spec.CredentialRef.SecretName, | ||
| Name: gateway.Spec.CredentialRef.SecretName, | ||
| }, | ||
| Key: provider.Spec.CredentialRef.Key, | ||
| Key: gateway.Spec.CredentialRef.Key, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find repository RBAC and admission-policy definitions that govern Gateway
# writers and Secret readers.
fd -a -t f . | rg -i '(role|rolebinding|clusterrole|clusterrolebinding|policy|constraint|gateway).*\.(yaml|yml|json)$' |
while IFS= read -r file; do
echo "--- $file"
rg -n -i -C 4 'gateways|secrets|create|update|get|Gateway|ValidatingAdmission' "$file" || true
doneRepository: konveyor/agentic-controller
Length of output: 7095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact Gateway reconciler credential/env/job construction paths and
# RBAC references that affect Gateway writers and Secret readers.
sed -n '220,290p' internal/controller/gateway_controller.go
printf '\n--- RBAC role binding ---\n'
sed -n '1,140p' config/rbac/role.yaml config/rbac/role_binding.yaml
printf '\n--- Gateway type/schema credential fields ---\n'
sed -n '60,85p' config/crd/bases/konveyor.io_gateways.yaml
sed -n '40,115p' api/v1alpha1/gateway_types.go
# Read-only verifier: identify whether GatewaySpec endpoint is passed directly to
# a Job container env while the referenced SecretKeyRef env source is non-empty.
python3 - <<'PY'
from pathlib import Path
import re, yaml
src = Path('internal/controller/gateway_controller.go').read_text()
crd = yaml.safe_load(Path('config/crd/bases/konveyor.io_gateways.yaml').read_text())
rbac = yaml.safe_load(Path('config/rbac/role.yaml').read_text())
checks = {
'secret_key_ref_used': 'SecretKeyRef: &corev1.SecretKeySelector{' in src,
'endpoint_in_env_value': '"LLM_ENDPOINT", Value: gateway.Spec.Endpoint' in src,
'credential_ref_key_used': 'gateway.Spec.CredentialRef.Key,}' in src,
'credential_ref_secret_used': 'gateway.Spec.CredentialRef.SecretName,' in src,
'role_has_gateways_all_verbs': any(
r.get('resources') == ['secrets','gateways'] and set(r.get('verbs', [])) >= {'create','get','list','watch','patch','update','delete'}
for r in rbac.get('rules') or []
),
'crd_no_endpoint_pattern': 'pattern' not in str(crd.get('spec', {}).get('versions', [{}])[0].get('schema', {}).get('openAPIV3Schema', {})),
'crd_allow_any_endpoint': bool(re.search(r'endpoint:.*type:\s*string', str(crd))))
print(repr(checks))
PYRepository: konveyor/agentic-controller
Length of output: 9059
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Moderate
Reachability path
● Entry
internal/controller/agentrun_controller.go:83
Reconcile
│
▼
● Sink
internal/controller/gateway_controller.go
Require Secret read access before mounting selected credentials.
createVerificationJob mounts gateway.Spec.CredentialRef.SecretName/Key as LLM_API_KEY and sends it to gateway.Spec.Endpoint. The controller RBAC allows Secrets, but Gateway writers are not required to have Secret access from this controller path, so a writer-controlled credential reference can expose a Secret value through a Job endpoint. Enforce that Gateway writers can read the referenced Secret and restrict or validate verification destinations before mounting credentials.
🤖 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 `@internal/controller/gateway_controller.go` around lines 248 - 257, Update
createVerificationJob so credential references are mounted only after confirming
the Gateway writer is authorized to read the referenced Secret; reject or fail
closed when access cannot be verified. Validate or restrict
gateway.Spec.Endpoint before creating the Job, allowing credentials only for
approved verification destinations. Preserve the existing no-credential path
when CredentialRef.Key is empty.
|
The api/ sub-module split is what's failing both red checks: the Dockerfile dep-cache layer copies only the root manifests, so Copying the sub-module manifests before the download fixes it: COPY go.mod go.mod
COPY go.sum go.sum
COPY api/go.mod api/go.mod
COPY api/go.sum api/go.sum
RUN go mod download |
Make api/ its own Go module so external consumers (e.g. Hub) can import CRD type definitions without pulling in controller-runtime and all its transitive dependencies. The api sub-module depends only on k8s.io/api and k8s.io/apimachinery. The root module uses a replace directive for local development. Makefile generate/manifests targets updated to explicitly include the api path since controller-gen does not descend into separate Go modules with paths="./...". Closes konveyor#97 Refs: konveyor#79 Signed-off-by: David Zager <david.j.zager@gmail.com>
0744851 to
142cf6e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/agentworkflowrun_controller_test.go (1)
137-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert Gateway propagation for stage-b.
The test checks
stageARun.Spec.Gatewayat line 168. It does not checkstageBRun.Spec.Gateway. AddExpect(stageBRun.Spec.Gateway).To(Equal(gwName))before simulating stage-b success. This detects regressions that copy the selected Gateway only to the first generatedAgentRun.🤖 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 `@internal/controller/agentworkflowrun_controller_test.go` around lines 137 - 168, Extend the workflow test’s stage-b verification to assert that stageBRun.Spec.Gateway equals gwName before simulating stage-b success, alongside the existing stage-a gateway assertion. Use the existing stageBRun symbol and preserve the current test flow.
🤖 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.
Nitpick comments:
In `@internal/controller/agentworkflowrun_controller_test.go`:
- Around line 137-168: Extend the workflow test’s stage-b verification to assert
that stageBRun.Spec.Gateway equals gwName before simulating stage-b success,
alongside the existing stage-a gateway assertion. Use the existing stageBRun
symbol and preserve the current test flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e203e35-cd83-4fe1-b199-aed638b671cb
⛔ Files ignored due to path filters (1)
api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
AGENTS.mdCONTEXT.mdDockerfileMakefilePROJECTREADME.mdapi/go.modapi/v1alpha1/agent_types.goapi/v1alpha1/agentrun_types.goapi/v1alpha1/agentworkflowrun_types.goapi/v1alpha1/gateway_types.goapi/v1alpha1/groupversion_info.goapi/v1alpha1/zz_generated.deepcopy.gochanges/unreleased/rename-llmprovider-to-gateway.yamlcmd/main.goconfig/crd/bases/konveyor.io_agentruns.yamlconfig/crd/bases/konveyor.io_agents.yamlconfig/crd/bases/konveyor.io_agentworkflowruns.yamlconfig/crd/bases/konveyor.io_gateways.yamlconfig/crd/kustomization.yamlconfig/rbac/role.yamldocs/slides/demo-2026-07-16.mdgo.modhack/e2e/resources.yamlhack/harness-test/resources.yamlhack/harness-test/workflow-resources.yamlhack/run-e2e.shhack/setup-e2e.shharness/README.mdharness/internal/config/config.goharness/internal/config/config_test.goharness/internal/goose/lifecycle.goimages/README.mdimages/agentic-controller-agent/Containerfileimages/agentic-controller-agent/entrypoint.shinternal/controller/agent_controller.gointernal/controller/agent_controller_test.gointernal/controller/agentrun_controller.gointernal/controller/agentrun_controller_test.gointernal/controller/agentworkflow_controller_test.gointernal/controller/agentworkflowrun_controller.gointernal/controller/agentworkflowrun_controller_test.gointernal/controller/crd_validation_test.gointernal/controller/gateway_controller.gointernal/controller/gateway_controller_test.gointernal/controller/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (41)
- cmd/main.go
- api/v1alpha1/agent_types.go
- hack/setup-e2e.sh
- harness/internal/goose/lifecycle.go
- api/v1alpha1/agentworkflowrun_types.go
- docs/slides/demo-2026-07-16.md
- images/README.md
- config/crd/kustomization.yaml
- internal/controller/agentworkflowrun_controller.go
- Makefile
- internal/controller/suite_test.go
- config/crd/bases/konveyor.io_agentruns.yaml
- api/v1alpha1/groupversion_info.go
- changes/unreleased/rename-llmprovider-to-gateway.yaml
- config/rbac/role.yaml
- config/crd/bases/konveyor.io_agentworkflowruns.yaml
- images/agentic-controller-agent/Containerfile
- api/go.mod
- go.mod
- AGENTS.md
- harness/README.md
- README.md
- api/v1alpha1/zz_generated.deepcopy.go
- config/crd/bases/konveyor.io_agents.yaml
- api/v1alpha1/agentrun_types.go
- internal/controller/crd_validation_test.go
- CONTEXT.md
- harness/internal/config/config_test.go
- hack/harness-test/workflow-resources.yaml
- internal/controller/agentrun_controller.go
- config/crd/bases/konveyor.io_gateways.yaml
- harness/internal/config/config.go
- internal/controller/agent_controller.go
- internal/controller/gateway_controller.go
- internal/controller/gateway_controller_test.go
- hack/e2e/resources.yaml
- PROJECT
- images/agentic-controller-agent/entrypoint.sh
- hack/harness-test/resources.yaml
- hack/run-e2e.sh
- api/v1alpha1/gateway_types.go
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
All three of my earlier points are addressed: spec.provider as a required field injected as KONVEYOR_LLM_PROVIDER, single-gateway defaulting in validateGateway (which retires the client-side model injection workaround), and gateways.konveyor.io in the e2e script. The Dockerfile fix for the api/ sub-module has both builds green.
One non-blocking note: LoadFromEnv no longer treats provider as required, so an empty value falls through to goose's default rather than failing at boot. The CRD's MinLength=1 covers every controller-driven pod, so this only bites hand-rolled ones — fine to leave as is.
2 participants
Fixes #79
Fixes #97
Summary by CodeRabbit
LLMProviderresources withGatewayresources, each representing one provider/model endpoint.KONVEYOR_LLM_*environment variables with backward-compatible legacy fallbacks.