-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Fleet] Fix epm endpoints return errors #171722
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
Changes from all commits
337da71
37a4f5f
1cf3702
8fb6039
ea6c281
9fe511c
d4e1f65
2af8450
595cef9
3500e85
d98c300
1ad3582
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 |
|---|---|---|
|
|
@@ -10,17 +10,18 @@ import { safeLoad, safeDump } from 'js-yaml'; | |
|
|
||
| import type { PackagePolicyConfigRecord } from '../../../../common/types'; | ||
| import { toCompiledSecretRef } from '../../secrets'; | ||
| import { PackageInvalidArchiveError } from '../../../errors'; | ||
|
|
||
| const handlebars = Handlebars.create(); | ||
|
|
||
| export function compileTemplate(variables: PackagePolicyConfigRecord, templateStr: string) { | ||
| const { vars, yamlValues } = buildTemplateVariables(variables, templateStr); | ||
| const { vars, yamlValues } = buildTemplateVariables(variables); | ||
| let compiledTemplate: string; | ||
| try { | ||
| const template = handlebars.compile(templateStr, { noEscape: true }); | ||
| compiledTemplate = template(vars); | ||
| } catch (err) { | ||
| throw new Error(`Error while compiling agent template: ${err.message}`); | ||
| throw new PackageInvalidArchiveError(`Error while compiling agent template: ${err.message}`); | ||
| } | ||
|
|
||
| compiledTemplate = replaceRootLevelYamlVariables(yamlValues, compiledTemplate); | ||
|
|
@@ -64,21 +65,25 @@ function replaceVariablesInYaml(yamlVariables: { [k: string]: any }, yaml: any) | |
| return yaml; | ||
| } | ||
|
|
||
| function buildTemplateVariables(variables: PackagePolicyConfigRecord, templateStr: string) { | ||
| function buildTemplateVariables(variables: PackagePolicyConfigRecord) { | ||
| const yamlValues: { [k: string]: any } = {}; | ||
| const vars = Object.entries(variables).reduce((acc, [key, recordEntry]) => { | ||
| // support variables with . like key.patterns | ||
| const keyParts = key.split('.'); | ||
| const lastKeyPart = keyParts.pop(); | ||
|
|
||
| if (!lastKeyPart || !isValidKey(lastKeyPart)) { | ||
| throw new Error('Invalid key'); | ||
| throw new PackageInvalidArchiveError( | ||
| `Error while compiling agent template: Invalid key ${lastKeyPart}` | ||
| ); | ||
|
Member
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. 👍 nice to use typed errors on these cases. |
||
| } | ||
|
|
||
| let varPart = acc; | ||
| for (const keyPart of keyParts) { | ||
| if (!isValidKey(keyPart)) { | ||
| throw new Error('Invalid key'); | ||
| throw new PackageInvalidArchiveError( | ||
| `Error while compiling agent template: Invalid key ${keyPart}` | ||
| ); | ||
| } | ||
| if (!varPart[keyPart]) { | ||
| varPart[keyPart] = {}; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import { | |
| } from '../../../../constants'; | ||
| import { getESAssetMetadata } from '../meta'; | ||
| import { retryTransientEsErrors } from '../retry'; | ||
| import { PackageESError, PackageInvalidArchiveError } from '../../../../errors'; | ||
|
|
||
| import { getDefaultProperties, histogram, keyword, scaledFloat } from './mappings'; | ||
|
|
||
|
|
@@ -102,7 +103,9 @@ export function getTemplate({ | |
| isIndexModeTimeSeries, | ||
| }); | ||
| if (template.template.settings.index.final_pipeline) { | ||
| throw new Error(`Error template for ${templateIndexPattern} contains a final_pipeline`); | ||
| throw new PackageInvalidArchiveError( | ||
| `Error template for ${templateIndexPattern} contains a final_pipeline` | ||
| ); | ||
|
Member
Author
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'm just not sure if these errors should be a 500. I found a few that happen when dealing with ES but I'm not totally sure what the correct response should be.
Member
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. The template here refers to the template obtained from the package? If this depends only on the content of the package this should probably be a Btw, since elastic/package-spec#587, we are more strict on what can be defined in the index template settings and we don't allow
Member
Author
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 think you're right, that is indeed the template obtained from the package. I'll change it to |
||
| } | ||
|
|
||
| const esBaseComponents = getBaseEsComponents(type, !!isIndexModeTimeSeries); | ||
|
|
@@ -427,8 +430,8 @@ function _generateMappings( | |
| matchingType = field.object_type_mapping_type ?? 'object'; | ||
| break; | ||
| default: | ||
| throw new Error( | ||
| `no dynamic mapping generated for field ${path} of type ${field.object_type}` | ||
| throw new PackageInvalidArchiveError( | ||
| `No dynamic mapping generated for field ${path} of type ${field.object_type}` | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -908,7 +911,9 @@ const rolloverDataStream = (dataStreamName: string, esClient: ElasticsearchClien | |
| alias: dataStreamName, | ||
| }); | ||
| } catch (error) { | ||
| throw new Error(`cannot rollover data stream [${dataStreamName}] due to error: ${error}`); | ||
| throw new PackageESError( | ||
| `Cannot rollover data stream [${dataStreamName}] due to error: ${error}` | ||
| ); | ||
|
Member
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 actually looks like a case where we should return a 500 error. |
||
| } | ||
| }; | ||
|
|
||
|
|
@@ -1055,7 +1060,11 @@ const updateExistingDataStream = async ({ | |
| { logger } | ||
| ); | ||
| } catch (err) { | ||
| throw new Error(`could not update lifecycle settings for ${dataStreamName}: ${err.message}`); | ||
| // Check if this error can happen because of invalid settings; | ||
| // We are returning a 500 but in that case it should be a 400 instead | ||
| throw new PackageESError( | ||
| `Could not update lifecycle settings for ${dataStreamName}: ${err.message}` | ||
|
Member
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 wonder if this can happen because the new settings are invalid, in this case we should produce a 400 error. But I am fine with producing a 500 error by now.
Member
Author
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'll add a comment to it so we can reconsider it in the future. |
||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1078,6 +1087,8 @@ const updateExistingDataStream = async ({ | |
| { logger } | ||
| ); | ||
| } catch (err) { | ||
| throw new Error(`could not update index template settings for ${dataStreamName}`); | ||
| // Same as above - Check if this error can happen because of invalid settings; | ||
| // We are returning a 500 but in that case it should be a 400 instead | ||
| throw new PackageESError(`Could not update index template settings for ${dataStreamName}`); | ||
|
Member
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. Same thing here. If the new index template settings are invalid this should produce a 400 error. But I am fine with leaving this as a 500 error if we don't have a way to discern the cases. |
||
| } | ||
| }; | ||
Uh oh!
There was an error while loading. Please reload this page.