From 3bb7aaff75bb0598706325c508e0d269fc490a72 Mon Sep 17 00:00:00 2001 From: Jeff Trent Date: Mon, 2 Oct 2023 13:47:06 -0400 Subject: [PATCH 1/6] Partial fix for issue #7698 --- common/tls/pom.xml | 5 ++ .../common/tls/ConfiguredTlsManager.java | 17 +++++ .../common/tls/spi/TlsManagerCache.java | 38 ++++++++++++ .../common/tls/spi/TlsManagerProvider.java | 22 +++++++ .../tls/spi/TlsManagerProviderTest.java | 62 +++++++++++++++++++ ...aultOciCertificatesTlsManagerProvider.java | 5 +- 6 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java create mode 100644 common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java diff --git a/common/tls/pom.xml b/common/tls/pom.xml index 478f8804096..932bbca54e7 100644 --- a/common/tls/pom.xml +++ b/common/tls/pom.xml @@ -71,6 +71,11 @@ hamcrest-all test + + org.mockito + mockito-core + test + diff --git a/common/tls/src/main/java/io/helidon/common/tls/ConfiguredTlsManager.java b/common/tls/src/main/java/io/helidon/common/tls/ConfiguredTlsManager.java index 16ac897a094..cbe75e8444e 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/ConfiguredTlsManager.java +++ b/common/tls/src/main/java/io/helidon/common/tls/ConfiguredTlsManager.java @@ -124,6 +124,14 @@ protected void reload(Optional keyManager, Optional CACHE = new ConcurrentHashMap<>(); + + private TlsManagerCache() { + } + + static TlsManager getOrCreate(Object configBean, + Function creator) { + Objects.requireNonNull(configBean); + Objects.requireNonNull(creator); + return CACHE.computeIfAbsent(configBean, creator); + } + +} diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java index fdc8eb22552..aac9be531bc 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java @@ -16,6 +16,10 @@ package io.helidon.common.tls.spi; +import java.util.Objects; +import java.util.function.Function; + +import io.helidon.common.config.Config; import io.helidon.common.config.ConfiguredProvider; import io.helidon.common.tls.TlsManager; @@ -24,4 +28,22 @@ */ public interface TlsManagerProvider extends ConfiguredProvider { + /** + * Provides the ability to have a unique {@link TlsManager} per unique {@link Config} instance provided. + * + * @param config the raw config instance + * @param configBean the config bean instance + * @param name the config bean instance name + * @param creator the creator to apply if not already in cache, which takes the config bean instance + * @return the tls manager instance from cache, defaulting to creation from the {@code creator} if not in cache + */ + static TlsManager getOrCreate(Config config, + String name, + Object configBean, + Function creator) { + Objects.requireNonNull(config); + Objects.requireNonNull(name); + return TlsManagerCache.getOrCreate(configBean, creator); + } + } diff --git a/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java b/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java new file mode 100644 index 00000000000..4dcf07bf4ef --- /dev/null +++ b/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2023 Oracle and/or its affiliates. + * + * Licensed 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 io.helidon.common.tls.spi; + +import io.helidon.common.tls.ConfiguredTlsManager; +import io.helidon.common.tls.TlsManager; +import io.helidon.config.Config; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +import java.util.concurrent.atomic.AtomicInteger; + +import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.MatcherAssert.assertThat; + +class TlsManagerProviderTest { + + @Test + void caching() { + TlsManager mock = Mockito.mock(TlsManager.class); + Config config = Config.create(); + AtomicInteger count = new AtomicInteger(); + + // we are using "1" and "2" here abstractly to stand in for Config beans, which would hash properly + TlsManager manager1 = TlsManagerProvider.getOrCreate(config, "test", "1", (c) -> { + count.incrementAndGet(); + return mock; + }); + assertThat(manager1, sameInstance(mock)); + assertThat(count.get(), is(1)); + + TlsManager manager2 = TlsManagerProvider.getOrCreate(config, "test", "1", (c) -> { + count.incrementAndGet(); + return Mockito.mock(TlsManager.class); + }); + assertThat(manager2, sameInstance(mock)); + assertThat(count.get(), is(1)); + + TlsManager manager3 = TlsManagerProvider.getOrCreate(config, "test", "2", (c) -> { + count.incrementAndGet(); + return Mockito.mock(TlsManager.class); + }); + assertThat(manager3, notNullValue()); + assertThat(manager3, not(sameInstance(mock))); + assertThat(count.get(), is(2)); + } + +} diff --git a/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java b/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java index 1ac5dff8d89..88a81312832 100644 --- a/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java +++ b/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java @@ -43,7 +43,10 @@ public String configKey() { public TlsManager create(Config config, String name) { OciCertificatesTlsManagerConfig cfg = OciCertificatesTlsManagerConfig.create(config); - return new DefaultOciCertificatesTlsManager(cfg, name, config); + return TlsManagerProvider.getOrCreate(config, + name, + cfg, + (c) -> new DefaultOciCertificatesTlsManager(cfg, name, config)); } } From cb3f16d580896ce8061952621ba5e548c1368c33 Mon Sep 17 00:00:00 2001 From: Jeff Trent Date: Wed, 4 Oct 2023 15:24:28 -0400 Subject: [PATCH 2/6] address review comments --- .../common/tls/spi/TlsManagerCache.java | 4 ++-- .../common/tls/spi/TlsManagerProvider.java | 11 ++--------- .../tls/spi/TlsManagerProviderTest.java | 19 ++++++++++--------- 3 files changed, 14 insertions(+), 20 deletions(-) diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java index cc0277fee4a..98c2746c1b7 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java @@ -28,8 +28,8 @@ class TlsManagerCache { private TlsManagerCache() { } - static TlsManager getOrCreate(Object configBean, - Function creator) { + static TlsManager getOrCreate(T configBean, + Function creator) { Objects.requireNonNull(configBean); Objects.requireNonNull(creator); return CACHE.computeIfAbsent(configBean, creator); diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java index aac9be531bc..df07d477fb7 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java @@ -16,7 +16,6 @@ package io.helidon.common.tls.spi; -import java.util.Objects; import java.util.function.Function; import io.helidon.common.config.Config; @@ -31,18 +30,12 @@ public interface TlsManagerProvider extends ConfiguredProvider { /** * Provides the ability to have a unique {@link TlsManager} per unique {@link Config} instance provided. * - * @param config the raw config instance * @param configBean the config bean instance - * @param name the config bean instance name * @param creator the creator to apply if not already in cache, which takes the config bean instance * @return the tls manager instance from cache, defaulting to creation from the {@code creator} if not in cache */ - static TlsManager getOrCreate(Config config, - String name, - Object configBean, - Function creator) { - Objects.requireNonNull(config); - Objects.requireNonNull(name); + static TlsManager getOrCreate(T configBean, + Function creator) { return TlsManagerCache.getOrCreate(configBean, creator); } diff --git a/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java b/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java index 4dcf07bf4ef..525848cc0b0 100644 --- a/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java +++ b/common/tls/src/test/java/io/helidon/common/tls/spi/TlsManagerProviderTest.java @@ -16,15 +16,17 @@ package io.helidon.common.tls.spi; -import io.helidon.common.tls.ConfiguredTlsManager; +import java.util.concurrent.atomic.AtomicInteger; + import io.helidon.common.tls.TlsManager; -import io.helidon.config.Config; + import org.junit.jupiter.api.Test; import org.mockito.Mockito; -import java.util.concurrent.atomic.AtomicInteger; - -import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.CoreMatchers.sameInstance; import static org.hamcrest.MatcherAssert.assertThat; class TlsManagerProviderTest { @@ -32,25 +34,24 @@ class TlsManagerProviderTest { @Test void caching() { TlsManager mock = Mockito.mock(TlsManager.class); - Config config = Config.create(); AtomicInteger count = new AtomicInteger(); // we are using "1" and "2" here abstractly to stand in for Config beans, which would hash properly - TlsManager manager1 = TlsManagerProvider.getOrCreate(config, "test", "1", (c) -> { + TlsManager manager1 = TlsManagerProvider.getOrCreate("1", (c) -> { count.incrementAndGet(); return mock; }); assertThat(manager1, sameInstance(mock)); assertThat(count.get(), is(1)); - TlsManager manager2 = TlsManagerProvider.getOrCreate(config, "test", "1", (c) -> { + TlsManager manager2 = TlsManagerProvider.getOrCreate("1", (c) -> { count.incrementAndGet(); return Mockito.mock(TlsManager.class); }); assertThat(manager2, sameInstance(mock)); assertThat(count.get(), is(1)); - TlsManager manager3 = TlsManagerProvider.getOrCreate(config, "test", "2", (c) -> { + TlsManager manager3 = TlsManagerProvider.getOrCreate("2", (c) -> { count.incrementAndGet(); return Mockito.mock(TlsManager.class); }); From 12fd2f0ab547c68094d28665b1995f1174ce1574 Mon Sep 17 00:00:00 2001 From: Jeff Trent Date: Wed, 4 Oct 2023 15:31:32 -0400 Subject: [PATCH 3/6] use new api --- .../DefaultOciCertificatesTlsManagerProvider.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java b/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java index 88a81312832..593824375da 100644 --- a/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java +++ b/integrations/oci/tls-certificates/src/main/java/io/helidon/integrations/oci/tls/certificates/DefaultOciCertificatesTlsManagerProvider.java @@ -43,9 +43,7 @@ public String configKey() { public TlsManager create(Config config, String name) { OciCertificatesTlsManagerConfig cfg = OciCertificatesTlsManagerConfig.create(config); - return TlsManagerProvider.getOrCreate(config, - name, - cfg, + return TlsManagerProvider.getOrCreate(cfg, (c) -> new DefaultOciCertificatesTlsManager(cfg, name, config)); } From 98b7e3b2b996e4f1d04390027ef1c86942946ed6 Mon Sep 17 00:00:00 2001 From: Jeff Trent Date: Wed, 4 Oct 2023 15:35:48 -0400 Subject: [PATCH 4/6] checkstyle --- .../main/java/io/helidon/common/tls/spi/TlsManagerProvider.java | 1 + 1 file changed, 1 insertion(+) diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java index df07d477fb7..bf3e2810630 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java @@ -32,6 +32,7 @@ public interface TlsManagerProvider extends ConfiguredProvider { * * @param configBean the config bean instance * @param creator the creator to apply if not already in cache, which takes the config bean instance + * @param the type of the config bean * @return the tls manager instance from cache, defaulting to creation from the {@code creator} if not in cache */ static TlsManager getOrCreate(T configBean, From 0f1d3699a2057b0d61b7409d8f649946ecfe7c95 Mon Sep 17 00:00:00 2001 From: Jeff Trent Date: Thu, 5 Oct 2023 10:47:22 -0400 Subject: [PATCH 5/6] review comment --- .../main/java/io/helidon/common/tls/spi/TlsManagerCache.java | 5 +++-- .../java/io/helidon/common/tls/spi/TlsManagerProvider.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java index 98c2746c1b7..9bc8acc9d66 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java @@ -28,11 +28,12 @@ class TlsManagerCache { private TlsManagerCache() { } + @SuppressWarnings("unchecked") static TlsManager getOrCreate(T configBean, - Function creator) { + Function creator) { Objects.requireNonNull(configBean); Objects.requireNonNull(creator); - return CACHE.computeIfAbsent(configBean, creator); + return CACHE.computeIfAbsent(configBean, (Function) creator); } } diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java index bf3e2810630..e89173eba14 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerProvider.java @@ -36,7 +36,7 @@ public interface TlsManagerProvider extends ConfiguredProvider { * @return the tls manager instance from cache, defaulting to creation from the {@code creator} if not in cache */ static TlsManager getOrCreate(T configBean, - Function creator) { + Function creator) { return TlsManagerCache.getOrCreate(configBean, creator); } From eea884fb823cf2cf70e6b2d02a78874dd261bd37 Mon Sep 17 00:00:00 2001 From: Jeff Trent Date: Fri, 6 Oct 2023 11:55:03 -0400 Subject: [PATCH 6/6] review comments --- .../common/tls/spi/TlsManagerCache.java | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java index 9bc8acc9d66..b1b374a3f7b 100644 --- a/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java +++ b/common/tls/src/main/java/io/helidon/common/tls/spi/TlsManagerCache.java @@ -16,24 +16,40 @@ package io.helidon.common.tls.spi; +import java.util.HashMap; +import java.util.Map; import java.util.Objects; -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Function; import io.helidon.common.tls.TlsManager; class TlsManagerCache { - private static final ConcurrentHashMap CACHE = new ConcurrentHashMap<>(); + private static final ReentrantLock LOCK = new ReentrantLock(); + private static final Map CACHE = new HashMap<>(); private TlsManagerCache() { } - @SuppressWarnings("unchecked") static TlsManager getOrCreate(T configBean, Function creator) { Objects.requireNonNull(configBean); Objects.requireNonNull(creator); - return CACHE.computeIfAbsent(configBean, (Function) creator); + LOCK.lock(); + try { + TlsManager manager = CACHE.get(configBean); + if (manager != null) { + return manager; + } + + manager = creator.apply(configBean); + Object existing = CACHE.put(configBean, manager); + assert (existing == null); + + return manager; + } finally { + LOCK.unlock(); + } } }