sashapolo commented on code in PR #1569:
URL: https://github.com/apache/ignite-3/pull/1569#discussion_r1119773213
##########
modules/api/src/main/java/org/apache/ignite/Ignition.java:
##########
@@ -168,8 +170,15 @@ CompletableFuture<Ignite> start(
* @param metaStorageNodeNames Names of nodes that will host the Meta
Storage.
* @param cmgNodeNames Names of nodes that will host the CMG.
* @param clusterName Human-readable name of the cluster.
+ * @param authenticationConfig REST authentication configuration.
* @throws IgniteException If the given node has not been started or has
been stopped.
* @see <a
href="https://cwiki.apache.org/confluence/display/IGNITE/IEP-77%3A+Node+Join+Protocol+and+Initialization+for+Ignite+3";>IEP-77</a>
*/
- void init(String nodeName, Collection<String> metaStorageNodeNames,
Collection<String> cmgNodeNames, String clusterName);
+ void init(
+ String nodeName,
Review Comment:
Looks like this list of parameters is becoming too large. I think we need to
replace it with an object, that can be constructed using a builder. This way we
will also be able to use some defaults, like disabled authentication and
`cmgNodeNames` being equal to `metaStorageNodeNames`
##########
modules/api/src/main/java/org/apache/ignite/IgnitionManager.java:
##########
@@ -137,22 +138,28 @@ public static void stop(String nodeName, @Nullable
ClassLoader clsLdr) {
* @param nodeName Name of the node that the initialization request will
be sent to.
* @param metaStorageNodeNames names of nodes that will host the Meta
Storage and the CMG.
* @param clusterName Human-readable name of the cluster.
+ * @param restAuthenticationConfig REST authentication configuration that
will be applied after the initialization.
* @throws IgniteException If the given node has not been started or has
been stopped.
* @throws NullPointerException If any of the parameters are null.
* @throws IllegalArgumentException If {@code metaStorageNodeNames} is
empty or contains blank strings.
* @throws IllegalArgumentException If {@code clusterName} is blank.
- * @see Ignition#init(String, Collection, String)
+ * @see Ignition#init(String, Collection, String, RestAuthenticationConfig)
*/
- public static synchronized void init(String nodeName, Collection<String>
metaStorageNodeNames, String clusterName) {
+ public static synchronized void init(
+ String nodeName,
Review Comment:
Same here, let's replace these parameters with a class
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -796,6 +862,8 @@ private CompletableFuture<CmgRaftService>
raftServiceAfterJoin() {
});
}
+
Review Comment:
Looks like some unneeded changes
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/rest/RestTestBase.java:
##########
@@ -40,6 +41,7 @@
* Cluster management REST test.
*/
@MicronautTest
+@ExtendWith(ConfigurationExtension.class)
Review Comment:
Why do you need this extension here? This class doesn't use configuration
anywhere
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -366,6 +391,47 @@ private void onElectedAsLeader(long term) {
LOG.warn("Error when executing onLeaderElected
callback", e);
}
});
+
+
raftServiceAfterJoin().thenCompose(this::pushAuthenticationConfigToCluster);
+
Review Comment:
Redundant space
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -366,6 +391,47 @@ private void onElectedAsLeader(long term) {
LOG.warn("Error when executing onLeaderElected
callback", e);
}
});
+
+
raftServiceAfterJoin().thenCompose(this::pushAuthenticationConfigToCluster);
+
+ }
+
+ private CompletableFuture<Void>
pushAuthenticationConfigToCluster(CmgRaftService service) {
+ return service.readClusterState()
+ .thenCompose(state -> {
+ 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");
+ return completedFuture(null);
+ } else {
+ LOG.info("REST auth configuration found in the Raft
storage, going to apply it");
+ RestAuthentication restAuthToApply =
state.restAuthToApply();
+ return
distributedConfigurationUpdater.updateRestAuthConfiguration(toRestAuthenticationConfig(restAuthToApply))
Review Comment:
I can see that `toRestAuthenticationConfig` is only needed to pass
`RestAuthenticationConfig` to `DistributedConfigurationUpdater`. Maybe we
should pass the `RestAuthentication` directly to the
`DistributedConfigurationUpdater`? (This way we can also get rid of both
`RestAuthenticationConfig` and `RestAuthConverter` classes)
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/auth/AuthProviderFactory.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.rest.auth;
+
+import io.micronaut.context.annotation.Bean;
+import io.micronaut.context.annotation.Factory;
+import io.micronaut.security.authentication.AuthenticationProvider;
+import jakarta.inject.Singleton;
+import org.apache.ignite.internal.rest.RestFactory;
+import org.apache.ignite.internal.rest.configuration.AuthConfiguration;
+
+/**
+ * Factory that creates beans that are needed for authentication.
+ */
+@Factory
+public class AuthProviderFactory implements RestFactory {
+
+ private final DelegatingAuthenticationProvider authenticationProvider;
+
+ public AuthProviderFactory(AuthConfiguration authConfiguration) {
+ this.authenticationProvider = new DelegatingAuthenticationProvider();
Review Comment:
okay!
##########
modules/rest/src/main/java/org/apache/ignite/internal/rest/configuration/AuthProviderConfigurationSchema.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.rest.configuration;
+
+import org.apache.ignite.configuration.annotation.PolymorphicConfig;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
+import org.apache.ignite.configuration.annotation.Value;
+
+/** Auth provider configuration. */
+@PolymorphicConfig
+public class AuthProviderConfigurationSchema {
+
+ public static final String TYPE_BASIC = "basic";
+
+ /** Auth type. */
+ @PolymorphicId
+ public String type;
+
+
+ @Value
+ public String name;
Review Comment:
But you don't need these names, maybe they can be generated automatically?
--
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]
