Prevent in-place downgrades and invalid upgrades #41731

DaveCTurner · 2019-05-02T05:52:13Z

Downgrading an Elasticsearch node to an earlier version is unsupported, because
we do not make any attempt to guarantee that a node can read any of the on-disk
data written by a future version. Yet today we do not actively prevent
downgrades, and sometimes users will attempt to roll back a failed upgrade with
an in-place downgrade and get into an unrecoverable state.

This change adds the current version of the node to the node metadata file, and
checks the version found in this file against the current version at startup.
If the node cannot be sure of its ability to read the on-disk data then it
refuses to start, preserving any on-disk data in its upgraded state.

This change also adds a command-line tool to overwrite the node metadata file
without performing any version checks, to unsafely bypass these checks and
recover the historical and lenient behaviour.

Downgrading an Elasticsearch node to an earlier version is unsupported, because we do not make any attempt to guarantee that a node can read any of the on-disk data written by a future version. Yet today we do not actively prevent downgrades, and sometimes users will attempt to roll back a failed upgrade with an in-place downgrade and get into an unrecoverable state. This change adds the current version of the node to the node metadata file, and checks the version found in this file against the current version at startup. If the node cannot be sure of its ability to read the on-disk data then it refuses to start, preserving any on-disk data in its upgraded state. This change also adds a command-line tool to overwrite the node metadata file without performing any version checks, to unsafely bypass these checks and recover the historical and lenient behaviour.

elasticmachine · 2019-05-02T05:52:15Z

Pinging @elastic/es-distributed

elasticmachine · 2019-05-02T05:52:16Z

Pinging @elastic/es-core-infra

DaveCTurner · 2019-05-03T09:40:14Z

ci/1 failed for mysterious reasons; I opened #41785 to help in future.

@elasticmachine please run elasticsearch-ci/1.

andrershov

Thanks, @DaveCTurner!
In general looks good, I've left some comments.

andrershov · 2019-05-06T15:19:29Z

server/src/main/java/org/elasticsearch/env/NodeMetaData.java

-            return new NodeMetaData(nodeId);
+            final Version nodeVersion;
+            if (this.nodeVersion == null) {
+                assert Version.CURRENT.major <= Version.V_7_0_0.major + 1 : "version is required in the node metadata from v9 onwards";


Why not to use V_8_0_0 instead of V_7_0_0+1 ?

When we upgrade master to version 9 the constant Version.V_8_0_0 will remain in existence, but Version.V_7_0_0 should be removed, giving a compile-time failure of this assertion.

andrershov · 2019-05-06T15:21:51Z

server/src/main/java/org/elasticsearch/env/NodeMetaData.java

+
+    public NodeMetaData upgradeToCurrentVersion() {
+        if (nodeVersion.equals(Version.V_EMPTY)) {
+            assert Version.CURRENT.major <= Version.V_7_0_0.major + 1 : "version is required in the node metadata from v9 onwards";


Why not to use V_8_0_0 instead of V_7_0_0+1 ?

When we upgrade master to version 9 the constant Version.V_8_0_0 will remain in existence, but Version.V_7_0_0 should be removed, giving a compile-time failure of this assertion.

andrershov · 2019-05-06T15:30:52Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentIT.java

+                @Override
+                public Settings onNodeStopped(String nodeName) {
+                    try {
+                        for (Path dataPath : dataPaths) {


This loop seems to be removing node metadata files. Why is it needed? You anyway re-write the files in the tests.

Ah, this is a throwback to a version of this PR that failed the node if the node metadata file was removed but the data directory was nonempty. I think we should be stricter about starting a node on such a data directory, but that change has wider impact on the tests and is worthy of a separate discussion. Removed in 3320d5a.

andrershov · 2019-05-06T15:34:32Z

server/src/test/java/org/elasticsearch/env/NodeMetaDataTests.java

+
+public class NodeMetaDataTests extends ESTestCase {
+    private Version randomVersion() {
+        return rarely() ? Version.fromId(randomInt()) : VersionUtils.randomVersion(random());


If we really want to use rarely part here, can we add a comment why Version from any int exists?

Sure, comment added in d4888db.

andrershov · 2019-05-06T15:38:02Z

server/src/test/java/org/elasticsearch/env/NodeMetaDataTests.java

+
+        final Path tempDir = createTempDir();
+        final Path stateDir = Files.createDirectory(tempDir.resolve(MetaDataStateFormat.STATE_DIR_NAME));
+        final InputStream resource = this.getClass().getResourceAsStream("testReadsFormatWithoutVersion.binary");


I would rather avoid adding a binary file a resource. Can we have instead a method for creating NodeMetadData without version?

I would rather do it this way, because the important behaviour is that we can correctly read versions of this file written by arbitrarily ancient versions of Elasticsearch of which the given resource is an example. If we create the metadata file in this test then we might change the format in a way that round-trips correctly but which doesn't reflect the behaviour of ancient versions of Elasticsearch any more.

Isn't this tested by our full / rolling cluster upgrade tests?

Today it is, but in future we won't be able to produce version-free metadata files any more and yet we'll want to have a test saying that we refuse to load them in place of the one here. However the comment I left is misleading and I adjusted it in 9cb083f.

andrershov · 2019-05-06T15:45:20Z

server/src/test/java/org/elasticsearch/env/OverwriteNodeVersionCommandTests.java

+        final Version nodeVersion = NodeMetaDataTests.tooNewVersion();
+        NodeMetaData.FORMAT.writeAndCleanup(new NodeMetaData(nodeId, nodeVersion), nodePaths);
+        final MockTerminal mockTerminal = new MockTerminal();
+        mockTerminal.addTextInput("n\n");


can do some randomization here as well

Sure, done in e805ec3.

andrershov · 2019-05-06T15:46:37Z

server/src/main/java/org/elasticsearch/env/OverwriteNodeVersionCommand.java

+    }
+
+    //package-private for testing
+    void testExecute(Terminal terminal, OptionSet options, Environment env) throws Exception {


My IDE claims this method is not used

So does mine. Removed in 4961fd1.

DaveCTurner

Thanks for the review @andrershov, I've addressed or answered your comments.

DaveCTurner · 2019-05-07T15:27:22Z

server/src/main/java/org/elasticsearch/env/NodeMetaData.java

+
+    public NodeMetaData upgradeToCurrentVersion() {
+        if (nodeVersion.equals(Version.V_EMPTY)) {
+            assert Version.CURRENT.major <= Version.V_7_0_0.major + 1 : "version is required in the node metadata from v9 onwards";


When we upgrade master to version 9 the constant Version.V_8_0_0 will remain in existence, but Version.V_7_0_0 should be removed, giving a compile-time failure of this assertion.

DaveCTurner · 2019-05-07T15:27:30Z

server/src/main/java/org/elasticsearch/env/NodeMetaData.java

-            return new NodeMetaData(nodeId);
+            final Version nodeVersion;
+            if (this.nodeVersion == null) {
+                assert Version.CURRENT.major <= Version.V_7_0_0.major + 1 : "version is required in the node metadata from v9 onwards";


When we upgrade master to version 9 the constant Version.V_8_0_0 will remain in existence, but Version.V_7_0_0 should be removed, giving a compile-time failure of this assertion.

DaveCTurner · 2019-05-07T15:28:16Z

server/src/test/java/org/elasticsearch/env/NodeMetaDataTests.java

+
+public class NodeMetaDataTests extends ESTestCase {
+    private Version randomVersion() {
+        return rarely() ? Version.fromId(randomInt()) : VersionUtils.randomVersion(random());


Sure, comment added in d4888db.

DaveCTurner · 2019-05-07T15:32:51Z

server/src/test/java/org/elasticsearch/env/NodeMetaDataTests.java

+
+        final Path tempDir = createTempDir();
+        final Path stateDir = Files.createDirectory(tempDir.resolve(MetaDataStateFormat.STATE_DIR_NAME));
+        final InputStream resource = this.getClass().getResourceAsStream("testReadsFormatWithoutVersion.binary");


I would rather do it this way, because the important behaviour is that we can correctly read versions of this file written by arbitrarily ancient versions of Elasticsearch of which the given resource is an example. If we create the metadata file in this test then we might change the format in a way that round-trips correctly but which doesn't reflect the behaviour of ancient versions of Elasticsearch any more.

ywelsch

Left a few smaller comments, looking good o.w.

ywelsch · 2019-05-17T13:19:03Z

docs/reference/commands/node-tool.asciidoc


 [float]
 === Synopsis

 [source,shell]
 --------------------------------------------------
-bin/elasticsearch-node repurpose|unsafe-bootstrap|detach-cluster
+bin/elasticsearch-node repurpose|unsafe-bootstrap|detach-cluster|overwrite-version


how about override-minimum-version instead of overwrite-version?

The version checks are not simply a minimum. Today they also include a maximum, and in future we may add more complex constraints. But I do prefer override, thanks, I adjusted things in ba6ae23.

ywelsch · 2019-05-17T13:23:16Z

server/src/main/java/org/elasticsearch/cluster/coordination/ElasticsearchNodeCommand.java

+        try {
+            return new NodeEnvironment.NodePath(path);
+        } catch (IOException e) {
+            throw new ElasticsearchException("Unable to investigate path: " + path + ": " + e.getMessage());


why not pass e as cause to the ElasticsearchException?

I blindly moved this method here from NodeRepurposeCommand, but yes an inner exception is preferable, see e12ca62.

ywelsch · 2019-05-17T13:28:47Z

server/src/test/java/org/elasticsearch/env/NodeMetaDataTests.java

+
+        final Path tempDir = createTempDir();
+        final Path stateDir = Files.createDirectory(tempDir.resolve(MetaDataStateFormat.STATE_DIR_NAME));
+        final InputStream resource = this.getClass().getResourceAsStream("testReadsFormatWithoutVersion.binary");


Isn't this tested by our full / rolling cluster upgrade tests?

Downgrading an Elasticsearch node to an earlier version is unsupported, because we do not make any attempt to guarantee that a node can read any of the on-disk data written by a future version. Yet today we do not actively prevent downgrades, and sometimes users will attempt to roll back a failed upgrade with an in-place downgrade and get into an unrecoverable state. This change adds the current version of the node to the node metadata file, and checks the version found in this file against the current version at startup. If the node cannot be sure of its ability to read the on-disk data then it refuses to start, preserving any on-disk data in its upgraded state. This change also adds a command-line tool to overwrite the node metadata file without performing any version checks, to unsafely bypass these checks and recover the historical and lenient behaviour.

…der-permit-primary-mode-only * elastic/master: Move the FIPS configuration back to the build plugin (elastic#41989) Remove stray back tick that's messing up table format (elastic#41705) Add missing comma in code section (elastic#41678) add 7.1.1 and 6.8.1 versions (elastic#42253) Use spearate testkit dir for each run (elastic#42013) Add experimental and warnings to vector functions (elastic#42205) Fix version in tests since elastic#41906 was merged Bump version in BWC check after backport Prevent in-place downgrades and invalid upgrades (elastic#41731) Mute date_histo interval bwc test

* master: (176 commits) Avoid unnecessary persistence of retention leases (elastic#42299) [ML][TEST] Fix limits in AutodetectMemoryLimitIT (elastic#42279) [ML Data Frame] Persist and restore checkpoint and position (elastic#41942) mute failing filerealm hash caching tests (elastic#42304) Safer Wait for Snapshot Success in ClusterPrivilegeTests (elastic#40943) Remove 7.0.2 (elastic#42282) Revert "Remove 7.0.2 (elastic#42282)" [DOCS] Copied note on slicing support to Slicing section. Closes 26114 (elastic#40426) Remove 7.0.2 (elastic#42282) Mute all ml_datafeed_crud rolling upgrade tests Move the FIPS configuration back to the build plugin (elastic#41989) Remove stray back tick that's messing up table format (elastic#41705) Add missing comma in code section (elastic#41678) add 7.1.1 and 6.8.1 versions (elastic#42253) Use spearate testkit dir for each run (elastic#42013) Add experimental and warnings to vector functions (elastic#42205) Fix version in tests since elastic#41906 was merged Bump version in BWC check after backport Prevent in-place downgrades and invalid upgrades (elastic#41731) Mute date_histo interval bwc test ...

Downgrading an Elasticsearch node to an earlier version is unsupported, because we do not make any attempt to guarantee that a node can read any of the on-disk data written by a future version. Yet today we do not actively prevent downgrades, and sometimes users will attempt to roll back a failed upgrade with an in-place downgrade and get into an unrecoverable state. This change adds the current version of the node to the node metadata file, and checks the version found in this file against the current version at startup. If the node cannot be sure of its ability to read the on-disk data then it refuses to start, preserving any on-disk data in its upgraded state. This change also adds a command-line tool to overwrite the node metadata file without performing any version checks, to unsafely bypass these checks and recover the historical and lenient behaviour.

Today we fail the node at startup if it contains an index that is too old to be compatible with the current version, unless that index is closed. If the index is closed then the node will start up and this puts us into a bad state: the index cannot be opened and must be reindexed using an earlier version, but we offer no way to get that index into a node running an earlier version so that it can be reindexed. Downgrading the node in-place is decidedly unsupported and cannot be expected to work since the node already started up and upgraded the rest of its metadata. Since elastic#41731 we actively reject downgrades to versions ≥ v7.2.0 too. This commit prevents the node from starting in the presence of any too-old indices (closed or not). In particular, it does not write any upgraded metadata in this situation, increasing the chances an in-place downgrade might be successful. We still actively reject the downgrade using elastic#41731, because we wrote the node metadata file before checking the index metadata, but at least there is a way to override this check. Relates elastic#21830, elastic#44230

Today we fail the node at startup if it contains an index that is too old to be compatible with the current version, unless that index is closed. If the index is closed then the node will start up and this puts us into a bad state: the index cannot be opened and must be reindexed using an earlier version, but we offer no way to get that index into a node running an earlier version so that it can be reindexed. Downgrading the node in-place is decidedly unsupported and cannot be expected to work since the node already started up and upgraded the rest of its metadata. Since #41731 we actively reject downgrades to versions ≥ v7.2.0 too. This commit prevents the node from starting in the presence of any too-old indices (closed or not). In particular, it does not write any upgraded metadata in this situation, increasing the chances an in-place downgrade might be successful. We still actively reject the downgrade using #41731, because we wrote the node metadata file before checking the index metadata, but at least there is a way to override this check. Relates #21830, #44230

based on elastic/elasticsearch#41731

DaveCTurner added >enhancement resiliency :Core/Infra/Core Core issues without another label :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v8.0.0 v7.2.0 labels May 2, 2019

DaveCTurner requested review from andrershov and ywelsch May 2, 2019 05:52

DaveCTurner added 2 commits May 2, 2019 08:22

Precommit fixes

b22d9dd

Merge branch 'master' into 2019-05-01-prevent-downgrades

374ff83

andrershov reviewed May 6, 2019

View reviewed changes

DaveCTurner added 5 commits May 7, 2019 16:17

Merge branch 'master' into 2019-05-01-prevent-downgrades

27c14af

Remove unused method

4961fd1

Add comment about unexpected versions

d4888db

Randomly choose negative inputs

e805ec3

No need to remove node metadata since we always overwrite it

3320d5a

DaveCTurner commented May 7, 2019

View reviewed changes

DaveCTurner added 2 commits May 7, 2019 16:44

Checkstyle etc

c154c2e

Moar checkstyle

00abb44

ywelsch approved these changes May 17, 2019

View reviewed changes

DaveCTurner added 4 commits May 17, 2019 21:50

Merge branch 'master' into 2019-05-01-prevent-downgrades

57425f4

Adjust comment

9cb083f

Use inner exceptions instead of strings

e12ca62

Rename overwrite -> override

ba6ae23

DaveCTurner merged commit 749135b into elastic:master May 21, 2019

DaveCTurner deleted the 2019-05-01-prevent-downgrades branch May 21, 2019 06:52

DaveCTurner added the backport pending label May 21, 2019

DaveCTurner removed the backport pending label May 21, 2019

colings86 removed the :Core/Infra/Core Core issues without another label label Jun 18, 2019

DaveCTurner mentioned this pull request Jul 11, 2019

Better handling of ancient indices #44230

Closed

DaveCTurner mentioned this pull request Jul 12, 2019

Fail node containing ancient closed index #44264

Merged

mkleen added a commit to crate/crate that referenced this pull request May 10, 2021

wip: version for node meta data

5e479ba

based on elastic/elasticsearch#41731

mkleen added a commit to crate/crate that referenced this pull request May 10, 2021

wip: version for node meta data

64a50a2

based on elastic/elasticsearch#41731

mkleen mentioned this pull request May 10, 2021

bp: Prevent in-place downgrades and invalid upgrades crate/crate#11368

Merged

5 tasks

mkleen added a commit to crate/crate that referenced this pull request May 10, 2021

wip: version for node meta data

cd7d196

based on elastic/elasticsearch#41731

mkleen added a commit to crate/crate that referenced this pull request May 11, 2021

wip: version for node meta data

74058df

based on elastic/elasticsearch#41731

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

Prevent in-place downgrades and invalid upgrades #41731

Prevent in-place downgrades and invalid upgrades #41731

DaveCTurner commented May 2, 2019

elasticmachine commented May 2, 2019

elasticmachine commented May 2, 2019

DaveCTurner commented May 3, 2019

andrershov left a comment

andrershov May 6, 2019

DaveCTurner May 7, 2019

andrershov May 6, 2019

DaveCTurner May 7, 2019

andrershov May 6, 2019

DaveCTurner May 7, 2019

andrershov May 6, 2019

DaveCTurner May 7, 2019

andrershov May 6, 2019

DaveCTurner May 7, 2019

ywelsch May 17, 2019

DaveCTurner May 18, 2019

andrershov May 6, 2019

DaveCTurner May 7, 2019

andrershov May 6, 2019

DaveCTurner May 7, 2019

DaveCTurner left a comment

DaveCTurner May 7, 2019

DaveCTurner May 7, 2019

DaveCTurner May 7, 2019

DaveCTurner May 7, 2019

ywelsch left a comment

ywelsch May 17, 2019

DaveCTurner May 18, 2019

ywelsch May 17, 2019

DaveCTurner May 18, 2019

ywelsch May 17, 2019

Prevent in-place downgrades and invalid upgrades #41731

Prevent in-place downgrades and invalid upgrades #41731

Conversation

DaveCTurner commented May 2, 2019

elasticmachine commented May 2, 2019

elasticmachine commented May 2, 2019

DaveCTurner commented May 3, 2019

andrershov left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a 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

Choose a reason for hiding this comment

ywelsch left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment