ibessonov commented on code in PR #2684:
URL: https://github.com/apache/ignite-3/pull/2684#discussion_r1360214884
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/rest/ItGeneratedRestClientTest.java:
##########
@@ -286,12 +286,26 @@ void updateTheSameNodeConfiguration() {
void updateNodeConfigurationWithInvalidParam() throws
JsonProcessingException {
ApiException thrown = assertThrows(
ApiException.class,
- () ->
clusterConfigurationApi.updateClusterConfiguration("security.authentication.enabled=true")
+ () -> clusterConfigurationApi.updateClusterConfiguration("{\n"
Review Comment:
Why so many quotes? It's HOCON, you don't need them
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetterTest.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import java.util.List;
+import
org.apache.ignite.internal.security.authentication.SecurityConfigurationModule;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class ClusterConfigurationDefaultsSetterTest {
+ private ConfigurationTreeGenerator generator;
+
+ private ClusterConfigurationDefaultsSetter setter;
+
+ @BeforeEach
+ void setUp() {
+ SecurityConfigurationModule module = new SecurityConfigurationModule();
+
+ generator = new ConfigurationTreeGenerator(
+ module.rootKeys(),
+ module.schemaExtensions(),
+ module.polymorphicSchemaExtensions()
+ );
+
+ setter = new ClusterConfigurationDefaultsSetter(module.rootKeys(),
generator);
+
+ setter.start();
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ setter.stop();
+ generator.close();
+ }
+
+ @Test
+ void setDefaultUserIfProvidersIsEmpty() {
+ String defaults = setter.setDefaults("").join();
+
+ Config config = ConfigFactory.parseString(defaults);
+
+ List<? extends Config> providers =
config.getConfigList("security.authentication.providers");
+
+ assertThat(providers.size(), equalTo(1));
+
+ Config defaultProvider = providers.get(0);
+
+ assertAll(
+ () -> assertThat(defaultProvider.getString("type"),
equalTo("basic")),
+ () -> assertThat(defaultProvider.getString("name"),
equalTo("default")),
+ () -> assertThat(defaultProvider.getString("username"),
equalTo("ignite")),
+ () -> assertThat(defaultProvider.getString("password"),
equalTo("ignite"))
+ );
+ }
+
+ @Test
+ void noSetDefaultUserIfProvidersIsNotEmpty() {
+ String defaults = setter.setDefaults(
+
"{security.authentication.providers:[{name:basic,password:password,type:basic,username:admin}]}")
+ .join();
+
+ Config config = ConfigFactory.parseString(defaults);
+
+ List<? extends Config> providers =
config.getConfigList("security.authentication.providers");
+
+ assertThat(providers.size(), equalTo(1));
+
+ Config defaultProvider = providers.get(0);
+
+ assertAll(
Review Comment:
Can it be 4 different lines? What's the reason for having the "assertAll",
looks inappropriate.
There are other instances of the same pattern as well, I wonder why
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -50,11 +55,20 @@ public class ClusterInitializer {
private final ClusterService clusterService;
+ private final ConfigurationDefaultsSetter configurationDefaultsSetter;
+
+ private final ConfigurationValidator clusterConfigurationValidator;
private final CmgMessagesFactory msgFactory = new CmgMessagesFactory();
Review Comment:
Formatting is broken.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -122,44 +136,57 @@ public CompletableFuture<Void> initCluster(
LOG.info("Resolved CMG nodes[nodes={}]", cmgNodes);
- CmgInitMessage initMessage = msgFactory.cmgInitMessage()
- .metaStorageNodes(msNodeNameSet)
- .cmgNodes(cmgNodeNameSet)
- .clusterName(clusterName)
- .clusterConfigurationToApply(clusterConfiguration)
- .build();
-
- return invokeMessage(cmgNodes, initMessage)
- .handle((v, e) -> {
- if (e == null) {
- LOG.info(
- "Cluster initialized [clusterName={},
cmgNodes={}, msNodes={}]",
- initMessage.clusterName(),
- initMessage.cmgNodes(),
- initMessage.metaStorageNodes()
- );
-
- return
CompletableFuture.<Void>completedFuture(null);
- } else {
- if (e instanceof CompletionException) {
- e = e.getCause();
- }
+ String initialClusterConfiguration = clusterConfiguration == null
+ ? ""
+ : clusterConfiguration;
Review Comment:
I find it strange that you must convert null to empty string, just to call a
private method, that does the same null-check. Something's wrong here, please
simplify your code.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/rest/ItNotInitializedClusterRestTest.java:
##########
@@ -210,7 +210,40 @@ void nodeVersion() throws IOException,
InterruptedException {
}
@Test
- @DisplayName("Cluster is not initialized, if config is invalid")
+ @DisplayName("Cluster is not initialized, if config has a syntax error")
+ void initClusterWithInvalidHoconConfig() throws IOException,
InterruptedException {
+ // When POST /management/v1/cluster/init with invalid config
+ String requestBody = "{\n"
+ + " \"metaStorageNodes\": [\n"
Review Comment:
Same comment about quotes. Let's make these tests more readable
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
Review Comment:
Another validation? Why?
Defaults must be covered with tests, there's no point in validating them on
every step. One time, before application, is enough
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -50,11 +55,20 @@ public class ClusterInitializer {
private final ClusterService clusterService;
+ private final ConfigurationDefaultsSetter configurationDefaultsSetter;
+
+ private final ConfigurationValidator clusterConfigurationValidator;
private final CmgMessagesFactory msgFactory = new CmgMessagesFactory();
/** Constructor. */
- public ClusterInitializer(ClusterService clusterService) {
+ public ClusterInitializer(
+ ClusterService clusterService,
+ ConfigurationDefaultsSetter configurationDefaultsSetter,
+ ConfigurationValidator clusterConfigurationValidator
Review Comment:
I don't see reasons to validate configuration here one more time. An
explanation in the comment somewhere would be helpful. Maybe we should remove it
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
+ configurationManager = new ConfigurationManager(
+ rootKeys,
+ new RuntimeConfigurationStorage(ConfigurationType.DISTRIBUTED),
+ generator,
+ configurationValidator
+ );
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public CompletableFuture<String> setDefaults(String hocon) {
+ ConverterToMapVisitor visitor = ConverterToMapVisitor.builder()
+ .includeInternal(true)
+ .maskSecretValues(false)
+ .build();
+
+ ConfigurationRegistry configurationRegistry =
configurationManager.configurationRegistry();
+
+ return validateCfg(hocon)
+ .thenCompose(v -> parseHocon(hocon))
+ .thenCompose(
+ v ->
setSecurityDefaults(configurationRegistry.getConfiguration(SecurityConfiguration.KEY))
+ )
+ .thenApply(
+ v -> HoconConverter.represent(configurationRegistry,
List.of(), visitor).render(ConfigRenderOptions.concise())
+ );
+ }
+
+ private CompletableFuture<Void> validateCfg(String hocon) {
+ try {
+ List<ValidationIssue> validationIssues =
configurationValidator.validateHocon(hocon);
+ if (validationIssues.isEmpty()) {
+ return CompletableFuture.completedFuture(null);
+ } else {
+ return failedFuture(new
ConfigurationValidationException(validationIssues));
+ }
+ } catch (Exception e) {
+ return failedFuture(new
ConfigurationValidationException(e.getMessage()));
+ }
+ }
+
+ private CompletableFuture<Void> parseHocon(String hocon) {
+ try {
+ Config config = ConfigFactory.parseString(hocon);
+ ConfigurationSource hoconSource =
HoconConverter.hoconSource(config.root());
+ ConfigurationRegistry configurationRegistry =
configurationManager.configurationRegistry();
+
+ return configurationRegistry.change(hoconSource);
+ } catch (Exception e) {
+ return failedFuture(e);
+ }
+ }
+
+ private static CompletableFuture<Void>
setSecurityDefaults(SecurityConfiguration securityCfg) {
+ if (securityCfg.authentication().providers().value().size() == 0) {
Review Comment:
Instead of updating `*Configuration` instance, you could update `*View`
instance. It's much simpler and doesn't require the storage.
Let's re-write this class a bit, I don't like current approach
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
+ configurationManager = new ConfigurationManager(
+ rootKeys,
+ new RuntimeConfigurationStorage(ConfigurationType.DISTRIBUTED),
+ generator,
+ configurationValidator
+ );
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public CompletableFuture<String> setDefaults(String hocon) {
+ ConverterToMapVisitor visitor = ConverterToMapVisitor.builder()
+ .includeInternal(true)
+ .maskSecretValues(false)
+ .build();
+
+ ConfigurationRegistry configurationRegistry =
configurationManager.configurationRegistry();
+
+ return validateCfg(hocon)
+ .thenCompose(v -> parseHocon(hocon))
+ .thenCompose(
+ v ->
setSecurityDefaults(configurationRegistry.getConfiguration(SecurityConfiguration.KEY))
Review Comment:
Instead of a hardcode, we may have asked corresponding configuration modules
to patch the tree.
Why have you decided to put these defaults into a modules that doesn't own
them?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/rest/ItNotInitializedClusterRestTest.java:
##########
@@ -210,7 +210,40 @@ void nodeVersion() throws IOException,
InterruptedException {
}
@Test
- @DisplayName("Cluster is not initialized, if config is invalid")
+ @DisplayName("Cluster is not initialized, if config has a syntax error")
+ void initClusterWithInvalidHoconConfig() throws IOException,
InterruptedException {
+ // When POST /management/v1/cluster/init with invalid config
Review Comment:
According to code style, we put a dot at the end.
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/cluster/ClusterManagementController.java:
##########
@@ -84,27 +74,15 @@ public CompletableFuture<Void> init(@Body InitCommand
initCommand) {
initCommand.cmgNodes());
}
- return validateConfiguration(initCommand.clusterConfiguration())
- .thenCompose(ignored -> clusterInitializer.initCluster(
- initCommand.metaStorageNodes(),
- initCommand.cmgNodes(),
- initCommand.clusterName(),
- initCommand.clusterConfiguration()
- ))
- .exceptionally(ex -> {
- throw mapException(ex);
- });
- }
-
- private CompletableFuture<Void> validateConfiguration(@Nullable String
configuration) {
- if (configuration != null) {
- List<ValidationIssue> validationIssues =
clusterCfgValidator.validateHocon(configuration);
- if (!validationIssues.isEmpty()) {
- return CompletableFuture.failedFuture(new
ConfigurationValidationException(validationIssues));
- }
- }
-
- return CompletableFuture.completedFuture(null);
+ //return validateConfiguration(initCommand.clusterConfiguration())
Review Comment:
Please remove this line
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.configuration;
+
+import java.util.concurrent.CompletableFuture;
+
+/**
+ * Configuration defaults setter. This interface is used to set the default
values for the cluster configuration that can not be set
+ * in the configuration itself.
+ */
+public interface ConfigurationDefaultsSetter {
+ /*
+ * Sets the default values for the cluster configuration. This method is
called when the cluster is started for the first time.
+ *
+ * @param hocon HOCON representation of the cluster configuration.
+ * @return Future that is completed when the default values are set.
+ */
+ CompletableFuture<String> setDefaults(String hocon);
Review Comment:
I would prefer another name. `set` usually means that we mutate the state of
the `this` instance, using argument as a source of changes. Here it's the
opposite, we update the argument using `this` as a source of changes.
It's also hard to understand what I said from the documentation, which is
confusing. Please rename the method and write better documentation.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/rest/ItNotInitializedClusterRestTest.java:
##########
@@ -210,7 +210,40 @@ void nodeVersion() throws IOException,
InterruptedException {
}
@Test
- @DisplayName("Cluster is not initialized, if config is invalid")
+ @DisplayName("Cluster is not initialized, if config has a syntax error")
+ void initClusterWithInvalidHoconConfig() throws IOException,
InterruptedException {
+ // When POST /management/v1/cluster/init with invalid config
Review Comment:
```suggestion
// When POST /management/v1/cluster/init with invalid config.
```
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -235,4 +262,20 @@ private static List<ClusterNode>
resolveNodes(ClusterService clusterService, Col
})
.collect(Collectors.toList());
}
+
+
+ private CompletableFuture<String>
setDefaultsToClusterConfiguration(@Nullable String hocon) {
Review Comment:
Does it really have to be a future? I don't think that there are any
asynchronous operations inside of the setter.
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/cluster/ClusterManagementController.java:
##########
@@ -133,7 +111,7 @@ private static RuntimeException mapException(Throwable ex) {
}
}
- private static Throwable unwrapExceptionCompletionException(Throwable ex) {
+ private static Throwable unwrapException(Throwable ex) {
Review Comment:
The same method exists in IgniteUtils I believe
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/cluster/ClusterManagementController.java:
##########
@@ -84,27 +74,15 @@ public CompletableFuture<Void> init(@Body InitCommand
initCommand) {
initCommand.cmgNodes());
}
- return validateConfiguration(initCommand.clusterConfiguration())
- .thenCompose(ignored -> clusterInitializer.initCluster(
- initCommand.metaStorageNodes(),
- initCommand.cmgNodes(),
- initCommand.clusterName(),
- initCommand.clusterConfiguration()
- ))
- .exceptionally(ex -> {
- throw mapException(ex);
- });
- }
-
- private CompletableFuture<Void> validateConfiguration(@Nullable String
configuration) {
- if (configuration != null) {
- List<ValidationIssue> validationIssues =
clusterCfgValidator.validateHocon(configuration);
- if (!validationIssues.isEmpty()) {
- return CompletableFuture.failedFuture(new
ConfigurationValidationException(validationIssues));
- }
- }
-
- return CompletableFuture.completedFuture(null);
+ //return validateConfiguration(initCommand.clusterConfiguration())
+ return clusterInitializer.initCluster(
+ initCommand.metaStorageNodes(),
+ initCommand.cmgNodes(),
+ initCommand.clusterName(),
+ initCommand.clusterConfiguration()
+ ).exceptionally(ex -> {
+ throw mapException(ex);
Review Comment:
A note for the future: I don't think that we have an adequate mapping for
configuration exceptions. Are there any tests for that?
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
+ configurationManager = new ConfigurationManager(
Review Comment:
Configuration registry is the only reason to create manager right?
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
+ configurationManager = new ConfigurationManager(
+ rootKeys,
+ new RuntimeConfigurationStorage(ConfigurationType.DISTRIBUTED),
+ generator,
+ configurationValidator
+ );
+ }
+
+ /**
+ * {@inheritDoc}
+ */
Review Comment:
Please remove it, we don't use these anymore
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/rest/authentication/ItAuthenticationTest.java:
##########
@@ -107,7 +107,38 @@ public void disabledAuthentication(TestInfo testInfo) {
}
@Test
- public void changeCredentials(TestInfo testInfo) throws
InterruptedException {
+ public void defaultUser(TestInfo testInfo) throws InterruptedException,
IOException {
+ // when
Review Comment:
Here you use absolutely different notation. Should be `// When.`, right? The
goal is to have uniformed approach
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
+ configurationManager = new ConfigurationManager(
Review Comment:
What was the problem with using existing configuration manager?
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/ClusterConfigurationDefaultsSetter.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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 java.util.concurrent.CompletableFuture.failedFuture;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+import com.typesafe.config.ConfigRenderOptions;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.configuration.ConfigurationDefaultsSetter;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import
org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.configuration.validation.ValidationIssue;
+import org.apache.ignite.internal.configuration.hocon.HoconConverter;
+import
org.apache.ignite.internal.configuration.storage.RuntimeConfigurationStorage;
+import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConverterToMapVisitor;
+import
org.apache.ignite.internal.configuration.validation.ConfigurationValidatorImpl;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import
org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
+import org.apache.ignite.internal.security.configuration.SecurityConfiguration;
+
+/**
+ * Default configuration setter for cluster configuration.
+ */
+public class ClusterConfigurationDefaultsSetter implements
ConfigurationDefaultsSetter, IgniteComponent {
+ private static final String DEFAULT_PROVIDER_NAME = "default";
+
+ private static final String DEFAULT_USERNAME = "ignite";
+
+ private static final String DEFAULT_PASSWORD = "ignite";
+
+ private final ConfigurationManager configurationManager;
+
+ private final ConfigurationValidatorImpl configurationValidator;
+
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Root keys.
+ * @param generator Configuration tree generator.
+ */
+ public ClusterConfigurationDefaultsSetter(
+ Collection<RootKey<?, ?>> rootKeys,
+ ConfigurationTreeGenerator generator
+ ) {
+ configurationValidator = new ConfigurationValidatorImpl(generator,
Set.of());
+ configurationManager = new ConfigurationManager(
+ rootKeys,
+ new RuntimeConfigurationStorage(ConfigurationType.DISTRIBUTED),
+ generator,
+ configurationValidator
+ );
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public CompletableFuture<String> setDefaults(String hocon) {
+ ConverterToMapVisitor visitor = ConverterToMapVisitor.builder()
+ .includeInternal(true)
+ .maskSecretValues(false)
+ .build();
+
+ ConfigurationRegistry configurationRegistry =
configurationManager.configurationRegistry();
+
+ return validateCfg(hocon)
+ .thenCompose(v -> parseHocon(hocon))
+ .thenCompose(
+ v ->
setSecurityDefaults(configurationRegistry.getConfiguration(SecurityConfiguration.KEY))
+ )
+ .thenApply(
+ v -> HoconConverter.represent(configurationRegistry,
List.of(), visitor).render(ConfigRenderOptions.concise())
+ );
+ }
+
+ private CompletableFuture<Void> validateCfg(String hocon) {
+ try {
+ List<ValidationIssue> validationIssues =
configurationValidator.validateHocon(hocon);
+ if (validationIssues.isEmpty()) {
+ return CompletableFuture.completedFuture(null);
+ } else {
+ return failedFuture(new
ConfigurationValidationException(validationIssues));
Review Comment:
Any specific reason for using futures here?
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/RuntimeConfigurationStorage.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.storage;
+
+import static java.util.concurrent.CompletableFuture.supplyAsync;
+import static java.util.stream.Collectors.toUnmodifiableMap;
+
+import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+
+/**
+ * Test configuration storage.
+ */
+public class RuntimeConfigurationStorage implements ConfigurationStorage {
Review Comment:
This class should not exist. Please delete it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
