Skip to content

fix(gateway): pass NamespaceConfig by pointer so OIDC config reaches gateway.toml #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bsquizz merged 2 commits into from
Aug 14, 2026
Merged

fix(gateway): pass NamespaceConfig by pointer so OIDC config reaches gateway.toml #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f72618e
fix(gateway): pass NamespaceConfig by pointer to reconcileKeycloakClient
bsquizz Aug 14, 2026
f729bdd
fix(keycloak): use {name}-{id} format for Keycloak client IDs
bsquizz Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions components/control-plane/internal/gateway/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func ReconcileGateway(
}

if opts.Keycloak != nil {
if err := reconcileKeycloakClient(ctx, opts, nsConfig); err != nil {
if err := reconcileKeycloakClient(ctx, opts, &nsConfig); err != nil {
return fmt.Errorf("reconcile keycloak client in %s: %w", nsConfig.Name, err)
}
}
Expand Down Expand Up @@ -192,11 +192,12 @@ func DeleteGatewayResources(
log.Printf("INFO deleted ClusterRoleBinding %s", crbName)
}

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 != "" {

@bsquizz bsquizz Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kcClientID := fmt.Sprintf("%s-%s", opts.GatewayName, opts.GatewayID)

@bsquizz bsquizz Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 err := opts.KeycloakClient.DeleteGatewayClient(ctx, kcClientID); err != nil {
log.Printf("WARN failed to delete keycloak client %s (orphaned): %v", kcClientID, err)
} else {
log.Printf("INFO deleted keycloak client %s", opts.GatewayName)
log.Printf("INFO deleted keycloak client %s", kcClientID)
}
}

Expand Down Expand Up @@ -885,38 +886,41 @@ func reconcileDatabaseCredentials(ctx context.Context, clientset *kubernetes.Cli
return nil
}

func reconcileKeycloakClient(ctx context.Context, opts ReconcileOpts, nsConfig NamespaceConfig) error {
func reconcileKeycloakClient(ctx context.Context, opts ReconcileOpts, nsConfig *NamespaceConfig) error {
kc := keycloak.NewClient(
opts.Keycloak.ServerURL,
opts.Keycloak.Realm,
opts.Keycloak.ClientID,
opts.Keycloak.ClientSecret,
)

gatewayName := opts.GatewayName
if gatewayName == "" {
if opts.GatewayName == "" {
return fmt.Errorf("gateway name is required for keycloak provisioning")
}
if opts.GatewayID == "" {
return fmt.Errorf("gateway ID is required for keycloak provisioning")
}
kcClientID := fmt.Sprintf("%s-%s", opts.GatewayName, opts.GatewayID)

existingUUID, err := kc.GetClientUUID(ctx, gatewayName)
existingUUID, err := kc.GetClientUUID(ctx, kcClientID)
if err != nil {
return fmt.Errorf("check existing keycloak client: %w", err)
}

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)

@bsquizz bsquizz Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
clientUUID, err := kc.ProvisionGatewayClient(ctx, gatewayName)
clientUUID, err := kc.ProvisionGatewayClient(ctx, kcClientID)
if err != nil {
return fmt.Errorf("provision keycloak client %s: %w", gatewayName, err)
return fmt.Errorf("provision keycloak client %s: %w", kcClientID, err)
}
log.Printf("INFO provisioned keycloak client %s (uuid=%s)", gatewayName, clientUUID)
log.Printf("INFO provisioned keycloak client %s (uuid=%s)", kcClientID, clientUUID)
}

oidcConfig := OIDCConfig{
Issuer: kc.Issuer(),
ClientID: gatewayName,
Audience: gatewayName,
ClientID: kcClientID,
Audience: kcClientID,
JwksTTL: 3600,
RolesClaim: "hypershell.roles",
AdminRole: "openshell-admin",
Expand All @@ -938,7 +942,7 @@ func reconcileKeycloakClient(ctx context.Context, opts ReconcileOpts, nsConfig N
return fmt.Errorf("marshal oidc config: %w", err)
}
if err := opts.UpdateOIDC(ctx, string(oidcJSON)); err != nil {
log.Printf("WARN failed to persist oidc config for %s: %v", gatewayName, err)
log.Printf("WARN failed to persist oidc config for %s: %v", kcClientID, err)
}
}

Expand Down
6 changes: 3 additions & 3 deletions components/control-plane/internal/reconciler/role_binding_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,15 @@ func (r *RoleBindingReconciler) Handle(ctx context.Context, event watcher.Event[
return nil
}

// resolveKeycloakClientID looks up the gateway by ID and returns the gateway
// name, which is used directly as the Keycloak client ID.
// resolveKeycloakClientID looks up the gateway by ID and returns the Keycloak
// client ID in the {name}-{id} format specified by the Keycloak provisioning spec.
func (r *RoleBindingReconciler) resolveKeycloakClientID(ctx context.Context, gatewayID string) (string, error) {
client := pb.NewGatewayServiceClient(r.grpcConn)
resp, err := client.GetGateway(ctx, &pb.GetGatewayRequest{Id: gatewayID})
if err != nil {
return "", fmt.Errorf("get gateway %s: %w", gatewayID, err)
}
return resp.GetGateway().GetName(), nil
return fmt.Sprintf("%s-%s", resp.GetGateway().GetName(), gatewayID), nil

@bsquizz bsquizz Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amber — Looks good. The format matches the gateway reconciler and the spec (clientId = "{name}-{id}"). The updated comment accurately describes the returned value.

}

// assignClientRoleWithRetry retries AssignClientRole to handle the race where
Expand Down
Loading