Re: [PR] reverted - [Netty] Fix client with TLS and custom SSL context [incubator-pekko-grpc]

["ihostage (via GitHub)" <gi...@apache.org>]

ihostage commented on PR #197:
URL: https://github.com/apache/incubator-pekko-grpc/pull/197#issuecomment-1869560910

   > this is going to have to be reverted - it looks like the code was taken from akka-grpc - this is unacceptable
   
   Sorry @pjfanning, It looks like some kind of nightmare to me 😱 
   
   I'll try to describe what this history was on my side. 
   
   1. As a co-maintainer @Playframework, I found time to work on Play gRPC that haven't had our love in long time (by many reasons and our focus to more priority issues)
   2. I caught "_ALPN must be enabled and list HTTP/2 as a supported protocol_" error while was working on this update https://github.com/playframework/play-grpc/pull/407
   3. Like you in https://github.com/apache/incubator-pekko-grpc/pull/200, I also analysed a diff between `2.1.5...2.1.6` and found a changes in https://github.com/apache/incubator-pekko-grpc/commit/b0ff79ecc4382b25bad4e041df6cdbaa3b305343 that were a root cause a problem in our tests.
   4. I didn't want revert these changes and back to using reflection 🤮 And I jumped to source `GrpcSslContexts` that was used before to find a solution. 
   5. Shortly, my route was next:
   [GrpcSslContexts.configure(SslContextBuilder builder, SslProvider provider)](https://github.com/grpc/grpc-java/blob/6e2e18bb728793df32b2ba195a954ad380e546de/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java#L155)
   ⬇️ 
   [return configure(builder, jdkProvider)](https://github.com/grpc/grpc-java/blob/6e2e18bb728793df32b2ba195a954ad380e546de/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java#L164)
   ⬇️ 
   [configure(SslContextBuilder builder, Provider jdkProvider)](https://github.com/grpc/grpc-java/blob/6e2e18bb728793df32b2ba195a954ad380e546de/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java#L189)
   ⬇️ 
   [apc = ALPN](https://github.com/grpc/grpc-java/blob/6e2e18bb728793df32b2ba195a954ad380e546de/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java#L212)
   ⬇️ 
   [private static final ApplicationProtocolConfig ALPN = ...](https://github.com/grpc/grpc-java/blob/6e2e18bb728793df32b2ba195a954ad380e546de/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java#L66-L70)
   ⬇️
   Got it! 
   6. A couldn't use `ALPN` variable directly because it's private 😞  That why I just copied the declaration of this variable and pass it instead of `ApplicationProtocolConfig.DISABLED`. Yes, I copied this line, but from gRPC-Java that is under APL 2 not from Akka gRPC. Of course, also needed to change a private variable `NEXT_PROTOCOL_VERSIONS` to `ApplicationProtocolNames.HTTP_2` but it was a trivial to find a public suitable variable. 
   7. I published locally a patched version Akka gRPC. **It was anought**, Play gRPC tests was green locally and I was happy.
   8. Because Play 2.9.x based on Akka and we support this version, I wanted to create an issue and PR for Akka gRPC to fix this problem and backport this fix to OSS `2.1.x` version. Then I found that it's already fixed and Akka gRPC has a related issue 🤦‍♂️ 
   8. I asked my friends (maybe it's a one way friendship but I consider them my friends) from LB backport their fix to `2.1.x` and created PR with **their** (`cherry-pick` helped me and it's important for the story) changes to simplify as much as possible their works to release a new OSS version. (You can find these discussions and related PR in Akka gRPC, I don't want to link their here)
   9. Also important to note, that as you can see I tried to fix a problem for Akka firstly when we haven't had a Play gRPC version is migrated to Pekko. After this adventure to fix this problem for Play 3 (Play gRPC `0.12.x`), I ported changes to Pekko gRPC repository that was applied to Akka gRPC. And in this step I made a mistake (don't remove unnecessary `ClientAuth.OPTIONAL` from my changes) and sorry for that 😢 Maybe I made a mistake on _step 8_, I already don't know, but I can't imagine how I can miss this step if it is in my nature 🤷‍♂️ 
   
   **Summary:**
   So, now you know how `ClientAuth.NONE -> ClientAuth.OPTIONAL` became a part of my PR. But I insist that other part is legal. And this whole situation is terrible to me, because I don't understand yet what my development process on Play should be without touching Akka source code (include the latest sometimes). Of course, as a solution I just may do nothing for Pekko but I dislike this way 😞 
   
   **Just thinking out loud**
   Instead of improve a software we should thinking about legal every line changes... Ouch, it's incredible, folks, really incredible... 


-- 
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: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org