This is an automated email from the ASF dual-hosted git repository.
apkhmv pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/ignite-3.git
The following commit(s) were added to refs/heads/main by this push:
new 3d2b36504d IGNITE-19315 Validate node configuration on node start
(#2058)
3d2b36504d is described below
commit 3d2b36504dc6cf76827fbcc926bd7b99ef17f845
Author: Ivan Gagarkin <[email protected]>
AuthorDate: Tue May 16 16:28:36 2023 +0400
IGNITE-19315 Validate node configuration on node start (#2058)
---
.../configuration/ConfigurationChanger.java | 32 ++++---
.../configuration/ConfigurationChangerTest.java | 17 ++--
.../configuration/ConfigurationRegistryTest.java | 12 ++-
.../storage/TestConfigurationStorage.java | 5 +
.../java/org/apache/ignite/lang/ErrorGroups.java | 5 +
.../ItNodeBootstrapConfigurationTest.java | 103 +++++++++++++++++++++
.../ItSslConfigurationValidationTest.java | 2 -
.../configuration/NodeConfigParseException.java | 31 +++++++
.../storage/LocalFileConfigurationStorage.java | 8 +-
9 files changed, 191 insertions(+), 24 deletions(-)
diff --git
a/modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
b/modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
index efed03a968..09ced7d79d 100644
---
a/modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
+++
b/modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
@@ -242,8 +242,7 @@ public abstract class ConfigurationChanger implements
DynamicConfigurationChange
/**
* Start component.
*/
- // ConfigurationChangeException, really?
- public void start() throws ConfigurationChangeException {
+ public void start() {
Data data;
try {
@@ -275,6 +274,9 @@ public abstract class ConfigurationChanger implements
DynamicConfigurationChange
//Workaround for distributed configuration.
addDefaults(superRoot);
+ // Validate the restored configuration.
+ validateConfiguration(new SuperRoot(rootCreator()), superRoot);
+
storageRoots = new StorageRoots(superRoot, data.changeId());
storage.registerConfigurationListener(configurationStorageListener());
@@ -588,17 +590,7 @@ public abstract class ConfigurationChanger implements
DynamicConfigurationChange
dropNulls(changes);
- List<ValidationIssue> validationIssues = ValidationUtil.validate(
- curRoots,
- changes,
- this::getRootNode,
- cachedAnnotations,
- validators
- );
-
- if (!validationIssues.isEmpty()) {
- throw new ConfigurationValidationException(validationIssues);
- }
+ validateConfiguration(curRoots, changes);
// "allChanges" map can be empty here in case the given update
matches the current state of the local configuration. We
// still try to write the empty update, because local
configuration can be obsolete. If this is the case, then the CAS will
@@ -619,6 +611,20 @@ public abstract class ConfigurationChanger implements
DynamicConfigurationChange
}
}
+ private void validateConfiguration(SuperRoot curRoots, SuperRoot changes) {
+ List<ValidationIssue> validationIssues = ValidationUtil.validate(
+ curRoots,
+ changes,
+ this::getRootNode,
+ cachedAnnotations,
+ validators
+ );
+
+ if (!validationIssues.isEmpty()) {
+ throw new ConfigurationValidationException(validationIssues);
+ }
+ }
+
private ConfigurationStorageListener configurationStorageListener() {
return new ConfigurationStorageListener() {
@Override
diff --git
a/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
b/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
index 38bea01769..b48b8a308c 100644
---
a/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
+++
b/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
@@ -23,13 +23,14 @@ import static java.util.concurrent.TimeUnit.SECONDS;
import static
org.apache.ignite.configuration.annotation.ConfigurationType.LOCAL;
import static org.apache.ignite.internal.configuration.FirstConfiguration.KEY;
import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.anEmptyMap;
import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.emptyString;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;
@@ -95,12 +96,12 @@ public class ConfigurationChangerTest {
*/
@Config
public static class SecondConfigurationSchema {
- @Value
+ @Value(hasDefault = true)
@Immutable
- public int intCfg;
+ public int intCfg = 0;
- @Value
- public String strCfg;
+ @Value(hasDefault = true)
+ public String strCfg = "";
}
/**
@@ -212,7 +213,9 @@ public class ConfigurationChangerTest {
/** {@inheritDoc} */
@Override
public void validate(MaybeInvalid annotation,
ValidationContext<Object> ctx) {
- ctx.addIssue(new ValidationIssue("key", "foo"));
+ if (ctx.getNewValue().equals("2")) {
+ ctx.addIssue(new ValidationIssue("key", "foo"));
+ }
}
};
@@ -274,7 +277,7 @@ public class ConfigurationChangerTest {
FirstView newRoot = (FirstView) changer.getRootNode(KEY);
assertNotNull(newRoot.child());
- assertNull(newRoot.child().strCfg());
+ assertThat(newRoot.child().strCfg(), is(emptyString()));
}
/**
diff --git
a/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java
b/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java
index bb6ca661ff..e7148eeb1c 100644
---
a/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java
+++
b/modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java
@@ -28,7 +28,9 @@ import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.io.Serializable;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.function.Consumer;
@@ -137,10 +139,18 @@ public class ConfigurationRegistryTest {
@Test
void testComplicatedPolymorphicConfig() throws Exception {
+
+ Map<String, Serializable> bootstrapConfig = Map.of(
+ "sixth.entity.poly.strVal", "val",
+ "sixth.poly.strVal", "val",
+ "sixth.entity.poly.intVal", 1,
+ "sixth.poly.intVal", 1
+ );
+
ConfigurationRegistry registry = new ConfigurationRegistry(
List.of(SixthRootConfiguration.KEY),
Set.of(),
- new TestConfigurationStorage(LOCAL),
+ new TestConfigurationStorage(LOCAL, bootstrapConfig),
List.of(),
List.of(Fourth0PolymorphicConfigurationSchema.class)
);
diff --git
a/modules/configuration/src/testFixtures/java/org/apache/ignite/internal/configuration/storage/TestConfigurationStorage.java
b/modules/configuration/src/testFixtures/java/org/apache/ignite/internal/configuration/storage/TestConfigurationStorage.java
index 997026312c..37aa61facc 100644
---
a/modules/configuration/src/testFixtures/java/org/apache/ignite/internal/configuration/storage/TestConfigurationStorage.java
+++
b/modules/configuration/src/testFixtures/java/org/apache/ignite/internal/configuration/storage/TestConfigurationStorage.java
@@ -54,7 +54,12 @@ public class TestConfigurationStorage implements
ConfigurationStorage {
* @param type Configuration type.
*/
public TestConfigurationStorage(ConfigurationType type) {
+ this(type, Map.of());
+ }
+
+ public TestConfigurationStorage(ConfigurationType type, Map<String,
Serializable> data) {
configurationType = type;
+ map.putAll(data);
}
@Override
diff --git a/modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java
b/modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java
index f10da443ed..1f5cbaa0f9 100755
--- a/modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java
+++ b/modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java
@@ -374,6 +374,11 @@ public class ErrorGroups {
* Config write error.
*/
public static final int CONFIG_WRITE_ERR =
NODE_CONFIGURATION_ERR_GROUP.registerErrorCode(3);
+
+ /**
+ * Config parse error.
+ */
+ public static final int CONFIG_PARSE_ERR =
NODE_CONFIGURATION_ERR_GROUP.registerErrorCode(4);
}
/**
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeBootstrapConfigurationTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeBootstrapConfigurationTest.java
new file mode 100644
index 0000000000..675c08133c
--- /dev/null
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeBootstrapConfigurationTest.java
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration;
+
+import static
org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static
org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Integration tests for the node bootstrap configuration.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeBootstrapConfigurationTest {
+
+ @WorkDirectory
+ private Path workDir;
+
+ @Test
+ public void wrongConfigurationFormat(TestInfo testInfo) {
+ String config = UUID.randomUUID().toString();
+
+ IgniteException igniteException = assertThrows(
+ IgniteException.class,
+ () -> TestIgnitionManager.start(testNodeName(testInfo, 0),
config, workDir)
+ );
+ assertThat(igniteException.getCause(),
is(instanceOf(NodeConfigParseException.class)));
+ assertThat(
+ igniteException.getCause().getMessage(),
+ containsString("Failed to parse config content from file " +
workDir + "/ignite-config.conf")
+ );
+ }
+
+ @Test
+ public void illegalConfigurationValueType(TestInfo testInfo) {
+ String config =
+ "{\n"
+ + " rest: {\n"
+ + " ssl: {\n"
+ + " enabled: true,\n"
+ + " clientAuth: none,\n"
+ + " keyStore: {\n"
+ + " path: 123\n"
+ + " }\n"
+ + " }\n"
+ + " }\n"
+ + "}";
+
+ assertThrowsWithCause(
+ () -> TestIgnitionManager.start(testNodeName(testInfo, 0),
config, workDir),
+ IllegalArgumentException.class,
+ "'String' is expected as a type for the
'rest.ssl.keyStore.path' configuration value");
+ }
+
+ @Test
+ public void illegalConfigurationValue(TestInfo testInfo) {
+ String config = "{\n"
+ + " rest: {\n"
+ + " ssl: {\n"
+ + " enabled: true,\n"
+ + " clientAuth: none,\n"
+ + " keyStore: {\n"
+ + " path: bad_path\n"
+ + " }\n"
+ + " }\n"
+ + " }\n"
+ + "}";
+
+ assertThrowsWithCause(
+ () -> TestIgnitionManager.start(testNodeName(testInfo, 0),
config, workDir),
+ ConfigurationValidationException.class,
+ "Validation did not pass for keys: [rest.ssl.keyStore, Key
store file doesn't exist at bad_path]");
+ }
+}
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItSslConfigurationValidationTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItSslConfigurationValidationTest.java
index db1144c138..36b858f33d 100644
---
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItSslConfigurationValidationTest.java
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItSslConfigurationValidationTest.java
@@ -25,7 +25,6 @@ import
org.apache.ignite.configuration.validation.ConfigurationValidationExcepti
import org.apache.ignite.internal.testframework.TestIgnitionManager;
import org.apache.ignite.internal.testframework.WorkDirectory;
import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
@@ -35,7 +34,6 @@ import org.junit.jupiter.params.provider.ValueSource;
* Integration test for checking SSL configuration validation.
*/
@ExtendWith(WorkDirectoryExtension.class)
-@Disabled("https://issues.apache.org/jira/browse/IGNITE-19315";)
public class ItSslConfigurationValidationTest {
@ParameterizedTest
@ValueSource(strings = {"clientConnector", "network", "rest"})
diff --git
a/modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java
b/modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java
new file mode 100644
index 0000000000..0e9fc495c7
--- /dev/null
+++
b/modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration;
+
+import org.apache.ignite.lang.ErrorGroups.NodeConfiguration;
+import org.apache.ignite.lang.IgniteException;
+
+/**
+ * Exception that gets thrown when a node bootstrap configuration file is
malformed.
+ */
+public class NodeConfigParseException extends IgniteException {
+
+ public NodeConfigParseException(String msg, Throwable cause) {
+ super(NodeConfiguration.CONFIG_PARSE_ERR, msg, cause);
+ }
+}
diff --git
a/modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java
b/modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java
index dee1865dd9..b52d5715a3 100644
---
a/modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java
+++
b/modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java
@@ -23,6 +23,7 @@ import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.fi
import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.toPrefixMap;
import com.typesafe.config.Config;
+import com.typesafe.config.ConfigException;
import com.typesafe.config.ConfigFactory;
import com.typesafe.config.ConfigObject;
import com.typesafe.config.ConfigParseOptions;
@@ -49,6 +50,7 @@ import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.ignite.configuration.annotation.ConfigurationType;
import org.apache.ignite.internal.configuration.ConfigurationTreeGenerator;
import org.apache.ignite.internal.configuration.NodeConfigCreateException;
+import org.apache.ignite.internal.configuration.NodeConfigParseException;
import org.apache.ignite.internal.configuration.NodeConfigWriteException;
import org.apache.ignite.internal.configuration.SuperRoot;
import org.apache.ignite.internal.configuration.hocon.HoconConverter;
@@ -134,7 +136,11 @@ public class LocalFileConfigurationStorage implements
ConfigurationStorage {
private Config readHoconFromFile() {
checkAndRestoreConfigFile();
- return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
+ try {
+ return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
+ } catch (ConfigException.Parse e) {
+ throw new NodeConfigParseException("Failed to parse config content
from file " + configPath, e);
+ }
}
@Override
