Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions e2e/cypress.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export default defineConfig({
'./cypress/tests/21-*/**/*.ts',
'./cypress/tests/22-*/*.ts',
'./cypress/tests/23-*/*.ts',
'./cypress/tests/99-*/*.ts',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

]
return config
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { v4 as uuidv4 } from 'uuid'

import { createRuntimeGroup, createSubsystem, uniqueSubsystemName } from '../../support/sdx-commands'

describe('SDX Subsystem Gateway Registration Recovery', () => {
let workingData: any

before(() => {
cy.buildOrgGatewayDatasetAndProduct().then((data) => {
workingData = data

const rg = uuidv4().replace(/-/g, '').toUpperCase().substring(0, 6)
workingData['runtimeGroupId'] = rg.toLowerCase()

createRuntimeGroup(workingData.org, workingData.runtimeGroupId, 'dev')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any way to predictably simulate a failed run to create partial state?

})
})

it('PUT /organizations/{org}/subsystems/{name}/gateway - retrying a registration for the same subsystem recovers instead of failing', () => {
const { org, runtimeGroupId } = workingData
const subsystemName = uniqueSubsystemName()

createSubsystem(org, subsystemName, () => {
cy.setRequestBody({ runtimeGroupName: runtimeGroupId })
cy.callAPI(
`ds/api/sdx/v1/organizations/${org.name}/subsystems/${subsystemName}/gateway`,
'PUT'
).then(({ apiRes: { status, body } }: any) => {
expect(status, body.message).to.be.equal(200)
expect(body).to.have.property('gatewayId')
const gatewayId = body.gatewayId

// Simulate a caller retrying the same registration request - e.g.
// because it never saw the first response (timeout, dropped
// connection, portal restart). The subsystem's gateway/namespace ID
// is fixed and immutable, so this always targets the exact same
// namespace as the first call.
cy.setRequestBody({ runtimeGroupName: runtimeGroupId })
cy.callAPI(
`ds/api/sdx/v1/organizations/${org.name}/subsystems/${subsystemName}/gateway`,
'PUT'
).then(({ apiRes: { status, body } }: any) => {
expect(status, body.message).to.be.equal(200)
expect(body).to.have.property('gatewayId')
expect(body.gatewayId).to.be.equal(gatewayId)

// The subsystem catalog should reflect a single, consistent
// registration - no duplicate/corrupted state left behind by the
// repeated registration attempt.
cy.callAPI(
`ds/api/sdx/v1/organizations/${org.name}/subsystems`,
'GET'
).then(({ apiRes: { status, body } }: any) => {
expect(status).to.be.equal(200)
const matches = body.filter((s: any) => s.name === subsystemName)
expect(matches.length).to.be.equal(1)
expect(matches[0].gatewayId).to.be.equal(gatewayId)
})
})
})
})
})
})
4 changes: 4 additions & 0 deletions src/services/org-groups/namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ export class NamespaceService {
assert.strictEqual(groupExists, false, 'Namespace already exists');
}

async namespaceExists(ns: string): Promise<boolean> {
return this.groupService.hasGroup('ns', ns);
}

async listAssignedNamespacesByOrg(org: string): Promise<OrgNamespace[]> {
const groups = await this.groupService.getGroups('ns', false);
assert.strictEqual(
Expand Down
8 changes: 6 additions & 2 deletions src/services/workflow/create-namespace-sdx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import { CreateNamespace, CreateNamespaceArgs } from './create-namespace';
import assert from '../user-assert';
import { EnvironmentContext, getEnvironmentContext } from './get-namespaces';
import { lookupProductEnvironmentServicesBySlug } from '../keystone';
import { createUmaPolicy, updateUmaPolicy } from './ns-uma-policy-access';
import {
createUmaPolicyIfMissing,
updateUmaPolicy,
} from './ns-uma-policy-access';
import { SysGroupAccessService } from '../org-groups/sys-group-access';
import { GroupAccessService } from '../org-groups';

Expand Down Expand Up @@ -296,6 +299,7 @@ async function createSDXNamespace(
'GatewayPattern.Publish',
];
args.includeSDXScopes = true;
args.allowResume = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


const resourceSet = await CreateNamespace(context, args);

Expand All @@ -308,7 +312,7 @@ async function createSDXNamespace(
scopes: ['GatewayConfig.Publish', 'Namespace.Manage'],
};

const umaResult = await createUmaPolicy(
const umaResult = await createUmaPolicyIfMissing(
context,
envCtx,
resourceSet.id,
Expand Down
33 changes: 28 additions & 5 deletions src/services/workflow/create-namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ export interface CreateNamespaceArgs {
routePaths?: string[];
assignedScopes?: string[];
includeSDXScopes?: boolean;
/**
* When true, an existing namespace with this exact name is treated as an
* in-progress/partially-completed registration to resume rather than a
* conflict to reject. Only safe when `name` is a stable, pre-allocated
* identifier (e.g. a Subsystem's own `namespace`) that can never
* legitimately collide with a different owner's namespace.
*/
allowResume?: boolean;
}

export async function CreateNamespace(
Expand Down Expand Up @@ -67,7 +75,15 @@ export async function CreateNamespace(
envCtx.issuerEnvConfig.clientId,
envCtx.issuerEnvConfig.clientSecret
);
await nsService.checkNamespaceAvailable(newNS);
const resuming = args.allowResume && (await nsService.namespaceExists(newNS));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

if (!resuming) {
await nsService.checkNamespaceAvailable(newNS);
} else {
logger.info(
'[CreateNamespace] Namespace %s already exists, resuming registration',
newNS
);
}

// This function gets all resources but also sets the accessToken in envCtx
// which we need to create the resource set
Expand Down Expand Up @@ -98,7 +114,10 @@ export async function CreateNamespace(
ownerManagedAccess: true,
};

const rset = await resourceApi.createResourceSet(res);
const existingRset = resuming
? await resourceApi.findResourceByName(newNS)
: undefined;
const rset = existingRset || (await resourceApi.createResourceSet(res));

if (isUserBasedResourceOwners(envCtx) == false) {
const permissionApi = new KeycloakPermissionTicketService(
Expand All @@ -111,7 +130,7 @@ export async function CreateNamespace(
'GatewayConfig.Publish',
'Access.Manage',
]) {
await permissionApi.createPermission(
await permissionApi.createOrUpdatePermission(
rset.id,
envCtx.subjectUuid,
true,
Expand All @@ -128,9 +147,13 @@ export async function CreateNamespace(
envCtx.issuerEnvConfig.clientSecret
);

const { id, created } = await kcGroupService.createIfMissing('ns', newNS);
const { id } = await kcGroupService.createIfMissing('ns', newNS);

if (created) {
{
// Reconcile attributes unconditionally (not just on first create) so a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

// resumed registration still lands on the fully-configured end state,
// even if an earlier attempt died after the group was created but
// before attributes were applied.
const gwGroup = await kcGroupService.getGroupById(id);
if (args.org) {
gwGroup.attributes['org'] = [args.org];
Expand Down
31 changes: 31 additions & 0 deletions src/services/workflow/ns-uma-policy-access.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,37 @@ export async function createUmaPolicy(
return umaPolicy;
}

export async function createUmaPolicyIfMissing(
context: any,
envCtx: EnvironmentContext,
resourceId: string,
policy: Policy
) {
logger.debug('[createUmaPolicyIfMissing] %s %j', resourceId, policy);

await enforceAccessToResource(envCtx, resourceId);

const policyApi = new UMAPolicyService(
envCtx.uma2.policy_endpoint,
envCtx.accessToken
);

const existing = (await policyApi.listPolicies({ resource: resourceId }))
.filter((p) => policy.clients?.every((c) => p.clients?.includes(c)))
.pop();

if (existing) {
logger.debug(
'[createUmaPolicyIfMissing] %s already has a policy for %j, skipping create',
resourceId,
policy.clients
);
return existing;
}

return createUmaPolicy(context, envCtx, resourceId, policy);
}

export async function updateUmaPolicy(
context: any,
envCtx: EnvironmentContext,
Expand Down