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

bump: grpc 1.58.0 #1856

Merged
merged 10 commits into from
Oct 17, 2023
Merged

bump: grpc 1.58.0 #1856

merged 10 commits into from
Oct 17, 2023

Conversation

patriknw
Copy link
Member

@patriknw patriknw commented Oct 13, 2023

  • guava dependency to 32.0.1 to address CVE-2023-2976
  • protobuf-java 3.24.0 to align with grpc 1.58.0
  • plugin-tester-java wasn't setting protoc version when inside sbt

See release notes https://github.com/grpc/grpc-java/releases

Still some problem in interop-tests...

* guava dependency to 32.0.1 to address CVE-2023-2976
* protobuf-java 3.22.3 to align with grpc 1.58.0
* plugin-tester-java wasn't setting protoc version when inside sbt
@@ -57,7 +61,7 @@ object Dependencies {

// Excluding grpc-alts works around a complex resolution bug
// Details are in https://github.com/akka/akka-grpc/pull/469
val grpcInteropTesting = ("io.grpc" % "grpc-interop-testing" % Versions.grpc)
val grpcInteropTesting = ("io.grpc" % "grpc-interop-testing" % Versions.grpcInterop)
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem was the type EnumType = EnumType scalapb/ScalaPB#1557

We also saw that in #1841 so it's something new that is brought in from proto-google-common-protos

Workaround is to test with old grpc-interop-testing until there is a new release of ScalaPB

Copy link
Member Author

Choose a reason for hiding this comment

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

Still some tests failing because of this. I was somewhat worried that this would be a showstopper for updating, but I have a tried a snapshot with various gprc projects and it seems to work fine.

@@ -155,6 +159,5 @@ object Dependencies {
Test.scalaTest,
Test.scalaTestPlusJunit,
Test.akkaTestkitTyped,
Protobuf.googleCommonProtos,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is somewhat weird. I saw the EnumType in the plugin tester project, but that was not the case in CI. I suspect those protos are also defined in protobuf-java so some kind of undefined ordering of which one that is used. The newer protobuf-java has this problem, so using the older proto-google-common-protos.

@patriknw patriknw marked this pull request as ready for review October 13, 2023 13:16
@johanandren
Copy link
Member

the "non balancing" netty test is known to be flaky, but I think the scripted test is something that needs looking into

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM (once CI agrees)

@patriknw
Copy link
Member Author

Test failures:

  • GrpcInteropIoWithIoSpec Failed to bind to address 0.0.0.0/0.0.0.0:8080
  • NonBalancingIntegrationSpec
  • paradox: Error writing content: Java heap space at /home/runner/work/akka-grpc/akka-grpc/docs/src/main/paradox/client/walkthrough.md

@patriknw
Copy link
Member Author

The paradox problem is that the dependencies directive seems to blow up with the new grpc dependency.

@@dependencies { projectId="akka-grpc-runtime" }

@patriknw
Copy link
Member Author

sbt-paradox-dependencies issue lightbend/sbt-paradox-dependencies#33

new InetSocketAddress("example.invalid", 80),
new InetSocketAddress("example.invalid", 80),
new InetSocketAddress("example.invalid", 80),
new InetSocketAddress("example.invalid", 80),
new InetSocketAddress("example.invalid", 80),
server.localAddress,
Copy link
Member Author

Choose a reason for hiding this comment

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

This test fail every time with Netty now, but it also fails with Akka HTTP if adding a few more invalid entries. How is this supposed to work? MutableServiceDiscovery just returns Future.successful(Resolved of the given entries. Isn't the intention that it would be failed futures for invalid entries?

Copy link
Member

Choose a reason for hiding this comment

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

I never looked into it, but have noticed it has been flaky.

@@ -27,7 +27,7 @@ final class MutableServiceDiscovery(targets: List[InetSocketAddress]) extends Se
services = Future.successful(
Resolved(
"greeter",
targets.map(target => ResolvedTarget(target.getHostString, Some(target.getPort), Some(target.getAddress)))))
targets.map(target => ResolvedTarget(target.getHostString, Some(target.getPort), Option(target.getAddress)))))
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was wrong, Some(null) is never a good thing. null is valid in InetSocketAddress. Now it will try to resolve the host, which I think was the intention with "example.invalid", but that doesn't work with netty backend. Created issue #1857 for that

@@ -61,7 +61,8 @@ object AkkaHttpClientUtils {
val clientConnectionSettings =
ClientConnectionSettings(sys).withTransport(ClientTransport.withCustomResolver((host, _) => {
settings.overrideAuthority.foreach { authority =>
assert(host == authority)
if (host != authority)
throw new IllegalArgumentException(s"Unexpected host [$host], expected authority [$authority]")
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated, but we don't use assert

* cycle between grpc-core and grpc-util
@patriknw patriknw merged commit 3a18a25 into main Oct 17, 2023
11 checks passed
@patriknw patriknw deleted the wip-bump-grpc-patriknw branch October 17, 2023 13:28
@patriknw patriknw added this to the 2.4.0-M2 milestone Oct 17, 2023
GreyPlane pushed a commit to GreyPlane/akka-grpc that referenced this pull request Oct 26, 2023
* guava dependency to 32.0.1 to address CVE-2023-2976
* protobuf-java 3.24.0, which is what grpc-protobuf:1.58.0 brings in
* plugin-tester-java wasn't setting protoc version when inside sbt
* workaround for ScalaPB EnumType issue, because of ScalaPB issue 1557
* not sure, but parallelExecution false
* tests with invalid endpoints not working
* Don't use assert
* bump: sbt-paradox-dependencies 0.2.4
  * cycle between grpc-core and grpc-util
johanandren pushed a commit that referenced this pull request Dec 14, 2023
* guava dependency to 32.0.1 to address CVE-2023-2976
* protobuf-java 3.24.0, which is what grpc-protobuf:1.58.0 brings in
* plugin-tester-java wasn't setting protoc version when inside sbt
* workaround for ScalaPB EnumType issue, because of ScalaPB issue 1557
* not sure, but parallelExecution false
* tests with invalid endpoints not working
* Don't use assert
* bump: sbt-paradox-dependencies 0.2.4
  * cycle between grpc-core and grpc-util
GreyPlane pushed a commit to GreyPlane/akka-grpc that referenced this pull request Feb 19, 2024
* guava dependency to 32.0.1 to address CVE-2023-2976
* protobuf-java 3.24.0, which is what grpc-protobuf:1.58.0 brings in
* plugin-tester-java wasn't setting protoc version when inside sbt
* workaround for ScalaPB EnumType issue, because of ScalaPB issue 1557
* not sure, but parallelExecution false
* tests with invalid endpoints not working
* Don't use assert
* bump: sbt-paradox-dependencies 0.2.4
  * cycle between grpc-core and grpc-util
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.

2 participants