-
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] Don't add extra quotes to YAML strings from manifest files #93585
Conversation
Pinging @elastic/fleet (Team:Fleet) |
@simitt If we agree on this solution, the |
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! It will help the Kafka package behave correctly in prod.
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 from the description! (did not run a local test)
fix in APM Server: elastic/apm-server#4917 |
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.
🚀
We should really push to have some package policy validation server side at some point
@EricDavisX When this is tested manually by the QA team, the steps starting with "Craft a package" and "Fix the package" should probably be skipped because they are too complex to set up. |
a93c0a7
to
e7c41f4
Compare
I have a use case where the substitution is part of a multi-line block
And the default definition for search is:
After substitution we want the result to be:
|
9b75990
to
1c410ea
Compare
@elasticmachine merge upstream |
@nchaulet for this do you mean at the package-spec level? |
@ph I mean at the API level, currently all the package validation is happening client side, so an user that is using the API could bypass the validation |
@elasticmachine merge upstream |
…h/kibana into 93253-handle-wildcards-strings-better
…lastic#93585) * Add UI validation for string YAML values in policies. * Do not quote YAML strings containing special characters. * Add test case for wildcards in the middle of strings. * Add multiline test case. * Polish test case. * Update API docs Co-authored-by: Kibana Machine <[email protected]>
…lastic#93585) * Add UI validation for string YAML values in policies. * Do not quote YAML strings containing special characters. * Add test case for wildcards in the middle of strings. * Add multiline test case. * Polish test case. * Update API docs Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # api_docs/fleet.json
…93585) (#93778) * Add UI validation for string YAML values in policies. * Do not quote YAML strings containing special characters. * Add test case for wildcards in the middle of strings. * Add multiline test case. * Polish test case. * Update API docs Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # api_docs/fleet.json
…93585) (#93773) * Add UI validation for string YAML values in policies. * Do not quote YAML strings containing special characters. * Add test case for wildcards in the middle of strings. * Add multiline test case. * Polish test case. * Update API docs Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
@nchaulet @ph When that happens, the endpoint will fail with an error from the YAML parser before a policy is created. So, in a way, we already have server-side validation, even if with a very unhelpful error message. I've created #93926 to improve this. |
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/uptime/rest/monitor_states_generated·ts.apis uptime uptime REST endpoints with generated data monitor state scoping test status filter should not return a monitor with mix state if check status filter is upStandard Out
Stack Trace
Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/uptime/rest/monitor_states_generated·ts.apis uptime uptime REST endpoints with generated data monitor state scoping test status filter should not return a monitor with mix state if check status filter is upStandard Out
Stack Trace
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @skh |
@@ -231,7 +231,7 @@ export interface RegistryElasticsearch { | |||
'index_template.mappings'?: object; | |||
} | |||
|
|||
export type RegistryVarType = 'integer' | 'bool' | 'password' | 'text' | 'yaml'; | |||
export type RegistryVarType = 'integer' | 'bool' | 'password' | 'text' | 'yaml' | 'string'; |
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.
sorry I am late to the official review, but I am surprised at this change. AFAIK there is no var type called string
on package spec side, only text
? https://github.com/elastic/package-spec/blob/master/versions/1/data_stream/manifest.spec.yml#L50-L55
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.
Good question. Right now packages use both, e.g. apm
here has a lot of string
s: https://github.com/elastic/package-storage/blob/snapshot/packages/apm/0.1.0-dev.7/manifest.yml#L44-L136
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.
Hmm, that seems inconsistent with the package spec, I also see type: int
which is also inconsistent. I'll file an issue elsewhere.
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.
Opened elastic/package-storage#1011
Hi @EricDavisX We have validated above fixes in 7.12 BC3 build and found issue as not fixed. We are still not able to add kafka integration to policy using quotes as well to value under 'Kafka home directory'. Please refer below sceenshot: Kafka package version: 3.6 We will revalidate above fixes in 7.12 BC4 and provide our observation if fixes will be available there. Thanks |
It would be available in recent Agent snapshots, but as you note - will not be in BC until BC4. Thanks. |
Hi @EricDavisX We have validated above fixes in 7.12 BC4 build and found issue as fixed.
Build details are as follows:
Thanks |
Summary
Fixes #93253
This reverts the changes made in #91418 , and adds extra validation to the Policy UI in Fleet to guard against invalid YAML string values.
With this change, string default values in YAML files in packages need to be enclosed in quotes like this when they start with the wildcard
*
:'"*"'
,'"*foo"'
or"\"*\""
,"\"*foo\""
When a user tries to enter such a string when creating a policy, it is caught by the form validation.
Background
Currently, the package registry reads in package metadata from YAML files in packages, and presents it to the Kibana Fleet plugin in JSON format. For an example, see the
policy_templates
section in https://epr.elastic.co/package/apache/0.3.4/ .If the package contains default values for string properties, the information if these strings were quoted or not gets lost during the translation to JSON, so
and
would both result in the JSON output
in the registry output. When this JSON object is turned into YAML again later, the string in question is left unquoted, so it is now
in both cases. When that YAML is parsed again, it leads to a parser error because the string starts with a
*
, which for YAML means this should be parsed as an alias.#91418 tried to amend this by cleverly adding quotes later in the process for strings which were deemed problematic. This led to other bugs (see #93253 ) and was clearly not a good solution.
This change now uses a different approach, and gives the responsibility to provide correctly quoted strings to the package author, and the user. Package authors now must take care to use escaped quotation marks in case they need to be preserved in the final YAML policy. Users are helped by additional form validation which points out when they need to enclose their strings in
""
.How to test this
Add any integration to any policy. Try to enter a string like
*
or*foo
in a text field, and verify that you can't save the integration. Play around with it and try to break it.Add quotes as requested by the form validation, save the integration, and verify that the operation is successful and the generated policy looks correct.
Craft a package with a string default value like
'*'
or"*foo"
and try to add it to a policy. Verify that you see the same error in the form validation as above. (The current APM package in the snapshot registry has this problem.)Fix the package so that the string default value now contains escaped quotes, like
'"*"'
or"\"*foo\""
. Verify that you can add the integration to a policy correctly.Verify that you can add the
kafka
integration to a policy, and that the policy contains these lines:Checklist
Delete any items that are not applicable to this PR.