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.

Sign up for GitHub

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

Prevent downgrades from 8.x to 7.x #78586

Merged
merged 4 commits into from
Oct 4, 2021
Merged

Prevent downgrades from 8.x to 7.x #78586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30ea08e
Prevent downgrades from 8.x to 7.x
DaveCTurner Oct 2, 2021
a21c32d
Fixes
DaveCTurner Oct 2, 2021
7f77a24
Don't count new file's disk usage in tests
DaveCTurner Oct 2, 2021
cb0fddb
Merge branch 'master' into 2021-10-02-prevent-downgrade-to-7x
elasticmachine Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions server/src/internalClusterTest/java/org/elasticsearch/env/NodeEnvironmentIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@

import org.elasticsearch.Version;
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
import org.elasticsearch.core.CheckedConsumer;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.core.CheckedConsumer;
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.gateway.PersistedClusterStateService;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.test.ESIntegTestCase;
Expand Down Expand Up @@ -138,8 +138,11 @@ public void testUpgradeDataFolder() throws IOException, InterruptedException {
// simulate older data path layout by moving data under "nodes/0" folder
final List<Path> dataPaths = List.of(PathUtils.get(Environment.PATH_DATA_SETTING.get(dataPathSettings)));
dataPaths.forEach(path -> {
final Path targetPath = path.resolve("nodes").resolve("0");
final Path nodesPath = path.resolve("nodes");
final Path targetPath = nodesPath.resolve("0");
try {
assertTrue(Files.isRegularFile(nodesPath));
Files.delete(nodesPath);
Files.createDirectories(targetPath);

try (DirectoryStream<Path> stream = Files.newDirectoryStream(path)) {
Expand Down Expand Up @@ -192,9 +195,9 @@ public void testUpgradeDataFolder() throws IOException, InterruptedException {
}

// check that upgrade works
dataPaths.forEach(path -> assertTrue(Files.exists(path.resolve("nodes"))));
dataPaths.forEach(path -> assertTrue(Files.isDirectory(path.resolve("nodes"))));
internalCluster().startNode(dataPathSettings);
dataPaths.forEach(path -> assertFalse(Files.exists(path.resolve("nodes"))));
dataPaths.forEach(path -> assertTrue(Files.isRegularFile(path.resolve("nodes"))));
assertEquals(nodeId, client().admin().cluster().prepareState().get().getState().nodes().getMasterNodeId());
assertTrue(indexExists("test"));
ensureYellow("test");
Expand Down
28 changes: 19 additions & 9 deletions server/src/main/java/org/elasticsearch/env/NodeEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,21 @@
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
import org.elasticsearch.core.CheckedFunction;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.common.Randomness;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.common.UUIDs;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.common.io.FileSystemUtils;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.core.CheckedFunction;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.gateway.MetadataStateFormat;
import org.elasticsearch.gateway.PersistedClusterStateService;
Expand All @@ -55,6 +55,7 @@
import java.io.Closeable;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.AtomicMoveNotSupportedException;
import java.nio.file.DirectoryStream;
import java.nio.file.FileStore;
Expand Down Expand Up @@ -269,6 +270,16 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce
assertCanWrite();
}

// versions 7.x and earlier put their data under ${path.data}/nodes/; leave a file at that location to prevent downgrades
final Path legacyNodesPath = environment.dataFile().resolve("nodes");
if (Files.isRegularFile(legacyNodesPath) == false) {
final String content = "written by Elasticsearch v" + Version.CURRENT +
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should follow-up with a 7.16 change to log the first 256 bytes of the file if nodes is a file and not a directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh yeah I suppose we could at least help 7.16 users here :)

" to prevent a downgrade to a version prior to v8.0.0 which would result in data loss";
Files.write(legacyNodesPath, content.getBytes(StandardCharsets.UTF_8));
IOUtils.fsync(legacyNodesPath, false);
IOUtils.fsync(environment.dataFile(), true);
}

if (DiscoveryNode.canContainData(settings) == false) {
if (DiscoveryNode.isMasterNode(settings) == false) {
ensureNoIndexMetadata(nodePath);
Expand Down Expand Up @@ -408,12 +419,11 @@ private static boolean upgradeLegacyNodeFolders(Logger logger, Settings settings
IOUtils.fsync(nodePath.path, true);
});

// now do the actual upgrade. start by upgrading the node metadata file before moving anything, since a downgrade in an
// intermediate state would be pretty disastrous
loadNodeMetadata(settings, logger, legacyNodeLock.getNodePath());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't actually do anything any more, the node metadata is in the persisted cluster state which doesn't get written until much later.

// now do the actual upgrade
for (CheckedRunnable<IOException> upgradeAction : upgradeActions) {
upgradeAction.run();
}

} finally {
legacyNodeLock.close();
}
Expand Down
30 changes: 19 additions & 11 deletions server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.test.NodeRoles;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
Expand All @@ -41,6 +42,7 @@
import static org.elasticsearch.test.NodeRoles.nonDataNode;
import static org.elasticsearch.test.NodeRoles.nonMasterNode;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.startsWith;
Expand Down Expand Up @@ -439,6 +441,23 @@ public void testEnsureNoShardDataOrIndexMetadata() throws IOException {
verifyFailsOnShardData(noDataNoMasterSettings, indexPath, shardDataDirName);
}

public void testBlocksDowngradeToVersionWithMultipleNodesInDataPath() throws IOException {
final Settings settings = buildEnvSettings(Settings.EMPTY);
for (int i = 0; i < 2; i++) { // ensure the file gets created again if missing
try (NodeEnvironment env = newNodeEnvironment(settings)) {
final Path nodesPath = env.nodeDataPath().resolve("nodes");
assertTrue(Files.isRegularFile(nodesPath));
assertThat(
Files.readString(nodesPath, StandardCharsets.UTF_8),
allOf(
containsString("written by Elasticsearch"),
containsString("prevent a downgrade"),
containsString("data loss")));
Files.delete(nodesPath);
}
}
}

private void verifyFailsOnShardData(Settings settings, Path indexPath, String shardDataDirName) {
IllegalStateException ex = expectThrows(IllegalStateException.class,
"Must fail creating NodeEnvironment on a data path that has shard data if node does not have data role",
Expand All @@ -459,17 +478,6 @@ private void verifyFailsOnMetadata(Settings settings, Path indexPath) {
assertThat(ex.getMessage(), startsWith("node does not have the data and master roles but has index metadata"));
}

/**
* Converts an array of Strings to an array of Paths, adding an additional child if specified
*/
private Path[] stringsToPaths(String[] strings, String additional) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, but unused.

Path[] locations = new Path[strings.length];
for (int i = 0; i < strings.length; i++) {
locations[i] = PathUtils.get(strings[i], additional);
}
return locations;
}

@Override
public NodeEnvironment newNodeEnvironment() throws IOException {
return newNodeEnvironment(Settings.EMPTY);
Expand Down
5 changes: 5 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/cluster/DiskUsageIntegTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.DirectoryStream;
import java.nio.file.FileStore;
import java.nio.file.FileSystem;
Expand Down Expand Up @@ -148,6 +149,10 @@ public long getUnallocatedSpace() throws IOException {

private static long getTotalFileSize(Path path) throws IOException {
if (Files.isRegularFile(path)) {
if (path.getFileName().toString().equals("nodes")
&& Files.readString(path, StandardCharsets.UTF_8).contains("prevent a downgrade")) {
return 0;
}
try {
return Files.size(path);
} catch (NoSuchFileException | FileNotFoundException e) {
Expand Down