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

[Synthetics] quote monitor name to prevent invalid yaml #156210

Merged

Conversation

dominiqueclarke
Copy link
Contributor

@dominiqueclarke dominiqueclarke commented Apr 28, 2023

Summary

Appropriately quotes monitor names for synthetics integration policies, to ensure that customers can use monitor names that would otherwise break yaml.

Testing

  1. Create a private location
  2. Save a monitor with name [Synthetics] test to that private location
  3. Navigate to the agent policy for that location. Confirm that the integration policy was added to the agent policy, and that the name is correct

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@dominiqueclarke dominiqueclarke added bug Fixes for quality problems that affect the customer experience Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0 v8.9.0 labels May 1, 2023
@dominiqueclarke dominiqueclarke changed the title synthetics - quote monitor name to prevent invalid yaml [Synthetics] quote monitor name to prevent invalid yaml May 1, 2023
@dominiqueclarke dominiqueclarke marked this pull request as ready for review May 1, 2023 23:21
@dominiqueclarke dominiqueclarke requested a review from a team as a code owner May 1, 2023 23:21
@dominiqueclarke dominiqueclarke added the release_note:skip Skip the PR/issue when compiling release notes label May 1, 2023
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Problem still exists in other text based fields like APM Service name

image

@@ -54,7 +54,7 @@ export default function ({ getService }: FtrProviderContext) {
});
});

it('project monitors - returns a failed monitor when creating integration fails', async () => {
it.skip('project monitors - returns a failed monitor when creating integration fails', async () => {
Copy link
Contributor Author

@dominiqueclarke dominiqueclarke May 8, 2023

Choose a reason for hiding this comment

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

@shahzad31 Now that this bug is fixed, we can't use this for testing. Same below. Any suggestions on how to alternatively force an error?

@@ -19,12 +23,12 @@ export const httpFormatters: HTTPFormatMap = {
[ConfigKey.RESPONSE_BODY_INDEX]: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a boolean value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
synthetics 1.2MB 1.2MB +60.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 398 401 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 478 481 +3
total +5

History

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

@dominiqueclarke dominiqueclarke merged commit a22561a into elastic:main May 8, 2023
@dominiqueclarke dominiqueclarke deleted the fix/synthetics-quote-monitor-name branch May 8, 2023 17:13
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.8 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.8:
- [Synthetics] Private location better error handling (#152695)

Manual backport

To create the backport manually run:

node scripts/backport --pr 156210

Questions ?

Please refer to the Backport tool documentation

@dominiqueclarke
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.8

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

Questions ?

Please refer to the Backport tool documentation

dominiqueclarke added a commit to dominiqueclarke/kibana that referenced this pull request May 8, 2023
## Summary

Appropriately quotes monitor names for synthetics integration policies,
to ensure that customers can use monitor names that would otherwise
break yaml.

### Testing
1. Create a private location
2. Save a monitor with name `[Synthetics] test` to that private location
3. Navigate to the agent policy for that location. Confirm that the
integration policy was added to the agent policy, and that the name is
correct

---------

Co-authored-by: shahzad31 <shahzad31comp@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit a22561a)
dominiqueclarke added a commit that referenced this pull request May 8, 2023
… (#157087)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Synthetics] quote monitor name to prevent invalid yaml
(#156210)](#156210)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Dominique
Clarke","email":"dominique.clarke@elastic.co"},"sourceCommit":{"committedDate":"2023-05-08T17:13:19Z","message":"[Synthetics]
quote monitor name to prevent invalid yaml (#156210)\n\n##
Summary\r\n\r\nAppropriately quotes monitor names for synthetics
integration policies,\r\nto ensure that customers can use monitor names
that would otherwise\r\nbreak yaml.\r\n\r\n### Testing\r\n1. Create a
private location\r\n2. Save a monitor with name `[Synthetics] test` to
that private location\r\n3. Navigate to the agent policy for that
location. Confirm that the\r\nintegration policy was added to the agent
policy, and that the name
is\r\ncorrect\r\n\r\n---------\r\n\r\nCo-authored-by: shahzad31
<shahzad31comp@gmail.com>\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"a22561a524c73bab055e2a7eb7411cc36158c856","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Team:uptime","release_note:skip","v8.8.0","v8.9.0"],"number":156210,"url":"https://github.com/elastic/kibana/pull/156210","mergeCommit":{"message":"[Synthetics]
quote monitor name to prevent invalid yaml (#156210)\n\n##
Summary\r\n\r\nAppropriately quotes monitor names for synthetics
integration policies,\r\nto ensure that customers can use monitor names
that would otherwise\r\nbreak yaml.\r\n\r\n### Testing\r\n1. Create a
private location\r\n2. Save a monitor with name `[Synthetics] test` to
that private location\r\n3. Navigate to the agent policy for that
location. Confirm that the\r\nintegration policy was added to the agent
policy, and that the name
is\r\ncorrect\r\n\r\n---------\r\n\r\nCo-authored-by: shahzad31
<shahzad31comp@gmail.com>\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"a22561a524c73bab055e2a7eb7411cc36158c856"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156210","number":156210,"mergeCommit":{"message":"[Synthetics]
quote monitor name to prevent invalid yaml (#156210)\n\n##
Summary\r\n\r\nAppropriately quotes monitor names for synthetics
integration policies,\r\nto ensure that customers can use monitor names
that would otherwise\r\nbreak yaml.\r\n\r\n### Testing\r\n1. Create a
private location\r\n2. Save a monitor with name `[Synthetics] test` to
that private location\r\n3. Navigate to the agent policy for that
location. Confirm that the\r\nintegration policy was added to the agent
policy, and that the name
is\r\ncorrect\r\n\r\n---------\r\n\r\nCo-authored-by: shahzad31
<shahzad31comp@gmail.com>\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"a22561a524c73bab055e2a7eb7411cc36158c856"}}]}]
BACKPORT-->
shahzad31 added a commit that referenced this pull request May 9, 2023
@shahzad31 shahzad31 restored the fix/synthetics-quote-monitor-name branch May 12, 2023 07:10
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 release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants