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] omit ssl fields when ssl is disabled #149087

Merged

Conversation

dominiqueclarke
Copy link
Contributor

@dominiqueclarke dominiqueclarke commented Jan 18, 2023

Summary

Resolves #149083

Omits SSL fields when TLS is disabled.

Testing

Testing should be done both in the Uptime and Synthetics app

  1. Create a TCP monitor in the Uptime and synthetics app with the following host 8.8.8.8:80
  2. Navigate to the monitor details page for each monitor, either in the Synthetics or Uptime app.
  3. Confirm that the url for the monitor is tcp://8.8.8.8:80, not ssl:/8.8.8.8:80.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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.3MB 1.3MB -174.0B

History

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

@dominiqueclarke dominiqueclarke added v8.7.0 8.6.1 release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability bug Fixes for quality problems that affect the customer experience labels Jan 18, 2023
@dominiqueclarke dominiqueclarke marked this pull request as ready for review January 18, 2023 15:58
@dominiqueclarke dominiqueclarke requested a review from a team as a code owner January 18, 2023 15:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke dominiqueclarke changed the title synthetics - omit ssl fields when ssl is disabled [Synthetics] omit ssl fields when ssl is disabled Jan 18, 2023
Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

LGTM!

Tested on main and works as expected.

@dominiqueclarke dominiqueclarke merged commit 55bb5c3 into elastic:main Jan 19, 2023
@dominiqueclarke dominiqueclarke deleted the fix/synthetics-uptime-tls-disable branch January 19, 2023 14:13
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 19, 2023
@dominiqueclarke dominiqueclarke added v8.6.1 auto-backport Deprecated: Automatically backport this PR after it's merged v8.6.0 and removed 8.6.1 backport:skip This commit does not require backporting labels Jan 19, 2023
kibanamachine pushed a commit that referenced this pull request Jan 19, 2023
## Summary
Resolves #149083

Omits SSL fields when TLS is disabled.

### Testing
Testing should be done both in the Uptime and Synthetics app
1. Create a TCP monitor in the Uptime and synthetics app with the
following host `8.8.8.8:80`
2. Navigate to the monitor details page for each monitor, either in the
Synthetics or Uptime app.
3. Confirm that the url for the monitor is `tcp://8.8.8.8:80`, not
`ssl:/8.8.8.8:80`.

(cherry picked from commit 55bb5c3)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.6

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

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

wayneseymour pushed a commit to wayneseymour/kibana that referenced this pull request Jan 19, 2023
## Summary
Resolves elastic#149083

Omits SSL fields when TLS is disabled.

### Testing
Testing should be done both in the Uptime and Synthetics app
1. Create a TCP monitor in the Uptime and synthetics app with the
following host `8.8.8.8:80`
2. Navigate to the monitor details page for each monitor, either in the
Synthetics or Uptime app.
3. Confirm that the url for the monitor is `tcp://8.8.8.8:80`, not
`ssl:/8.8.8.8:80`.
kibanamachine added a commit that referenced this pull request Jan 19, 2023
…49228)

# Backport

This will backport the following commits from `main` to `8.6`:
- [[Synthetics] omit ssl fields when ssl is disabled
(#149087)](#149087)

<!--- 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-01-19T14:13:55Z","message":"[Synthetics]
omit ssl fields when ssl is disabled (#149087)\n\n## Summary\r\nResolves
#149083 SSL fields
when TLS is disabled.\r\n\r\n### Testing\r\nTesting should be done both
in the Uptime and Synthetics app\r\n1. Create a TCP monitor in the
Uptime and synthetics app with the\r\nfollowing host `8.8.8.8:80`\r\n2.
Navigate to the monitor details page for each monitor, either in
the\r\nSynthetics or Uptime app.\r\n3. Confirm that the url for the
monitor is `tcp://8.8.8.8:80`,
not\r\n`ssl:/8.8.8.8:80`.","sha":"55bb5c307015662fd309c5cc86ebbf59fb6f8e77","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:uptime","auto-backport","v8.7.0","v8.6.1"],"number":149087,"url":"#149087
omit ssl fields when ssl is disabled (#149087)\n\n## Summary\r\nResolves
#149083 SSL fields
when TLS is disabled.\r\n\r\n### Testing\r\nTesting should be done both
in the Uptime and Synthetics app\r\n1. Create a TCP monitor in the
Uptime and synthetics app with the\r\nfollowing host `8.8.8.8:80`\r\n2.
Navigate to the monitor details page for each monitor, either in
the\r\nSynthetics or Uptime app.\r\n3. Confirm that the url for the
monitor is `tcp://8.8.8.8:80`,
not\r\n`ssl:/8.8.8.8:80`.","sha":"55bb5c307015662fd309c5cc86ebbf59fb6f8e77"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"#149087
omit ssl fields when ssl is disabled (#149087)\n\n## Summary\r\nResolves
#149083 SSL fields
when TLS is disabled.\r\n\r\n### Testing\r\nTesting should be done both
in the Uptime and Synthetics app\r\n1. Create a TCP monitor in the
Uptime and synthetics app with the\r\nfollowing host `8.8.8.8:80`\r\n2.
Navigate to the monitor details page for each monitor, either in
the\r\nSynthetics or Uptime app.\r\n3. Confirm that the url for the
monitor is `tcp://8.8.8.8:80`,
not\r\n`ssl:/8.8.8.8:80`.","sha":"55bb5c307015662fd309c5cc86ebbf59fb6f8e77"}},{"branch":"8.6","label":"v8.6.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Dominique Clarke <dominique.clarke@elastic.co>
@emilioalvap emilioalvap self-assigned this Feb 10, 2023
@emilioalvap
Copy link
Contributor

Failing on Uptime 8.7.0-SNAPSHOT. How to repro:

  • Create a new TCP monitor on a public location with TLS disabled and 8.8.8.8:80.
  • Go to Monitor overview, check that url is tcp://8.8.8.8:80.
  • Go to Monitor Management -> Edit monitor. Notice TLS is enabled automatically.
  • If saved, URL will change to ssl://8.8.8.8:80.

@emilioalvap emilioalvap removed their assignment Feb 10, 2023
@dominiqueclarke dominiqueclarke self-assigned this Feb 10, 2023
@dominiqueclarke
Copy link
Contributor Author

Raised #150949

@dominiqueclarke
Copy link
Contributor Author

#152239 and #150949 have been merged into 8.7. @emilioalvap You can retest if you'd like

@justinkambic
Copy link
Contributor

Post-FF Testing LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged bug Fixes for quality problems that affect the customer experience release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.6.1 v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Synthetics] [Uptime] Omit ssl fields when tls is disabled
7 participants