PR #3 review fixes (batch) - #4
Merged
Merged
Conversation
Replace the single static, region/partition-specific Bedrock entry in litellm.yaml with self-service enrollment that mirrors the self-hosted path: ./platformctl new-model --source bedrock --list-available-models ./platformctl new-model --source bedrock <model> --deploy - New BedrockModel CRD (bedrock.ai-platform/v1alpha1) - a plain, infra-less marker applied automatically by the platform-config ArgoCD app. - litellm-sync generalized to watch BedrockModels alongside the serving tiers and register them as native bedrock/ upstreams (IRSA auth, no api_base). Same finalizer/reconcile lifecycle, so a live CR protects the model from the orphan sweep. Health probe no longer crashloops when a watched CRD is absent (kro=false / Bedrock-only installs). - platformctl --source bedrock resolves the region/partition-correct invocation id (a cross-region inference profile where one exists, else the base model id) from the live Bedrock APIs, looks up per-token price from the AWS Price List API (best-effort; --input-cost/--output-cost override for partitions it doesn't cover, e.g. ESC), and commits a BedrockModel CR (reusing deploy_model). - litellm.yaml now ships model_list: [] - nothing model-specific pinned in git, so the committed config is correct in any region/partition (ESC included). - Docs: README day-one story + quick start, litellm-sync and workloads READMEs. No Terraform changes: the LiteLLM pod's existing IRSA/region/STS/endpoint wiring already covers invocation.
radudobrinescu
changed the title
radudobrinescu
marked this pull request as draft
September 17, 2026 13:59
Re-lock all six stages on the current provider set (aws ~> 6.28, helm ~> 3.0) using 'terraform providers lock' for linux_amd64, linux_arm64, darwin_amd64, and darwin_arm64. Restores the cryptographic hash verification that terraform init enforces, which was lost when the lock files were deleted during the 5.x -> 6.x / helm 2.x -> 3.x provider bump. Resolved: aws 6.28.0, helm 3.3.0, kubernetes 2.38.0, alekc/kubectl 2.4.1, null 3.3.2, time 0.14.2 (0.10.0 in oss-observability, pinned). All stages pass 'terraform validate'.
…Redis PVCs PR #3 dropped 'storageClass: gp3' from the ClickHouse and Redis persistence blocks. That is the correct, portable choice — but it was undocumented and the review flagged the resulting dependency on an implicit default. The PVCs bind the cluster DEFAULT StorageClass, which Terraform marks per mode: gp3 (ebs.csi.aws.com) on classic/self-managed clusters incl. ESC (capabilities.blockstorage), and auto-ebs-sc (ebs.csi.eks.amazonaws.com) on EKS Auto Mode. Pinning gp3 would break Auto Mode (no such class). Add comments on both blocks stating the intent and the per-mode default, and note the edge where a non-Auto cluster with blockstorage=false has no default SC. No behavior change — comments only.
…ping
_yaml_scalar hand-rolled its YAML quoting and left backslashes unescaped in the
single-quote and no-quote branches: a token ending in '\' escaped the closing
quote (unterminated scalar -> a manifest ArgoCD can't parse), and a token like
'a\tc' was silently reinterpreted ('\t' -> TAB). --extra-arg is user-supplied and
interpolated into a committed, cluster-applied manifest, so this is an
injection / structure-alteration hazard, not just cosmetic.
YAML is a strict superset of JSON, so json.dumps(v) is a valid YAML
double-quoted scalar that decodes back to exactly v. Delegate to the stdlib
encoder (already imported) — no new runtime dependency (avoids requiring PyYAML
at runtime, which the prereqs don't include). Newlines stay rejected as an
input-sanity guard.
Adds tests/test_render.py::TestYamlScalarQuoting: round-trips adversarial tokens
(trailing/embedded backslashes, both quote styles, \t/\0/\x/\u sequences, a real
TAB, non-ASCII) back to the exact input, plus newline rejection and a full
extraArgs-fragment parse.
_ensure_hf_token_secret rendered the Secret with `kubectl create secret --from-literal=token=<token>`, placing the HuggingFace token on argv — readable via /proc/<pid>/cmdline by any co-tenant on a shared operator/CI host, and prone to landing in shell history / CI logs. Build the Secret manifest in-process (token base64-encoded in memory) and pipe it to `kubectl apply -f -`, so the token reaches kubectl only via stdin, never on a command line. Idempotent (apply creates or updates); the manual fallbacks now model the safe pattern (--from-file=token=/dev/stdin). base64/json are stdlib. Adds tests/test_gitops.py asserting the token never appears on argv and is applied only as base64 in the Secret's data.
_status_check() was changed to unconditionally `return 0`, so CI/automation — which the README recommends running as the post-deploy smoke test — could no longer detect a broken platform: LiteLLM not responding, a registered model returning non-200, or Langfuse unhealthy all only printed warn lines. Track failures and return non-zero when a real check fails, while keeping exit 0 for the legitimately-empty case (no models registered yet — expected now that nothing ships by default). Chose a meaningful default exit over a --strict flag: a health check whose exit code is always 0 isn't checkable. Also drop the stale "configure a Bedrock model in litellm.yaml" hint (Bedrock is now self-service via `new-model --source bedrock`) and include `bedrockmodels` in the `status` serving-resource listing.
… closed #3 turned cluster_endpoint_public_access_cidrs into an ACTIVE assignment with a placeholder ["<REPLACE>/32"] in example.tfvars and example-esc.tfvars. That is not a valid CIDR, so copying the template and running before editing errors on the malformed CIDR — and it even slips past the fail-closed precondition (the value is non-empty and != 0.0.0.0/0), so the failure surfaces later/downstream as a confusing parse error instead of the intended allowlist guidance. Comment the line (with a valid example) so an unedited copy uses the variable's default [] and the plan-time precondition fails closed with its clear "...is not scoped. Set cluster_endpoint_public_access_cidrs..." message. Conscious opt-in; ships no 0.0.0.0/0. terraform fmt -check clean.
…dule #3 moved the Karpenter node IAM role out of the module (create_node_iam_role = false) so its trust principal is ec2.amazonaws.com (the module builds ec2.<dns_suffix> = ec2.amazonaws.eu, which ESC IAM rejects). The hand-rolled role must then carry every policy the module used to attach, on BOTH the commercial and ESC paths. Verified parity (module v21.1.5): the module's node-role base set is AmazonEKSWorkerNodePolicy + AmazonEKS_CNI_Policy(ipv4) + AmazonEC2ContainerRegistryReadOnly (no node_iam_role_additional_policies passed); the hand-rolled role attaches those three PLUS AmazonSSMManagedInstanceCore (+ optional ECR pull-through) — a strict superset. No instance-profile regression: create_instance_profile defaults false and isn't enabled, so Karpenter 1.x creates the profile at runtime from the EC2NodeClass spec.role, and the module scopes iam:PassRole + the EKS access entry to node_iam_role_arn (this role). Adds a parity-guard comment enumerating the module base set + the instance-profile rationale, with a note to re-verify on any karpenter module bump so a future change can't silently drop a node policy. Comment only; no behavior change.
The workloads AppProject namespaceResourceWhitelist allowed only the four kro.run
kinds, so ArgoCD refused to sync a BedrockModel CR committed by
'platformctl new-model --source bedrock' ('resource bedrock.ai-platform:BedrockModel
is not permitted in project ai-platform-workloads') — self-service Bedrock
enrollment never reached the cluster. Whitelist group bedrock.ai-platform / kind
BedrockModel (still namespaced-only; no cluster-scoped grant). Found via e2e test.
The ai-platform-workloads AppProject allowed the in-cluster destination only by server URL (https://kubernetes.default.svc). Self-managed ArgoCD canonicalizes the apps' 'name: local-cluster' destination to that URL, so it matched. But EKS-managed ArgoCD (managed capabilities) registers local-cluster with a different server URL, so every workload app fails with InvalidSpecError ('destination server local-cluster ... not match allowed destinations') — all self-service models/teams (incl. BedrockModel) break on managed capabilities. Also allow the destination by name (same namespaces/boundary). Found via e2e test.
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
1 participant
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.
PR #3 review — batch of follow-up fixes + self-service Bedrock
Integration branch for the fixes surfaced while reviewing #3, plus self-service
Bedrock enrollment. Validated end-to-end on a real EKS cluster in eu-central-1,
both with and without EKS Managed Capabilities (see summary at the bottom).
Included fixes
BedrockModelCRD).new-model --source bedrock [--list-available-models]resolves the region/partition-correctid + price from the live Bedrock/Price List APIs and commits a
BedrockModelCR;litellm-syncregisters it;litellm.yamlshipsmodel_list: []. Replaces thestatic, region-pinned default from AWS European Sov. Cloud Support + Minor Refactoring #3.
gp3breaks Auto Mode; comments only).
--extra-argYAML escaping viajson.dumps(a trailing\produced anunparseable manifest;
a\tcbecame a TAB). AddsTestYamlScalarQuoting.kubectlargv (was readable in/proc/<pid>/cmdline). Addstests/test_gitops.py.status --checkexits non-zero on real failures (gateway down / modelnon-200 / Langfuse unhealthy), exit 0 for the expected "no models yet" case.
cluster_endpoint_public_access_cidrsplaceholder in the tfvarsso an unedited copy fails closed with the intended message, not a CIDR parse error.
strict superset of the module's base set; no instance-profile regression. Comment.
fix(argocd): permitBedrockModelin theai-platform-workloadsAppProject.(Found via e2e.) The workloads AppProject whitelist allowed only the four
kro.runkinds, so ArgoCD refused every Bedrock enrollment (
resource … BedrockModel is not permitted in project). Whitelistedbedrock.ai-platform/BedrockModel(stillnamespaced-only).
fix(argocd): allow the workloads destination by name for EKS-managed ArgoCD.(Found via e2e; a AWS European Sov. Cloud Support + Minor Refactoring #3 regression.) PR AWS European Sov. Cloud Support + Minor Refactoring #3's
esccommit changed the workloadsAppProject destinations from
name: local-cluster→server: https://kubernetes.default.svc. That works on self-managed ArgoCD but breaksEKS-managed ArgoCD (managed capabilities — the original tested setup), where the
in-cluster destination is matched by name: every workload app failed with
InvalidSpecError. Re-added the by-name match alongside the URL entries so bothmodes work.
End-to-end validation (eu-central-1, real cluster)
Provisioned + torn down a full cluster twice — once without and once with
EKS Managed Capabilities — using the README flow. Validated on both:
platformctl up→ healthy cluster; all ArgoCD apps Synced/Healthy;BedrockModelCRD present.status --check: gateway ✓, Langfuse ✓, correct exit codes (0 with no models; live-model path 200).--list-available-models: live Bedrock catalog +eu.-profile resolution + Price List pricing.nova-lite→eu.amazon.nova-lite-v1:0) → CR → litellm-sync register → live chat 200.new-model(Qwen2.5-0.5B) → Karpenter GPU node → vLLM → live chat 200.tunnel(all 5 UIs),undeploy(deregistration for both types),onboard-team(AITeam → ns/quota).down→ clean teardown; residual checks (clusters/VPCs/instances/roles) empty.The last two fixes above were discovered and fixed during this e2e (self-service
was broken on managed capabilities before them). Teardown needed the documented
manual cleanup for orphan Karpenter instances + security groups (noted in the run).
Merge once satisfied (merge commit preserves each fix's commit; squash collapses them).