fix(gateway): pass NamespaceConfig by pointer so OIDC config reaches gateway.toml - #132
Conversation
reconcileKeycloakClient received nsConfig by value, so its OIDC mutation (nsConfig.Gateway.OIDC = oidcConfig) was invisible to the caller. deployGateway then rendered gateway.toml with an empty OIDC config, producing gateways without an [openshell.gateway.oidc] section. The UpdateOIDC callback persisted the OIDC data to the API server, but the phase gate prevented re-reconciliation, so the gateway never self-healed. Pass *NamespaceConfig so the OIDC config is visible when deployGateway renders the ConfigMap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The spec requires clientId = "{name}-{id}" to prevent name clashes
when gateways share a name across fleets or are deleted and recreated.
The code was using just the gateway name, so two gateways named
"my-gateway" would collide in Keycloak.
Apply the {name}-{id} format in all three paths:
- reconcileKeycloakClient (provisioning)
- DeleteGatewayResources (cleanup)
- RoleBindingReconciler.resolveKeycloakClientID (role assignment)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
claude didn't make any tests? |
bsquizz
left a comment
bsquizz
left a comment
There was a problem hiding this comment.
Amber Review
Both changes are correct and spec-aligned. The pass-by-pointer fix resolves the OIDC config not reaching gateway.toml, and the {name}-{id} client ID format matches the Keycloak provisioning spec. Three inline observations below — none are blockers.
Confidence: High (95%)
| Finding | Severity | Verdict |
|---|---|---|
Duplicated {name}-{id} format string across 3 sites |
Minor | Consider extracting a helper to prevent drift |
DeleteGatewayResources silently skips Keycloak cleanup when GatewayID is empty |
Minor | Correct guard, but could orphan old clients |
No test coverage for reconcileKeycloakClient or resolveKeycloakClientID |
Major | Pre-existing gap, not introduced here |
| if err := opts.KeycloakClient.DeleteGatewayClient(ctx, opts.GatewayName); err != nil { | ||
| log.Printf("WARN failed to delete keycloak client %s (orphaned): %v", opts.GatewayName, err) | ||
| if opts.KeycloakClient != nil && opts.GatewayName != "" && opts.GatewayID != "" { | ||
| kcClientID := fmt.Sprintf("%s-%s", opts.GatewayName, opts.GatewayID) |
There was a problem hiding this comment.
Amber — Minor: The {name}-{id} format string (fmt.Sprintf("%s-%s", ...)) now appears in three places: here, reconcileKeycloakClient (line 903), and resolveKeycloakClientID in role_binding_reconciler.go. If the format ever changes (e.g. a separator change for Keycloak compatibility), all three must be updated in lockstep.
Consider extracting a small helper:
func keycloakClientID(gatewayName, gatewayID string) string {
return fmt.Sprintf("%s-%s", gatewayName, gatewayID)
}Not a blocker — three call sites is borderline — but it would eliminate a drift risk.
| if opts.KeycloakClient != nil && opts.GatewayName != "" { | ||
| if err := opts.KeycloakClient.DeleteGatewayClient(ctx, opts.GatewayName); err != nil { | ||
| log.Printf("WARN failed to delete keycloak client %s (orphaned): %v", opts.GatewayName, err) | ||
| if opts.KeycloakClient != nil && opts.GatewayName != "" && opts.GatewayID != "" { |
There was a problem hiding this comment.
Amber — Minor: Adding opts.GatewayID != "" to the guard is correct — without it we'd construct a malformed client ID. However, this is a behavioral change: gateways that were previously cleaned up using only GatewayName will now silently skip Keycloak cleanup if GatewayID is empty.
If any gateways were provisioned under the old {name}-only format (before this PR), their Keycloak clients would be orphaned on deletion since the new code won't find them. If this is the first deployment with Keycloak enabled, this is a non-issue. Otherwise, a one-time migration/cleanup script for existing clients may be needed.
Confidence: Medium — depends on whether any gateways exist with old-format client IDs.
|
|
||
| if existingUUID != "" { | ||
| log.Printf("INFO keycloak client %s already exists (uuid=%s), skipping provisioning", gatewayName, existingUUID) | ||
| log.Printf("INFO keycloak client %s already exists (uuid=%s), skipping provisioning", kcClientID, existingUUID) |
There was a problem hiding this comment.
Amber — Note (pre-existing): This skipping provisioning path is a create-or-skip pattern. Per CLAUDE.md: "Reconcile, don't create-or-skip: Use update-or-create patterns." Ideally this would verify the existing client's configuration (roles, mappers, fullScopeAllowed, redirect URIs) matches the spec and update if needed.
Not introduced by this PR and not a blocker, but worth noting since the Keycloak spec requires specific client properties (fullScopeAllowed=false, PKCE, scopes) that could drift if the client was provisioned by a previous code version.
| return "", fmt.Errorf("get gateway %s: %w", gatewayID, err) | ||
| } | ||
| return resp.GetGateway().GetName(), nil | ||
| return fmt.Sprintf("%s-%s", resp.GetGateway().GetName(), gatewayID), nil |
There was a problem hiding this comment.
Amber — Looks good. The format matches the gateway reconciler and the spec (clientId = "{name}-{id}"). The updated comment accurately describes the returned value.
bsquizz
added
amber/approved
2 participants
Summary
Two bugs in the Keycloak OIDC provisioning path:
OIDC config lost on first provision:
reconcileKeycloakClienttooknsConfig NamespaceConfigby value, so its OIDC mutation was invisible to the caller.deployGatewayrenderedgateway.tomlwithout the[openshell.gateway.oidc]section. TheUpdateOIDCcallback persisted OIDC to the API server, but the phase gate blocked re-reconciliation, so gateways never self-healed. Fix: pass*NamespaceConfig.Keycloak client ID missing resource ID suffix: The spec requires
clientId = "{name}-{id}"to prevent collisions when gateways share a name across fleets or are deleted/recreated. The code used just the gateway name everywhere — provisioning, deletion, and role assignment. Fix: computefmt.Sprintf("%s-%s", name, id)in all three paths.Observed on
openshell-fc593ad6a828a608— gateway deployed withallow_unauthenticated_users = falsebut no[openshell.gateway.oidc]section.Test plan
gateway.tomlcontains[openshell.gateway.oidc]on first provision{name}-{id}format (e.g.,my-gateway-2FhMpQzXBz){name}-{id}client ID when assigning roles{name}-{id}Keycloak client is cleaned upgo vet ./...passesgo build ./...passes🤖 Generated with Claude Code