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

Upgrade Play 2.9.0 and drop Java 8 #479

Merged
merged 43 commits into from
Dec 21, 2023
Merged

Conversation

sentenza
Copy link
Contributor

@sentenza sentenza commented Dec 14, 2023

sun.security.x509.

This Play doc](https://www.playframework.com/documentation/2.9.x/Migration29#Generation-of-Self-Signed-Certificates-Fails-in-Java-17-and-Java-21) explains why we get the error in pipelines.

In fact, we set the https.port only in staging and so on, but we do not have any valid certificate.

In the process of binding an HTTPS port, Play typically generates a self-signed certificate as a default procedure. However, when using Java 17, this operation may lead to the following exception:

java.lang.IllegalAccessError: class com.typesafe.sslconfig.ssl.FakeKeyStore$ 
(in unnamed module @0x68c8dd0e) 
cannot access class sun.security.x509.X509CertInfo (in module java.base) 
because module java.base does not export sun.security.x509 to unnamed 
module @0x68c8dd0e (FakeKeyStore.scala:89)
com.typesafe.sslconfig.ssl.FakeKeyStore$.createSelfSignedCertificate(...

To solve this issue in production/staging we have to export a specific var:

    export JAVA_TOOL_OPTIONS="$JAVA_TOOL_OPTIONS --add-exports=java.base/sun.security.x509=ALL-UNNAMED";

@sentenza sentenza changed the title Upgrade Play 2.8.21 and drop Java 8 Upgrade Play 2.9.0 and drop Java 8 Dec 14, 2023
@sentenza
Copy link
Contributor Author

@mkurz @ihostage Do you know how to solve the following error?

java.lang.IllegalArgumentException: GrpcClientSettings requires a server endpoint with ssl, but non provided (JavaAkkaGrpcClientHelpers.java:66)

@ihostage
Copy link
Member

Hi, @sentenza!

@mkurz @ihostage Do you know how to solve the following error?

No, I haven't seen that yet 🤷‍♂️ I'll look this, but only when we will have released a 0.9.2 which is compatible with Play 2.8.x.

@sentenza
Copy link
Contributor Author

sentenza commented Dec 19, 2023

The exception that is blocking tests for JDK 11 right now is:

PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

This blog post describes this kind of errors.

As pointed out in this comment it should be also possible to avoid any check on SSL certs but I don't see where to put this kind of configuration in this project:

ws.acceptAnyCertificate = true
play.ws.ssl.loose.acceptAnyCertificate = true

@mkurz
Copy link
Member

mkurz commented Dec 19, 2023

Rebased etc, will try to make this working asap.

@mkurz
Copy link
Member

mkurz commented Dec 20, 2023

@sentenza It seems you force pushed this branch which overrides changes that we had merged in the main branch already (and my rebase with fixed conflicts yesterday). I fixed that now and force pushed myself again, so we should have a clean state now. Can you do me a favor and make sure you are up to date locally? Fetch the changes and then when on your upgrade-play28 branch please run git reset --hard origin/upgrade-play28, if origin refers to the the remote of your github fork, should be like this:

$ git remote -v
origin        git@github.com:sentenza/play-grpc.git (fetch)
origin        git@github.com:sentenza/play-grpc.git (push)

@sentenza
Copy link
Contributor Author

@sentenza It seems you force pushed this branch which overrides changes that we had merged in the main branch already (and my rebase with fixed conflicts yesterday). I fixed that now and force pushed myself again, so we should have a clean state now.

@mkurz I've double checked and the problem was generated by my previous pull strategy (ff). After switching to git config pull.rebase true this issue is gone. My remotes were already correct.

@mkurz
Copy link
Member

mkurz commented Dec 21, 2023

Depends on

Copy link
Member

@mkurz mkurz left a comment

Choose a reason for hiding this comment

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

Added some comments.
If you don't mind I will force push my changes which will override yours, but some are similar. Please take a look at my comments to see how I got it working.

CI should be green then.

build.sbt Outdated
@@ -211,4 +212,6 @@ val playInteropTestJava = Project("play-grpc-interop-test-java", file("play-inte
.enablePlugins(build.play.grpc.NoPublish)
.pluginTestingSettings

Test / javaOptions ++= Seq("--add-exports=java.base/sun.security.x509=ALL-UNNAMED")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to go into the CommonPlugin so it takes effect for all the projects.

play-scalatest/src/test/resources/application.conf Outdated Show resolved Hide resolved
override def newAppForTest(testData: TestData): Application = fakeApplication()

protected def newServerForTest(app: Application, testData: TestData): RunningServer =
testServerFactory.start(app)
Copy link
Member

Choose a reason for hiding this comment

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

No need to override all those methods, see my other comment.

@mkurz mkurz merged commit deeea2e into playframework:main Dec 21, 2023
8 checks passed
@sentenza
Copy link
Contributor Author

@mkurz I would have preferred to see my changes as well, but anyway thank you for solving the issues.

@sentenza
Copy link
Contributor Author

@mkurz are you going to publish a new version targeting Play 2.9?

@ihostage
Copy link
Member

@mkurz are you going to publish a new version targeting Play 2.9?

I think we should try to support Scala 3 before release a new version.

@mkurz
Copy link
Member

mkurz commented Dec 21, 2023

@mkurz I would have preferred to see my changes as well, but anyway thank you for solving the issues.

Sorry, I was working on it since yesterday and diffed to your branch some things were similiar but I had already commited more locally. To save time I just pushed for now instead of reverting some of your changes that weren't necessary and to safe time solving conflicts. Usually I don't do that, just now I wanted to move on quickly.

@mkurz are you going to publish a new version targeting Play 2.9?

Actually it would be nice if we could cross compile to Scala 3 first... I started a PR for this.
I will branch off a 0.11.x branch that will be for Play 2.9, the Pekko PR we can merge on main and then on top try to get Scala 3 working, because it is hard to get it working with 2.9 because akka-http 10.2 is not published for Scala 3, so this is more cumbersome. Once we have Scala 3 working in the main branch (on top of Pekko), we can backport the relevant Scala 3 changes to the 0.11.x branch and then figure out how to get things working with the old akka-http artifacts (probably using the cross(CrossVersion.for3Use2_13) workaround in some places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants