Do not restore autoscaling policy from snapshots #66023

ywangd · 2020-12-08T13:13:38Z

This PR excludes autoscaling policies from being restored from a snapshot.

elasticmachine · 2020-12-08T13:13:42Z

Pinging @elastic/es-security (Team:Security)

ywangd · 2020-12-08T13:21:08Z

The tricky part of this change is that snapshot/restore, autoscaling and operator privileges are alll in different packages. Other than snapshot/restore, the other two are in xpack. This creates a problem when snapshot/restore needs to know whether operator privileges are enabled and whether the custom metadata is for "autoscaling".

As an initial attempt, I just took the simplest approach to duplicate the strings (autoscaling for metadata custom and xpack.security.operator_privileges.enabled for the setting) in RestoreService. This is likely non-ideal. It would be better if these information can be pulled by snapshot/restore code without having to explicity spell out autoscaling or operator privileges. I am open to suggestions.

henningandersen

I left a couple of initial comments to discuss/clarify before reviewing the tests.

henningandersen · 2020-12-08T13:53:26Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

@@ -454,7 +459,8 @@ public ClusterState execute(ClusterState currentState) {
                            if (metadata.customs() != null) {
                                for (ObjectObjectCursor<String, Metadata.Custom> cursor : metadata.customs()) {
                                    if (RepositoriesMetadata.TYPE.equals(cursor.key) == false
-                                            && DataStreamMetadata.TYPE.equals(cursor.key) == false) {
+                                            && DataStreamMetadata.TYPE.equals(cursor.key) == false
+                                            && (operatorPrivilegesEnabled == false || "autoscaling".equals(cursor.key) == false)) {


I wonder if we should just skip the operatorPrivilegesEnabled check. It is fine to not restore autoscaling policies for now and we can revisit once we have the proper framework around operator privileges and restore in general.

Removing the check makes both the logic and code dependency simpler. Another practical advantage is that Cloud probably will not enable opereator privileges straight away after 7.11 release. Therefore removing this check makes sure the restore behavioiur of autoscaling policy stay the same before and after operator privileges get enabled in Cloud. Having a consistent behaviour is one of the motivations for this work to be included in 7.11. So it seems to make sense. But it would be different than what we have agreed during the meeting. @colings86 Could you please verify whether removing this check is desirable?

But it would be different than what we have agreed during the meeting

I believe we discussed that approach at the meeting and I came out of the meeting with the impression that this was the direction we would take (to just disable restore for autoscaling).

I'm fine with changing this so we never restore autoscaling policies regardless of whether operator privileges are enabled. At least for this first version

Thanks both. I updated the code to drop the operator privileges check.

henningandersen · 2020-12-08T13:55:42Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

@@ -454,7 +459,8 @@ public ClusterState execute(ClusterState currentState) {
                            if (metadata.customs() != null) {
                                for (ObjectObjectCursor<String, Metadata.Custom> cursor : metadata.customs()) {
                                    if (RepositoriesMetadata.TYPE.equals(cursor.key) == false
-                                            && DataStreamMetadata.TYPE.equals(cursor.key) == false) {
+                                            && DataStreamMetadata.TYPE.equals(cursor.key) == false
+                                            && (operatorPrivilegesEnabled == false || "autoscaling".equals(cursor.key) == false)) {


Rather than switch on the "autoscaling" key I would prefer to add a marker interface "NotRestorableCustomMetadata", implemented by AutoscalingMetadata and use instanceof to omit it instead.

Updated. Thanks!

…scaling-policy

elasticmachine · 2020-12-09T10:55:46Z

Pinging @elastic/es-distributed (Team:Distributed)

ywangd · 2020-12-09T10:58:37Z

I have updated the PR based on the discussion, including the code and its title and description. Since operator privileges are no longer relevant for this behaviour, this PR is no longer really relevant to security. Hence I dropped the :Security/Authorization tag and added :Distributed/Snapshot/Restore.

henningandersen

Thanks @ywangd this looks good. There is a bit of excess changes from prior iterations that I think we should remove from this PR (at least they are not autoscaling related).

henningandersen · 2020-12-10T14:31:35Z

...src/internalClusterTest/java/org/elasticsearch/xpack/autoscaling/AutoscalingSnapshotsIT.java

+            .setIncludeGlobalState(true)
+            .get();
+
+        RestStatus status = createSnapshotResponse.getSnapshotInfo().status();


nit: why not just:

RestStatus status = createSnapshotResponse.status();

henningandersen · 2020-12-10T14:34:11Z

...src/internalClusterTest/java/org/elasticsearch/xpack/autoscaling/AutoscalingSnapshotsIT.java

+            .get();
+        assertEquals(RestStatus.OK, restoreSnapshotResponse.status());
+
+        final GetAutoscalingPolicyAction.Request getRequest = new GetAutoscalingPolicyAction.Request(policy.name());


I think we should extend the test to also have the case where a policy does exist when restoring to check that it survives the restore. To protect us from "restore-global-state" clearing everything during restore. Could be just randomly doing that or the delete case.

Good point. I have expanded the test to:

Randomly deleting or mutating the existing policy after snapshot is taken

Add another policy after snapshot is taken

Then assert things stay the same after restore.

henningandersen · 2020-12-10T14:36:52Z

...ts/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java

@@ -100,4 +99,86 @@ public void testOperatorPrivilegesXpackUsage() throws IOException {
        assertTrue((boolean) operatorPrivileges.get("available"));
        assertTrue((boolean) operatorPrivileges.get("enabled"));
    }
+


I think we should just remove this and the associated build file changes etc. from this PR? Once the real framework for avoiding to restore autoscaling policies on operator privilege enabled setups goes in, this might look different anyway.

Agreed and all unrelated changes removed.

A bit more context: I thought about deleting these changes in last commit. But I got a bit paranoid and was trying to assert that operator privileges do not interfere with the restore behaviour. Looking back now, this thinking was an artefact of how this PR evolved, which started with the assumption that operator privileges would impact the behaviour. If it had started purely from the autoscaling side, I don't think I would add anything related to operator privileges. In short, it's the right move to delete these unrelated code.

henningandersen · 2020-12-10T14:37:40Z

...ts/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java

-        final String authHeader = "Basic "
-            + Base64.getEncoder().encodeToString("test_operator:x-pack-test-password".getBytes(StandardCharsets.UTF_8));
-        postVotingConfigExclusionsRequest.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", authHeader));
+        postVotingConfigExclusionsRequest.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", OPERATOR_AUTH_HEADER));


This also looks unrelated to at least the PR description, perhaps move to separate PR?

Removed. See also above.

…scaling-policy

ywangd · 2020-12-11T00:26:56Z

@henningandersen The PR is now updated to address your feedback. It is ready for another look. Thanks!

henningandersen

LGTM, thanks @ywangd

Autoscaling policies (custom metadata) are marked as non-restorable and hence they are never restored from a snapshot.

Initial working version

2b5ac26

ywangd added >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.11.0 labels Dec 8, 2020

elasticmachine added the Team:Security Meta label for security team label Dec 8, 2020

ywangd marked this pull request as draft December 8, 2020 13:14

ywangd requested review from henningandersen and tvernum December 8, 2020 13:15

ywangd added 2 commits December 9, 2020 00:26

spotless

00b1c40

more spotless

eeeca94

henningandersen reviewed Dec 8, 2020

View reviewed changes

ywangd added 3 commits December 9, 2020 08:34

address feedback

475dbc1

Merge remote-tracking branch 'origin/master' into do-not-restore-auto…

7a6dbd8

…scaling-policy

address feedback

1aedcfb

ywangd marked this pull request as ready for review December 9, 2020 10:53

ywangd changed the title ~~Do not restore autoscaling policy when operator privileges are enabled~~ Do not restore autoscaling policy from snapshots Dec 9, 2020

ywangd added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Dec 9, 2020

elasticmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. and removed Team:Security Meta label for security team labels Dec 9, 2020

ywangd requested a review from henningandersen December 9, 2020 10:58

henningandersen reviewed Dec 10, 2020

View reviewed changes

ywangd added 3 commits December 11, 2020 10:22

Merge remote-tracking branch 'origin/master' into do-not-restore-auto…

ef1c636

…scaling-policy

Address feedback

85e61ae

revert more irrelevant changes from earlier commits

b7b4c23

ywangd requested a review from henningandersen December 11, 2020 00:27

henningandersen approved these changes Dec 11, 2020

View reviewed changes

ywangd merged commit 78ad68f into elastic:master Dec 11, 2020

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Dec 11, 2020

Do not restore autoscaling policy from snapshots (elastic#66023)

6f03b1d

Autoscaling policies (custom metadata) are marked as non-restorable and hence they are never restored from a snapshot.

ywangd added a commit that referenced this pull request Dec 11, 2020

Do not restore autoscaling policy from snapshots (#66023) (#66199)

d439654

Autoscaling policies (custom metadata) are marked as non-restorable and hence they are never restored from a snapshot.

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not restore autoscaling policy from snapshots #66023

Do not restore autoscaling policy from snapshots #66023

ywangd commented Dec 8, 2020 •

edited

Loading

elasticmachine commented Dec 8, 2020

ywangd commented Dec 8, 2020

henningandersen left a comment

henningandersen Dec 8, 2020

ywangd Dec 8, 2020

henningandersen Dec 9, 2020 •

edited

Loading

colings86 Dec 9, 2020 •

edited

Loading

ywangd Dec 9, 2020

henningandersen Dec 8, 2020

ywangd Dec 8, 2020

elasticmachine commented Dec 9, 2020

ywangd commented Dec 9, 2020

henningandersen left a comment

henningandersen Dec 10, 2020

henningandersen Dec 10, 2020

ywangd Dec 11, 2020 •

edited

Loading

henningandersen Dec 10, 2020

ywangd Dec 11, 2020

henningandersen Dec 10, 2020

ywangd Dec 11, 2020

ywangd commented Dec 11, 2020

henningandersen left a comment

Do not restore autoscaling policy from snapshots #66023

Do not restore autoscaling policy from snapshots #66023

Conversation

ywangd commented Dec 8, 2020 • edited Loading

elasticmachine commented Dec 8, 2020

ywangd commented Dec 8, 2020

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningandersen Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

colings86 Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 9, 2020

ywangd commented Dec 9, 2020

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Dec 11, 2020

henningandersen left a comment

Choose a reason for hiding this comment

ywangd commented Dec 8, 2020 •

edited

Loading

henningandersen Dec 9, 2020 •

edited

Loading

colings86 Dec 9, 2020 •

edited

Loading

ywangd Dec 11, 2020 •

edited

Loading