fix: send and read keycloak settings in vdeployer's nested shape - #63
Merged
Conversation
vdeployer moved keycloak_client_id / client_secret / organization_id / organization_name under a nested `keycloak:` config class as a clean break, with no compatibility aliases. Every flat key v8x wrote now binds to nothing and is dropped by pydantic's extra="ignore", so a cluster deployed through v8x would come up with no keycloak config at all -- vdeployer derives cluster_name from client_id and raises ValueError building the tunnel URL without it. Five write sites migrated (vdeployer_web/deploy.py x2, cluster/utils.py, cluster/create.py, apps/lxd/slurm/app.py) and one read site that would otherwise have gone quietly empty: apps/lxd/slurm/app.py reads organization_id/organization_name out of the same settings dict to pass --organization-id/--organization-name to the gateway Istio auth policy. A write-only rename would have disabled that policy without an error. That file already reads a nested vantage_lxd_autoscaler block from the same dict, so nesting is the established shape there. Each site merges into any existing `keycloak` block rather than assigning over it. Also drops autoscaler_enabled from the --settings examples in CLI help and docstrings. The key no longer exists -- the correct autoscaler is selected by cloud provider in vdeployer's Phase 0 orchestration -- so the examples were teaching a key that is silently ignored. Swapped for kubeflow_enabled, which is a real toggle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The nesting change shipped with no test asserting the wire shape -- grepping tests/ for "keycloak" found only an unrelated JWT fixture. The flat form produced no error anywhere, so a regression would be silent and would break deploys rather than degrade them: vdeployer derives cluster_name from client_id and raises ValueError building the tunnel URL without it. Five tests on build_vdeployer_settings: the nested block is produced and no flat keycloak_* key survives; a caller-supplied block is merged rather than overwritten; the caller's dict is not mutated, since dict(settings) is only a shallow copy; unrelated settings pass through; and the missing org-context guard still aborts. Three on the read side, using the existing provider-command harness. --organization-id/--organization-name come from the nested block; absent keycloak settings omit the flags rather than raising, since older clusters have none; and a stale *flat* blob is explicitly asserted NOT to work. That last one would pass silently if the read still honoured the old keys, which is exactly the half-migrated state worth catching. Also drops autoscaler_enabled from docusaurus/docs/commands.mdx, which is generated from CLI help and went stale when the help text changed. Hand-edited to match the source: scripts/generate_complete_docs.py hangs when run, and the justfile has its invocation commented out, so a regeneration was not available. docusaurus/build/ is a build artifact and was left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI lint failed on C408 in the test file added by the previous commit. I missed it locally by piping `ruff check` through `tail -1` and reading "No fixes available" while "Found 1 error" scrolled past -- the project lints tests/ as well as the source, so `just lint` was the check to run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What
Sends and reads the keycloak settings in the nested shape
vdeployernow expects, and dropsautoscaler_enabledfrom the CLI examples.Why
vdeployermovedkeycloak_client_id/client_secret/organization_id/organization_nameunder a nestedkeycloak:config class as a clean break — no compatibility aliases.Every flat key v8x wrote now binds to nothing and is dropped by pydantic's
extra="ignore". That breaks deploys outright:vdeployerderivescluster_namefromclient_idand raisesValueErrorbuilding the tunnel URL without it.The read site is the subtle one
Five write sites were the easy part.
apps/lxd/slurm/app.pyalso readsorganization_id/organization_nameback out of the same settings dict to pass--organization-id/--organization-nameinto the gateway Istio auth policy.A write-only rename would have left those reads returning
Noneand silently disabled the org-claim policy — no error, no log, just a missing auth policy. That file already reads a nestedvantage_lxd_autoscalerblock from the same dict, which confirmed nesting is the established shape there.Sites changed
commands/vdeployer_web/deploy.py(×2)commands/cluster/utils.pycommands/cluster/create.pyapps/lxd/slurm/app.pyapps/lxd/slurm/app.pyEach write merges into an existing
keycloakblock rather than assigning over it.Also:
autoscaler_enabledexamplesThe key no longer exists — the correct autoscaler is selected by cloud provider in
vdeployer's Phase 0 orchestration, with no settings flag consulted. The--settingsexamples in CLI help and docstrings were teaching users a key that is silently ignored. Swapped forkubeflow_enabled, which is a real toggle.Verification
Requires the corresponding
vdeployerchange to be deployed; the two are a matched pair.🤖 Generated with Claude Code