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.

Sign up for GitHub

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

Jump to bottom

[Synthetics] preserve id field on monitor attributes #142478

Merged
merged 13 commits into from
Oct 4, 2022
Merged

[Synthetics] preserve id field on monitor attributes #142478

merged 13 commits into from
Oct 4, 2022

Conversation

dominiqueclarke
Copy link
Contributor

@dominiqueclarke dominiqueclarke commented Oct 3, 2022 •
edited by kibanamachine
Loading

Summary

Preserves monitor attribute id on monitor saved objects. Also preserves attribute urls on tcp monitors.

Background

In #140553, we hardened our validation to ensure that unknown keys cannot be added to monitor saved objects. This will be release in 8.5.0

Prior to 8.5.0, however, the enable/disable toggle in Uptime Monitor management already introduced an unknown key: id. This id key was added to the attributes of every monitor that was enabled or disabled via the enable/disable toggle.

Because our monitor saved object contains encrypted secrets, values on the monitor saved object are added to the encryption AAD (additionally authenticated data), but default. If values are omitted from the AAD, the secrets will not be able to be decrypted.

Because of this limitation, we must continue to keep the id field on all monitors until the time at which we are able to remove the id field through a saved object migration.

Note on the urls field for TCP:
This PR also ensures that the urls field for tcp is preserved. The urls field was defined on all tcp monitors prior to 8.4.2 through our now removed hydration feature, which updated monitor saved objects with extra values based on the heartbeat documents. Preserving urls on TCP fields is necessary for the same reason it is necessary to preserve the id field: to prevent decryption errors resulting from missing fields that are part of AAD.

Testing

  1. Before checking out this pr, checkout 8.4.0 git checkout v8.4.0
  2. Create a cluster via oblt-cli
  3. Connect your local Kibana to the oblt-cli cluster by pasting in the provided kibana.dev.yml file.
  4. Update xpack.uptime.index: remote_cluster:heartbeat-* to xpack.uptime.index: synthetics-*.
  5. Navigate to Uptime. Create an http, icmp, tcp, and browser monitor. (Ideally, create the browser monitor with a schedule of 2 minutes so you don't have to wait too long for multiple results)
  6. Wait for hydration to occur by waiting for the urls field for the browser monitor to populate.
  7. Disable all the monitors
  8. Re-enable all the monitors
  9. Check out this branch and rebuild
  10. Edit each monitor
  11. Ensure there are no decryption errors in your service logs. Also ensure monitors are able to be decrypted by trying to edit each monitor once again. If the monitor loads when you try to edit it, no decryption errors are present.

@dominiqueclarke dominiqueclarke added v8.5.0 bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v8.6.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Oct 3, 2022
@dominiqueclarke dominiqueclarke changed the title synthetics - preserve id field on monitor attributes [Synthetics] preserve id field on monitor attributes Oct 3, 2022
@dominiqueclarke dominiqueclarke marked this pull request as ready for review October 3, 2022 19:38
@dominiqueclarke dominiqueclarke requested a review from a team as a code owner October 3, 2022 19:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke
Copy link
Contributor Author

@elasticmachine merge upstream

@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.0MB 1.0MB +245.0B

History

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

justinkambic
justinkambic approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Tested per the instructions in the description, did not see any decryption issues.

Code LGTM.

@dominiqueclarke dominiqueclarke merged commit 61342b3 into elastic:main Oct 4, 2022
@dominiqueclarke dominiqueclarke deleted the fix/synthetics-preserve-monitor-id-field-on-monitor-attributes branch October 4, 2022 17:39
@kibanamachine kibanamachine mentioned this pull request Oct 4, 2022
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 4, 2022
@dominiqueclarke
[Synthetics] preserve id field on monitor attributes (elastic#142478)
62ca045 
* synthetics - preserve id field on monitor attributes

* adjust tests

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* adjust jest tests

* adjust tests

* adjust types

* adjust tests

* adjust types

* update tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 61342b3)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.5

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 4, 2022
@kibanamachine @dominiqueclarke
[Synthetics] preserve id field on monitor attributes (#142478) (#142648)
9444b70 
* synthetics - preserve id field on monitor attributes

* adjust tests

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* adjust jest tests

* adjust tests

* adjust types

* adjust tests

* adjust types

* update tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 61342b3)

Co-authored-by: Dominique Clarke <[email protected]>
@andrewvc andrewvc self-assigned this Oct 4, 2022
@andrewvc
Copy link
Contributor

andrewvc commented Oct 4, 2022

I'm not sure if I tested this right, but I:

  1. Created an 8.4.2 deployment in CFT
  2. Created a 1 min browser monitor
  3. Let it run
  4. Upgraded to 8.5.0

Both before and after monitor management looked like this:

image

So, I saw no URL before, or after. Did I test things wrong? The URL shows up in the monitor detail page

@dominiqueclarke
Copy link
Contributor Author

I personally post FF tested this by doing the following.

  1. Creating a 8.4.0 deployment in CFT
  2. Creating an icmp, tcp, http, and browser monitor
  3. Creating 3 browser project monitors
  4. Waiting about 20 minutes or so for all of them to run and all of them to run through the legacy hydration logic
  5. Enabling and disabling the monitor
  6. Upgrading to 8.5.0-SNAPSHOT
  7. Editing each monitor either in the UI directly or through push
  8. Confirming no decryption errors occurred

Steps 4 and 5 ensure that the keys id, urls and urls.port are added to the monitor. Updating them now in 8.5.0-SNAPSHOT ensures these keys are preserved, so that decryption errors do not occur due to missing AAD.

WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
@dominiqueclarke @kibanamachine @WafaaNasr
[Synthetics] preserve id field on monitor attributes (elastic#142478)
7c52a4b 
* synthetics - preserve id field on monitor attributes

* adjust tests

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* adjust jest tests

* adjust tests

* adjust types

* adjust tests

* adjust types

* update tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

Co-authored-by: kibanamachine <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
@dominiqueclarke @kibanamachine @WafaaNasr
[Synthetics] preserve id field on monitor attributes (elastic#142478)
644c7b3 
* synthetics - preserve id field on monitor attributes

* adjust tests

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

* adjust jest tests

* adjust tests

* adjust types

* adjust tests

* adjust types

* update tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinkambic justinkambic justinkambic approved these changes

Assignees

@andrewvc andrewvc

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.5.0 v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dominiqueclarke @elasticmachine @kibana-ci @kibanamachine @andrewvc @justinkambic