diff --git a/docs/commands/publish.md b/docs/commands/publish.md index 8d14dc32..5a0eb6bf 100644 --- a/docs/commands/publish.md +++ b/docs/commands/publish.md @@ -203,6 +203,8 @@ The output lists each resource and the planned action (create, update, or delete For a full publish, `--delete-unmatched` deletes APIM resources that are **not** present in the source artifacts. With `--commit-id`, it deletes only resources and Product associations removed by the selected commit. +Deletes run from the most dependent resources to the least (tier 4 to tier 1). Within a tier, resources can use each other — a backend pool its members, a backend or logger the named value in its credentials, a policy fragment a named value — and APIM rejects deleting one that something still uses. Such a delete is tried again once the rest of the tier is deleted, so one run removes them all. A full `--delete-unmatched` leaves APIM's built-in resources alone (the built-in groups, the `unlimited` and `starter` products, and `echo-api`); with `--commit-id`, a commit that removes one of their artifacts deletes it. The built-in `master` subscription is never deleted. + > **Warning:** Use with caution. Resources created manually in the Azure portal that are not in your artifact directory will be removed. ## Exit codes diff --git a/docs/guides/environment-overrides.md b/docs/guides/environment-overrides.md index b6be22fa..77f8bc1a 100644 --- a/docs/guides/environment-overrides.md +++ b/docs/guides/environment-overrides.md @@ -307,7 +307,10 @@ subscriptions: scope: "/apis/petstore-api" ``` -> **Note:** The built-in `master` subscription is automatically skipped during publish. +> **Note:** APIM's built-in `master` subscription (the all-access one, scoped to the whole instance) is left alone: +> extract doesn't write it, publish doesn't publish it, and neither `--delete-unmatched` nor an incremental publish +> deletes it, since APIM can't recreate it. An artifact for it from an earlier extract is ignored, and +> `--remove-stale` removes it when subscriptions are in its scope. > When publish creates a new product, the subscriptions APIM automatically creates for it are deleted, > so only the extracted product subscriptions (including ones with 24-character hex IDs) are published. > Deletion is best-effort: a failed delete is logged as a warning and does not fail the publish. diff --git a/src/clients/apim-client.ts b/src/clients/apim-client.ts index 06adbf5e..658daae2 100644 --- a/src/clients/apim-client.ts +++ b/src/clients/apim-client.ts @@ -44,6 +44,22 @@ export function isLinkAlreadyExistsError(error: unknown): boolean { return error instanceof HttpError && error.status === 409; } +/** + * Returns true when APIM rejects a DELETE because another resource still uses + * the one being deleted, for example "The Api Management Property 'x' is used + * by the following entities: /backends/y", "Backend Entity 'x' is referenced + * in Backend 'pool'" or "The Policy Fragment 'x' is used by the following + * entities". The message can be the error's own or one of its details, so the + * whole response text is matched. + */ +export function isResourceInUseError(error: unknown): boolean { + return ( + error instanceof HttpError && + error.status === 400 && + /is used by the following entities|is referenced in/i.test(error.message) + ); +} + /** Returns true when APIM reports that an association's referenced API/group is absent. */ export function isAssociationReferenceNotFoundError(error: unknown): boolean { if ( diff --git a/src/models/resource-types.ts b/src/models/resource-types.ts index 6686edb1..215f38bf 100644 --- a/src/models/resource-types.ts +++ b/src/models/resource-types.ts @@ -97,6 +97,22 @@ export interface ResourceTypeMetadata { readonly workspaceLinkIdProperty?: string; } +/** + * Whether a descriptor is APIM's built-in all-access subscription: `master`, + * scoped to the service root. APIM creates it and manages it: it can't be + * created or updated through the API (a subscription's scope has to be an API + * or a product), and one that is deleted can't be recreated. Extract, publish + * and deletes leave it alone. + */ +export function isBuiltInSubscription(descriptor: { type: ResourceType; nameParts: string[]; workspace?: string }): boolean { + return ( + descriptor.type === ResourceType.Subscription && + !descriptor.workspace && + descriptor.nameParts.length === 1 && + descriptor.nameParts[0]?.toLowerCase() === 'master' + ); +} + export const RESOURCE_TYPE_METADATA: Record = { [ResourceType.NamedValue]: { armPathSuffix: 'namedValues/{0}', diff --git a/src/services/delete-unmatched-service.ts b/src/services/delete-unmatched-service.ts index bbb77691..ccfe275b 100644 --- a/src/services/delete-unmatched-service.ts +++ b/src/services/delete-unmatched-service.ts @@ -27,7 +27,7 @@ import type { IApimClient } from '../clients/iapim-client.js'; import type { IArtifactStore } from '../clients/iartifact-store.js'; import type { ApimServiceContext, ResourceDescriptor } from '../models/types.js'; import type { PublishConfig } from '../models/config.js'; -import { ResourceType } from '../models/resource-types.js'; +import { isBuiltInSubscription, ResourceType } from '../models/resource-types.js'; import { getTopologicalOrder } from '../lib/dependency-graph.js'; import { getNameFromNameParts, @@ -86,7 +86,6 @@ const BUILT_IN_GROUPS = ['administrators', 'developers', 'guests']; * System resources that should never be deleted */ const SYSTEM_RESOURCES = new Set([ - 'master', // Master product 'unlimited', // Unlimited product 'starter', // Starter product 'echo-api', // Echo API (system test API) @@ -135,7 +134,11 @@ export async function computeDeleteActions( for await (const resource of apimResources) { const deployedDescriptor = parseResourceDescriptor(resource, resourceType); - if (!deployedDescriptor) { + // APIM's built-in `master` subscription can't be recreated once deleted. + // It is checked under its name on the instance: with an environment + // affix, a source name can deploy as `master`, and `master` can read as + // another source name. + if (!deployedDescriptor || isBuiltInSubscription(deployedDescriptor)) { continue; } diff --git a/src/services/dry-run-reporter.ts b/src/services/dry-run-reporter.ts index 4431e810..4b6e8008 100644 --- a/src/services/dry-run-reporter.ts +++ b/src/services/dry-run-reporter.ts @@ -10,7 +10,7 @@ import type { IApimClient } from '../clients/iapim-client.js'; import type { IArtifactStore } from '../clients/iartifact-store.js'; import type { ApimServiceContext, ResourceDescriptor } from '../models/types.js'; import type { PublishConfig } from '../models/config.js'; -import { ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js'; +import { isBuiltInSubscription, ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js'; import { getTopologicalOrder } from '../lib/dependency-graph.js'; import { buildResourceLabel } from '../lib/resource-uri.js'; import { getResourceDescriptorKey } from '../lib/resource-path.js'; @@ -167,6 +167,10 @@ export async function generateDryRunReport( const deployedDescriptor = config.envMapping ? mapDescriptor(descriptor, config.envMapping) : descriptor; + // As publish: never the built-in `master` subscription, by its name on the instance. + if (isBuiltInSubscription(deployedDescriptor)) { + continue; + } const resolvedDescriptor = await resolveAssociationDeleteDescriptor( client, context, diff --git a/src/services/publish-service.ts b/src/services/publish-service.ts index 6d1e966a..90402064 100644 --- a/src/services/publish-service.ts +++ b/src/services/publish-service.ts @@ -7,10 +7,11 @@ */ import { IApimClient } from '../clients/iapim-client.js'; +import { isResourceInUseError } from '../clients/apim-client.js'; import { IArtifactStore } from '../clients/iartifact-store.js'; import { FilterConfig, PublishConfig, OverrideConfig } from '../models/config.js'; import { ApimServiceContext, ResourceDescriptor } from '../models/types.js'; -import { ResourceType } from '../models/resource-types.js'; +import { isBuiltInSubscription, ResourceType } from '../models/resource-types.js'; import { getResourceTier, TIER_LABELS } from '../lib/dependency-graph.js'; import { runParallel } from '../lib/parallel-runner.js'; import { logger } from '../lib/logger.js'; @@ -125,7 +126,7 @@ async function runPublishPipeline( await client.validatePreFlight(config.service); // Step 1: Determine target descriptors based on incremental mode - const { targetDescriptors, deletedDescriptors } = await determinePublishTargets( + const { targetDescriptors: selectedDescriptors, deletedDescriptors } = await determinePublishTargets( store, config ); @@ -139,10 +140,13 @@ async function runPublishPipeline( // every unchanged artifact in incremental runs. const allDescriptors = config.commitId ? await store.listResources(config.sourceDir) - : targetDescriptors; + : selectedDescriptors; // Step 1b: Validate env mapping and store it on config for downstream use validateAndBuildEnvMapping(config.overrides, allDescriptors, config); + const targetDescriptors = selectedDescriptors.filter( + (descriptor) => !deploysAsBuiltInSubscription(descriptor, config) + ); // Step 1c: Build known artifact sets for policy ref rewriting config.knownArtifactSets = buildKnownArtifactSets(allDescriptors); @@ -299,6 +303,8 @@ async function determinePublishTargets( deletedDescriptors = []; } + // APIM's built-in `master` subscription can't be published (isBuiltInSubscription). + targetDescriptors = targetDescriptors.filter((descriptor) => !isBuiltInSubscription(descriptor)); targetDescriptors = filterPublishDescriptors(targetDescriptors, config.filter); deletedDescriptors = filterPublishDescriptors(deletedDescriptors, config.filter); @@ -316,6 +322,22 @@ async function determinePublishTargets( return { targetDescriptors, deletedDescriptors }; } +/** + * Whether an environment affix makes a subscription's name on the instance + * `master`: publishing it would write to APIM's built-in subscription + * (isBuiltInSubscription), so it is left out with a warning. The source's + * own `master` is left out quietly when choosing what to publish. + */ +function deploysAsBuiltInSubscription(descriptor: ResourceDescriptor, config: PublishConfig): boolean { + if (!config.envMapping || !isBuiltInSubscription(mapDescriptor(descriptor, config.envMapping))) { + return false; + } + logger.warn( + `Skipping subscription "${descriptor.nameParts.join('/')}": the environment affix would publish it as APIM's built-in "master" subscription` + ); + return true; +} + async function expandTransitivePublishTargets( store: IArtifactStore, sourceDir: string, @@ -988,7 +1010,13 @@ async function executeDeletesForDescriptors( // removes all of its revisions in one call. Drop individual ;rev=N deletes // whose base API is also in the delete set to avoid redundant/racy deletes and // the "Cannot delete the current revision of an API" error. - const effectiveDescriptors = filterRevisionDeletesHandledByBaseApi(deleteDescriptors); + // APIM's built-in `master` subscription is never deleted: one that is can't + // be recreated (isBuiltInSubscription). It is checked under the name on the + // instance, which an environment affix can make `master`. + const effectiveDescriptors = filterRevisionDeletesHandledByBaseApi(deleteDescriptors) + .filter((descriptor) => !isBuiltInSubscription( + config.envMapping && !descriptorsAreDeployed ? mapDescriptor(descriptor, config.envMapping) : descriptor + )); // Group by tier const tierGroups = new Map(); @@ -1064,10 +1092,6 @@ async function deleteTier( status: deleted ? ('success' as const) : ('skipped' as const), }; } catch (error) { - logger.error( - `Failed to delete ${buildResourceLabel(descriptor)}:`, - error - ); return { descriptor, action: 'delete' as const, @@ -1077,18 +1101,17 @@ async function deleteTier( } }; - const tasks = descriptors.map((descriptor) => async () => { - const { value, retries } = await trackRetries(() => deleteDescriptor(descriptor)); - return attachRetries([value], retries)[0] ?? value; - }); - - const taskResults = await runParallel(tasks, 5); - - return taskResults.map((tr, index) => { - if (tr.status === 'fulfilled' && tr.value) { - return tr.value; - } else { - const descriptor = descriptors[index]; + const deleteAll = async (pending: ResourceDescriptor[]): Promise => { + const tasks = pending.map((descriptor) => async () => { + const { value, retries } = await trackRetries(() => deleteDescriptor(descriptor)); + return attachRetries([value], retries)[0] ?? value; + }); + const taskResults = await runParallel(tasks, 5); + return taskResults.map((tr, index) => { + if (tr.status === 'fulfilled' && tr.value) { + return tr.value; + } + const descriptor = pending[index]; if (!descriptor) { throw new Error('No descriptor found for failed task'); } @@ -1098,7 +1121,47 @@ async function deleteTier( status: 'failed' as const, error: tr.reason || new Error('Unknown error'), }; + }); + }; + + // Resources in one tier can use each other (a backend pool its members, a + // backend or logger the named value in its credentials, a fragment a named + // value), and APIM rejects deleting one that something still uses. Such a + // delete is tried again once the rest of the tier is gone, for as long as + // each round deletes something. + const finalResults = new Map(); + // HTTP retries add up across rounds: a delete can be retried before APIM + // rejects it as in use, then succeed in a later round. + const retries = new Map(); + let pending = descriptors; + while (pending.length > 0) { + const roundResults = await deleteAll(pending); + for (const result of roundResults) { + retries.set(result.descriptor, (retries.get(result.descriptor) ?? 0) + (result.retries ?? 0)); + } + const inUse = roundResults.filter((result) => result.status === 'failed' && isResourceInUseError(result.error)); + const progressed = roundResults.some((result) => result.status !== 'failed'); + for (const result of roundResults) { + if (!inUse.includes(result) || !progressed) { + finalResults.set(result.descriptor, result); + } + } + if (!progressed) { + break; + } + pending = inUse.map((result) => result.descriptor); + if (pending.length > 0) { + logger.debug(`Deleting ${pending.length} resource(s) again that others being deleted were still using`); + } + } + + return descriptors.map((descriptor) => { + const total = retries.get(descriptor) ?? 0; + const result = { ...finalResults.get(descriptor)!, ...(total > 0 ? { retries: total } : {}) }; + if (result.status === 'failed') { + logger.error(`Failed to delete ${buildResourceLabel(descriptor)}:`, result.error); } + return result; }); } diff --git a/src/services/resource-extractor.ts b/src/services/resource-extractor.ts index c62c8c6a..24b3d70a 100644 --- a/src/services/resource-extractor.ts +++ b/src/services/resource-extractor.ts @@ -9,7 +9,7 @@ import { IApimClient } from '../clients/iapim-client.js'; import { IArtifactStore } from '../clients/iartifact-store.js'; import { ApimServiceContext, ResourceDescriptor } from '../models/types.js'; -import { ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js'; +import { isBuiltInSubscription, ResourceType, RESOURCE_TYPE_METADATA } from '../models/resource-types.js'; import { redactSecrets } from './secret-redactor.js'; import { shouldIncludeResource } from './filter-service.js'; import { FilterConfig } from '../models/config.js'; @@ -104,6 +104,10 @@ export async function extractResourceType( logger.debug(`Filtered out ${buildResourceLabel(descriptor)}`); continue; } + if (isBuiltInSubscription(descriptor)) { + logger.debug(`Skipping ${buildResourceLabel(descriptor)}: APIM manages the built-in subscription`); + continue; + } // Some APIM list endpoints return a shallow response that omits the // heavyweight payload we need for round-trip publish. ApiSchema list diff --git a/src/services/resource-publisher.ts b/src/services/resource-publisher.ts index fe9f3729..055bb240 100644 --- a/src/services/resource-publisher.ts +++ b/src/services/resource-publisher.ts @@ -417,9 +417,8 @@ export async function publishResource( // but the PUT endpoint requires a relative APIM path (e.g. "/apis" or // "/apis/{apiId}" or "/products/{productId}"). Strip the service base path // prefix when it is present so that the publish round-trip works correctly. - // HOWEVER: Skip publishing subscriptions with root-level scope (e.g. the built-in - // "master" subscription), as APIM treats these as read-only system resources - // and returns ValidationError when attempting to update them. + // The built-in root-scoped `master` subscription never gets here: publish + // leaves it out (isBuiltInSubscription). if (descriptor.type === ResourceType.Subscription) { const subscriptionName = getNamePart(descriptor.nameParts, 0); const eligibility = await evaluateResourceEligibility( @@ -921,15 +920,6 @@ export async function evaluateResourceEligibility( return { eligible: true }; } - const properties = json.properties as Record | undefined; - const scope = properties?.scope as string | undefined; - if ( - scope && - (scope.endsWith('/') || (!scope.includes('/apis') && !scope.includes('/products'))) - ) { - return { eligible: false, reason: 'root-scoped subscriptions are managed by APIM' }; - } - for (const target of findSubscriptionTargets(json, descriptor.workspace)) { const eligibility = await evaluateAssociationEligibility( store, diff --git a/tests/unit/models/resource-types.test.ts b/tests/unit/models/resource-types.test.ts index f28b6698..b928aa3a 100644 --- a/tests/unit/models/resource-types.test.ts +++ b/tests/unit/models/resource-types.test.ts @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. import { describe, it, expect } from 'vitest'; -import { ResourceType, RESOURCE_TYPE_METADATA } from '../../../src/models/resource-types.js'; +import { ResourceType, RESOURCE_TYPE_METADATA, isBuiltInSubscription } from '../../../src/models/resource-types.js'; describe('ResourceType enum', () => { it('should have exactly 35 resource types', () => { @@ -146,3 +146,13 @@ describe('RESOURCE_TYPE_METADATA', () => { } }); }); + +describe('isBuiltInSubscription', () => { + it("should recognise only the service-level master subscription", () => { + expect(isBuiltInSubscription({ type: ResourceType.Subscription, nameParts: ['master'] })).toBe(true); + expect(isBuiltInSubscription({ type: ResourceType.Subscription, nameParts: ['MASTER'] })).toBe(true); + expect(isBuiltInSubscription({ type: ResourceType.Subscription, nameParts: ['master'], workspace: 'team-a' })).toBe(false); + expect(isBuiltInSubscription({ type: ResourceType.Subscription, nameParts: ['orders-sub'] })).toBe(false); + expect(isBuiltInSubscription({ type: ResourceType.Product, nameParts: ['master'] })).toBe(false); + }); +}); diff --git a/tests/unit/services/delete-unmatched-service.test.ts b/tests/unit/services/delete-unmatched-service.test.ts index 615b567e..16af1f90 100644 --- a/tests/unit/services/delete-unmatched-service.test.ts +++ b/tests/unit/services/delete-unmatched-service.test.ts @@ -130,7 +130,6 @@ describe('delete-unmatched-service', () => { it('should skip system products', async () => { const apimResources = new Map[]>([ [ResourceType.Product, [ - { name: 'master', id: '/products/master' }, { name: 'unlimited', id: '/products/unlimited' }, { name: 'starter', id: '/products/starter' }, { name: 'custom-product', id: '/products/custom-product' }, @@ -143,12 +142,51 @@ describe('delete-unmatched-service', () => { const result = await computeDeleteActions(client, store, testContext, testConfig); const products = result.filter((d) => d.type === ResourceType.Product).map((d) => d.nameParts[0]); - expect(products).not.toContain('master'); expect(products).not.toContain('unlimited'); expect(products).not.toContain('starter'); expect(products).toContain('custom-product'); }); + it("should never delete APIM's built-in master subscription", async () => { + const apimResources = new Map[]>([ + [ResourceType.Subscription, [ + { name: 'master', id: '/subscriptions/master', properties: { scope: '/' } }, + { name: 'old-sub', id: '/subscriptions/old-sub', properties: { scope: '/apis/orders' } }, + ]], + ]); + + const result = await computeDeleteActions(createMockClient(apimResources), createMockStore([]), testContext, testConfig); + + expect(result.filter((d) => d.type === ResourceType.Subscription).map((d) => d.nameParts[0])).toEqual(['old-sub']); + }); + + it("should decide master by its name on the instance when names are affixed", async () => { + const apimResources = new Map[]>([ + [ResourceType.Subscription, [ + { name: 'master', id: '/subscriptions/master', properties: { scope: '/' } }, + { name: 'mold-sub', id: '/subscriptions/mold-sub', properties: { scope: '/apis/orders' } }, + ]], + ]); + const subscriptions = async (namePrefix: string) => (await computeDeleteActions( + createMockClient(apimResources), createMockStore([]), testContext, { ...testConfig, envMapping: buildEnvMapping({ namePrefix }) } + )).filter((d) => d.type === ResourceType.Subscription).map((d) => d.nameParts[0]); + + // With prefix `m`, master reads as source name `aster`; it is still never deleted. + expect(await subscriptions('m')).toEqual(['mold-sub']); + }); + + it("should delete a user subscription whose source name is master", async () => { + const apimResources = new Map[]>([ + [ResourceType.Subscription, [{ name: 'dev-master', id: '/subscriptions/dev-master', properties: { scope: '/apis/orders' } }]], + ]); + + const result = await computeDeleteActions( + createMockClient(apimResources), createMockStore([]), testContext, { ...testConfig, envMapping: buildEnvMapping({ namePrefix: 'dev-' }) } + ); + + expect(result.map((d) => d.nameParts[0])).toEqual(['dev-master']); + }); + it('should skip echo-api system API', async () => { const apimResources = new Map[]>([ [ResourceType.Api, [ diff --git a/tests/unit/services/dry-run-reporter.test.ts b/tests/unit/services/dry-run-reporter.test.ts index dc530e9e..a78cc608 100644 --- a/tests/unit/services/dry-run-reporter.test.ts +++ b/tests/unit/services/dry-run-reporter.test.ts @@ -750,9 +750,6 @@ describe('dry-run-reporter', () => { ['Subscription:team:workspace-product-sub', { properties: { scope: '/products/team-store' }, }], - ['Subscription::root-sub', { - properties: { scope: armPrefix }, - }], ['Product::store', { properties: {} }], ['Api:team:orders', { properties: {} }], ['Product:team:team-store', { properties: {} }], @@ -766,7 +763,7 @@ describe('dry-run-reporter', () => { const config: PublishConfig = { ...testConfig, filter: { - subscriptions: ['product-sub', 'excluded-api-sub', 'missing-product-sub', 'root-sub'], + subscriptions: ['product-sub', 'excluded-api-sub', 'missing-product-sub'], products: ['store', 'missing'], apis: ['!legacy', '*'], workspaces: ['team'], @@ -799,7 +796,6 @@ describe('dry-run-reporter', () => { nameParts: ['workspace-product-sub'], workspace: 'team', }, - { type: ResourceType.Subscription, nameParts: ['root-sub'] }, ] ); @@ -809,13 +805,8 @@ describe('dry-run-reporter', () => { { operation: 'SKIP', name: 'missing-product-sub' }, { operation: 'PUT', name: 'workspace-api-sub' }, { operation: 'PUT', name: 'workspace-product-sub' }, - { - operation: 'SKIP', - name: 'root-sub', - reason: 'root-scoped subscriptions are managed by APIM', - }, ]); - expect(report.summary).toEqual({ creates: 3, patches: 0, deletes: 0, skips: 3 }); + expect(report.summary).toEqual({ creates: 3, patches: 0, deletes: 0, skips: 2 }); expect(client.putResource).not.toHaveBeenCalled(); expect(client.patchResource).not.toHaveBeenCalled(); expect(client.deleteResource).not.toHaveBeenCalled(); diff --git a/tests/unit/services/publish-service.test.ts b/tests/unit/services/publish-service.test.ts index 6bcac061..51ad68ae 100644 --- a/tests/unit/services/publish-service.test.ts +++ b/tests/unit/services/publish-service.test.ts @@ -8,7 +8,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { ResourceType } from '../../../src/models/resource-types.js'; import { ResourceDescriptor, ApimServiceContext } from '../../../src/models/types.js'; import { PublishConfig } from '../../../src/models/config.js'; -import { LogLevel } from '../../../src/lib/logger.js'; +import { LogLevel, logger } from '../../../src/lib/logger.js'; +import { HttpError } from '../../../src/clients/apim-client.js'; import { recordRetry, withRetryResource } from '../../../src/lib/retry-tracker.js'; import { getResourceDescriptorKey } from '../../../src/lib/resource-path.js'; @@ -1138,6 +1139,152 @@ describe('publish-service', () => { expect(computeGitDiff).toHaveBeenCalledWith('/source', 'abc123'); }); + it("should not delete the built-in master subscription that an affixed source name deploys as", async () => { + const client = createMockClient(); + vi.mocked(computeGitDiff).mockResolvedValue({ + changedDescriptors: [], + deletedDescriptors: [{ type: ResourceType.Subscription, nameParts: ['aster'] }], + }); + + await runPublish(client, createMockStore([]), { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: true, + commitId: 'abc123', + overrides: { environment: { namePrefix: 'm' } }, + logLevel: LogLevel.INFO, + }); + + expect(client.deleteResource).not.toHaveBeenCalled(); + }); + + it('should not publish, or plan in dry-run, a subscription that an affix deploys as the built-in master', async () => { + const store = createMockStore([ + { type: ResourceType.Subscription, nameParts: ['aster'] }, + { type: ResourceType.Subscription, nameParts: ['orders-sub'] }, + ]); + const warn = vi.spyOn(logger, 'warn'); + const base: PublishConfig = { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: false, + overrides: { environment: { namePrefix: 'm' } }, + logLevel: LogLevel.INFO, + }; + + const client = createMockClient(); + await runPublish(client, store, base); + expect(client.putResource.mock.calls.map(([, descriptor]) => descriptor.nameParts[0])).toEqual(['morders-sub']); + expect(warn.mock.calls.map(([message]) => String(message)).filter((message) => message.includes('"aster"'))).toHaveLength(1); + + await runPublish(client, store, { ...base, dryRun: true }); + expect(vi.mocked(generateDryRunReport).mock.calls.at(-1)?.[4]).toEqual([ + { type: ResourceType.Subscription, nameParts: ['orders-sub'] }, + ]); + warn.mockRestore(); + }); + + it("should neither publish nor delete APIM's built-in master subscription, without a warning", async () => { + const client = createMockClient(); + const master = { type: ResourceType.Subscription, nameParts: ['master'] }; + const store = createMockStore([master, { type: ResourceType.Subscription, nameParts: ['orders-sub'] }]); + const warn = vi.spyOn(logger, 'warn'); + const base: PublishConfig = { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: false, + logLevel: LogLevel.INFO, + }; + + await runPublish(client, store, base); + expect(client.putResource.mock.calls.map(([, descriptor]) => descriptor.nameParts[0])).toEqual(['orders-sub']); + expect(warn.mock.calls.map(([message]) => String(message)).filter((message) => message.includes('master'))).toEqual([]); + + // An incremental publish lists it as deleted when its artifact is removed. + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [], deletedDescriptors: [master] }); + await runPublish(client, store, { ...base, deleteUnmatched: true, commitId: 'abc123' }); + expect(client.deleteResource).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + describe('deleting resources that use each other', () => { + const inUse = (name: string, user: string) => new HttpError( + 400, + `HTTP 400: {"error":{"code":"ValidationError","message":"The Api Management Property '${name}' is used by the following entities: /${user}"}}`, + 'ValidationError' + ); + const descriptor = (type: ResourceType, name: string): ResourceDescriptor => ({ type, nameParts: [name] }); + const run = async (deleted: ResourceDescriptor[], deleteResource: (name: string) => Promise) => { + const client = createMockClient(); + client.deleteResource.mockImplementation(async (_context: ApimServiceContext, d: ResourceDescriptor) => deleteResource(d.nameParts[0] ?? '')); + vi.mocked(computeGitDiff).mockResolvedValue({ changedDescriptors: [], deletedDescriptors: deleted }); + const result = await runPublish(client, createMockStore([]), { + service: testContext, + sourceDir: '/source', + dryRun: false, + deleteUnmatched: true, + commitId: 'abc123', + logLevel: LogLevel.INFO, + }); + const calls = client.deleteResource.mock.calls.map(([, d]) => (d as ResourceDescriptor).nameParts[0]); + return { result, calls }; + }; + + it('should delete a pool and its member, and a logger and the named value in its credentials, in one run', async () => { + const existing = new Set(['pool', 'member', 'key', 'ai']); + const { result, calls } = await run( + [descriptor(ResourceType.Backend, 'member'), descriptor(ResourceType.NamedValue, 'key'), + descriptor(ResourceType.Backend, 'pool'), descriptor(ResourceType.Logger, 'ai')], + async (name) => { + if (name === 'member' && existing.has('pool')) throw inUse('member', 'backends/pool'); + if (name === 'key' && existing.has('ai')) throw inUse('key', 'loggers/ai'); + existing.delete(name); + return true; + } + ); + + expect(existing.size).toBe(0); + expect(result.exitCode).toBe(0); + expect(calls.filter((name) => name === 'member')).toHaveLength(2); + }); + + it('should report the HTTP retries a delete needed across rounds', async () => { + const existing = new Set(['pool', 'member']); + const { result } = await run( + [descriptor(ResourceType.Backend, 'member'), descriptor(ResourceType.Backend, 'pool')], + async (name) => { + if (name === 'member' && existing.has('pool')) { + recordRetry(); + throw inUse('member', 'backends/pool'); + } + existing.delete(name); + return true; + } + ); + + expect(result.actions.find((action) => action.descriptor.nameParts[0] === 'member')?.retries).toBe(1); + expect(result.totalRetries).toBe(1); + }); + + it('should not try again a delete that failed for another reason, or one nothing in the run frees', async () => { + const { result, calls } = await run( + [descriptor(ResourceType.Tag, 'broken'), descriptor(ResourceType.Tag, 'busy'), descriptor(ResourceType.Tag, 'free')], + async (name) => { + if (name === 'broken') throw new HttpError(500, 'HTTP 500: boom'); + if (name === 'busy') throw inUse('busy', 'apis/elsewhere'); + return true; + } + ); + + expect(calls.filter((name) => name === 'broken')).toHaveLength(1); + expect(calls.filter((name) => name === 'busy')).toHaveLength(2); + expect(result.exitCode).toBe(1); + }); + }); + it('should delete descriptors removed in the commit when explicitly enabled', async () => { const client = createMockClient(); const store = createMockStore([]); diff --git a/tests/unit/services/resource-extractor.test.ts b/tests/unit/services/resource-extractor.test.ts index 9c828123..3611f808 100644 --- a/tests/unit/services/resource-extractor.test.ts +++ b/tests/unit/services/resource-extractor.test.ts @@ -107,6 +107,17 @@ describe('resource-extractor', () => { expect(result.extracted[0]?.descriptor.nameParts[0]).toBe('nv-1'); }); + it("should leave out APIM's built-in master subscription", async () => { + const client = createMockClient([ + { name: 'master', properties: { scope: '/subscriptions/s/resourceGroups/rg/providers/Microsoft.ApiManagement/service/apim-1' } }, + { name: 'orders-sub', properties: { scope: '/apis/orders' } }, + ]); + + const result = await extractResourceType(client, createMockStore(), testContext, ResourceType.Subscription, '/output'); + + expect(result.extracted.map((extracted) => extracted.descriptor.nameParts[0])).toEqual(['orders-sub']); + }); + it('should redact secret named values', async () => { const client = createMockClient([ { name: 'secret-nv', properties: { secret: true, value: 'hidden' } }, diff --git a/tests/unit/services/resource-publisher.test.ts b/tests/unit/services/resource-publisher.test.ts index 8e1154e5..81f99339 100644 --- a/tests/unit/services/resource-publisher.test.ts +++ b/tests/unit/services/resource-publisher.test.ts @@ -1366,37 +1366,6 @@ describe('resource-publisher', () => { expect(client.putResource).toHaveBeenCalledOnce(); }); - it('should skip subscription with root scope (master subscription)', async () => { - // The master subscription has scope as the service root, which results in - // "/" after ARM path stripping. This is invalid and the subscription should be skipped. - const client = createMockClient(); - const store = createMockStore(); - - const armScopePrefix = - '/subscriptions/sub-1/resourceGroups/rg-1/providers/Microsoft.ApiManagement/service/apim-1'; - - const subscriptionJson = { - name: 'master', - properties: { - displayName: 'Master subscription', - scope: armScopePrefix, // service root, results in "/" after stripping - state: 'active', - }, - }; - store.readResource.mockResolvedValue(subscriptionJson); - - const descriptor: ResourceDescriptor = { - type: ResourceType.Subscription, - nameParts: ['master'], - }; - - const result = await publishResource(client, store, testContext, descriptor, testConfig); - - expect(result.status).toBe('skipped'); - expect(result.action).toBe('noop'); - expect(client.putResource).not.toHaveBeenCalled(); - }); - it('should publish 24-hex product subscription with empty displayName', async () => { const client = createMockClient(); const store = createMockStore();