[SR] Prevent change to snapshot name / repository for managed SLM policies

[renshuki]

Summary

Managed SLM policies Snapshot name and Repository field values can be changed from the Snapshot and Restore UI. This is particularly an issue with the cloud-snapshot-policy as changes to the snapshot name or target repository will lead to plan failures on ESS. Besides having a warning message displayed for managed SLM policies:

This is a managed policy. Changing this policy might affect other systems that use it. Proceed with caution.
field values being editable is prone to errors.

This PR disable the Snapshot name and Repository field for managed SLM policies to prevent edits.

Example - Regular SLM policy:

Example - Managed SLM policy:

I'm open to suggestions if there is a better way to do this.

This PR fixes #124916

Checklist

• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

Release Note

Input fields to change snapshot name and repository are now disabled when editing managed SLM policies in Snapshot & Restore.

[elasticmachine]

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

[yuliacech]

Thanks a lot for raising a PR for this issue, @renshuki!
Even though the proposed change is straight forward, I think there are still some things to discuss:

• It looks like changing managed repos/policies on Cloud can lead to serious issues for the deployment. Should we consider first disabling the changes on the ES/Control plane side? Even with some UI elements disabled for managed policies, users will still be able to change the repo/policy via Console for example.
• The issue links to the docs that warns the user of changing the repo found-snapshots and the policy cloud-snapshot-policy. So should we only disable editing for those items specifically or do we want to rely on the managed: true property?
• Instead of disabling snapshot name and repo in the policy edit form, should we consider disabling the edit form all together?
• Should we also consider similar changes for the repo found-snapshots?

Let me know what you think @renshuki, also tagging @alisonelizabeth and @sixstringcode for their input.

[renshuki]

@yuliacech thank you for raising these questions.

It looks like changing managed repos/policies on Cloud can lead to serious issues for the deployment.

Correct, that why I wanted to implement this change primarily.

Should we consider first disabling the changes on the ES/Control plane side? Even with some UI elements disabled for managed policies, users will still be able to change the repo/policy via Console for example.

You are right, some server side validation / limit the scope of editable fields for managed SLM policies via API would be worth discussing with ES / Control Plane.

The issue links to the docs that warns the user of changing the repo found-snapshots and the policy cloud-snapshot-policy. So should we only disable editing for those items specifically or do we want to rely on the managed: true property?

I'm not aware of any other Elastic product defining a managed SLM policy other than Cloud. Also, I don't see a scenario where the snapshot name or repository for a managed SLM policy need to be configurable by the end user. That's why I kept this "generic" approach with isManaged: true.

Instead of disabling snapshot name and repo in the policy edit form, should we consider disabling the edit form all together?

No, the reason is that users should still have the possibility to customize the schedule and retention period for the snapshots.

Should we also consider similar changes for the repo found-snapshots?

I agree, for managed Repositories, preventing changes to Client, Bucket and Base Path fields via the UI would make sense. Maybe we want to tackle this part in a different Kibana issue / PR. Similar to what you mentioned above, I can also raise an issue for ES / Control Plan to see if we need to put some restrictions for managed Respositories related APIs - but Cloud may want to keep a cerain level of flexibility in the case of repositories (for example, scenarios where a S3 bucket need to be recreated)

My thought was that disabling fields in the UI would be "good enough" to mitigate most user errors with little time investment.

[yuliacech]

Thanks a lot for providing the context, @renshuki!
Would you be able to start the conversation about limiting changes on the backend with the ES and the Control plane teams please? I don't think our team has enough details to do that. Also could you please open an issue about similar UI work needed for the repository edit form?
To get this PR merged, we need to add some integration tests, there is an example here, the mock policy would need to be a managed policy. If that is difficult, let me know and I could add the tests directly to your branch.

[alisonelizabeth]

Thank you @renshuki and @yuliacech for the discussion on this.

I agree with Yulia that it would be preferred to enforce this at the API layer rather than only putting in controls on the client side. @renshuki please let us know if you are able to start a conversation around that. I'm OK to proceed with this as a partial fix for now, as this appears to be causing a number of customer issues.

To get this PR merged, we need to add some integration tests, there is an example here, the mock policy would need to be a managed policy.

++ please let us know if you need help here

[renshuki]

Thank you @yuliacech, @alisonelizabeth for your comments / feedback.
Below, a recap of the items that need to be addressed:

• Open an issue about similar UI work needed for the repository edit form
  • [SR] Prevent some fields to be edited in managed repositories #172476
• Reach out to ES / Control Plan to discuss how we can limit the changes via API
  • Prevent changes to managed SLM policies and snapshot repositories via APIs elasticsearch#103066
• Create the integration tests for this PR
• Add tooltips explaining why the fields are disabled
  • [SR] Add tooltips explaining why some managed repository / managed SLM policies fields are disabled #173124

[renshuki]

@yuliacech sorry, I need some guidance for the integration tests. I'm wondering if we want to keep it simple by modifying the the current mock policy like adding isManagedPolicy: true to POLICY_EDIT in constants.ts:

kibana/x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/constant.ts

Lines 18 to 22 in 8e33d5d

	export const POLICY_EDIT = getPolicy({
	name: POLICY_NAME,
	retention: { minCount: 1 },
	config: { includeGlobalState: true, featureStates: ['kibana'] },
	});

and then add some tests in policy_edit.test.ts like:

    test('should disable the snapshot name field for managed policy', () => {
      const { find } = testBed;

      const snapshotNameInput = find('snapshotNameInput');
      expect(snapshotNameInput.props().disabled).toEqual(true);
    });

    test('should disable the respository select field for managed policy', () => {
      const { find } = testBed;

      const repositorySelect = find('repositorySelect');
      expect(repositorySelect.props().disabled).toEqual(true);
    });

or should we treat managed SLM policy tests in a completely different integration test file / mock policy?

[yuliacech]

@renshuki I would add a new test to this file with a mock that uses the existing policy that you linked, but changes the data only for this test. I could just push my suggestion to your branch, if this PR is open to be updated by maintainers, if that's okay for you?

[yuliacech]

Here is the code suggestion

test('should disable the repo and snapshot fields for managed policies', async () => {
      httpRequestsMockHelpers.setGetPolicyResponse(POLICY_EDIT.name, {
        policy: {
          ...POLICY_EDIT,
          isManagedPolicy: true,
        },
      });

      await act(async () => {
        testBed = await setup(httpSetup);
      });
      testBed.component.update();

      const { find } = testBed;

      const snapshotInput = find('snapshotNameInput');
      expect(snapshotInput.props().disabled).toEqual(true);

      const repoSelect = find('repositorySelect');
      expect(repoSelect.props().disabled).toEqual(true);
    });

[renshuki]

Thank you @yuliacech for the code suggestion 😃 Please, feel free to push it to the feat-slm-managed-policy-ui-nomodify branch 🙏

[yuliacech]

Thanks @renshuki! I pushed a commit with the test and with this, I'm good with merging this PR if you want to add the tooltip in a separate PR.

[renshuki]

LGTM @yuliacech, please feel free to merge. I opened #173124 separetely for the tooltips. I assigned it to me and will do the implementation in a different PR.

To circle back around the discussion with Elasticsearch / Control Plane to restrict APIs, they want to "keep an escape hatch for unforeseen situations where they must be changed" - but they may reconsider in the future:
elastic/elasticsearch#103066 (comment)

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: bc2f32f

Failed CI Steps

• FTR Configs #3

Test Failures

• [job] [logs] FTR Configs #3 / APM API tests alerts/transaction_duration.spec.ts basic no archive transaction duration alert create rule for opbeans-node using kql filter shows alert count=1 for opbeans-node on service inventory

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
snapshotRestore	259.6KB	259.7KB	+94.0B

History

• 💔 Build #182993 failed 55c274b
• 💔 Build #182965 failed 69ae53a
• 💛 Build #180621 was flaky d09cfc4
• 💔 Build #180523 failed 5057919
• 💔 Build #180514 failed 8366907

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