ibessonov commented on code in PR #1991:
URL: https://github.com/apache/ignite-3/pull/1991#discussion_r1182336990
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlConnectBasicTest.java:
##########
@@ -21,20 +21,30 @@
import static
org.apache.ignite.internal.cli.commands.cliconfig.TestConfigManagerHelper.createJdbcTestsBasicSecretConfig;
import static org.junit.jupiter.api.Assertions.assertAll;
-import java.util.List;
import org.apache.ignite.InitParametersBuilder;
-import org.apache.ignite.security.AuthenticationConfig;
-import org.apache.ignite.security.BasicAuthenticationProviderConfig;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
class ItSqlConnectBasicTest extends CliSqlConnectCommandTestBase {
@Override
protected void configureInitParameters(InitParametersBuilder builder) {
- builder.authenticationConfig(new AuthenticationConfig(
- true,
- List.of(new BasicAuthenticationProviderConfig("basic", "usr",
"pwd")))
+ builder.clusterConfiguration(
+ "{\n"
+ + " \"security\": {\n"
Review Comment:
You don't have to use enclosing braces and escaped quotes. This is HOCON,
not JSON, this string can be drastically simplified.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlConnectSslBasicTest.java:
##########
@@ -38,9 +35,22 @@ protected String nodeBootstrapConfigTemplate() {
@Override
protected void configureInitParameters(InitParametersBuilder builder) {
- builder.authenticationConfig(new AuthenticationConfig(
- true,
- List.of(new BasicAuthenticationProviderConfig("basic", "usr",
"pwd")))
+ builder.clusterConfiguration(
+ "{\n"
Review Comment:
Same here. By the way, I see that this string matches the content of the
file. What's the reason for copy-paste?
Can we use a single source in similar tests?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -415,15 +412,13 @@ private CompletableFuture<Void>
pushAuthenticationConfigToCluster(CmgRaftService
if (state == null) {
LOG.info("No CMG state found in the Raft storage");
return completedFuture(null);
- } else if (state.restAuthToApply() == null) {
- // auth config has already been successfully pushed to
the distributed configuration
- LOG.info("No REST auth configuration found in the Raft
storage");
+ } else if (state.clusterConfigurationToApply() == null) {
+ // config was applied or wasn't provided
+ LOG.info("No cluster configuration found in the Raft
storage");
Review Comment:
What's a `Raft storage`? I'm not sure that this terminology is correct
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterState.java:
##########
@@ -64,7 +63,7 @@ default IgniteProductVersion igniteVersion() {
}
/**
- * Returns a REST authentication configuration that should be applied.
+ * Returns a cluster configuration that should be applied.
*/
- Authentication restAuthToApply();
+ String clusterConfigurationToApply();
Review Comment:
Why is this a part of cluster state?
"toApply" is a confusing naming. Who applies it? When? Javadoc doesn't help,
I can't understand this code.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -415,15 +412,13 @@ private CompletableFuture<Void>
pushAuthenticationConfigToCluster(CmgRaftService
if (state == null) {
LOG.info("No CMG state found in the Raft storage");
return completedFuture(null);
- } else if (state.restAuthToApply() == null) {
- // auth config has already been successfully pushed to
the distributed configuration
- LOG.info("No REST auth configuration found in the Raft
storage");
+ } else if (state.clusterConfigurationToApply() == null) {
Review Comment:
I think that the method `pushAuthenticationConfigToCluster` deserves
renaming too
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -415,15 +412,13 @@ private CompletableFuture<Void>
pushAuthenticationConfigToCluster(CmgRaftService
if (state == null) {
LOG.info("No CMG state found in the Raft storage");
return completedFuture(null);
- } else if (state.restAuthToApply() == null) {
- // auth config has already been successfully pushed to
the distributed configuration
- LOG.info("No REST auth configuration found in the Raft
storage");
+ } else if (state.clusterConfigurationToApply() == null) {
+ // config was applied or wasn't provided
Review Comment:
```suggestion
// Config was applied or wasn't provided.
```
##########
modules/cli/src/integrationTest/resources/cluster-configuration-with-enabled-auth.json:
##########
@@ -0,0 +1,15 @@
+{
Review Comment:
I think we should call this file "***.conf", like we do for local
configuration
##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/IgniteCliInterfaceTest.java:
##########
@@ -525,4 +525,10 @@ private void assertOutputEqual(String exp) {
private void assertErrOutputEqual(String exp) {
assertEqualsIgnoreLineSeparators(exp, err.toString(UTF_8));
}
+
+ private String readFile(File file) throws IOException {
+ try (Stream<String> lines = Files.lines(file.toPath())) {
+ return lines.collect(Collectors.joining(System.lineSeparator()));
Review Comment:
I'm just as confused here
##########
modules/cli/src/test/resources/cluster-configuration-with-enabled-auth.json:
##########
@@ -0,0 +1,15 @@
+{
Review Comment:
Please rename the file and get rid of quotes
##########
modules/api/src/main/java/org/apache/ignite/InitParametersBuilder.java:
##########
@@ -93,19 +97,29 @@ public InitParametersBuilder clusterName(String
clusterName) {
}
/**
- * Sets authentication configuration, that will be applied after
initialization. If not set, the authentication will be disabled.
+ * Sets cluster configuration, that will be applied after initialization.
*
- * @param authenticationConfig Authentication configuration.
+ * @param clusterConfiguration Cluster configuration.
* @return {@code this} for chaining.
*/
- public InitParametersBuilder authenticationConfig(AuthenticationConfig
authenticationConfig) {
- if (authenticationConfig == null) {
- throw new IllegalArgumentException("Authentication config cannot
be null.");
- }
- this.authenticationConfig = authenticationConfig;
+ public InitParametersBuilder clusterConfiguration(String
clusterConfiguration) {
+ this.clusterConfiguration = clusterConfiguration;
return this;
}
+ /**
+ * Sets cluster configuration, that will be applied after initialization.
+ *
+ * @param clusterConfiguration Cluster configuration.
+ * @return {@code this} for chaining.
+ */
+ public InitParametersBuilder clusterConfiguration(File
clusterConfiguration) throws IOException {
+ try (Stream<String> lines =
Files.lines(clusterConfiguration.toPath())) {
+ this.clusterConfiguration =
lines.collect(Collectors.joining(System.lineSeparator()));
Review Comment:
Ok, maybe I don't understand something. What's the reason of splitting the
file into lines and then concatenating them back?
Wouldn't it be easier to just read the entire file as a string? I'm so
confused.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/cluster/init/ClusterInitOptions.java:
##########
@@ -55,22 +68,64 @@ public class ClusterInitOptions {
@Option(names = {CLUSTER_NAME_OPTION, CLUSTER_NAME_OPTION_SHORT}, required
= true, description = CLUSTER_NAME_OPTION_DESC)
private String clusterName;
- @Mixin
- private AuthenticationOptions authenticationOptions;
+ @ArgGroup
+ private ClusterConfigOptions clusterConfigOptions;
+ private static class ClusterConfigOptions {
+ @Option(names = {CLUSTER_CONFIG_OPTION, CLUSTER_CONFIG_OPTION_SHORT},
description = CLUSTER_CONFIG_OPTION_DESC)
+ private String config;
+
+ @Option(names = {CLUSTER_CONFIG_FILE_OPTION,
CLUSTER_CONFIG_FILE_OPTION_SHORT}, description =
CLUSTER_CONFIG_FILE_OPTION_DESC)
+ private File file;
+ }
+
+ /**
+ * Consistent IDs of the nodes that will host the Meta Storage.
+ *
+ * @return Meta storage node ids.
+ */
public List<String> metaStorageNodes() {
return metaStorageNodes;
}
+ /**
+ * Consistent IDs of the nodes that will host the Cluster Management
Group; if empty,
+ * {@code metaStorageNodeIds} will be used to host the CMG as well.
+ *
+ * @return Cluster management node ids.
+ */
public List<String> cmgNodes() {
return cmgNodes;
}
+ /**
+ * Human-readable name of the cluster.
+ *
+ * @return Cluster name.
+ */
public String clusterName() {
return clusterName;
}
- public AuthenticationOptions authenticationOptions() {
- return authenticationOptions;
+ /**
+ * Cluster configuration.
+ *
+ * @return Cluster configuration.
+ */
+ @Nullable
+ public String clusterConfiguration() {
+ if (clusterConfigOptions == null) {
+ return null;
+ } else if (clusterConfigOptions.config != null) {
+ return clusterConfigOptions.config;
+ } else if (clusterConfigOptions.file != null) {
+ try (Stream<String> lines =
Files.lines(clusterConfigOptions.file.toPath())) {
Review Comment:
Can't we store `Path` in cluster config options? Why is there a File?
Just a question, you don't have to fix it
##########
modules/configuration-presentation/build.gradle:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+
+apply from: "$rootDir/buildscripts/java-core.gradle"
+apply from: "$rootDir/buildscripts/publishing.gradle"
+apply from: "$rootDir/buildscripts/java-junit5.gradle"
+apply from: "$rootDir/buildscripts/java-test-fixtures.gradle"
+
+description = 'ignite-configuration'
Review Comment:
Please fix the description
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/DistributedConfigurationUpdater.java:
##########
@@ -38,56 +33,31 @@ public class DistributedConfigurationUpdater implements
IgniteComponent {
private static final IgniteLogger LOG =
Loggers.forClass(DistributedConfigurationUpdater.class);
+ private final CompletableFuture<ConfigurationPresentation<String>>
clusterCfgPresentation = new CompletableFuture<>();
private final CompletableFuture<SecurityConfiguration>
securityConfigurationFuture = new CompletableFuture<>();
- public void setClusterRestConfiguration(SecurityConfiguration
securityConfiguration) {
- securityConfigurationFuture.complete(securityConfiguration);
+ public void
setDistributedConfigurationPresentation(ConfigurationPresentation<String>
presentation) {
Review Comment:
This method looks very much like a cycle in dependencies.
Maybe we should think twice about how the code is organized. Right now it
seems a little messy.
--
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]
