Skip to content

Commit

Permalink
[8.9] [Fleet] Improve invalid package parsing error messages (#160368) (
Browse files Browse the repository at this point in the history
#160621)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[Fleet] Improve invalid package parsing error messages
(#160368)](#160368)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jill
Guyonnet","email":"jill.guyonnet@elastic.co"},"sourceCommit":{"committedDate":"2023-06-27T13:19:31Z","message":"[Fleet]
Improve invalid package parsing error messages (#160368)\n\n##
Summary\r\n\r\nImprove error messages of `PackageInvalidArchiveError`
errors thrown\r\nwhen parsing a package archive to include the package
name.","sha":"27f67e12009f98d3d70fc8219b9746a6e195a186","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","backport:prev-minor","v8.10.0"],"number":160368,"url":"#160368
Improve invalid package parsing error messages (#160368)\n\n##
Summary\r\n\r\nImprove error messages of `PackageInvalidArchiveError`
errors thrown\r\nwhen parsing a package archive to include the package
name.","sha":"27f67e12009f98d3d70fc8219b9746a6e195a186"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"#160368
Improve invalid package parsing error messages (#160368)\n\n##
Summary\r\n\r\nImprove error messages of `PackageInvalidArchiveError`
errors thrown\r\nwhen parsing a package archive to include the package
name.","sha":"27f67e12009f98d3d70fc8219b9746a6e195a186"}}]}] BACKPORT-->

Co-authored-by: Jill Guyonnet <jill.guyonnet@elastic.co>
  • Loading branch information
kibanamachine and jillguyonnet committed Jun 27, 2023
1 parent 29d32cc commit 5d38dea
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 12 deletions.
16 changes: 12 additions & 4 deletions x-pack/plugins/fleet/server/services/epm/archive/parse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,13 +379,17 @@ describe('parseAndVerifyArchive', () => {
expect(() =>
parseAndVerifyArchive(['input_only-0.1.0/manifest.yml', 'dummy/manifest.yml'], {})
).toThrowError(
new PackageInvalidArchiveError('Package contains more than one top-level directory.')
new PackageInvalidArchiveError(
'Package contains more than one top-level directory; top-level directory found: input_only-0.1.0; filePath: dummy/manifest.yml'
)
);
});

it('should throw on missing manifest file', () => {
expect(() => parseAndVerifyArchive(['input_only-0.1.0/test/manifest.yml'], {})).toThrowError(
new PackageInvalidArchiveError('Package must contain a top-level manifest.yml file.')
new PackageInvalidArchiveError(
'Package at top-level directory input_only-0.1.0 must contain a top-level manifest.yml file.'
)
);
});

Expand All @@ -396,7 +400,9 @@ describe('parseAndVerifyArchive', () => {
parseAndVerifyArchive(['input_only-0.1.0/manifest.yml'], {
'input_only-0.1.0/manifest.yml': buf,
})
).toThrowError('Could not parse top-level package manifest: YAMLException');
).toThrowError(
'Could not parse top-level package manifest at top-level directory input_only-0.1.0: YAMLException'
);
});

it('should throw on missing required fields', () => {
Expand All @@ -416,7 +422,9 @@ version: 0.1.0
parseAndVerifyArchive(['input_only-0.1.0/manifest.yml'], {
'input_only-0.1.0/manifest.yml': buf,
})
).toThrowError('Invalid top-level package manifest: one or more fields missing of ');
).toThrowError(
'Invalid top-level package manifest at top-level directory input_only-0.1.0 (package name: input_only): one or more fields missing of '
);
});

it('should throw on name or version mismatch', () => {
Expand Down
14 changes: 10 additions & 4 deletions x-pack/plugins/fleet/server/services/epm/archive/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,23 +182,29 @@ export function parseAndVerifyArchive(
const toplevelDir = topLevelDirOverride || paths[0].split('/')[0];
paths.forEach((filePath) => {
if (!filePath.startsWith(toplevelDir)) {
throw new PackageInvalidArchiveError('Package contains more than one top-level directory.');
throw new PackageInvalidArchiveError(
`Package contains more than one top-level directory; top-level directory found: ${toplevelDir}; filePath: ${filePath}`
);
}
});

// The package must contain a manifest file ...
const manifestFile = path.posix.join(toplevelDir, MANIFEST_NAME);
const manifestBuffer = manifests[manifestFile];
if (!paths.includes(manifestFile) || !manifestBuffer) {
throw new PackageInvalidArchiveError(`Package must contain a top-level ${MANIFEST_NAME} file.`);
throw new PackageInvalidArchiveError(
`Package at top-level directory ${toplevelDir} must contain a top-level ${MANIFEST_NAME} file.`
);
}

// ... which must be valid YAML
let manifest: ArchivePackage;
try {
manifest = yaml.safeLoad(manifestBuffer.toString());
} catch (error) {
throw new PackageInvalidArchiveError(`Could not parse top-level package manifest: ${error}.`);
throw new PackageInvalidArchiveError(
`Could not parse top-level package manifest at top-level directory ${toplevelDir}: ${error}.`
);
}

// must have mandatory fields
Expand All @@ -208,7 +214,7 @@ export function parseAndVerifyArchive(
if (!requiredKeysMatch) {
const list = requiredArchivePackageProps.join(', ');
throw new PackageInvalidArchiveError(
`Invalid top-level package manifest: one or more fields missing of ${list}`
`Invalid top-level package manifest at top-level directory ${toplevelDir} (package name: ${manifest.name}): one or more fields missing of ${list}.`
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export default function (providerContext: FtrProviderContext) {
.send(buf)
.expect(400);
expect(res.error.text).to.equal(
'{"statusCode":400,"error":"Bad Request","message":"Package contains more than one top-level directory."}'
'{"statusCode":400,"error":"Bad Request","message":"Package contains more than one top-level directory; top-level directory found: apache-0.1.4; filePath: apache-0.1.3/manifest.yml"}'
);
});

Expand All @@ -207,7 +207,7 @@ export default function (providerContext: FtrProviderContext) {
.send(buf)
.expect(400);
expect(res.error.text).to.equal(
'{"statusCode":400,"error":"Bad Request","message":"Package must contain a top-level manifest.yml file."}'
'{"statusCode":400,"error":"Bad Request","message":"Package at top-level directory apache-0.1.4 must contain a top-level manifest.yml file."}'
);
});

Expand All @@ -220,7 +220,7 @@ export default function (providerContext: FtrProviderContext) {
.send(buf)
.expect(400);
expect(res.error.text).to.equal(
'{"statusCode":400,"error":"Bad Request","message":"Could not parse top-level package manifest: YAMLException: bad indentation of a mapping entry at line 2, column 7:\\n name: apache\\n ^."}'
'{"statusCode":400,"error":"Bad Request","message":"Could not parse top-level package manifest at top-level directory apache-0.1.4: YAMLException: bad indentation of a mapping entry at line 2, column 7:\\n name: apache\\n ^."}'
);
});

Expand All @@ -233,7 +233,7 @@ export default function (providerContext: FtrProviderContext) {
.send(buf)
.expect(400);
expect(res.error.text).to.equal(
'{"statusCode":400,"error":"Bad Request","message":"Invalid top-level package manifest: one or more fields missing of name, version, description, title, format_version, owner"}'
'{"statusCode":400,"error":"Bad Request","message":"Invalid top-level package manifest at top-level directory apache-0.1.4 (package name: apache): one or more fields missing of name, version, description, title, format_version, owner."}'
);
});

Expand Down

0 comments on commit 5d38dea

Please sign in to comment.