You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ibessonov (via GitHub)" <gi...@apache.org> on 2023/05/02 10:49:39 UTC
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1991: IGNITE-19077 Apply cutom cluster config on cluster init
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org