-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Fleet] Make upload and registry package info consistent #126915
Conversation
Pinging @elastic/fleet (Team:Fleet) |
@@ -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 comment
The 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 comment
The 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 build/
directory in order to properly copy over fixture files.
The solution I landed on with some input from the ops folks was to introduce a developer.bundledPackageLocation
option that allows us to override Fleet's default bundled package location with one /tmp
to prevent mutation of the build artifact during CI. It was quite the runaround (the commit log for this PR is a sight to behold), but I think this is probably the cleanest solution we can have to support this very specific use case in tests.
} | ||
|
||
if (elasticsearch?.index_template?.mappings) { | ||
parsedElasticsearchEntry['index_template.mappings'] = expandDottedEntries( |
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.
These mappings/settings fields come through the manifest with dotted.key.names
, but the package info object expects them to be expanded to nested object structures.
); | ||
} | ||
|
||
// 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 comment
The 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 undefined
the same as an omitted value, but I opted to be as one-to-one as possible 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.
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?
@elasticmachine merge upstream |
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.
Tested locally with the apm bundled package and saw the ingest pipelines and settings to be complete and correct.
); | ||
} | ||
|
||
// 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 comment
The 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?
As we discussed offline, I'm also +1 to renaming this code to something other than |
@elasticmachine merge upstream |
@@ -75,7 +75,7 @@ export default function (providerContext: FtrProviderContext) { | |||
.type('application/gzip') | |||
.send(buf) | |||
.expect(200); | |||
expect(res.body.items.length).to.be(27); | |||
expect(res.body.items.length).to.be(29); |
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.
These responses have ingest pipelines, etc now, so they have additional elements.
@elasticmachine merge upstream |
'application/zip' | ||
); | ||
|
||
expect(archivePackageInfo.packageInfo.data_streams).toEqual( |
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
and vars
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
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.
🚀
@elasticmachine merge upstream |
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.
LGTM, manually tested with this config (and bundled packages correctly added to directory):
xpack.fleet.packages:
- package: apm
version: 8.1.0
Got these logs:
[2022-03-09T15:58:24.821+01:00][DEBUG][plugins.fleet] Setting up Fleet Elasticsearch assets
[2022-03-09T15:58:24.821+01:00][DEBUG][plugins.fleet] Creating Fleet component template and ingest pipeline
[2022-03-09T15:58:25.279+01:00][DEBUG][plugins.fleet] Setting up initial Fleet packages
[2022-03-09T15:58:25.280+01:00][DEBUG][plugins.fleet] kicking off bulk install of apm
[2022-03-09T15:58:25.287+01:00][DEBUG][plugins.fleet] found bundled package for requested install of apm-8.1.0 - installing from bundled package archive
[2022-03-09T15:58:25.361+01:00][DEBUG][plugins.fleet] setting file list to the cache for apm-8.1.0
[2022-03-09T15:58:25.361+01:00][DEBUG][plugins.fleet] setting package info to the cache for apm-8.1.0
[2022-03-09T15:58:34.785+01:00][DEBUG][plugins.fleet] retrieved installed package apm-8.1.0 from cache
And APM assets were correctly installed, including ingest pipelines 👍
const BUNDLED_PACKAGE_DIRECTORY = config?.developer?.bundledPackageLocation | ||
? config.developer.bundledPackageLocation | ||
: path.join(__dirname, '../../../../target/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.
I think it'd be ideal if we put this default in the config schema in x-pack/plugins/fleet/server/index.ts
:
bundledPackageLocation: schema.string({ defaultValue: '...' }),
// Ingest pipelines are specified in a `data_stream/<name>/elasticsearch/ingest_pipeline/` directory where a `default` | ||
// ingest pipeline should be specified by one of these filenames. |
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.
Thank you for these comments 💯
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…' of github.com:kpollich/kibana into 126695-make-upload-and-registry-package-info-consistent
💚 Build SucceededMetrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @kpollich |
⚪ Backport skippedThe pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually. Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
…move-pdf-generation-to-screenshotting * 'main' of github.com:elastic/kibana: (62 commits) [Lens] Drop partial buckets option (elastic#127153) chore(NA): remove unused translation xpack.ml.management.jobsSpacesList.objectNoun from fr-FR (elastic#127457) Add data to user details page (elastic#127019) [Fleet] Make upload and registry package info consistent (elastic#126915) [Reporting] Capture browser errors (elastic#127135) Initial readme commit with some stub articles (elastic#127420) skip flaky suite (elastic#121482) skip flaky suite (elastic#127416) Tests to ensure Kibana is handling multi-space import of saved objects correctly (elastic#127229) [Aggs] remove toAngularJson (elastic#127267) [i18n] Integrate 8.2.0 Translations (elastic#127309) [Security Solution] [Endpoint] Creates generic policy tab artifact component to be used for all of our artifacts (elastic#126685) [Kibana React] Fix Page Template `solutionNav` propagation (elastic#127140) [Cases] Export getRelatedCases API from cases client (elastic#127065) [Cloud Posture]add support for sorting benchmark page (elastic#126983) [User experience] Fix filters for the app (elastic#127295) [Fleet] Fix timeserie dimension mapping (elastic#127328) [data view mgmt] fix data view name wrap (elastic#127319) [kbn/optimizer] extract string diffing logic (elastic#127394) [ResponseOps] Add pagination and sorting to the alerts search strategy (elastic#126813) ... # Conflicts: # x-pack/plugins/screenshotting/common/errors.ts # x-pack/plugins/screenshotting/common/index.ts # x-pack/plugins/screenshotting/server/screenshots/observable.ts
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Fixes #126695
Alters our validation/processing flow for package archives to bring them in line with the processing we do in the package registry.