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

[Snapshot & Restore] Decode URIs and fix editing of a policy #76278

Conversation

jloleysens
Copy link
Contributor

Summary

Add logic for decoding snapshot policy, snapshot repository, snapshot and restore names.

Fix: #75951

Also discovered an issue when editing existing policies. The form will block (incorrectly) when editing a policy's name (or any other value) because the validation thinks that indices need to be set. This is a regression from the behaviour added here: #68078. The fix for now is to bypass this validation unless we are specifically editing index name values in the UI.

How to test decoding - after fix

  1. Start ES with a local repo configured (yarn es snapshot -E path.repo=/tmp/es-snaps)
  2. Go to snapshot and restore
  3. Create a repo with a colon (or other special char) in it's name
  4. Attempt to view repo details and edit (should work)
  5. Do steps 3 and 4 for a snapshot policy
  6. Run the snapshot policy
  7. Attempt to view details of the snapshot that should have been created

How to test editing policy fix

This should be covered in the steps mentioned before if you repeated steps 3 and 4 for editing a snapshot policy after creating it. But to reiterate:

  1. Create a snapshot policy
  2. Edit the policy name
  3. Save the policy

Release note

Fixed an issue in Snapshot and Restore UI where creating a policy, repository or snapshot with a special character, like a colon, in the name would result in a 404 when viewing details or editing any of the aforementioned.

@jloleysens jloleysens added release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.1 labels Aug 31, 2020
@jloleysens jloleysens requested a review from a team as a code owner August 31, 2020 12:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these bugs @jloleysens!

The change does indeed fix the problem of using : in the name, however, there is still an error if using a %. You can take a look in the component templates code for what I did to address this. Re: #69732 (comment).

I also noticed both the summary tab in the review step and the details panel sometimes has no value under the Data streams and indices section when all indices/data streams are selected.

Screen Shot 2020-08-31 at 9 05 08 AM

Screen Shot 2020-08-31 at 9 05 37 AM

@jloleysens
Copy link
Contributor Author

@alisonelizabeth Thanks for the review! It looks like there was an issue with how serialisation of policy config was being done and that was the issue that helped me to uncover the weird validation issue too.

I've addressed your feedback, would you mind taking another look?

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for making those fixes! I think there's still an issue with the decoding logic that I left a comment in the code about. I'm going to go ahead and approve to not block you, but happy to retest again if you'd like.

export const attemptToURIDecode = (value: string) => {
let result: string;
try {
result = decodeURIComponent(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might also have to try decodeURI. Otherwise, when doing a full page refresh with the details panel open, you might get a 404.

Screen Shot 2020-08-31 at 10 51 43 AM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
snapshotRestore 171 +3 168

async chunks size

id value diff baseline
snapshotRestore 761.5KB +3.3KB 758.2KB

History

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

@jloleysens jloleysens merged commit 3fb839e into elastic:master Aug 31, 2020
@jloleysens jloleysens deleted the snapshot-restore/fix-uri-encoding-and-policy-edit branch August 31, 2020 19:32
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 31, 2020
…#76278)

* fix URI decoding and editing of a policy which backs up all indices

* fix type issue

* fix general use of encoding and update decode algo

* fix serialisation of snapshots and added a test

* Fix test description name

* Update attempt_to_uri_decode.ts

* catch errors from decoding in the already throwing code

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 31, 2020
…#76278)

* fix URI decoding and editing of a policy which backs up all indices

* fix type issue

* fix general use of encoding and update decode algo

* fix serialisation of snapshots and added a test

* Fix test description name

* Update attempt_to_uri_decode.ts

* catch errors from decoding in the already throwing code

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit that referenced this pull request Aug 31, 2020
…#76320)

* fix URI decoding and editing of a policy which backs up all indices

* fix type issue

* fix general use of encoding and update decode algo

* fix serialisation of snapshots and added a test

* Fix test description name

* Update attempt_to_uri_decode.ts

* catch errors from decoding in the already throwing code

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit that referenced this pull request Sep 1, 2020
…#76322)

* fix URI decoding and editing of a policy which backs up all indices

* fix type issue

* fix general use of encoding and update decode algo

* fix serialisation of snapshots and added a test

* Fix test description name

* Update attempt_to_uri_decode.ts

* catch errors from decoding in the already throwing code

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@LeeDr LeeDr removed the v7.9.1 label Sep 2, 2020
@LeeDr LeeDr added the v7.9.2 label Sep 2, 2020
@LeeDr LeeDr mentioned this pull request Sep 2, 2020
1 task
@alisonelizabeth alisonelizabeth added the Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI label Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.9.2 v7.10.0 v8.0.0
Projects
None yet
5 participants