sashapolo commented on code in PR #1569: URL: https://github.com/apache/ignite-3/pull/1569#discussion_r1122709652
########## modules/api/src/main/java/org/apache/ignite/InitParameters.java: ########## @@ -0,0 +1,89 @@ +/* + * 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; + +import java.util.Collection; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Initialization parameters. */ +public class InitParameters { + + /** Name of the node that the initialization request will be sent to. */ + private final String nodeName; Review Comment: Maybe it would be better to rename this field to `destinationNodeName` ########## modules/api/src/main/java/org/apache/ignite/InitParameters.java: ########## @@ -0,0 +1,89 @@ +/* + * 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; + +import java.util.Collection; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Initialization parameters. */ +public class InitParameters { + + /** Name of the node that the initialization request will be sent to. */ + private final String nodeName; + + /** Names of nodes that will host the Meta Storage <b>and</b> the CMG. */ + private final Collection<String> metaStorageNodeNames; + + /** Names of nodes that will host the CMG. */ + private final Collection<String> cmgNodeNames; + + /** Human-readable name of the cluster. */ + private final String clusterName; + + /** Authentication configuration, that will be applied after initialization. */ + private final AuthenticationConfig authenticationConfig; + + /** + * Constructor. + * + * @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 <b>and</b> the CMG. Review Comment: This javadoc is incorrect ########## modules/api/src/main/java/org/apache/ignite/InitParametersBuilder.java: ########## @@ -0,0 +1,70 @@ +/* + * 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; + +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Builder of {@link org.apache.ignite.InitParameters}. */ +public class InitParametersBuilder { + private String nodeName; + private Collection<String> metaStorageNodeNames = List.of(); + private Collection<String> cmgNodeNames = List.of(); + private String clusterName; + private AuthenticationConfig authenticationConfig = AuthenticationConfig.disabled(); + + public InitParametersBuilder setNodeName(String nodeName) { Review Comment: We don't use `set` prefixes in setters, so these methods should be called `nodeName`, `metaStorageNodeNames`, etc ########## modules/api/src/main/java/org/apache/ignite/InitParameters.java: ########## @@ -0,0 +1,89 @@ +/* + * 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; + +import java.util.Collection; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Initialization parameters. */ +public class InitParameters { + + /** Name of the node that the initialization request will be sent to. */ + private final String nodeName; + + /** Names of nodes that will host the Meta Storage <b>and</b> the CMG. */ + private final Collection<String> metaStorageNodeNames; + + /** Names of nodes that will host the CMG. */ + private final Collection<String> cmgNodeNames; + + /** Human-readable name of the cluster. */ + private final String clusterName; + + /** Authentication configuration, that will be applied after initialization. */ + private final AuthenticationConfig authenticationConfig; + + /** + * Constructor. + * + * @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 <b>and</b> the CMG. + * @param clusterName Human-readable name of the cluster. + * @param authenticationConfig REST authentication configuration. + */ + public InitParameters(String nodeName, Collection<String> metaStorageNodeNames, Collection<String> cmgNodeNames, String clusterName, Review Comment: If the intended way to create an object of this class is via a builder, then why is the constructor public? ########## modules/api/src/main/java/org/apache/ignite/InitParametersBuilder.java: ########## @@ -0,0 +1,70 @@ +/* + * 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; + +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Builder of {@link org.apache.ignite.InitParameters}. */ +public class InitParametersBuilder { + private String nodeName; + private Collection<String> metaStorageNodeNames = List.of(); + private Collection<String> cmgNodeNames = List.of(); + private String clusterName; + private AuthenticationConfig authenticationConfig = AuthenticationConfig.disabled(); + + public InitParametersBuilder setNodeName(String nodeName) { + this.nodeName = nodeName; + return this; + } + + public InitParametersBuilder setMetaStorageNodeNames(Collection<String> metaStorageNodeNames) { + this.metaStorageNodeNames = metaStorageNodeNames; + return this; + } + + public InitParametersBuilder setCmgNodeNames(Collection<String> cmgNodeNames) { + this.cmgNodeNames = cmgNodeNames; + return this; + } + + public InitParametersBuilder setClusterName(String clusterName) { + this.clusterName = clusterName; + return this; + } + + public InitParametersBuilder setRestAuthenticationConfig(AuthenticationConfig authenticationConfig) { + this.authenticationConfig = authenticationConfig; + return this; + } + + /** Builds {@link InitParameters}. */ + public InitParameters build() { + cmgNodeNames = cmgNodeNames.isEmpty() ? metaStorageNodeNames : cmgNodeNames; + + Objects.requireNonNull(nodeName); Review Comment: I think it makes sense to check that these parameters are not null in the corresponding setters. And here we should check that the required fields have been set (and throw an appropriate exception if not) ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/AuthenticationConverter.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.cluster.management; + +import java.util.List; +import java.util.stream.Collectors; +import org.apache.ignite.internal.cluster.management.network.auth.Authentication; +import org.apache.ignite.internal.cluster.management.network.auth.AuthenticationProvider; +import org.apache.ignite.internal.cluster.management.network.auth.BasicAuthenticationProvider; +import org.apache.ignite.rest.AuthenticationConfig; +import org.apache.ignite.rest.AuthenticationProviderConfig; +import org.apache.ignite.rest.AuthenticationType; +import org.apache.ignite.rest.BasicAuthenticationProviderConfig; + +/** Converter for {@link Authentication}. */ +class AuthenticationConverter { Review Comment: I still think that this class is not needed. It's methods are only used in one place ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java: ########## @@ -170,13 +177,30 @@ public void initCluster( Collection<String> metaStorageNodeNames, Collection<String> cmgNodeNames, String clusterName + ) throws NodeStoppingException { + initCluster(metaStorageNodeNames, cmgNodeNames, clusterName, disabled()); + } + + /** + * Initializes the cluster that this node is present in. + * + * @param metaStorageNodeNames Names of nodes that will host the Meta Storage. + * @param cmgNodeNames Names of nodes that will host the Cluster Management Group. + * @param clusterName Human-readable name of the cluster. + * @param authenticationConfig REST authentication configuration. + */ + public void initCluster( + Collection<String> metaStorageNodeNames, Review Comment: Same question about `InitParameters` ########## modules/cluster-management/src/test/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftGroupListenerTest.java: ########## @@ -146,6 +149,31 @@ void restoreFromSnapshotTriggersTopologyLeapEvent() { verify(logicalTopology).fireTopologyLeap(); } + @Test + void absentAuthConfigUpdateErasesAuthConfig() { + ClusterState clusterState = clusterState( + msgFactory, + Set.of("foo"), + Set.of("bar"), + IgniteProductVersion.CURRENT_VERSION, + clusterTag, + authentication(msgFactory, disabled()) + ); + + listener.onWrite(iterator(msgFactory.initCmgStateCommand().node(node).clusterState(clusterState).build())); + + ClusterState clusterStateToUpdate = clusterState( + msgFactory, + clusterState.cmgNodes(), + clusterState.metaStorageNodes(), + clusterState.igniteVersion(), + clusterState.clusterTag() + ); + + listener.onWrite(iterator(msgFactory.updateClusterStateCommand().clusterState(clusterStateToUpdate).build())); + assertNull(listener.storage().getClusterState().restAuthToApply()); Review Comment: I think we should also check that other fields remained unchanged ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterState.java: ########## @@ -80,12 +86,35 @@ static ClusterState clusterState( Collection<String> msNodes, IgniteProductVersion igniteVersion, ClusterTag clusterTag + ) { + return clusterState(msgFactory, cmgNodes, msNodes, igniteVersion, clusterTag, null); + } + + /** + * Creates a new cluster state instance. Acts like a constructor replacement. + * + * @param msgFactory Message factory to instantiate builder. + * @param cmgNodes Collection of CMG nodes. + * @param msNodes Collection of Metastorage nodes. + * @param igniteVersion Ignite product version. + * @param clusterTag Cluster tag instance. + * @param authentication REST authentication configuration. + * @return Cluster state instance. + */ + static ClusterState clusterState( Review Comment: This method looks redundant and has a lot of parameters. Can we simply inline it? ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java: ########## @@ -67,6 +70,25 @@ public CompletableFuture<Void> initCluster( Collection<String> metaStorageNodeNames, Collection<String> cmgNodeNames, String clusterName + ) { + return initCluster(metaStorageNodeNames, cmgNodeNames, clusterName, disabled()); + } + + /** + * Initializes the cluster that this node is present in. + * + * @param metaStorageNodeNames Names of nodes that will host the Meta Storage. Cannot be empty. + * @param cmgNodeNames Names of nodes that will host the Cluster Management Group. Can be empty, in which case {@code + * metaStorageNodeNames} will be used instead. + * @param clusterName Human-readable name of the cluster. + * @param authenticationConfig REST authentication configuration. + * @return Future that represents the state of the operation. + */ + public CompletableFuture<Void> initCluster( + Collection<String> metaStorageNodeNames, Review Comment: Why don't we pass `InitParameters` here? ########## modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageMultipleNodesTest.java: ########## @@ -115,13 +120,17 @@ private static class Node { var logicalTopology = new LogicalTopologyImpl(clusterStateStorage); + var distributedConfigurationUpdater = new DistributedConfigurationUpdater(); Review Comment: This object should be started and stopped like all other components ########## modules/rest/src/integrationTest/java/org/apache/ignite/internal/rest/cluster/ItClusterManagementControllerTest.java: ########## @@ -56,8 +68,13 @@ void testControllerLoaded() { @Test void testInitNoSuchNode() { // Given body with nodename that does not exist - String givenInvalidBody = "{\"metaStorageNodes\": [\"nodename\"], \"cmgNodes\": [], \"clusterName\": \"cluster\"}"; - + String givenInvalidBody = "{\n" Review Comment: What's the point of this change? ########## modules/rest/src/main/java/org/apache/ignite/internal/rest/authentication/DelegatingAuthenticationProvider.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.authentication; + +import io.micronaut.http.HttpRequest; +import io.micronaut.security.authentication.AuthenticationProvider; +import io.micronaut.security.authentication.AuthenticationRequest; +import io.micronaut.security.authentication.AuthenticationResponse; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; +import org.apache.ignite.configuration.NamedListView; +import org.apache.ignite.configuration.notifications.ConfigurationListener; +import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent; +import org.apache.ignite.internal.configuration.AuthenticationConfiguration; +import org.apache.ignite.internal.configuration.AuthenticationProviderView; +import org.apache.ignite.internal.configuration.AuthenticationView; +import org.apache.ignite.internal.logger.IgniteLogger; +import org.apache.ignite.internal.logger.Loggers; +import org.jetbrains.annotations.Nullable; +import org.reactivestreams.Publisher; +import reactor.core.publisher.Flux; +import reactor.core.publisher.FluxSink; + +/** + * Implementation of {@link AuthenticationProvider}. Creates a list of {@link Authenticator} according to provided + * {@link AuthenticationConfiguration} and updates them on configuration changes. Delegates {@link AuthenticationRequest} to the list of + * {@link Authenticator}. + */ +public class DelegatingAuthenticationProvider implements AuthenticationProvider, ConfigurationListener<AuthenticationView> { + + private static final IgniteLogger LOG = Loggers.forClass(DelegatingAuthenticationProvider.class); + + private final ReadWriteLock rwLock = new ReentrantReadWriteLock(); + private final List<Authenticator> authenticators = new ArrayList<>(); + private boolean authEnabled = false; + + @Override + public Publisher<AuthenticationResponse> authenticate(HttpRequest<?> httpRequest, AuthenticationRequest<?, ?> authenticationRequest) { + return Flux.create(emitter -> { + rwLock.readLock().lock(); + try { + if (authEnabled) { + Optional<AuthenticationResponse> successResponse = authenticators.stream() + .map(it -> it.authenticate(authenticationRequest)) + .filter(AuthenticationResponse::isAuthenticated) + .findFirst(); + if (successResponse.isPresent()) { + emitter.next(successResponse.get()); + emitter.complete(); + } else { + emitter.error(AuthenticationResponse.exception()); + } + } else { + emitter.next(AuthenticationResponse.success("Unknown")); + emitter.complete(); + } + } finally { + rwLock.readLock().unlock(); + } + }, FluxSink.OverflowStrategy.ERROR); + } + + @Override + public CompletableFuture<?> onUpdate(ConfigurationNotificationEvent<AuthenticationView> ctx) { + return CompletableFuture.runAsync(() -> refreshProviders(ctx.newValue())); + } + + private void refreshProviders(@Nullable AuthenticationView view) { + rwLock.writeLock().lock(); + try { + if (view == null || !view.enabled()) { + authEnabled = false; + authenticators.clear(); + } else if (view.enabled() && view.providers().size() != 0) { + authenticators.clear(); + authenticators.addAll(providersFromAuthView(view)); + authEnabled = true; + } else { + LOG.error("Invalid configuration: auth enabled, but no providers. Leave the old settings"); + } + } catch (Exception exception) { + LOG.error("Couldn't refresh authentication providers. Leave the old settings", exception); Review Comment: ```suggestion LOG.error("Couldn't refresh authentication providers. Leaving the old settings", exception); ``` ########## modules/rest/src/test/java/org/apache/ignite/internal/rest/RestComponentTest.java: ########## @@ -55,6 +58,12 @@ class RestComponentTest { private String keyStorePath; + @InjectConfiguration + AuthenticationConfiguration authenticationConfiguration; Review Comment: These fields can be `private` ########## modules/rest/src/test/java/org/apache/ignite/internal/rest/authentication/TestAuthenticationSubscriber.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.authentication; + +import io.micronaut.security.authentication.AuthenticationResponse; +import org.jetbrains.annotations.Nullable; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; + +/** Implementation of {@link Subscriber} for tests. */ +public class TestAuthenticationSubscriber implements Subscriber<AuthenticationResponse> { Review Comment: We have a `TestFlowUtils#subscribeToValue` method. Can it be reused here? ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItDistributedConfigurationPropertiesTest.java: ########## @@ -130,14 +134,17 @@ private static class Node { var clusterStateStorage = new TestClusterStateStorage(); var logicalTopology = new LogicalTopologyImpl(clusterStateStorage); + var distributedConfigurationUpdater = new DistributedConfigurationUpdater(); Review Comment: Again, since this is a component, it should be started and stopped properly ########## modules/api/src/main/java/org/apache/ignite/InitParametersBuilder.java: ########## @@ -0,0 +1,70 @@ +/* + * 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; + +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Builder of {@link org.apache.ignite.InitParameters}. */ +public class InitParametersBuilder { + private String nodeName; + private Collection<String> metaStorageNodeNames = List.of(); + private Collection<String> cmgNodeNames = List.of(); + private String clusterName; + private AuthenticationConfig authenticationConfig = AuthenticationConfig.disabled(); + + public InitParametersBuilder setNodeName(String nodeName) { + this.nodeName = nodeName; + return this; + } + + public InitParametersBuilder setMetaStorageNodeNames(Collection<String> metaStorageNodeNames) { + this.metaStorageNodeNames = metaStorageNodeNames; + return this; + } + + public InitParametersBuilder setCmgNodeNames(Collection<String> cmgNodeNames) { + this.cmgNodeNames = cmgNodeNames; + return this; + } + + public InitParametersBuilder setClusterName(String clusterName) { + this.clusterName = clusterName; + return this; + } + + public InitParametersBuilder setRestAuthenticationConfig(AuthenticationConfig authenticationConfig) { + this.authenticationConfig = authenticationConfig; + return this; + } + + /** Builds {@link InitParameters}. */ + public InitParameters build() { + cmgNodeNames = cmgNodeNames.isEmpty() ? metaStorageNodeNames : cmgNodeNames; Review Comment: We should also check that: 1. `metaStorageNodeNames` is not empty or null 2. `clusterName` is not empty or null or blank 3. `nodeName` is not empty or null or blank ########## modules/api/src/main/java/org/apache/ignite/InitParameters.java: ########## @@ -0,0 +1,89 @@ +/* + * 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; + +import java.util.Collection; +import java.util.Objects; +import org.apache.ignite.rest.AuthenticationConfig; + +/** Initialization parameters. */ +public class InitParameters { + + /** Name of the node that the initialization request will be sent to. */ + private final String nodeName; + + /** Names of nodes that will host the Meta Storage <b>and</b> the CMG. */ + private final Collection<String> metaStorageNodeNames; + + /** Names of nodes that will host the CMG. */ + private final Collection<String> cmgNodeNames; + + /** Human-readable name of the cluster. */ + private final String clusterName; + + /** Authentication configuration, that will be applied after initialization. */ + private final AuthenticationConfig authenticationConfig; + + /** + * Constructor. + * + * @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 <b>and</b> the CMG. + * @param clusterName Human-readable name of the cluster. + * @param authenticationConfig REST authentication configuration. + */ + public InitParameters(String nodeName, Collection<String> metaStorageNodeNames, Collection<String> cmgNodeNames, String clusterName, + AuthenticationConfig authenticationConfig) { + + Objects.requireNonNull(nodeName); + Objects.requireNonNull(metaStorageNodeNames); + Objects.requireNonNull(cmgNodeNames); + Objects.requireNonNull(clusterName); + Objects.requireNonNull(authenticationConfig); + + this.nodeName = nodeName; + this.metaStorageNodeNames = metaStorageNodeNames; Review Comment: Since this is a user-facing API, we need to create defensive copies ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/storage/ItDistributedConfigurationStorageTest.java: ########## @@ -104,14 +109,17 @@ private static class Node { var clusterStateStorage = new TestClusterStateStorage(); var logicalTopology = new LogicalTopologyImpl(clusterStateStorage); + var distributedConfigurationUpdater = new DistributedConfigurationUpdater(); Review Comment: Same here ########## modules/cluster-management/src/test/java/org/apache/ignite/internal/cluster/management/ClusterInitializerTest.java: ########## @@ -210,13 +210,17 @@ private CompletableFuture<NetworkMessage> initCompleteMessage() { */ @Test void testInitIllegalArguments() { - assertThrows(IllegalArgumentException.class, () -> clusterInitializer.initCluster(List.of(), List.of(), "cluster")); + assertThrows(IllegalArgumentException.class, Review Comment: What are these changes for? ########## modules/security/src/testFixtures/java/org/apache/ignite/internal/configuration/stub/StubAuthenticationProviderListView.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.stub; + +import java.util.List; +import java.util.stream.Collectors; +import org.apache.ignite.configuration.NamedListView; +import org.apache.ignite.internal.configuration.AuthenticationProviderView; +import org.jetbrains.annotations.Nullable; + +/** Stub of {@link NamedListView} for tests. */ +public class StubAuthenticationProviderListView implements NamedListView<AuthenticationProviderView> { Review Comment: Can these classes be replaced with `InjectConfiguration`? ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/rest/RestNodeBuilder.java: ########## @@ -0,0 +1,115 @@ +/* + * 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; + +import java.nio.file.Path; + +/** Builder of {@link RestNode}. */ +public class RestNodeBuilder { + private String keyStorePath = "ssl/keystore.p12"; + private String keyStorePassword = "changeit"; + private String trustStorePath = "ssl/truststore.jks"; + private String trustStorePassword = "changeit"; + private Path workDir; + private String name; + private int networkPort; + private int httpPort; + private int httpsPort; + private boolean sslEnabled = false; + + private boolean sslClientAuthEnabled = false; + private boolean dualProtocol = false; + + public RestNodeBuilder setKeyStorePath(String keyStorePath) { + this.keyStorePath = keyStorePath; + return this; + } + + public RestNodeBuilder setKeyStorePassword(String keyStorePassword) { Review Comment: Please remove the `set` prefixes ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/storage/ItRebalanceDistributedTest.java: ########## @@ -645,14 +650,17 @@ private class Node { var clusterStateStorage = new TestClusterStateStorage(); var logicalTopology = new LogicalTopologyImpl(clusterStateStorage); + var distributedConfigurationUpdater = new DistributedConfigurationUpdater(); Review Comment: And here ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/rest/authentication/ItAuthenticationTest.java: ########## @@ -0,0 +1,310 @@ +/* + * 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.authentication; + + +import static java.util.stream.Collectors.toList; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.waitForCondition; +import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpRequest.BodyPublishers; +import java.net.http.HttpResponse; +import java.net.http.HttpResponse.BodyHandlers; +import java.nio.file.Path; +import java.time.Duration; +import java.util.Base64; +import java.util.Collections; +import java.util.List; +import java.util.stream.IntStream; +import org.apache.ignite.internal.rest.RestNode; +import org.apache.ignite.internal.testframework.WorkDirectory; +import org.apache.ignite.internal.testframework.WorkDirectoryExtension; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.extension.ExtendWith; + + +/** Tests for the REST authentication configuration. */ +@ExtendWith(WorkDirectoryExtension.class) +public class ItAuthenticationTest { + + /** HTTP client that is expected to be defined in subclasses. */ + private HttpClient client; + + private List<RestNode> nodes; + + @WorkDirectory + private Path workDir; + + @BeforeEach + void setUp(TestInfo testInfo) { + client = HttpClient.newBuilder() + .build(); + + nodes = IntStream.range(0, 3) + .mapToObj(id -> { + return RestNode.builder() + .setWorkDir(workDir) + .setName(testNodeName(testInfo, id)) + .setNetworkPort(3344 + id) + .setHttpPort(10300 + id) + .setHttpsPort(10400 + id) + .setSslEnabled(false) + .setDualProtocol(false) + .build(); + }) + .collect(toList()); + + nodes.forEach(RestNode::start); + } + + @Test + public void disabledAuthentication(TestInfo testInfo) { + RestNode metaStorageNode = nodes.get(0); + + // when + String initClusterBody = "{\n" + + " \"metaStorageNodes\": [\n" + + " \"" + metaStorageNode.name() + "\"\n" + + " ],\n" + + " \"cmgNodes\": [],\n" + + " \"clusterName\": \"cluster\",\n" + + " \"authenticationConfig\": {\n" + + " \"enabled\": false\n" + + " }\n" + + "}"; + + initCluster(metaStorageNode.httpAddress(), initClusterBody); + + // then + for (RestNode node : nodes) { + assertTrue(isRestAvailable(node.httpAddress(), "", "")); + } + } + + @Test + public void changeCredentials(TestInfo testInfo) throws InterruptedException { + // when + RestNode metaStorageNode = nodes.get(0); + + String initClusterBody = "{\n" + + " \"metaStorageNodes\": [\n" + + " \"" + metaStorageNode.name() + "\"\n" + + " ],\n" + + " \"cmgNodes\": [],\n" + + " \"clusterName\": \"cluster\",\n" + + " \"authenticationConfig\": {\n" + + " \"enabled\": true,\n" + + " \"providers\": [\n" + + " {\n" + + " \"name\": \"basic\",\n" + + " \"type\": \"basic\",\n" + + " \"login\": \"admin\",\n" + + " \"password\": \"password\"\n" + + " }\n" + + " ]\n" + + " }\n" + + " }"; + + initCluster(metaStorageNode.httpAddress(), initClusterBody); + + // then + // authentication is enabled + for (RestNode node : nodes) { + assertTrue(waitForCondition(() -> isRestNotAvailable(node.httpAddress(), "", ""), + Duration.ofSeconds(5).toMillis())); + } + + // REST is available with valid credentials + for (RestNode node : nodes) { + assertTrue(isRestAvailable(node.httpAddress(), "admin", "password")); + } + + // REST is not available with invalid credentials + for (RestNode node : nodes) { + assertFalse(isRestAvailable(node.httpAddress(), "admin", "wrong-password")); + } + + // change password + String updateRestAuthConfigBody = "{\n" + + " \"security\": {\n" + + " \"authentication\": {\n" + + " \"enabled\": true,\n" + + " \"providers\": [\n" + + " {\n" + + " \"name\": \"basic\",\n" + + " \"type\": \"basic\",\n" + + " \"login\": \"admin\",\n" + + " \"password\": \"new-password\"\n" + + " }\n" + + " ]\n" + + " }\n" + + " }\n" + + "}"; + + updateClusterConfiguration(metaStorageNode.httpAddress(), "admin", "password", updateRestAuthConfigBody); + + // REST is not available with old credentials + for (RestNode node : nodes) { + assertTrue(waitForCondition(() -> isRestNotAvailable(node.httpAddress(), "admin", "password"), + Duration.ofSeconds(5).toMillis())); + } + + // REST is available with new credentials + for (RestNode node : nodes) { + assertTrue(isRestAvailable(node.httpAddress(), "admin", "new-password")); + } + } + + @Test + public void enableAuthenticationAndRestartNode(TestInfo testInfo) throws InterruptedException { + // when + RestNode metaStorageNode = nodes.get(0); + + String initClusterBody = "{\n" + + " \"metaStorageNodes\": [\n" + + " \"" + metaStorageNode.name() + "\"\n" + + " ],\n" + + " \"cmgNodes\": [],\n" + + " \"clusterName\": \"cluster\",\n" + + " \"authenticationConfig\": {\n" + + " \"enabled\": true,\n" + + " \"providers\": [\n" + + " {\n" + + " \"name\": \"basic\",\n" + + " \"type\": \"basic\",\n" + + " \"login\": \"admin\",\n" + + " \"password\": \"password\"\n" + + " }\n" + + " ]\n" + + " }\n" + + " }"; + + initCluster(metaStorageNode.httpAddress(), initClusterBody); + + // then + // authentication is enabled + for (RestNode node : nodes) { + assertTrue(waitForCondition(() -> isRestNotAvailable(node.httpAddress(), "", ""), + Duration.ofSeconds(5).toMillis())); + } + + // REST is available with valid credentials + for (RestNode node : nodes) { + assertTrue(isRestAvailable(node.httpAddress(), "admin", "password")); + } + + // REST is not available with invalid credentials + for (RestNode node : nodes) { + assertFalse(isRestAvailable(node.httpAddress(), "admin", "wrong-password")); + } + + // restart one of the nodes + RestNode nodeToRestart = nodes.get(2); + nodeToRestart.restart(); + waitForAllNodesStarted(Collections.singletonList(nodeToRestart)); + + // REST is available with valid credentials + for (RestNode node : nodes) { + assertTrue(isRestAvailable(node.httpAddress(), "admin", "password")); + } + + // REST is not available with invalid credentials + for (RestNode node : nodes) { + assertFalse(isRestAvailable(node.httpAddress(), "admin", "wrong-password")); + } + } + + private void initCluster(String baseUrl, String initClusterBody) { + URI clusterInitUri = URI.create(baseUrl + "/management/v1/cluster/init"); + HttpRequest initRequest = HttpRequest.newBuilder(clusterInitUri) + .header("content-type", "application/json") + .POST(BodyPublishers.ofString(initClusterBody)) + .build(); + + HttpResponse<String> response = sendRequest(client, initRequest); + assertThat(response.statusCode(), is(200)); + waitForAllNodesStarted(nodes); + } + + private void updateClusterConfiguration(String baseUrl, String login, String password, String configToApply) { + URI updateClusterConfigUri = URI.create(baseUrl + "/management/v1/configuration/cluster/"); + HttpRequest updateClusterConfigRequest = HttpRequest.newBuilder(updateClusterConfigUri) + .header("content-type", "text/plain") + .header("Authorization", basicAuthenticationHeader(login, password)) + .method("PATCH", BodyPublishers.ofString(configToApply)) + .build(); + + HttpResponse<String> updateClusterConfigResponse = sendRequest(client, updateClusterConfigRequest); + assertThat(updateClusterConfigResponse.statusCode(), is(200)); + } + + private boolean isRestNotAvailable(String baseUrl, String login, String password) { + return !isRestAvailable(baseUrl, login, password); + } + + private boolean isRestAvailable(String baseUrl, String login, String password) { + URI clusterConfigUri = URI.create(baseUrl + "/management/v1/configuration/cluster/"); + HttpRequest clusterConfigRequest = HttpRequest.newBuilder(clusterConfigUri) + .header("Authorization", basicAuthenticationHeader(login, password)) + .build(); + int code = sendRequest(client, clusterConfigRequest).statusCode(); + if (code == 200) { + return true; + } else if (code == 401) { + return false; + } else { + throw new IllegalStateException("Unexpected response code: " + code); + } + } + + private static HttpResponse<String> sendRequest(HttpClient client, HttpRequest request) { + try { + return client.send(request, BodyHandlers.ofString()); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + } + + @AfterEach + void tearDown() { + nodes.forEach(RestNode::stop); Review Comment: I would suggest to use a parallel stream to stop the nodes concurrently ########## modules/cluster-management/src/testFixtures/java/org/apache/ignite/internal/cluster/management/MockNode.java: ########## @@ -102,14 +107,17 @@ private void init(int port) throws IOException { var logicalTopologyService = new LogicalTopologyImpl(clusterStateStorage); + var distributedConfigurationUpdater = new DistributedConfigurationUpdater(); Review Comment: This object should be added to the `components` list (just to be consistent, I know that it is not necessary) ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java: ########## @@ -397,7 +406,6 @@ public class IgniteImpl implements Ignite { modules.distributed().internalSchemaExtensions(), modules.distributed().polymorphicSchemaExtensions() ); - Review Comment: What is this change for? ########## modules/security/src/main/java/org/apache/ignite/internal/configuration/package-info.java: ########## @@ -0,0 +1,22 @@ +/* + * 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. + */ + +/** + * This package contains REST component core implementation. Review Comment: This javadoc is incorrect ########## modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageWatchTest.java: ########## @@ -109,13 +115,17 @@ private static class Node { var logicalTopology = new LogicalTopologyImpl(clusterStateStorage); + var distributedConfigurationUpdater = new DistributedConfigurationUpdater(); Review Comment: Same here ########## modules/rest/src/test/java/org/apache/ignite/internal/rest/authentication/TestAuthenticationSubscriber.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.authentication; + +import io.micronaut.security.authentication.AuthenticationResponse; +import org.jetbrains.annotations.Nullable; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; + +/** Implementation of {@link Subscriber} for tests. */ +public class TestAuthenticationSubscriber implements Subscriber<AuthenticationResponse> { + + private volatile Subscription source; Review Comment: This field is redundant ########## 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: > RestAuthenticationConfig is used in java API. RestAuthentication is used for messaging. So we need both. Didn't notice, you are right. >About DistributedConfigurationUpdater, I would like to avoid using messaging classes there and use different classes for different application layers. Why? You just create redundant classes and move stuff from one to the other. What's the point? ########## modules/rest/src/main/java/org/apache/ignite/internal/rest/cluster/ClusterManagementController.java: ########## @@ -52,7 +64,10 @@ public class ClusterManagementController implements ClusterManagementApi { * @param clusterInitializer cluster initializer. * @param clusterManagementGroupManager cluster management group manager. */ - public ClusterManagementController(ClusterInitializer clusterInitializer, ClusterManagementGroupManager clusterManagementGroupManager) { + public ClusterManagementController( + ClusterInitializer clusterInitializer, Review Comment: What's the point of this change? -- 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]
