Skip to content
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] Fix incorrect conversion of string to numeric values in agent YAML #90371

Merged
merged 5 commits into from
Feb 5, 2021

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Feb 5, 2021

Summary

Resolves #89955. If the user enters an all-numeric value for an integration configuration field that should be text, even though Fleet does save the value correctly as a string in the saved object, it ends up as a number instead of string in the final agent yaml. This is due to the way we compile the agent yaml templates and convert it back to a JS object. Here's an example:

// User value as saved in the package policy saved object
{"some_key": {"type": "text", "value": "123"}}

// Agent yaml handlebars template defined by the package
some_key: {{some_key}}

// Compiled agent yaml using saved object values + the template
some_key: 123

// Conversion back to a JS object
{"some_key": 123}  <-- note that it is no longer a string, but a number, due to automatic typing during conversion from yaml

This PR adds another condition to our pre-compilation processing of template variables so that all-numeric values which should be treated as strings are escaped with quotes so that the compiled yaml has quotes around the value too.

@jen-huang jen-huang added bug Fixes for quality problems that affect the customer experience v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Fleet Fleet team's agent central management project Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 labels Feb 5, 2021
@jen-huang jen-huang self-assigned this Feb 5, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Feature:Fleet)

@jen-huang jen-huang changed the title [Fleet] Fix conversion of string to numeric values in agent YAML [Fleet] Fix incorrect conversion of string to numeric values in agent YAML Feb 5, 2021
@jen-huang jen-huang requested a review from a team February 5, 2021 00:18
@nchaulet
Copy link
Member

nchaulet commented Feb 5, 2021

I am wondering if we could fix it when we generate the template if have this kind of template this will not work

// User value as saved in the package policy saved object
{"variableWithANameDifferentThanTheKey": {"type": "text", "value": "123"}}

// Agent yaml handlebars template defined by the package
some_key: {{variableWithANameDifferentThanTheKey}}

Could we escape the string when we generate the template like this ? and does it solve our issue?

some_key: "123"

@jen-huang
Copy link
Contributor Author

Good catch @nchaulet, I'll modify the test case...

Could we escape the string when we generate the template like this ?

Do you mean after we compile the template? Could you suggest a way to do this?

@nchaulet
Copy link
Member

nchaulet commented Feb 5, 2021

@jen-huang I think it could work with something like this, not sure on the complexity this is adding

diff --git a/x-pack/plugins/fleet/server/services/epm/agent/agent.ts b/x-pack/plugins/fleet/server/services/epm/agent/agent.ts
index 6b1d84ea28b..c706f2a5ffe 100644
--- a/x-pack/plugins/fleet/server/services/epm/agent/agent.ts
+++ b/x-pack/plugins/fleet/server/services/epm/agent/agent.ts
@@ -84,6 +84,13 @@ function buildTemplateVariables(variables: PackagePolicyConfigRecord, templateSt
       const yamlKeyPlaceholder = `##${key}##`;
       varPart[lastKeyPart] = `"${yamlKeyPlaceholder}"`;
       yamlValues[yamlKeyPlaceholder] = recordEntry.value ? safeLoad(recordEntry.value) : null;
+    } else if (recordEntry.type && recordEntry.type === 'text') {
+      if (Array.isArray(recordEntry.value)) {
+        varPart[lastKeyPart] = recordEntry.value.map((val) => `"${recordEntry.value}"`);
+      } else {
+        // TODO escape double quote
+        varPart[lastKeyPart] = `"${recordEntry.value}"`;
+      }
     } else {
       varPart[lastKeyPart] = recordEntry.value;
     }
(END)

@jen-huang
Copy link
Contributor Author

@nchaulet Thanks for the suggestion. I'll take a closer look to make sure this works for values which are already strings (and don't need escaping).

@jen-huang
Copy link
Contributor Author

@nchaulet I've adjusted this PR to take most of your suggestion but restricting it to only numeric strings. Could you take another look?

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@jen-huang jen-huang merged commit f4dc6d0 into elastic:master Feb 5, 2021
@jen-huang jen-huang deleted the fix/number-to-string-yaml branch February 5, 2021 22:14
jen-huang added a commit that referenced this pull request Feb 6, 2021
… YAML (#90371) (#90524)

* Convert user values back to string after yaml template compilation if they were strings originally

* Add better test cases and adjust patch

* Fix when field is undefined

* Handle array of strings too
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 8, 2021
* master: (55 commits)
  [APM-UI][E2E] use githubNotify step (elastic#90514)
  [APM] Export ProcessorEvent type (elastic#90540)
  [Lens] Retain column config (elastic#90048)
  [Data Table] Add unit tests (elastic#90173)
  Migrate most plugins to synchronous lifecycle (elastic#89562)
  skip flaky suite (elastic#90555)
  skip flaky suite (elastic#64473)
  [actions] improve email action doc (elastic#90020)
  [Fleet] Support Fleet server system indices (elastic#89372)
  skip flaky suite (elastic#90552)
  Bump immer dependencies (elastic#90267)
  Unrevert "Migrations v2: don't auto-create indices + FTR/esArchiver support (elastic#85778)" (elastic#89992)
  [Search Sessions] Use sync config (elastic#90138)
  chore(NA): add safe guard to remove bazelisk from yarn global at bootstrap (elastic#90538)
  [test] Await retry.waitFor (elastic#90456)
  chore(NA): integrate build buddy with our bazel setup and remote cache for ci (elastic#90116)
  Skip failing suite (elastic#90526)
  [Fleet] Fix incorrect conversion of string to numeric values in agent YAML (elastic#90371)
  [Docs] Update reporting troubleshooting for arm rhel/centos (elastic#90385)
  chore(NA): build bazel projects all at once in the distributable build process (elastic#90328)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Fleet Fleet team's agent central management project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Incorrect handling of text fields in the integration settings.
4 participants