-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fleet] Make upload and registry package info consistent #126915
Changes from 3 commits
cf96755
625f582
5ff6ee6
c564592
586c08c
6a9c1e9
62a45cc
cae823e
37f60f3
4212c27
0f779d9
000f3a1
6fdfd1f
8656915
29d903c
a316ab1
0ba1902
14e7b87
eadab35
1da1eba
a1254e7
dadfe20
5d97961
9122a63
bd2e6c2
bfffa79
f0e9da1
29a30e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,4 +70,5 @@ export interface PackageSpecScreenshot { | |
title: string; | ||
size?: string; | ||
type?: string; | ||
path?: string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import path from 'path'; | ||
import fs from 'fs/promises'; | ||
|
||
import JSON5 from 'json5'; | ||
import { REPO_ROOT } from '@kbn/utils'; | ||
|
||
import * as Registry from '../services/epm/registry'; | ||
import { parseAndVerifyArchiveEntries } from '../services/epm/archive'; | ||
|
||
import { createAppContextStartContractMock } from '../mocks'; | ||
import { appContextService } from '../services'; | ||
|
||
import { useDockerRegistry } from './helpers'; | ||
|
||
describe('validate bundled packages', () => { | ||
const registryUrl = useDockerRegistry(); | ||
let mockContract: ReturnType<typeof createAppContextStartContractMock>; | ||
|
||
beforeEach(() => { | ||
mockContract = createAppContextStartContractMock({ registryUrl }); | ||
appContextService.start(mockContract); | ||
}); | ||
|
||
async function getBundledPackageEntries() { | ||
const configFilePath = path.resolve(REPO_ROOT, 'fleet_packages.json'); | ||
const configFile = await fs.readFile(configFilePath, 'utf8'); | ||
const bundledPackages = JSON5.parse(configFile); | ||
|
||
return bundledPackages as Array<{ name: string; version: string }>; | ||
} | ||
|
||
async function setupPackageObjects() { | ||
const bundledPackages = await getBundledPackageEntries(); | ||
|
||
const packageObjects = await Promise.all( | ||
bundledPackages.map(async (bundledPackage) => { | ||
const registryPackage = await Registry.getRegistryPackage( | ||
bundledPackage.name, | ||
bundledPackage.version | ||
); | ||
|
||
const packageArchive = await Registry.fetchArchiveBuffer( | ||
bundledPackage.name, | ||
bundledPackage.version | ||
); | ||
|
||
return { registryPackage, packageArchive }; | ||
}) | ||
); | ||
|
||
return packageObjects; | ||
} | ||
|
||
it('generates matching package info objects for uploaded and registry packages', async () => { | ||
const packageObjects = await setupPackageObjects(); | ||
|
||
for (const packageObject of packageObjects) { | ||
const { registryPackage, packageArchive } = packageObject; | ||
|
||
const archivePackageInfo = await parseAndVerifyArchiveEntries( | ||
packageArchive.archiveBuffer, | ||
'application/zip' | ||
); | ||
|
||
expect(archivePackageInfo.packageInfo.data_streams).toEqual( | ||
registryPackage.packageInfo.data_streams | ||
); | ||
} | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
* 2.0. | ||
*/ | ||
|
||
import { merge } from '@kbn/std'; | ||
import yaml from 'js-yaml'; | ||
import { pick, uniq } from 'lodash'; | ||
|
||
|
@@ -32,6 +33,42 @@ import { unpackBufferEntries } from './index'; | |
const MANIFESTS: Record<string, Buffer> = {}; | ||
const MANIFEST_NAME = 'manifest.yml'; | ||
|
||
const DEFAULT_RELEASE_VALUE = 'ga'; | ||
|
||
const DEFAULT_INGEST_PIPELINE_VALUE = 'default'; | ||
const DEFAULT_INGEST_PIPELINE_FILE_NAME_YML = 'default.yml'; | ||
const DEFAULT_INGEST_PIPELINE_FILE_NAME_JSON = 'default.json'; | ||
|
||
// Borrowed from https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/utils/expand_dotted.ts | ||
// With some alterations around non-object values | ||
const expandDottedField = (dottedFieldName: string, val: unknown): object => { | ||
const parts = dottedFieldName.split('.'); | ||
|
||
if (parts.length === 1) { | ||
return { [parts[0]]: val }; | ||
} else { | ||
return { [parts[0]]: expandDottedField(parts.slice(1).join('.'), val) }; | ||
} | ||
}; | ||
|
||
export const expandDottedObject = (dottedObj: object) => { | ||
if (typeof dottedObj !== 'object' || Array.isArray(dottedObj)) { | ||
return dottedObj; | ||
} | ||
return Object.entries(dottedObj).reduce( | ||
(acc, [key, val]) => merge(acc, expandDottedField(key, val)), | ||
{} | ||
); | ||
}; | ||
|
||
export const expandDottedEntries = (obj: object) => { | ||
return Object.entries<any>(obj).reduce<any>((acc, [key, value]) => { | ||
acc[key] = expandDottedObject(value); | ||
|
||
return acc; | ||
}, {} as Record<string, any>); | ||
}; | ||
|
||
// not sure these are 100% correct but they do the job here | ||
// keeping them local until others need them | ||
type OptionalPropertyOf<T extends object> = Exclude< | ||
|
@@ -144,8 +181,13 @@ function parseAndVerifyArchive(paths: string[]): ArchivePackage { | |
); | ||
} | ||
|
||
parsed.data_streams = parseAndVerifyDataStreams(paths, parsed.name, parsed.version); | ||
const parsedDataStreams = parseAndVerifyDataStreams(paths, parsed.name, parsed.version); | ||
if (parsedDataStreams.length) { | ||
parsed.data_streams = parsedDataStreams; | ||
} | ||
|
||
parsed.policy_templates = parseAndVerifyPolicyTemplates(manifest); | ||
|
||
// add readme if exists | ||
const readme = parseAndVerifyReadme(paths, parsed.name, parsed.version); | ||
if (readme) { | ||
|
@@ -202,40 +244,87 @@ export function parseAndVerifyDataStreams( | |
|
||
const { | ||
title: dataStreamTitle, | ||
release, | ||
release = DEFAULT_RELEASE_VALUE, | ||
type, | ||
dataset, | ||
ingest_pipeline: ingestPipeline, | ||
streams: manifestStreams, | ||
elasticsearch, | ||
...restOfProps | ||
} = manifest; | ||
if (!(dataStreamTitle && type)) { | ||
throw new PackageInvalidArchiveError( | ||
`Invalid manifest for data stream '${dataStreamPath}': one or more fields missing of 'title', 'type'` | ||
); | ||
} | ||
|
||
let ingestPipeline; | ||
const ingestPipelinePaths = paths.filter((path) => | ||
path.startsWith(`${pkgKey}/data_stream/${dataStreamPath}/elasticsearch/ingest_pipeline`) | ||
); | ||
|
||
if ( | ||
ingestPipelinePaths.length && | ||
(ingestPipelinePaths.some((ingestPipelinePath) => | ||
ingestPipelinePath.endsWith(DEFAULT_INGEST_PIPELINE_FILE_NAME_YML) | ||
) || | ||
ingestPipelinePaths.some((ingestPipelinePath) => | ||
ingestPipelinePath.endsWith(DEFAULT_INGEST_PIPELINE_FILE_NAME_JSON) | ||
)) | ||
) { | ||
ingestPipeline = DEFAULT_INGEST_PIPELINE_VALUE; | ||
} | ||
joshdover marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const streams = parseAndVerifyStreams(manifestStreams, dataStreamPath); | ||
|
||
const parsedElasticsearchEntry: Record<string, any> = {}; | ||
|
||
if (ingestPipeline) { | ||
parsedElasticsearchEntry['ingest_pipeline.name'] = DEFAULT_INGEST_PIPELINE_VALUE; | ||
} | ||
|
||
if (elasticsearch?.privileges) { | ||
parsedElasticsearchEntry.privileges = elasticsearch.privileges; | ||
} | ||
|
||
if (elasticsearch?.index_template?.mappings) { | ||
parsedElasticsearchEntry['index_template.mappings'] = expandDottedEntries( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These mappings/settings fields come through the manifest with |
||
elasticsearch.index_template.mappings | ||
); | ||
} | ||
|
||
if (elasticsearch?.index_template?.settings) { | ||
parsedElasticsearchEntry['index_template.settings'] = expandDottedEntries( | ||
elasticsearch.index_template.settings | ||
); | ||
} | ||
|
||
// Build up the stream object here so we can conditionally insert nullable fields. The package registry omits undefined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lot of the changes here are related to this concept. The registry service omits undefined values. We could probably get away with not doing this and more loosely comparing values, treating an explicit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to be conservative for now. When we're done with this PR, could we add notes to #115032 about potential any cleanup like this? |
||
// fields, so we're mimicking that behavior here. | ||
const dataStreamObject: RegistryDataStream = { | ||
title: dataStreamTitle, | ||
release, | ||
type, | ||
package: pkgName, | ||
dataset: dataset || `${pkgName}.${dataStreamPath}`, | ||
path: dataStreamPath, | ||
elasticsearch: parsedElasticsearchEntry, | ||
}; | ||
|
||
if (ingestPipeline) { | ||
dataStreamObject.ingest_pipeline = ingestPipeline; | ||
} | ||
|
||
if (streams.length) { | ||
dataStreamObject.streams = streams; | ||
} | ||
|
||
dataStreams.push( | ||
Object.entries(restOfProps).reduce( | ||
(validatedDataStream, [key, value]) => { | ||
if (registryDataStreamProps.includes(key as RegistryDataStreamKeys)) { | ||
// @ts-expect-error | ||
validatedDataStream[key] = value; | ||
} | ||
return validatedDataStream; | ||
}, | ||
{ | ||
title: dataStreamTitle, | ||
release, | ||
type, | ||
package: pkgName, | ||
dataset: dataset || `${pkgName}.${dataStreamPath}`, | ||
ingest_pipeline: ingestPipeline, | ||
path: dataStreamPath, | ||
streams, | ||
Object.entries(restOfProps).reduce((validatedDataStream, [key, value]) => { | ||
if (registryDataStreamProps.includes(key as RegistryDataStreamKeys)) { | ||
validatedDataStream[key] = value; | ||
} | ||
) | ||
return validatedDataStream; | ||
}, dataStreamObject) | ||
); | ||
}); | ||
|
||
|
@@ -261,25 +350,28 @@ export function parseAndVerifyStreams( | |
`Invalid manifest for data stream ${dataStreamPath}: stream is missing one or more fields of: input, title` | ||
); | ||
} | ||
|
||
const vars = parseAndVerifyVars(manifestVars, `data stream ${dataStreamPath}`); | ||
|
||
const streamObject: RegistryStream = { | ||
input, | ||
title: streamTitle, | ||
template_path: templatePath || 'stream.yml.hbs', | ||
}; | ||
|
||
if (vars.length) { | ||
streamObject.vars = vars; | ||
} | ||
|
||
// default template path name see https://github.com/elastic/package-registry/blob/master/util/dataset.go#L143 | ||
kpollich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
streams.push( | ||
Object.entries(restOfProps).reduce( | ||
(validatedStream, [key, value]) => { | ||
if (registryStreamProps.includes(key as RegistryStreamKeys)) { | ||
// @ts-expect-error | ||
validatedStream[key] = value; | ||
} | ||
return validatedStream; | ||
}, | ||
{ | ||
input, | ||
title: streamTitle, | ||
vars, | ||
template_path: templatePath || 'stream.yml.hbs', | ||
} as RegistryStream | ||
) | ||
Object.entries(restOfProps).reduce((validatedStream, [key, value]) => { | ||
if (registryStreamProps.includes(key as RegistryStreamKeys)) { | ||
// @ts-expect-error | ||
validatedStream[key] = value; | ||
} | ||
return validatedStream; | ||
}, streamObject) | ||
); | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ export default function loadTests({ loadTestFile }) { | |
loadTestFile(require.resolve('./file')); | ||
loadTestFile(require.resolve('./template')); | ||
loadTestFile(require.resolve('./ilm')); | ||
// loadTestFile(require.resolve('./install_bundled')); | ||
loadTestFile(require.resolve('./install_bundled')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was somehow commented out and missed in a previous PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to make some additional changes to get these tests passing. Since they were commented out incidentally, there were some gotchas to sort out with bundled package tests in CI. Namely, we needed a way to place fixture test files in the bundled package directory, which proved difficult in an integration test environment where we'd need to mutate the The solution I landed on with some input from the ops folks was to introduce a |
||
loadTestFile(require.resolve('./install_by_upload')); | ||
loadTestFile(require.resolve('./install_endpoint')); | ||
loadTestFile(require.resolve('./install_overrides')); | ||
|
@@ -29,5 +29,6 @@ export default function loadTests({ loadTestFile }) { | |
loadTestFile(require.resolve('./package_install_complete')); | ||
loadTestFile(require.resolve('./install_error_rollback')); | ||
loadTestFile(require.resolve('./final_pipeline')); | ||
loadTestFile(require.resolve('./validate_bundled_packages')); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we test more things than the
data_streams
property here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am taking a look at other properties to place under test here and there don't seem to be any good candidates.
policy_templates
andvars
were my first thoughts, but the current registry logic "fills out" variable values that we infer default for in Fleet, so these fields are unlikely to match for now. I am going to merge this PR for now, and these tests will likely change in #115032