SP136: Subsystem gateway registration is create-only, no partial-state recovery - #1526
SP136: Subsystem gateway registration is create-only, no partial-state recovery#1526ikethecoder wants to merge 1 commit into
Conversation
…ilure
PUT .../subsystems/{name}/gateway always targets the same fixed, pre-allocated
namespace ID for a given subsystem, but the registration sequence
(CreateNamespace/createSDXNamespace) was create-only: checkNamespaceAvailable
hard-rejected any second call, and createResourceSet/createUmaPolicy had no
idempotency, so a call interrupted partway (timeout, restart, dropped
connection) could never be retried or completed - only failed forever with
"Namespace already exists".
CreateNamespace now accepts allowResume, under which an existing namespace is
treated as an in-progress registration to resume: the existing UMA resource
set is reused (findResourceByName) instead of duplicated, permission tickets
are upserted (createOrUpdatePermission) instead of blindly created, and group
attributes are reconciled unconditionally instead of only on first create.
createSDXNamespace opts into this and swaps createUmaPolicy for the new
createUmaPolicyIfMissing, which skips creating a duplicate policy for a
client that already has one on the resource.
Adds a failing-before/passing-after e2e test under tests/99-sp136 that
registers a subsystem's gateway twice and asserts the retry recovers with the
same gatewayId instead of erroring.
rustyjux
left a comment
rustyjux
left a comment
There was a problem hiding this comment.
I think we should cover the case of UMA resource exists but group doesn't - see comment on create-namespace.ts
| const rg = uuidv4().replace(/-/g, '').toUpperCase().substring(0, 6) | ||
| workingData['runtimeGroupId'] = rg.toLowerCase() | ||
|
|
||
| createRuntimeGroup(workingData.org, workingData.runtimeGroupId, 'dev') |
There was a problem hiding this comment.
any way to predictably simulate a failed run to create partial state?
| 'GatewayPattern.Publish', | ||
| ]; | ||
| args.includeSDXScopes = true; | ||
| args.allowResume = true; |
There was a problem hiding this comment.
PR only talks about subsystem, but this will apply to org and runtime group gateways too. Those all have fixed gateway names so should be fine too.
| envCtx.issuerEnvConfig.clientSecret | ||
| ); | ||
| await nsService.checkNamespaceAvailable(newNS); | ||
| const resuming = args.allowResume && (await nsService.namespaceExists(newNS)); |
There was a problem hiding this comment.
Resume is keyed only on the Keycloak ns group
If the first attempt created the UMA resource but died before the ns group, retry thinks it’s a fresh create and may fail on createResourceSet (name already taken). The stuck cases this PR fixes well are “group exists, rest incomplete.” The earlier half of the pipeline is still fragile.
Fix would involve treating 'already in progress' as the group or UMA resource already exists.
| './cypress/tests/21-*/**/*.ts', | ||
| './cypress/tests/22-*/*.ts', | ||
| './cypress/tests/23-*/*.ts', | ||
| './cypress/tests/99-*/*.ts', |
There was a problem hiding this comment.
Could the existing duplicate-registration cases be updated as part of this change? 21-sdx-api/v1/05-gateways.ts is already included above and still expects the second runtime-group and subsystem gateway PUTs to return 422 with Namespace already exists, while the new 99-sp136 case expects the second PUT to return 200. With this pattern enabled, the full E2E suite has contradictory expectations and the older cases appear likely to fail.
|
|
||
| if (created) { | ||
| { | ||
| // Reconcile attributes unconditionally (not just on first create) so a |
There was a problem hiding this comment.
Could we guard the resume path against a different requested configuration? A second subsystem gateway PUT can supply another runtimeGroupName; once resume is allowed, this block overwrites perm-runtime-group and rewrites its domains. That appears to turn the create-only endpoint into a gateway move/update rather than an idempotent retry. If moving is intended, could that behavior be documented and tested? Otherwise, could the resume path verify that the stored assignment matches the request and reject a mismatch?
3 participants
Summary
PUT /organizations/{org}/subsystems/{name}/gatewayalways targets a subsystem's fixed, pre-allocated namespace ID, but the registration flow (CreateNamespaceForSubsystem→createSDXNamespace→CreateNamespace) was create-only:checkNamespaceAvailablehard-rejected any second call for that namespace, andcreateResourceSet/createUmaPolicyhad no idempotency. A registration interrupted partway (timeout, pod restart, dropped connection) could never be retried or completed — every subsequent call just failed withNamespace already exists, forever.src/services/org-groups/namespace.ts: addnamespaceExists, a non-throwing check alongside the existingcheckNamespaceAvailable.src/services/workflow/create-namespace.ts: addCreateNamespaceArgs.allowResume. When set and the namespace already exists, skip the hard failure and instead reuse the existing UMA resource set (findResourceByName), upsert permission tickets (createOrUpdatePermission), and always reconcile thensgroup's attributes instead of only on first create. Off by default — every other namespace-creation caller (org, runtime group) is unaffected.src/services/workflow/create-namespace-sdx.ts:createSDXNamespace(the only caller with a stable, pre-allocated namespace ID) passesallowResume: trueand calls the newcreateUmaPolicyIfMissing.src/services/workflow/ns-uma-policy-access.ts: addcreateUmaPolicyIfMissing, which skips creating a duplicate UMA policy for a client that already has one on the resource.No
provisioner-apichanges — gateway registration never calls the provisioner; it only grants thesdx-provisionerservice account UMA scopes for its own later, already-idempotent publish flow.Test plan
e2e/cypress/tests/99-sp136/01-subsystem-gateway-registration-recovery.ts: registers a subsystem's gateway, then registers it again (simulating a retry) and asserts200+ the samegatewayId, then confirms the subsystem list is still consistent.422 Namespace already exists).e2e/cypress/tests/21-sdx-api/v1/01-subsystems.ts(existing subsystem CRUD/gateway suite) — 11/11 passing, no regression.