Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TlsEngineType enum #5029

Merged
merged 13 commits into from Apr 3, 2024
Merged

Add TlsEngineType enum #5029

merged 13 commits into from Apr 3, 2024

Conversation

seonWKim
Copy link
Contributor

Motivation:

Add TlsEngineType to let users choose the type of tls engine. (Using Flags)

Modifications:

  • Add TlsEngineType enum
  • Deprecate useOpenSsl

Result:

@seonWKim seonWKim changed the title Add tls engine type enum Add TlsEngineType enum Jul 14, 2023
@seonWKim seonWKim force-pushed the add-tlsEngineType-enum branch 2 times, most recently from 3c99037 to 9bd86a5 Compare July 17, 2023 22:38
@seonWKim seonWKim requested a review from ikhoon July 27, 2023 05:35
core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved
}

private static void setUseOpenSslAndDumpOpenSslInfo() {
final boolean useOpenSsl = getValue(FlagsProvider::useOpenSsl, "useOpenSsl");
if (!useOpenSsl) {
TLS_ENGINE_TYPE = getValue(FlagsProvider::tlsEngineType, "tlsEngineType");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For smooth migration, -Dcom.linecorp.armeria.useOpenSsl=false needs to return TlsEngineType.JDK if no TlsEngineType is specified by a user. Otherwise, an OpenSSL engine is used unexpectedly.

How about adding getUserValue() method that ignores DefaultFlagsProvider's values and returns only user-defined values?

final Boolean useOpenSsl = getUserValue(FlagsProvider::useOpenSsl, "useOpenSsl");
final TlsEngineType tlsEngineType = getUserValue(FlagsProvider::tlsEngineType, "tlsEngineType");

if (useOpenSsl == null) {
   // Respect tlsEngineType
} else if (tlsEngineType == null) {
  // Respect useOpenSsl
} else {
   // Check if the values of useOpenSsl and tlsEngineType are compatible. 
}

Copy link
Contributor Author

@seonWKim seonWKim Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikhoon Thank you for the detailed comment. I've added getUserValue() which ignores DefaultFlagsProvider.

In the case of incompatible useOpenSsl and tlsEngineType, I left a warning log and used the value from tlsEngineType. I chose tlsEngineType instead of useOpenSsl because in that case, user is aware of and using -Dcom.linecorp.armeria.tlsEngineType, so I think it's might be fine to use the value of tlsEngineType.

else {
      if (useOpenSsl != (tlsEngineType == TlsEngineType.OPENSSL)) {
          logger.warn("useOpenSsl({}) and tlsEngineType({}) are incompatible, tlsEngineType will be used",
                      useOpenSsl, tlsEngineType);
      }
      TLS_ENGINE_TYPE = tlsEngineType;
}

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty great! 👍

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved
@@ -1506,6 +1535,29 @@ private static <T> T getValue(Function<FlagsProvider, @Nullable T> method,
throw new Error();
}

@Nullable
private static <T> T getUserValue(Function<FlagsProvider, @Nullable T> method, String flagName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method looks pretty much the same as the above getValue method.
How about removing this method and adding ignoreDefaultFlags to the above method instead?

private static <T> T getValue(Function<FlagsProvider, @Nullable T> method,
                              String flagName, boolean ignoreDefaultFlags, Predicate<T> validator) {
    for (FlagsProvider provider : FLAGS_PROVIDERS) {
        if (provider instanceof DefaultFlagsProvider) {
            continue;
        }
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's also possible to delegate the getUserValue call to getValue (so getValue calls getUserValue and falls back to DefaultFlagsProvider)
The reasoning is although it would take up slightly more cpu cycles, I think the method itself is useful.

Copy link
Contributor Author

@seonWKim seonWKim Sep 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how you think of getUserValue method. Is it a temporary or not temporary?

I split the method because I thought getUserValue method seems to be a temporary method(simply for migration and deleted afterwards). If it's not temporary, I think it's better to add ignoreDefaultFlags.

ref: #5029 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how you think of getUserValue method. Is it a temporary or not temporary?

Although temporary, we will probably keep the logic for a while (at least until 2.0)

On second thought, would it be simpler to just delete getUserValue and modify DefaultFlagsProvider#useOpenSsl, DefaultFlagsProvider#tlsEngineType to return null by default?

Copy link
Contributor Author

@seonWKim seonWKim Sep 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrhee17 I think we should preserve getUserValue because Flags#getValue throws new Error() when all the values retrieved by the FlagsProviders is null.

    private static <T> T getValue(Function<FlagsProvider, @Nullable T> method,
                                  String flagName, Predicate<T> validator) {
        for (FlagsProvider provider : FLAGS_PROVIDERS) {
            try {
                final T value = method.apply(provider);
                if (value == null) {
                    continue;
                }
                if (!validator.test(value)) {
                    logger.warn("{}: {} ({}, validation failed)", flagName, value, provider.name());
                    continue;
                }
                logger.info("{}: {} ({})", flagName, value, provider.name());
                return value;
            } catch (Exception ex) {
                logger.warn("{}: ({}, {})", flagName, provider.name(), ex.getMessage());
            }
        }
        // Should never reach here because DefaultFlagsProvider always returns a normal value.
        throw new Error();
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Then we can probably do

if (ignoreDefaultFlags) {
    return null;
}
// Should never reach here because DefaultFlagsProvider always returns a normal value.
throw new Error();

at the end.
So it's your call now. We can gather it into one method, which makes the method a little longer, or we can have separate methods. I'm fine with both. It's your call. 😉

Copy link
Contributor Author

@seonWKim seonWKim Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I will separate methods because combining getValue and getUserValue will require a lot of changes when @Nullable has to be handled by the callers of getValue method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea on how to delegate @nullable getUserValue call to non-@nullable getValue ?

I meant something like the following. Having said this, I think I'm fine with the current approach:

diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java
index ff40a0de6..e30115a4c 100644
--- a/core/src/main/java/com/linecorp/armeria/common/Flags.java
+++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java
@@ -105,7 +105,6 @@ public final class Flags {
                              .sorted(Comparator.comparingInt(FlagsProvider::priority).reversed())
                              .collect(Collectors.toList());
         flagsProviders.add(0, SystemPropertyFlagsProvider.INSTANCE);
-        flagsProviders.add(DefaultFlagsProvider.INSTANCE);
         FLAGS_PROVIDERS = ImmutableList.copyOf(flagsProviders);
     }
 
@@ -561,8 +560,9 @@ public final class Flags {
             return;
         }
 
-        final Boolean useOpenSsl = getUserValue(FlagsProvider::useOpenSsl, "useOpenSsl");
-        final TlsEngineType tlsEngineTypeValue = getUserValue(FlagsProvider::tlsEngineType, "tlsEngineType");
+        final Boolean useOpenSsl = getUserValue(FlagsProvider::useOpenSsl, "useOpenSsl", ignored -> true);
+        final TlsEngineType tlsEngineTypeValue = getUserValue(FlagsProvider::tlsEngineType, "tlsEngineType",
+                                                              ignored -> true);
 
         if (useOpenSsl == null) {
             tlsEngineType = tlsEngineTypeValue != null ? tlsEngineTypeValue : TlsEngineType.OPENSSL;
@@ -1549,28 +1549,16 @@ public final class Flags {
 
     private static <T> T getValue(Function<FlagsProvider, @Nullable T> method,
                                   String flagName, Predicate<T> validator) {
-        for (FlagsProvider provider : FLAGS_PROVIDERS) {
-            try {
-                final T value = method.apply(provider);
-                if (value == null) {
-                    continue;
-                }
-                if (!validator.test(value)) {
-                    logger.warn("{}: {} ({}, validation failed)", flagName, value, provider.name());
-                    continue;
-                }
-                logger.info("{}: {} ({})", flagName, value, provider.name());
-                return value;
-            } catch (Exception ex) {
-                logger.warn("{}: ({}, {})", flagName, provider.name(), ex.getMessage());
-            }
+        final T t = getUserValue(method, flagName, validator);
+        if (t != null) {
+            return t;
         }
-        // Should never reach here because DefaultFlagsProvider always returns a normal value.
-        throw new Error();
+        return method.apply(DefaultFlagsProvider.INSTANCE);
     }
 
     @Nullable
-    private static <T> T getUserValue(Function<FlagsProvider, @Nullable T> method, String flagName) {
+    private static <T> T getUserValue(Function<FlagsProvider, @Nullable T> method, String flagName,
+                                      Predicate<T> validator) {
         for (FlagsProvider provider : FLAGS_PROVIDERS) {
             if (provider instanceof DefaultFlagsProvider) {
                 continue;
@@ -1581,6 +1569,10 @@ public final class Flags {
                 if (value == null) {
                     continue;
                 }
+                if (!validator.test(value)) {
+                    logger.warn("{}: {} ({}, validation failed)", flagName, value, provider.name());
+                    continue;
+                }
 
                 logger.info("{}: {} ({})", flagName, value, provider.name());
                 return value;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for the great detail. I think above approach looks much clean. Let me apply above first and see whether no tests fail.

Copy link
Contributor Author

@seonWKim seonWKim Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrhee17
I think calling getUserValue from getValue adds some overhead because of provider instanceof DefaultFlagsProvider(even though we don't need except for useOpenSsl and tlsEngineType). But I think the overheads are small. I want to first ask you whether this overheads are negligible.

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved
core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved
@minwoox minwoox added this to the 1.26.0 milestone Aug 24, 2023
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments, but mostly happy about the changes!

@@ -1506,6 +1535,29 @@ private static <T> T getValue(Function<FlagsProvider, @Nullable T> method,
throw new Error();
}

@Nullable
private static <T> T getUserValue(Function<FlagsProvider, @Nullable T> method, String flagName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's also possible to delegate the getUserValue call to getValue (so getValue calls getUserValue and falls back to DefaultFlagsProvider)
The reasoning is although it would take up slightly more cpu cycles, I think the method itself is useful.

@@ -57,7 +58,6 @@ void reloadFlags() throws ClassNotFoundException {

@Test
void overrideDefaultFlagsProvider() throws Throwable {
assertFlags("useOpenSsl").isEqualTo(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be converted to the following?

assertFlags("tlsEngineType").equals(TlsEngineType.OPENSSL);

Copy link
Contributor Author

@seonWKim seonWKim Sep 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll add that.

Btw, after adding above test code, I don't know why this keeps on failing with below exceptions. To my expectation and debugging, it should run fine...

Added code.

assertFlags("tlsEngineType").isEqualTo(TlsEngineType.OPENSSL);

Exception message:

FlagsProviderTest > overrideDefaultFlagsProvider() FAILED
    java.lang.IllegalAccessException: no such method: com.linecorp.armeria.common.Flags.tlsEngineType()TlsEngineType/invokeStatic
        at java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:972)
        at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1117)
        at java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:3664)
        at java.base/java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:2598)
        at com.linecorp.armeria.common.FlagsProviderTest.assertFlags(FlagsProviderTest.java:163)
        at com.linecorp.armeria.common.FlagsProviderTest.overrideDefaultFlagsProvider(FlagsProviderTest.java:61)

        Caused by:
        java.lang.LinkageError: bad method type alias: ()TlsEngineType not visible from class com.linecorp.armeria.common.Flags
            at java.base/java.lang.invoke.MemberName.checkForTypeAlias(MemberName.java:878)
            at java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1089)
            at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1114)
            ... 4 more

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the TlsEngineType was loaded from the default class loader, whereas the assertion is using the FlagsClassLoader.

Can you try the following?

user@AL02437565 armeria % git diff
diff --git a/it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java b/it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
index a010976bf..619bfd3eb 100644
--- a/it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
+++ b/it/flags-provider/src/test/java/com/linecorp/armeria/common/FlagsProviderTest.java
@@ -158,11 +158,8 @@ class FlagsProviderTest {
     }
 
     private ObjectAssert<Object> assertFlags(String flagsMethod) throws Throwable {
-        final Lookup lookup = MethodHandles.publicLookup();
-        final MethodHandle method =
-                lookup.findStatic(flags, flagsMethod, MethodType.methodType(
-                        Flags.class.getMethod(flagsMethod).getReturnType()));
-        return assertThat(method.invoke());
+        Method method = flags.getDeclaredMethod(flagsMethod);
+        return assertThat(method.invoke(null));
     }
 
     private static class FlagsClassLoader extends ClassLoader {
user@AL02437565 armeria % 

I also realized it should actually be isNotEqualTo (following the previous implementation)

Comment on lines 124 to 125
System.setProperty("com.linecorp.armeria.useOpenSsl", "false");
System.setProperty("com.linecorp.armeria.tlsEngineType", "OPENSSL");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these values will be set while the jvm is running. Do you mind using either assumeThat or @SetSystemProperty instead?

Suggested change
System.setProperty("com.linecorp.armeria.useOpenSsl", "false");
System.setProperty("com.linecorp.armeria.tlsEngineType", "OPENSSL");
System.setProperty("com.linecorp.armeria.useOpenSsl", "false");
System.setProperty("com.linecorp.armeria.tlsEngineType", "OPENSSL");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change the test to use @SetSystemProperty.
cf) I think this test leads to the problems you mentioned. Should we consider setting ways to prevent above test running in parallel(even if we use --parallel flag)

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Attention: Patch coverage is 48.07692% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 74.08%. Comparing base (655d48c) to head (13e6e79).

Files Patch % Lines
...c/main/java/com/linecorp/armeria/common/Flags.java 42.42% 12 Missing and 7 partials ⚠️
...rp/armeria/common/SystemPropertyFlagsProvider.java 25.00% 5 Missing and 1 partial ⚠️
.../linecorp/armeria/common/DefaultFlagsProvider.java 0.00% 1 Missing ⚠️
...ava/com/linecorp/armeria/server/ServerBuilder.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5029       +/-   ##
===========================================
+ Coverage        0   74.08%   +74.08%     
- Complexity      0    20973    +20973     
===========================================
  Files           0     1819     +1819     
  Lines           0    77296    +77296     
  Branches        0     9874     +9874     
===========================================
+ Hits            0    57267    +57267     
- Misses          0    15369    +15369     
- Partials        0     4660     +4660     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks almost done 👍

@@ -1506,6 +1535,29 @@ private static <T> T getValue(Function<FlagsProvider, @Nullable T> method,
throw new Error();
}

@Nullable
private static <T> T getUserValue(Function<FlagsProvider, @Nullable T> method, String flagName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how you think of getUserValue method. Is it a temporary or not temporary?

Although temporary, we will probably keep the logic for a while (at least until 2.0)

On second thought, would it be simpler to just delete getUserValue and modify DefaultFlagsProvider#useOpenSsl, DefaultFlagsProvider#tlsEngineType to return null by default?

@seonWKim
Copy link
Contributor Author

seonWKim commented Sep 14, 2023

@jrhee17

On second thought, would it be simpler to just delete getUserValue and modify DefaultFlagsProvider#useOpenSsl, DefaultFlagsProvider#tlsEngineType to return null by default?

We still have BaseFlagsProvider which sets useOpenSsl to false by default, so only modifying DefaultFlagsPRovider#useOpenSsl method to return null won't work. And as we have discussed in the above comments, if we let getValue method to return a null value, the @Nullable should be handled by all the Flags which require many code changes.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks, @seonwoo960000! 😄

@jrhee17
Copy link
Contributor

jrhee17 commented Sep 18, 2023

We still have BaseFlagsProvider which sets useOpenSsl to false by default, so only modifying DefaultFlagsPRovider#useOpenSsl method to return null won't work.

BaseFlagsProvider is for an integration test, I'm not sure I follow the reasoning 😅

I think the current approach is fine for this iteration though

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Thanks @seonwoo960000 🙇 👍 🙇

Comment on lines 610 to 611
* <p>If {@link #tlsEngineType()} does not return {@link TlsEngineType#OPENSSL}, this also returns
* {@code false} no matter you specified the JVM option.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matching the javadoc in FlagsProvider

Suggested change
* <p>If {@link #tlsEngineType()} does not return {@link TlsEngineType#OPENSSL}, this also returns
* {@code false} no matter you specified the JVM option.
* <p>If {@link #tlsEngineType()} does not return {@link TlsEngineType#OPENSSL}, this also returns
* {@code false} no matter what the specified JVM option is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix that right away 🏃‍♂️

@seonWKim
Copy link
Contributor Author

seonWKim commented Sep 18, 2023

We still have BaseFlagsProvider which sets useOpenSsl to false by default, so only modifying DefaultFlagsPRovider#useOpenSsl method to return null won't work.

BaseFlagsProvider is for an integration test, I'm not sure I follow the reasoning 😅.

I think the current approach is fine for this iteration though

Oh yeah, I think I've missed that point. Thank you for helping better understanding🥺 . Thank you so much of your detailed review(learned a lot!)

Flags.useOpenSsl = false;
dumpOpenSslInfo = false;
return;
}
if (!OpenSsl.isAvailable()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a regression of #2184
Should we check OpenSsl.isAvailable() lazily when other constrains are met?

Copy link
Contributor Author

@seonWKim seonWKim Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply 😅
If the cost of OpenSsl.isAvailable() is high, I think it's better to remain as-is.

@minwoox minwoox removed this from the 1.26.0 milestone Oct 20, 2023
@minwoox minwoox added this to the 1.27.0 milestone Oct 20, 2023
@ikhoon ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 24, 2024
@seonWKim
Copy link
Contributor Author

seonWKim commented Mar 6, 2024

@ikhoon Just a ping 😄.

@ikhoon ikhoon requested review from jrhee17 and minwoox March 25, 2024 12:50
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Thanks! 🙇 👍 🙇

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @seonwoo960000! 🚀🚀

@ikhoon
Copy link
Contributor

ikhoon commented Apr 3, 2024

@minwoox (Not urgent) Could you take another look? I've re-requested a review.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good! Thanks!
I pushed a small patch that

  • Adds UnstableApi
  • Removes DefaultFlagsProvider.INSTANCE from flagsProrivers.

@jrhee17 jrhee17 merged commit 33ab3bb into line:main Apr 3, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants