exceptionfactory commented on code in PR #7481: URL: https://github.com/apache/nifi/pull/7481#discussion_r1264461875
########## nifi-nar-bundles/nifi-redis-bundle/nifi-redis-extensions/src/test/java/org/apache/nifi/redis/testcontainers/RedisSentinelContainer.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.nifi.redis.testcontainers; + +import org.springframework.lang.NonNull; +import org.springframework.lang.Nullable; +import org.testcontainers.utility.DockerImageName; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +public class RedisSentinelContainer extends RedisContainer { + + public static final int REDIS_SENTINEL_PORT = 26379; + + public RedisSentinelContainer(final @NonNull DockerImageName dockerImageName) { + super(dockerImageName); + + setPort(REDIS_SENTINEL_PORT); + } + + public RedisSentinelContainer(final @NonNull String fullImageName) { + this(DockerImageName.parse(fullImageName)); + } + + @NonNull + protected String masterHost = "localhost"; + protected int masterPort = REDIS_PORT; + @NonNull + protected String masterName = "mymaster"; + @Nullable + protected String sentinelPassword = null; + private long downAfterMilliseconds = 60000L; + private long failoverTimeout = 180000L; + private int parallelSyncs = 1; + private int quorumSize = 1; + + public void setMasterHost(final @NonNull String masterHost) { + this.masterHost = masterHost; + } + + public void setMasterPort(final int masterPort) { + this.masterPort = masterPort; + } + + public void setMasterName(final @NonNull String masterName) { + this.masterName = masterName; + } + + public void setSentinelPassword(final @Nullable String sentinelPassword) { + this.sentinelPassword = sentinelPassword; + } + + public void setQuorumSize(final int quorumSize) { + this.quorumSize = quorumSize; + } + + public void setDownAfterMilliseconds(final long downAfterMilliseconds) { + this.downAfterMilliseconds = downAfterMilliseconds; + } + + public void setFailoverTimeout(final long failoverTimeout) { + this.failoverTimeout = failoverTimeout; + } + + public void setParallelSyncs(final int parallelSyncs) { + this.parallelSyncs = parallelSyncs; + } + + + @Override + protected void adjustConfiguration() { + addConfigurationOption("port " + port); + + addConfigurationOption(String.format("sentinel monitor %s %s %d %d", masterName, masterHost, masterPort, quorumSize)); + addConfigurationOption(String.format("sentinel down-after-milliseconds %s %d", masterName, downAfterMilliseconds)); + addConfigurationOption(String.format("sentinel failover-timeout %s %d", masterName, failoverTimeout)); + addConfigurationOption(String.format("sentinel parallel-syncs %s %d", masterName, parallelSyncs)); + + if (password != null) { + addConfigurationOption("sentinel auth-pass " + masterName + " " + password); + } + if (sentinelPassword != null) { + addConfigurationOption("requirepass " + sentinelPassword); + addConfigurationOption("sentinel sentinel-pass " + sentinelPassword); + } + } + + @Override + protected void configure() { + super.configure(); + + System.err.println("SENTINEL AT PORT " + port); + System.err.println(configurationOptions); Review Comment: Recommend removing System print statements. Loggers could be used if absolutely necessary. ########## nifi-nar-bundles/nifi-redis-bundle/nifi-redis-extensions/pom.xml: ########## @@ -82,6 +82,11 @@ <artifactId>nifi-record-path</artifactId> <version>2.0.0-SNAPSHOT</version> </dependency> + <dependency> + <groupId>org.apache.nifi</groupId> + <artifactId>nifi-ssl-context-service-api</artifactId> + <scope>compile</scope> Review Comment: This dependency should be marked as provided. ```suggestion <scope>provided</scope> ``` ########## nifi-nar-bundles/nifi-redis-bundle/nifi-redis-extensions/src/test/java/org/apache/nifi/redis/testcontainers/RedisContainer.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.nifi.redis.testcontainers; + +import org.springframework.lang.NonNull; +import org.springframework.lang.Nullable; +import org.testcontainers.containers.BindMode; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.utility.DockerImageName; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; + +public class RedisContainer extends GenericContainer<RedisContainer> { + + public static final int REDIS_PORT = 6379; + + public RedisContainer(@NonNull DockerImageName dockerImageName) { + super(dockerImageName); + } + + public RedisContainer(@NonNull String fullImageName) { + this(DockerImageName.parse(fullImageName)); + } + + public int port = REDIS_PORT; + + @Nullable + protected String password = null; + @Nullable + protected Path configurationMountDirectory = null; + + protected final List<String> configurationOptions = new ArrayList<>(); + + public void setPassword(final @Nullable String password) { + this.password = password; + } + + public void setPort(final int port) { + this.port = port; + } + + public void mountConfigurationFrom(final Path mountDirectory) { + this.configurationMountDirectory = mountDirectory; + } + + public void addConfigurationOption(final String configurationOption) { + this.configurationOptions.add(configurationOption); + } + + protected void adjustConfiguration() { + addConfigurationOption("port " + port); + + if (password != null) { + addConfigurationOption("requirepass " + password); + } + } + + /** + * Sets up a static binding between a port on the host and one in the container. + * In order for auto-discovery mechanisms of Redis to work, 1-to-1 mapped ports are useful. + */ + public void addPortBinding(int hostPort, int containerPort) { + addFixedExposedPort(hostPort, containerPort); + } + + @Override + protected void configure() { + adjustConfiguration(); + + Path configurationFilePath = writeConfigurationFile().toAbsolutePath(); + String hostPath = configurationFilePath.toString(); + String containerPath = "/usr/local/etc/redis/redis.conf"; + addFileSystemBind(hostPath, containerPath, BindMode.READ_WRITE); + + setCommand(containerPath); + } + + protected Path writeConfigurationFile() { + try { + Path mountDirectory = this.configurationMountDirectory; + if (mountDirectory == null) { + mountDirectory = Files.createTempDirectory("redis-container-configuration"); + } + + Path configFile = mountDirectory.resolve("redis-" + UUID.randomUUID() + ".conf"); + Files.write(configFile, configurationOptions, StandardCharsets.UTF_8); + + return configFile; + } catch (IOException ioException) { + throw new IllegalStateException("Cannot start container because configuration could not be written!", ioException); Review Comment: Exclamation marks should be avoided in error messages. ```suggestion throw new IllegalStateException("Cannot start container because configuration could not be written", ioException); ``` ########## nifi-nar-bundles/nifi-redis-bundle/nifi-redis-extensions/src/test/java/org/apache/nifi/redis/service/ITRedisDistributedMapCacheClientService.java: ########## @@ -64,22 +73,154 @@ */ public class ITRedisDistributedMapCacheClientService { - private TestRedisProcessor proc; - private TestRunner testRunner; - private RedisServer redisServer; + @TempDir + private Path testDirectory; + + private static final String masterName = "redisLeader"; + + private final TestRedisProcessor proc = new TestRedisProcessor(); + private final TestRunner testRunner = TestRunners.newTestRunner(proc); + + private final List<RedisContainer> redisContainers = new ArrayList<>(); private RedisConnectionPoolService redisConnectionPool; - private RedisDistributedMapCacheClientService redisMapCacheClientService; - private int redisPort; - @BeforeEach - public void setup() throws IOException { - this.redisPort = getAvailablePort(); + @Test + public void testStandaloneRedis() throws InitializationException, IOException { + int redisPort = setupStandaloneRedis(null).port; + setUpRedisConnectionPool(portsToConnectionString(redisPort), pool -> { + // uncomment this to test using a different database index than the default 0 + // testRunner.setProperty(pool, RedisUtils.DATABASE, "1"); + }); + setupRedisMapCacheClientService(); + + executeProcessor(); + } + + @Test + public void testStandaloneRedisWithAuthentication() throws InitializationException, IOException { + final String redisPassword = "foobared"; + final int redisPort = setupStandaloneRedis(redisPassword).port; + setUpRedisConnectionPool(portsToConnectionString(redisPort), pool -> { + testRunner.setProperty(redisConnectionPool, RedisUtils.PASSWORD, redisPassword); + }); + setupRedisMapCacheClientService(); + + executeProcessor(); + } + + @Test + public void testSentinelRedis() throws InitializationException, IOException { + RedisContainer redisMasterContainer = setupStandaloneRedis(null); + String masterHost = "127.0.0.1"; + int masterPort = redisMasterContainer.port; + setUpRedisReplica(masterHost, masterPort, null); + setUpRedisReplica(masterHost, masterPort, null); + + int sentinelAPort = setUpSentinel(masterHost, masterPort, null, 2, null).port; + int sentinelBPort = setUpSentinel(masterHost, masterPort, null, 2, null).port; + int sentinelCPort = setUpSentinel(masterHost, masterPort, null, 2, null).port; + + setUpRedisConnectionPool(portsToConnectionString(sentinelAPort, sentinelBPort, sentinelCPort), pool -> { + testRunner.setProperty(redisConnectionPool, RedisUtils.REDIS_MODE, REDIS_MODE_SENTINEL); + testRunner.setProperty(redisConnectionPool, RedisUtils.SENTINEL_MASTER, masterName); + }); + + setupRedisMapCacheClientService(); + + executeProcessor(); + } + + @Test + public void testSentinelRedisWithAuthentication() throws InitializationException, IOException { + String redisPassword = "t0p_53cr35"; + String sentinelPassword = "otherPassword"; + + RedisContainer redisMasterContainer = setupStandaloneRedis(redisPassword); + String masterHost = "127.0.0.1"; + int masterPort = redisMasterContainer.port; + setUpRedisReplica(masterHost, masterPort, redisPassword); + setUpRedisReplica(masterHost, masterPort, redisPassword); + + int sentinelAPort = setUpSentinel(masterHost, masterPort, redisPassword, 2, sentinelPassword).port; + int sentinelBPort = setUpSentinel(masterHost, masterPort, redisPassword, 2, sentinelPassword).port; + int sentinelCPort = setUpSentinel(masterHost, masterPort, redisPassword, 2, sentinelPassword).port; + + setUpRedisConnectionPool(portsToConnectionString(sentinelAPort, sentinelBPort, sentinelCPort), pool -> { + testRunner.setProperty(redisConnectionPool, RedisUtils.REDIS_MODE, REDIS_MODE_SENTINEL); + testRunner.setProperty(redisConnectionPool, RedisUtils.SENTINEL_MASTER, masterName); + + testRunner.setProperty(redisConnectionPool, RedisUtils.PASSWORD, redisPassword); + testRunner.setProperty(redisConnectionPool, RedisUtils.SENTINEL_PASSWORD, sentinelPassword); + }); + setupRedisMapCacheClientService(); + + executeProcessor(); + } + + @AfterEach + public void teardown() { + if (redisConnectionPool != null) { + redisConnectionPool.onDisabled(); + } + + redisContainers.forEach(RedisContainer::stop); + } + + private RedisContainer setupStandaloneRedis(final @Nullable String redisPassword) throws IOException { + int redisPort = getAvailablePort(); + + RedisContainer redisContainer = new RedisContainer("redis:7.0.12-alpine"); Review Comment: Recommend setting a static member variable for the image name and version, and reusing it in all locations. ```suggestion RedisContainer redisContainer = new RedisContainer(CONTAINER_IMAGE_TAG); ``` -- 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]
