[Fleet] Only escape single '*' characters in YAML strings.

[skh]

Summary

Fixes a problem with the kafka integration described in #93253

(Thanks @mtojek for the analysis!)

This revisits the changes made in #91418 , which lead to the bug above.
Now the behavior is changed so that only the exact string "*" will be forced to be quoted, and everything else left as it was before.

The test cases verify the new behavior:

• asterisk: { value: '*', type: 'string' } is quoted so that it does not cause a YAML parser error (this is a fix for [Fleet] Adding integration: List of strings does not allow value ['*']  #91401 which was what [Fleet] Escape YAML string values if necessary #91418 attempted)
• a: { value: '/opt/foo/*', type: 'string' } and b: { value: '/something/else.log*', type: 'string' } stay unquoted so they can be concatenated into /opt/foo/*/something/else.log* (which is what the Kafka package does).

How to test this

In the Fleet UI, verify that you can add both the apm and the kafka packages to a policy. For APM, be sure to enable RUM.

Then verify that you find the following snippets in the policy, and that the wildcards in the policy are correct. For Kafka:

    data_stream:
          dataset: kafka.log
          type: logs
        paths:
          - /opt/kafka*/logs/controller.log*
          - /opt/kafka*/logs/server.log*
          - /opt/kafka*/logs/state-change.log*
          - /opt/kafka*/logs/kafka-*.log*

For APM:

    data_stream:
      namespace: default
    apm-server:
      host: 'localhost:8200'
      secret_token: null
      max_event_size: 307200
      capture_personal_data: true
      kibana:
        api_key: null
      rum:
        enabled: true
        source_mapping.elasticsearch.api_key: null
        allow_origins: '*'
        allow_headers: null
        library_pattern: null
        exclude_from_grouping: null
        response_headers: null
        event_rate.limit: 300
        event_rate.lru_size: 1000

[elasticmachine]

Pinging @elastic/fleet (Team:Fleet)

[skh]

@mtojek @ycombinator if you have opinions about this I'd love to hear them.

[nchaulet]

@skh it is still going to escape something like test: *id I think we should it was the initial issue

[skh]

it was the initial issue

The initial issue was '*'.

Do we have *id in a package somewhere?

I believe now *id should be treated as a valid YAML alias, and stay unchanged.

[nchaulet]

Do we have *id in a package somewhere?

This will be coming from a text variable populated by the user, so I guess we should not treat as an alias.

[skh]

This will be coming from a text variable populated by the user, so I guess we should not treat as an alias.

Do you have an issue for that, or can you describe in more detail what breaks in this case? I am only aware of #91401 which was a default variable in a package file.

Please also have a look at how the kafka package uses wildcards and concatenated strings. If we put all strings containing * in double quotes, that package will no longer work.

[skh]

@nchaulet I have tested this with this branch. If a user wants to enter a string starting with a * manually, they can enclose it in double quotes in the UI, and it will be added with double quotes to the policy.

Let me summarize. The issue is how to treat strings starting with * which we use as wildcard, but which in YAML syntax specifies that the value is not a string, but a YAML alias.

The current behavior after the changes in this PR is:

• the literal string * is being escaped when it comes in from either a package or the UI.
• strings containing a * in the middle or at the end go through unchanged and without problems. (This was needed so that the kafka package works again, see this test case: https://github.com/elastic/kibana/pull/93257/files#diff-765deefdff232a5eefcd5f7c04cd49754e383497853d3a77a5f62bfdcc4d3cd8R187 )
• strings starting with * are assumed to be YAML aliases and rejected when no corresponding anchor is present. The error message is a bit arcane, it only says unidentified alias.
• strings starting with a * can be enclosed in double quotes in the UI by the user and then go through unchanged and without causing problems.
• I did not find a way how to specify a string value in a manifest.yml file starting with a * in a way that the quotes survive our parsing and processing. This is the actual bug, which we maybe want to revisit later. It will appear in the UI as prefilled value without any quotes, and cause an error. edited: solved in [Fleet] Don't add extra quotes to YAML strings from manifest files #93585
• We currently assume that in packages, strings that start with * are valid aliases, and otherwise do not occur, not even in quotes. This can be changed, see below.

For this PR I would suggest:

• as packages currently don't seem to use YAML aliases, we can extend the special handling to strings starting with a *, but this would mean that we can't use YAML aliases and anchors later, so would only be a band aid.
• merge so that the kafka package works again

In addition I would suggest opening separate issues for:

• a discussion on the support of YAML aliases and anchors in packages
• better handling of any errors during YAML parsing, so that users have a chance to add quotes or make any other necessary changes to the values they enter in the UI, based on the parser errors and additional information we may be able to give
• dig into the code to find out if we can stop the overeager stripping of double quotes from strings in the first place

[skh]

cc @exekias @ruflin @ph

[exekias]

Thanks for the detailed explanation! I see how template variables expanding to unexpected chars could lead to issues. I'm wondering if in the given * example we should do the quoting in the template itself, instead of guessing from Kibnaa.

Would this work?

Switching from:

allow_origins: {{rum_allow_origins}}

to:

allow_origins: "{{rum_allow_origins}}"

[skh]

@exekias Thank you, this indeed works.

• Would this be an acceptable fix for packages? @mtojek @simitt @ruflin It would bring Kibana behavior back to what it was before February 16, and the apm package would need to be changed.

• This still leaves the problem what happens when a user enters a value starting with a * in the UI, but we could amend that with documentation and better error messages.

[ruflin]

++ on the simplest solution for now and "enforcing" it on the package side. I don't see any reasons at the moment we should add support for yaml aliases / references, I think it would only complicate things.

[mtojek]

Keep in mind that such change modifies the standard behavior of Kibana, which may mean that we'll have to adjust all templates for all packages. It's also not backwards compatible, so we'd have to add a constraint for specific Kibana version.

[skh]

@mtojek @ruflin thanks for your comments. Did I understand correctly that you prefer the solution @exekias suggested?

This would mean I could revert the changes made in #91418 .

[ph]

I don't see any reasons at the moment we should add support for yaml aliases / references, I think it would only complicate things.

+1 on this, this really overcomplicate things, if we want to support aliases and references I think it should be handled by the tooling, where you can control the "include" path or reference. In an integration managed by fleet we should try to be flat.

Looking at what @exekias propose I think this is indeed the simplest solution and I think this an acceptable way of handling.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c15b582
• Storybooks Preview

Metrics [docs]

✅ unchanged

History

• 💚 Build #110392 succeeded e5c9fedb7b8675fa5b6aafba18351ec07ef395a9

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

cc @skh

[skh]

Closed in favor of #93585