[SR] Prevent snapshots in Cloud-managed repository from being deleted in the UI #40104

jen-huang · 2019-07-01T22:52:53Z

Relates to https://github.com/elastic/cloud/issues/34704

Summary

This PR prevents snapshots stored in the ECE/ESS-managed repository from being deleted via the UI. The reason for this is because ECE and ESS have their own snapshot retention policies. Until we have a stack SLM UI that can fully replace Cloud's current SLM UI, let's prevent users from deleting these snapshots as to not conflict with Cloud's retention policy.

This is similar to the behavior we did to prevent users from deleting the managed repository from the UI: #36947

Note that we do allow users to restore managed repository snapshots from the UI. This disabling behavior only applies to deletion.

cc @elastic/cloud-stack-n-soln

Testing instructions

Add cluster.metadata.managed_repository setting via Console:

PUT /_cluster/settings
{
  "persistent": {
    "cluster.metadata.managed_repository": "found-snapshots"
  }
}

Create a repository with the same name as your setting value (found-snapshots)

Note: Neither step is necessary when this goes live on Cloud, as both the repository and the setting will already exist, we are just mocking the environment locally here 🙂

Screenshots

In snapshots table, deletion is disabled (user is not able to select the row and trash can icon is disabled):

In snapshot details, Delete button is disabled:

elasticmachine · 2019-07-01T22:52:55Z

Pinging @elastic/es-ui

cjcenizal

Tested locally, code LGTM! Just had a couple suggestions on copy and a comment.

cjcenizal · 2019-07-01T23:44:51Z

...napshot_restore/public/app/sections/home/snapshot_list/snapshot_details/snapshot_details.tsx

+                                'xpack.snapshotRestore.snapshotDetails.deleteManagedRepositorySnapshotButtonTitle',
+                                {
+                                  defaultMessage:
+                                    'You cannot delete a managed repository snapshot.',


I defer to @gchaps on this one but I think this wording is a bit ambiguous, because it's not clear if "repository snapshot" is the object qualified by the word "managed". How about rephrasing this to "You cannot delete a snapshot stored in a managed repository."

This comment applies to similar language elsewhere in the PR.

+1 to "You cannot delete a snapshot stored in a managed repository."

cjcenizal · 2019-07-01T23:48:32Z

x-pack/legacy/plugins/snapshot_restore/server/lib/get_managed_repository_name.ts

+  callWithInternalUser: any
+): Promise<string | undefined> => {
+  try {
+    const { persistent, transient, defaults } = await callWithInternalUser('cluster.getSettings', {


Can we add a comment here to help people connect the dots with the Cloud-based business logic we're supporting? Here's my suggestion.

// Cloud has its own system for managing snapshots and we want to make // this clear when Snapshot Restore is used in a Cloud deployment.

cjcenizal · 2019-07-01T23:49:43Z

x-pack/legacy/plugins/snapshot_restore/server/lib/get_managed_repository_name.ts

+    };
+    return managedRepositoryName;
+  } catch (e) {
+    // Silently swallow error and return undefined


Not a blocker but I'm still not quite getting this. Can you help me understand over Zoom?

if the request to get cluster settings fails for some reason, simply return undefined for managedRepositoryName. downstream logic will act as if there is no managed_repository setting, which is the behavior for a non-cloud instance such as local dev environment

for some reason

I think that's what sets my spidey sense tingling. If an error is expected for a particular reason, then I can see swallowing it because it has some kind of meaning that we've identified as inconsequential. But if the error is unexpected then that's something I'd expect to bubble up and cause a 500. Otherwise if users are on Cloud and we get an error here, won't they be able to delete snapshots?

I view this as an extreme edge case because

this is call made with callWithInternalUser (superuser), so it won't fail due to things like permissions

the call is made to the cluster API /_cluster/settings, if this call fails with a superuser, I expect the user's instance is having larger problems than this UI

I don't expect the catch logic to really ever be hit for the above reasons. previously this method was not wrapped in try...catch, and any errors would prevent the downstream APIs from returning any information.

if users are on Cloud and we get an error here, won't they be able to delete snapshots?

the minimal possibility of getting an error here notwithstanding, Cloud users are able to override any UI blocks we put into place if they want to 🙂

Cloud users are able to override any UI blocks we put into place if they want to

You're right, I should have phrased my point better: won't the UI no longer distinguish between Cloud and non-Cloud repos/snaps? So won't the user be in a much more dangerous situation?

any errors would prevent the downstream APIs from returning any information.

I see. Do I understand the reason correctly: we've identified the information here as optional, and if this part of the handler fails then the client can still safely continue without the information this request provides. If this is the reasoning, then can we change the comment to communicate that intention, so the comment describes the "why" instead of the "what"?

elasticmachine · 2019-07-01T23:52:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-02T00:28:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth

Tested locally, LGTM.

elasticmachine · 2019-07-02T17:34:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jen-huang · 2019-07-02T18:03:47Z

retest

elasticmachine · 2019-07-02T19:06:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

… in the UI (#40104) (#40200) * Extract `getManagedRepositoryName` to `lib/` * Prevent managed repository snapshots from being deleted in table UI * Prevent delete of managed repository snapshot from its details UI * Fix test * PR feedback and empty restore tab copy edits

jen-huang added 3 commits July 1, 2019 14:50

Extract getManagedRepositoryName to lib/

69eb99a

Prevent managed repository snapshots from being deleted in table UI

a640b17

Prevent delete of managed repository snapshot from its details UI

c0514af

jen-huang requested review from cjcenizal and alisonelizabeth July 1, 2019 22:53

Fix test

2a8f2fc

cjcenizal approved these changes Jul 1, 2019

View reviewed changes

alisonelizabeth approved these changes Jul 2, 2019

View reviewed changes

PR feedback and empty restore tab copy edits

c7f8937

jen-huang merged commit 2eeea97 into elastic:master Jul 2, 2019

jen-huang deleted the fix/sr-cloud branch July 2, 2019 19:16

jen-huang mentioned this pull request Jul 2, 2019

[7.x] [SR] Prevent snapshots in Cloud-managed repository from being deleted in the UI (#40104) #40200

Merged

alisonelizabeth mentioned this pull request Aug 23, 2019

[IM] prevent users from editing and deleting cloud-managed templates #43901

Merged

alisonelizabeth mentioned this pull request Oct 16, 2019

[SR] Support for SLM on Cloud #48402

Closed

cjcenizal added the NeededFor:Cloud label Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SR] Prevent snapshots in Cloud-managed repository from being deleted in the UI #40104

[SR] Prevent snapshots in Cloud-managed repository from being deleted in the UI #40104

jen-huang commented Jul 1, 2019

elasticmachine commented Jul 1, 2019

cjcenizal left a comment

cjcenizal Jul 1, 2019

gchaps Jul 2, 2019

cjcenizal Jul 1, 2019

cjcenizal Jul 1, 2019

jen-huang Jul 2, 2019

cjcenizal Jul 2, 2019 •

edited

Loading

jen-huang Jul 2, 2019

cjcenizal Jul 2, 2019

elasticmachine commented Jul 1, 2019

elasticmachine commented Jul 2, 2019

alisonelizabeth left a comment

elasticmachine commented Jul 2, 2019

jen-huang commented Jul 2, 2019

elasticmachine commented Jul 2, 2019

[SR] Prevent snapshots in Cloud-managed repository from being deleted in the UI #40104

[SR] Prevent snapshots in Cloud-managed repository from being deleted in the UI #40104

Conversation

jen-huang commented Jul 1, 2019

Summary

Testing instructions

Screenshots

elasticmachine commented Jul 1, 2019

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Jul 1, 2019

Choose a reason for hiding this comment

gchaps Jul 2, 2019

Choose a reason for hiding this comment

cjcenizal Jul 1, 2019

Choose a reason for hiding this comment

cjcenizal Jul 1, 2019

Choose a reason for hiding this comment

jen-huang Jul 2, 2019

Choose a reason for hiding this comment

cjcenizal Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

jen-huang Jul 2, 2019

Choose a reason for hiding this comment

cjcenizal Jul 2, 2019

Choose a reason for hiding this comment

elasticmachine commented Jul 1, 2019

💔 Build Failed

elasticmachine commented Jul 2, 2019

💚 Build Succeeded

alisonelizabeth left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 2, 2019

💔 Build Failed

jen-huang commented Jul 2, 2019

elasticmachine commented Jul 2, 2019

💚 Build Succeeded

cjcenizal Jul 2, 2019 •

edited

Loading