[Fleet] fixed undefined error when missing policy vars in template #124215

juliaElastic · 2022-02-01T12:16:45Z

Summary

@mdelapenya reported an issue in e2e-tests that seems to be surfaced by improving package policy validation

When trying to create a package policy with this request body, there is an undefined error coming:

POST http://elastic:changeme@localhost:5620/api/fleet/package_policies
kbn-xsrf: kibana

{
   "description": "Linux Metrics",
   "enabled": true,
   "inputs": [
      {
         "enabled": true,
         "streams": [
            {
               "data_stream": {
                  "dataset": "linux.memory",
                  "type": "metrics"
               },
               "enabled": true,
               "id": "linux/metrics-linux.memory-b0df6872-ff9a-443f-babc-5d7785e35b98"
            }
         ],
         "type": "linux/metrics",
         "vars": {
            "period": {
               "type": "string",
               "value": "1s"
            }
         }
      }
   ],
   "name": "linux-3d13ada6-a9ae-46df-8e57-ff5050f4b671",
   "namespace": "default",
   "output_id": "",
   "package": {
      "name": "linux",
      "title": "Linux Metrics",
      "version": "0.6.2"
   },
   "policy_id": "b25cb6e0-8347-11ec-96f9-6590c25bacf9"
}

Response:

{
   "statusCode": 500,
   "error": "Internal Server Error",
   "message": "Cannot read properties of undefined (reading 'period')"
 }

Stack trace:

 │ proc [kibana] [2022-02-01T13:02:19.415+01:00][ERROR][plugins.fleet] TypeError: Cannot read properties of undefined (reading 'period')
 │ proc [kibana]     at reduce (/Users/juliabardi/kibana/kibana/x-pack/plugins/fleet/common/services/validate_package_policy.ts:146:15)
 │ proc [kibana]     at Array.reduce (<anonymous>)
 │ proc [kibana]     at forEach (/Users/juliabardi/kibana/kibana/x-pack/plugins/fleet/common/services/validate_package_policy.ts:142:47)
 │ proc [kibana]     at Array.forEach (<anonymous>)
 │ proc [kibana]     at validatePackagePolicy (/Users/juliabardi/kibana/kibana/x-pack/plugins/fleet/common/services/validate_package_policy.ts:129:24)
 │ proc [kibana]     at validatePackagePolicyOrThrow (/Users/juliabardi/kibana/kibana/x-pack/plugins/fleet/server/services/package_policy.ts:702:63)
 │ proc [kibana]     at PackagePolicyService.create (/Users/juliabardi/kibana/kibana/x-pack/plugins/fleet/server/services/package_policy.ts:119:7)

There seems to be a nullcheck missing. After adding it, a more meaningful validation error comes:

 {
   "statusCode": 400,
   "error": "Bad Request",
   "message": "Package policy is invalid: inputs.linux/metrics.streams.linux.memory.vars.period: Period is required"
 }

elasticmachine · 2022-02-01T12:16:49Z

Pinging @elastic/fleet (Team:Fleet)

nchaulet · 2022-02-01T12:36:56Z

x-pack/plugins/fleet/common/services/validate_package_policy.ts

@@ -143,7 +143,7 @@ export const validatePackagePolicy = (
 results[name] = input.enabled
 ? validatePackagePolicyConfig(
 configEntry,
- inputVarDefsByPolicyTemplateAndType[inputKey][name],
+ (inputVarDefsByPolicyTemplateAndType[inputKey] ?? {})[name],


Should we have a second level of inputVarDefs just by input type if there is no policy_template something like this? otherwise we loose validation and I do not think we should allow to create inputs that are not valid.

inputVarDefsByPolicyTemplateAndType[inputKey] ? inputVarDefsByPolicyTemplateAndType[inputKey][name] : (inputVarDefsByType[inputType] ?? {})[name] // here I am wondering if we should not throw if there is no input matching the input type

I'm not sure what do you mean to put in inputVarDefsByType, in this scenario, there are empty vars in policy_templates.inputs.linux/metrics.vars. The validation is already there for data_streams.streams.input.vars.

See the package details:

{ "item": { "policy_templates": [ { "name": "system", "title": "Linux kernel metrics", "description": "Collect system metrics from Linux operating systems", "inputs": [ { "title": "Collect low-level system metrics from Linux instances", "vars": [], "type": "linux/metrics", "description": "Collecting Linux conntrack, ksm, pageinfo metrics." } ], "multiple": true } ], "data_streams": [ { "dataset": "linux.memory", "package": "linux", "path": "memory", "streams": [ { "input": "linux/metrics", "title": "Linux memory metrics", "vars": [ { "name": "period", "type": "text", "title": "Period", "multi": false, "required": true, "show_user": true, "default": "10s" } ], "template_path": "stream.yml.hbs", "description": "Linux paging and memory management metrics" } ], "title": "Linux-only memory metrics", "release": "experimental", "type": "metrics" } ] } }

Here the validatePackagePolicyConfig function take as the first argument the user provided value and as the second one the package provided spec to validate against if we do not provide a second argument we are not validating the input.
What I suggest is in case we are not able to map the user provided input to a package var def to try to map only using the input type.

Does it make clearer?

I see what you mean, though in this scenario, inputVarDefsByType would build the same object as inputVarDefsByPolicyTemplateAndType (none of the policy_templates have that var provided by the user, the vars are really just misplaced in the request):
hasIntegration is false here, so the object keys will beinput.typeonly:
https:/elastic/kibana/blob/main/x-pack/plugins/fleet/common/services/validate_package_policy.ts#L104

the question is, what should happen if there are extra vars provided that are not there in the template, should there be a validation error coming?

the question is, what should happen if there are extra vars provided that are not there in the template, should there be a validation error coming?

I personally think we should reject the variable, but it's kind of a breaking change, but otherwise this will result in non validated package and I am not sure these variables that are not are the correct level are not used in the agent stream template after.

I agree with your assessment. I'll update the pr to give a validation error instead of silencing the error in case of extra/unexpected vars. I think this change is not breaking in a sense that it only improves the error message (the request would fail with undefined error anyway).
Though the original change might cause some unexpected impact where this API was called with invalid package request, like in e2e-testing.

jen-huang · 2022-02-01T19:09:30Z

@juliaElastic Might this be the same issue as #110202 (comment)?

juliaElastic · 2022-02-02T07:41:45Z

Might this be the same issue as #110202 (comment)?

@jen-huang No, I think this is a different one recently happening due to the improved validation.

juliaElastic · 2022-02-02T08:57:59Z

x-pack/plugins/fleet/common/services/validate_package_policy.ts

+ ? inputVarDefsByPolicyTemplateAndType[inputKey] === undefined
+ ? [
+ i18n.translate('xpack.fleet.packagePolicyValidation.missingInputKeyMessage', {
+ defaultMessage: '{inputKey} has no vars in policy template',


I added validation error here, though I don't like it too much, it would be better to include this logic in validatePackagePolicyConfig.
I'm thinking the best place to add the validation error is when varDef is not defined here, meaning all unrecognized variables would be rejected with validation error. What do you think?

kibana/x-pack/plugins/fleet/common/services/validate_package_policy.ts

Lines 212 to 217 in fbe6836

if (varDef === undefined) {

// eslint-disable-next-line no-console

console.debug(`No variable definition for ${varName} found`);

return null;

}

Yes it would be better 👍

kibana-ci · 2022-02-02T16:14:24Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`fleet`	109.2KB	109.3KB	+149.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`fleet`	41	40	-1

Total ESLint disabled count

id	before	after	diff
`fleet`	49	48	-1

History

💔 Build #21319 failed 97523e6
💔 Build #21309 failed 345bad1
💚 Build #21277 succeeded 77a0559
💔 Build #21256 failed 895027b
💛 Build #21021 was flaky 35e2d19

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @juliaElastic

nchaulet

Thanks for fixing that 🚀

…lastic#124215) * fixed undefined error when missing policy vars in template * added validation error if input has no vars * fixed checks * added unit test * fixed checks * fixed checks * fixed checks * moved validation error to validatePackagePolicyConfig (cherry picked from commit 67430f9)

kibanamachine · 2022-02-02T17:07:08Z

💔 Some backports could not be created

Status	Branch	Result
✅	8.0
❌	7.17	Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 124215

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…124215) (#124379) * fixed undefined error when missing policy vars in template * added validation error if input has no vars * fixed checks * added unit test * fixed checks * fixed checks * fixed checks * moved validation error to validatePackagePolicyConfig (cherry picked from commit 67430f9) Co-authored-by: Julia Bardi <[email protected]>

…lastic#124215) * fixed undefined error when missing policy vars in template * added validation error if input has no vars * fixed checks * added unit test * fixed checks * fixed checks * fixed checks * moved validation error to validatePackagePolicyConfig

…124658) * [Fleet] fixed undefined error when missing policy vars in template (#124215) * fixed undefined error when missing policy vars in template * added validation error if input has no vars * fixed checks * added unit test * fixed checks * fixed checks * fixed checks * moved validation error to validatePackagePolicyConfig * fix cypress

dikshachauhan-qasource · 2022-02-07T11:49:44Z

Hi @juliaElastic

Could you please let us know details how to validate this.

Thanks
QAS

juliaElastic · 2022-02-07T12:19:50Z

@dikshachauhan-qasource if you check the issue description, you can verify by running the http query and verifying that the undefined error message doesn't come, instead validation errors come.

dikshachauhan-qasource · 2022-02-07T13:34:13Z

Hi @juliaElastic

Using the API provided in description, I attempted to validate it on self 8.0 bc1 managed environment and observed that undefined error message is showing up.

API Used:

curl --request POST \
  --url http://localhost:5601/api/fleet/package_policies \
  --header 'Authorization: Basic changeme=' \
  --header 'kbn-xsrf: kibana \

@body:
{
   "description": "Linux Metrics",
   "enabled": true,
   "inputs": [
      {
         "enabled": true,
         "streams": [
            {
               "data_stream": {
                  "dataset": "linux.memory",
                  "type": "metrics"
               },
               "enabled": true,
               "id": "linux/metrics-linux.memory-b0df6872-ff9a-443f-babc-5d7785e35b98"
            }
         ],
         "type": "linux/metrics",
         "vars": {
            "period": {
               "type": "string",
               "value": "1s"
            }
         }
      }
   ],
   "name": "linux",
   "namespace": "default",
   "output_id": "",
   "package": {
      "name": "linux",
      "title": "Linux Metrics",
      "version": "0.6.2"
   },
   "policy_id": "2016d7cc-135e-5583-9758-3ba01f5a06e5"
}

Screenshot:

Please let us know if we are missing any other specific input.

Thanks
QAS

juliaElastic · 2022-02-07T13:50:32Z

@dikshachauhan-qasource I think the fix is not in 8.0-BC1, it seems to include changes up to Jan 31 (7 days ago), while this fix was merged 5 days ago.
So you should either check this in 8.0-SNAPSHOT or wait for BC2.

dikshachauhan-qasource · 2022-02-07T14:05:51Z

Ok Thank you for the confirmation. @juliaElastic I'll try later again.

…lastic#124215) * fixed undefined error when missing policy vars in template * added validation error if input has no vars * fixed checks * added unit test * fixed checks * fixed checks * fixed checks * moved validation error to validatePackagePolicyConfig (cherry picked from commit 67430f9) # Conflicts: # x-pack/plugins/fleet/common/services/validate_package_policy.ts

…late (#124215) (#136324) * [Fleet] fixed undefined error when missing policy vars in template (#124215) * fixed undefined error when missing policy vars in template * added validation error if input has no vars * fixed checks * added unit test * fixed checks * fixed checks * fixed checks * moved validation error to validatePackagePolicyConfig (cherry picked from commit 67430f9) # Conflicts: # x-pack/plugins/fleet/common/services/validate_package_policy.ts * Fix duplicate test title Co-authored-by: Julia Bardi <[email protected]>

fixed undefined error when missing policy vars in template

35e2d19

juliaElastic added release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Feb 1, 2022

juliaElastic requested a review from a team as a code owner February 1, 2022 12:16

juliaElastic self-assigned this Feb 1, 2022

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 1, 2022

juliaElastic requested a review from nchaulet February 1, 2022 12:17

nchaulet reviewed Feb 1, 2022

View reviewed changes

jen-huang added v7.17.0 v8.0.0 v7.17.1 and removed v7.17.0 labels Feb 1, 2022

added validation error if input has no vars

895027b

juliaElastic commented Feb 2, 2022

View reviewed changes

juliaElastic added 6 commits February 2, 2022 10:56

fixed checks

77a0559

added unit test

345bad1

fixed checks

96e48e5

fixed checks

97523e6

fixed checks

5713420

moved validation error to validatePackagePolicyConfig

2e9556e

juliaElastic added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 2, 2022

nchaulet approved these changes Feb 2, 2022

View reviewed changes

juliaElastic merged commit 67430f9 into elastic:main Feb 2, 2022

juliaElastic deleted the fix-package-policy-validate-error branch February 2, 2022 17:03

kibanamachine mentioned this pull request Feb 2, 2022

[8.0] [Fleet] fixed undefined error when missing policy vars in template (#124215) #124379

Merged

This was referenced Feb 3, 2022

[7.17] fixed undefined error when missing policy vars in template #124480

Closed

[7.17] fixed undefined error when missing policy vars in template #124482

Merged

juliaElastic mentioned this pull request Feb 4, 2022

[8.1] fixed undefined error when missing policy vars in template (#… #124658

Merged

joshdover mentioned this pull request Feb 15, 2022

/package_policies: package vars should be on input level #125625

Closed

This was referenced Feb 15, 2022

[Fleet] Improve package policy validation #125655

Closed

[Fleet] revert package policy validation that caused issue with input groups #125657

Merged

kibanamachine mentioned this pull request Jul 13, 2022

[Fleet] Fix missing package-level vars in GCP policy editor #132068

Merged

kpollich mentioned this pull request Jul 13, 2022

[7.17] [Fleet] fixed undefined error when missing policy vars in template (#124215) #136324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] fixed undefined error when missing policy vars in template #124215

[Fleet] fixed undefined error when missing policy vars in template #124215

juliaElastic commented Feb 1, 2022 •

edited

Loading

elasticmachine commented Feb 1, 2022

nchaulet Feb 1, 2022

juliaElastic Feb 1, 2022

nchaulet Feb 1, 2022

juliaElastic Feb 1, 2022

nchaulet Feb 1, 2022 •

edited

Loading

juliaElastic Feb 1, 2022

jen-huang commented Feb 1, 2022

juliaElastic commented Feb 2, 2022

juliaElastic Feb 2, 2022 •

edited

Loading

nchaulet Feb 2, 2022

juliaElastic Feb 2, 2022

kibana-ci commented Feb 2, 2022

ESLint disabled line counts

Total ESLint disabled count

nchaulet left a comment

kibanamachine commented Feb 2, 2022

dikshachauhan-qasource commented Feb 7, 2022

juliaElastic commented Feb 7, 2022

dikshachauhan-qasource commented Feb 7, 2022

juliaElastic commented Feb 7, 2022

dikshachauhan-qasource commented Feb 7, 2022

	if (varDef === undefined) {
	// eslint-disable-next-line no-console
	console.debug(`No variable definition for ${varName} found`);

	return null;
	}

[Fleet] fixed undefined error when missing policy vars in template #124215

[Fleet] fixed undefined error when missing policy vars in template #124215

Conversation

juliaElastic commented Feb 1, 2022 • edited Loading

Summary

elasticmachine commented Feb 1, 2022

nchaulet Feb 1, 2022

Choose a reason for hiding this comment

juliaElastic Feb 1, 2022

Choose a reason for hiding this comment

nchaulet Feb 1, 2022

Choose a reason for hiding this comment

juliaElastic Feb 1, 2022

Choose a reason for hiding this comment

nchaulet Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

juliaElastic Feb 1, 2022

Choose a reason for hiding this comment

jen-huang commented Feb 1, 2022

juliaElastic commented Feb 2, 2022

juliaElastic Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

nchaulet Feb 2, 2022

Choose a reason for hiding this comment

juliaElastic Feb 2, 2022

Choose a reason for hiding this comment

kibana-ci commented Feb 2, 2022

💚 Build Succeeded

Metrics [docs]

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

nchaulet left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 2, 2022

💔 Some backports could not be created

How to fix

Questions ?

dikshachauhan-qasource commented Feb 7, 2022

juliaElastic commented Feb 7, 2022

dikshachauhan-qasource commented Feb 7, 2022

juliaElastic commented Feb 7, 2022

dikshachauhan-qasource commented Feb 7, 2022

juliaElastic commented Feb 1, 2022 •

edited

Loading

nchaulet Feb 1, 2022 •

edited

Loading

juliaElastic Feb 2, 2022 •

edited

Loading