Skip to content
Merged
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
64 changes: 39 additions & 25 deletions packages/kbn-api-contracts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,40 +55,21 @@ oasdiff detects these as breaking:
| **Required property added** | `new-required-request-property` | New required `email` field on request body |
| **Optional made required** | `request-parameter-became-required` | `filter` query param becomes required |
| **Type changed** | `response-property-type-changed` | `id` changed from string to number |
| **Request body tightened** | `kbn:request-additional-properties-tightened` | Request body schema gains `additionalProperties: false` (clients sending unknown keys now receive 400) |

⚠️ oasdiff classifies these as warnings, but they are promoted to blocking here because Terraform provider configurations depend on these fields.

## Allowlist

For approved breaking changes, add entries to `allowlist.json`:
For approved breaking changes, add entries to `allowlist.json`. **Always prefer the granular form below** — it scopes suppression to one specific breaking change instead of muting everything on the endpoint.

```json
{
"entries": [
{
"path": "/api/saved_objects/{type}/{id}",
"method": "delete",
"reason": "Intentional removal as part of saved objects migration",
"approvedBy": "elastic/kibana-core",
"prUrl": "https://github.com/elastic/kibana/pull/12345",
"expiresAt": "2026-12-31"
}
]
}
```

**Required fields:** `path`, `method`, `reason`, `approvedBy`
**Optional fields:** `prUrl`, `expiresAt`, `oasdiffId`, `source`

### Granular suppression
### Granular form (recommended)

By default, an allowlist entry suppresses **all** breaking changes for a given `(path, method)`. To scope suppression to a specific breaking change, use `oasdiffId` and/or `source`:
Use `oasdiffId` together with `source` to suppress exactly one breaking change. These fields are AND'd with `path` and `method`: the entry only matches changes for which all four fields agree.

- `oasdiffId` — matches the oasdiff rule ID (e.g. `request-property-removed`). See the [Breaking Change Rules](#breaking-change-rules) table for known IDs.
- `oasdiffId` — matches the oasdiff rule ID (e.g. `request-property-removed`, `kbn:request-additional-properties-tightened`). See the [Breaking Change Rules](#breaking-change-rules) table for known IDs.
- `source` — matches the JSON pointer / source location reported by oasdiff (e.g. `/components/schemas/Output/properties/name`).

When present, these fields are AND'd: the entry only suppresses changes that match **all** specified fields. Omitted fields are not checked (legacy behavior).

```json
{
"path": "/api/fleet/outputs",
Expand All @@ -100,7 +81,40 @@ When present, these fields are AND'd: the entry only suppresses changes that mat
}
```

This entry only suppresses `request-property-removed` at that specific schema location — other breaking changes on the same endpoint will still be flagged.
Example targeting the new request-body tightening rule:

```json
{
"path": "/api/data_views/data_view",
"method": "post",
"reason": "Intentional tightening — coordinated with @elastic/terraform-provider",
"approvedBy": "@elastic/kibana-data-discovery",
"oasdiffId": "kbn:request-additional-properties-tightened",
"source": "/components/schemas/Data_views_create_data_view_request_object"
}
```

**Required fields:** `path`, `method`, `reason`, `approvedBy`, `oasdiffId`, `source` (the last two only required for granular suppression).
**Optional fields:** `prUrl`, `expiresAt`.

### Coarse form (⚠️ avoid unless absolutely necessary — this masks all future breaks on the endpoint)

Omitting `oasdiffId` and `source` makes the entry suppress **every** breaking change for that `(path, method)`. This is dangerous: a coarse entry approved today silently swallows any unrelated tightening, removal, or type change that lands on the same endpoint in the future. Reach for it only when several distinct, approved changes ship together and a single granular entry per change is impractical.

```json
{
"entries": [
{
"path": "/api/saved_objects/{type}/{id}",
"method": "delete",
"reason": "Intentional removal as part of saved objects migration",
"approvedBy": "elastic/kibana-core",
"prUrl": "https://github.com/elastic/kibana/pull/12345",
"expiresAt": "2026-12-31"
}
]
}
```

## API Ownership

Expand Down
110 changes: 109 additions & 1 deletion packages/kbn-api-contracts/scripts/check_contracts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,23 @@ jest.mock('../src/diff/run_oasdiff', () => ({
runOasdiff: jest.fn(),
}));

jest.mock('../src/diff/run_oasdiff_structural', () => ({
runOasdiffStructural: jest.fn(),
}));

jest.mock('../src/diff/parse_oasdiff', () => ({
parseOasdiff: jest.fn(),
}));

jest.mock('../src/input/load_oas', () => ({
loadOas: jest.fn().mockResolvedValue({
openapi: '3.0.0',
info: { title: 't', version: '1' },
paths: {},
components: { schemas: {} },
}),
}));

jest.mock('../src/terraform/check_terraform_impact', () => ({
checkTerraformImpact: jest.fn(),
}));
Expand All @@ -55,8 +68,9 @@ jest.mock('../src/report/format_failure', () => ({

import { execSync } from 'child_process';
import { writeFileSync, rmSync } from 'fs';
import { runOasdiff, parseOasdiff, applyAllowlist } from '../src/diff';
import { runOasdiff, runOasdiffStructural, parseOasdiff, applyAllowlist } from '../src/diff';
import type { BreakingChange } from '../src/diff';
import { loadOas } from '../src/input/load_oas';
import { checkTerraformImpact } from '../src/terraform/check_terraform_impact';
import { loadAllowlist } from '../src/allowlist/load_allowlist';
import { formatFailure } from '../src/report/format_failure';
Expand All @@ -65,7 +79,11 @@ const mockRun = jest.requireMock('@kbn/dev-cli-runner').run as jest.Mock;
const mockExecSync = execSync as jest.MockedFunction<typeof execSync>;
const mockWriteFileSync = writeFileSync as jest.MockedFunction<typeof writeFileSync>;
const mockRunOasdiff = runOasdiff as jest.MockedFunction<typeof runOasdiff>;
const mockRunOasdiffStructural = runOasdiffStructural as jest.MockedFunction<
typeof runOasdiffStructural
>;
const mockParseOasdiff = parseOasdiff as jest.MockedFunction<typeof parseOasdiff>;
const mockLoadOas = loadOas as jest.MockedFunction<typeof loadOas>;
const mockCheckTerraformImpact = checkTerraformImpact as jest.MockedFunction<
typeof checkTerraformImpact
>;
Expand Down Expand Up @@ -98,6 +116,12 @@ describe('check_contracts', () => {
error: jest.fn(),
};
mockExecSync.mockReturnValue('openapi: 3.0.0\npaths: {}');
mockLoadOas.mockResolvedValue({
openapi: '3.0.0',
info: { title: 't', version: '1' },
paths: {},
components: { schemas: {} },
});
});

const defaultFlags = {
Expand Down Expand Up @@ -424,4 +448,88 @@ describe('check_contracts', () => {

expect(mockLog.info).toHaveBeenCalledWith('1 allowlisted change(s) ignored');
});

describe('additionalProperties tightening (E2E reverse-index path)', () => {
const realParseOasdiff = jest.requireActual('../src/diff/parse_oasdiff')
.parseOasdiff as typeof parseOasdiff;

it('surfaces a synthetic component-level entry exactly once for the consumer endpoint', async () => {
const consumerPath = '/api/data_views/data_view';
const componentName = 'Data_views_create_data_view_request_object';

mockLoadOas.mockResolvedValue({
openapi: '3.0.0',
info: { title: 't', version: '1' },
paths: {
[consumerPath]: {
post: {
requestBody: {
content: {
'application/json': {
schema: { $ref: `#/components/schemas/${componentName}` },
},
},
},
},
},
},
components: {
schemas: {
[componentName]: { type: 'object' },
},
},
});

mockRunOasdiff.mockReturnValue([]);
mockRunOasdiffStructural.mockReturnValue({
components: {
schemas: {
modified: {
[componentName]: {
additionalPropertiesAllowed: { from: null, to: false },
},
},
},
},
});

// Use the real parseOasdiff so the synthetic entry flows through the
// same parse pipeline as oasdiff entries; pass-through applyAllowlist so
// nothing is filtered out.
mockParseOasdiff.mockImplementation(realParseOasdiff);
mockApplyAllowlist.mockImplementation((changes) => ({
breakingChanges: changes,
allowlistedChanges: [],
}));

mockCheckTerraformImpact.mockImplementation((changes) => ({
hasImpact: changes.length > 0,
impactedChanges: changes.map((change) => ({
change,
terraformResource: 'elasticstack_kibana_data_view',
owners: ['@elastic/kibana-data-discovery'],
})),
}));
mockLoadAllowlist.mockReturnValue({ entries: [] });
mockFormatFailure.mockReturnValue('FAILURE REPORT');

await expect(runCallback({ flags: defaultFlags, log: mockLog })).rejects.toThrow(
'Found 1 breaking change(s)'
);

expect(mockFormatFailure).toHaveBeenCalledTimes(1);
const breakingChanges = mockFormatFailure.mock.calls[0][0];
expect(breakingChanges).toEqual([
{
type: 'request_body_tightened',
path: consumerPath,
method: 'POST',
reason:
'Request body schema disallows extra fields (additionalProperties: false). Clients sending unknown keys will now receive 400.',
oasdiffId: 'kbn:request-additional-properties-tightened',
source: `/components/schemas/${componentName}`,
},
]);
});
});
});
24 changes: 22 additions & 2 deletions packages/kbn-api-contracts/scripts/check_contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ import { execSync } from 'child_process';
import { writeFileSync, mkdirSync, rmSync } from 'fs';
import { resolve } from 'path';
import { run } from '@kbn/dev-cli-runner';
import { runOasdiff, parseOasdiff, applyAllowlist } from '../src/diff';
import {
runOasdiff,
runOasdiffStructural,
parseOasdiff,
applyAllowlist,
buildRequestBodyIndex,
detectAdditionalPropertiesTightening,
} from '../src/diff';
import { loadOas } from '../src/input/load_oas';
import { formatFailure } from '../src/report/format_failure';
import { writeImpactReport } from '../src/report/write_impact_report';
import { loadAllowlist } from '../src/allowlist/load_allowlist';
Expand Down Expand Up @@ -185,8 +193,10 @@ run(
log.info(`Filtering oasdiff to ${terraformApis.length} Terraform provider API paths`);
}
let diffEntries;
let structuralDiff: unknown;
try {
diffEntries = runOasdiff(basePath, currentPath, { matchPath });
structuralDiff = runOasdiffStructural(basePath, currentPath, { matchPath });
} catch (error: unknown) {
// Some older branch specs (e.g. 9.3) have example objects incorrectly
// placed under `#/components/schemas/` instead of `#/components/examples/`.
Expand All @@ -202,7 +212,17 @@ run(
}
throw error;
}
const allBreakingChanges = parseOasdiff(diffEntries);

const currentOas = await loadOas(currentPath);
const requestBodyIndex = buildRequestBodyIndex(currentOas);
const { entries: syntheticEntries, warnings: detectorWarnings } =
detectAdditionalPropertiesTightening(structuralDiff, requestBodyIndex);

for (const warning of detectorWarnings) {
log.warning(warning);
}

const allBreakingChanges = parseOasdiff([...diffEntries, ...syntheticEntries]);

if (allBreakingChanges.length === 0) {
log.success('No breaking changes detected');
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
openapi: 3.0.0
info:
title: component_body_tightened
version: '1'
paths:
/api/test:
post:
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/Body'
responses:
'200':
description: ok
components:
schemas:
Body:
type: object
properties:
a:
type: string
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
openapi: 3.0.0
info:
title: component_body_tightened
version: '1'
paths:
/api/test:
post:
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/Body'
responses:
'200':
description: ok
components:
schemas:
Body:
type: object
additionalProperties: false
properties:
a:
type: string
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
openapi: 3.0.0
info:
title: inline_body_tightened
version: '1'
paths:
/api/test:
post:
requestBody:
content:
application/json:
schema:
type: object
properties:
a:
type: string
responses:
'200':
description: ok
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
openapi: 3.0.0
info:
title: inline_body_tightened
version: '1'
paths:
/api/test:
post:
requestBody:
content:
application/json:
schema:
type: object
additionalProperties: false
properties:
a:
type: string
responses:
'200':
description: ok
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
openapi: 3.0.0
info:
title: loosening_no_flag
version: '1'
paths:
/api/test:
post:
requestBody:
content:
application/json:
schema:
type: object
additionalProperties: false
properties:
a:
type: string
responses:
'200':
description: ok
Loading
Loading