PakhomovAlexander commented on code in PR #1632: URL: https://github.com/apache/ignite-3/pull/1632#discussion_r1097660747
########## modules/network/src/main/java/org/apache/ignite/internal/network/configuration/KeyStoreConfigurationValidatorImpl.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.network.configuration; + +import static org.apache.ignite.internal.util.StringUtils.nullOrBlank; + +import java.util.Arrays; +import org.apache.ignite.configuration.validation.ValidationContext; +import org.apache.ignite.configuration.validation.ValidationIssue; +import org.apache.ignite.configuration.validation.Validator; + +/** + * Key store configuration validator implementation. + */ +public class KeyStoreConfigurationValidatorImpl implements Validator<KeyStoreConfigurationValidator, KeyStoreView> { + + public static final KeyStoreConfigurationValidatorImpl INSTANCE = new KeyStoreConfigurationValidatorImpl(); + + @Override + public void validate(KeyStoreConfigurationValidator annotation, ValidationContext<KeyStoreView> ctx) { + KeyStoreView keyStore = ctx.getNewValue(); + String type = keyStore.type(); + String path = keyStore.path(); + String password = keyStore.password(); + if (nullOrBlank(type) && nullOrBlank(path) && nullOrBlank(password)) { + return; + } else { + + KeyStoreType keyStoreType = KeyStoreType.fromName(type); + if (keyStoreType == null) { + ctx.addIssue(new ValidationIssue( + ctx.currentKey(), + String.format("Key store type must be one of %s", Arrays.toString(KeyStoreType.values())) + )); + } + + if (nullOrBlank(path)) { + ctx.addIssue(new ValidationIssue( + ctx.currentKey(), + "Key store path must not be blank" Review Comment: I am not sure but probably we could use the default keystore ########## modules/network/src/main/java/org/apache/ignite/internal/network/configuration/KeyStoreType.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.network.configuration; + +import java.util.Arrays; +import org.jetbrains.annotations.Nullable; + +/** Supported key store types. */ +public enum KeyStoreType { + JKS, PKCS12; Review Comment: I don't think we even should restrict the list of keystore types. The user might want to add his own keystore type and implementation and we should support such scenarios. ########## modules/rest/src/main/java/org/apache/ignite/internal/rest/RestComponent.java: ########## @@ -99,37 +104,77 @@ public RestComponent(List<RestFactory> restFactories, RestConfiguration restConf @Override public void start() { RestView restConfigurationView = restConfiguration.value(); + RestSslView sslConfigurationView = restConfigurationView.ssl(); - int desiredPort = restConfigurationView.port(); + boolean sslEnabled = sslConfigurationView.enabled(); + boolean dualProtocol = restConfiguration.dualProtocol().value(); + int desiredHttpPort = restConfigurationView.port(); int portRange = restConfigurationView.portRange(); + int desiredHttpsPort = sslConfigurationView.port(); + int httpsPortRange = sslEnabled ? sslConfigurationView.portRange() : 0; + int httpPortCandidate = desiredHttpPort; + int httpsPortCandidate = desiredHttpsPort; - for (int portCandidate = desiredPort; portCandidate <= desiredPort + portRange; portCandidate++) { - try { - port = portCandidate; - context = buildMicronautContext(portCandidate) - .deduceEnvironment(false) - .environments(BARE_METAL) - .start(); - LOG.info("REST protocol started successfully"); + while (httpPortCandidate <= desiredHttpPort + portRange + && httpsPortCandidate <= desiredHttpsPort + httpsPortRange) { + if (startHttpServer(httpPortCandidate, httpsPortCandidate)) { return; - } catch (ApplicationStartupException e) { - BindException bindException = findBindException(e); - if (bindException != null) { - LOG.debug("Got exception during node start, going to try again [port={}]", portCandidate); - continue; - } - throw new RuntimeException(e); + } + + LOG.debug("Got exception during node start, going to try again [httpPort={}, httpsPort={}]", + httpPortCandidate, + httpsPortCandidate); + + if (sslEnabled && dualProtocol) { + httpPortCandidate++; + httpsPortCandidate++; + } else if (sslEnabled) { + httpsPortCandidate++; + } else { + httpPortCandidate++; } } - LOG.debug("Unable to start REST endpoint. All ports are in use [ports=[{}, {}]]", desiredPort, (desiredPort + portRange)); + LOG.debug("Unable to start REST endpoint." + + " Couldn't find available port for HTTP or HTTPS" + + " [HTTP ports=[{}, {}]]," + + " [HTTP ports=[{}, {}]]", + desiredHttpPort, (desiredHttpPort + portRange), + desiredHttpsPort, (desiredHttpsPort + httpsPortRange)); - String msg = "Cannot start REST endpoint. " + "All ports in range [" + desiredPort + ", " + (desiredPort + portRange) - + "] are in use."; + String msg = "Cannot start REST endpoint." + + " Couldn't find available port for HTTP or HTTPS" + + " [HTTP ports=[" + desiredHttpPort + ", " + desiredHttpPort + portRange + "]]," + + " [HTTPS ports=[" + desiredHttpsPort + ", " + desiredHttpsPort + httpsPortRange + "]]"; throw new RuntimeException(msg); } + /** Starts Micronaut application using the provided ports. + * + * @param httpPortCandidate HTTP port candidate. + * @param httpsPortCandidate HTTPS port candidate. + * @return {@code True} if server was started successfully, {@code False} if couldn't bind one of the ports. + */ + private boolean startHttpServer(int httpPortCandidate, int httpsPortCandidate) { Review Comment: Could you rename it like startServer or startRestServer ########## modules/network/src/test/java/org/apache/ignite/internal/network/configuration/TestUtils.java: ########## @@ -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. + */ + +package org.apache.ignite.internal.network.configuration; + +/** Utility class for configuration tests. */ +class TestUtils { + + /** Create a stub for {@link KeyStoreView}. */ + static KeyStoreView stubKeyStoreView(String type, String path, String password) { Review Comment: I think it is better to create a class named `StubKeyStoreView` instead of `TestUtils#stubKeyStoreView` ########## modules/runner/src/test/java/org/apache/ignite/internal/component/RestAddressReporterTest.java: ########## @@ -39,25 +39,53 @@ class RestAddressReporterTest { private static final String REST_ADDRESS_FILENAME = "rest-address"; @Test - @DisplayName("REST server network address is reported to file") - void networkAddressReported(@TempDir Path tmpDir) throws IOException { + @DisplayName("REST server network addresses is reported to file") + void httpAndHttpsAddressesReported(@TempDir Path tmpDir) throws IOException { // Given RestAddressReporter reporter = new RestAddressReporter(tmpDir); // When - reporter.writeReport(new NetworkAddress("localhost", 9999)); + reporter.writeReport(new NetworkAddress("localhost", 9999), new NetworkAddress("localhost", 8443)); + + // Then there is a report + String restAddress = Files.readString(tmpDir.resolve(REST_ADDRESS_FILENAME)); + assertThat(restAddress, equalTo("http://localhost:9999, https://localhost:8443";)); + } + + @Test + @DisplayName("REST server HTTP address is reported to file") + void httpAddressReported(@TempDir Path tmpDir) throws IOException { + // Given + RestAddressReporter reporter = new RestAddressReporter(tmpDir); + + // When + reporter.writeReport(new NetworkAddress("localhost", 9999), null); // Then there is a report String restAddress = Files.readString(tmpDir.resolve(REST_ADDRESS_FILENAME)); assertThat(restAddress, equalTo("http://localhost:9999";)); } + @Test + @DisplayName("REST server HTTPS address is reported to file") + void httpsAddressReported(@TempDir Path tmpDir) throws IOException { + // Given + RestAddressReporter reporter = new RestAddressReporter(tmpDir); + + // When + reporter.writeReport(null, new NetworkAddress("localhost", 8443)); Review Comment: changing the reporting string might corrupt the message that is printed after node start. could you please test it manually if ./ignite3db start printing readable message in case SSL is enabled? -- 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]
