Jetty12 removal of all property is causing possible migration difference from some customer using external libs

[ludoch]

This is only for AppEngine customer using Jetty12 instead of Jetty9.

Some external library is still testing appengine path with the presence of this property being not NULL: "org.eclipse.jetty.util.log.class"

But it was removed in Jetty12 code (other than in

jetty.project/jetty-core/jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/Slf4jEffort.java

Line 167 in 369d9f7

return getLineStream(propFile).anyMatch((line) -> line.startsWith("org.eclipse.jetty.util.log.class="));

)

We could have the Jetty12 code base to define it again with a value "unused" and set it up when the jetty legacy flag is used (which is used for AppEngine code: com.google.apphosting.runtime.jetty94.LEGACY_MODE

So this bug would be for Jetty to treat the value of unused the same as being a null value in the jetty link above.

See usage of this property in
https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L68
and
https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L321

[joakime]

Starting in Jetty 10, Jetty uses slf4j entirely.

There are no Jetty logging classes anymore in Jetty 10 onwards.
All of the infrastructure for Jetty logging has been removed.

[joakime]

I looks like the logic in https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L319-L322 is attempting to figure out if jetty logging is present.

But why?
That property (org.eclipse.jetty.util.log.class), back in Jetty 5 thru Jetty 9 days, was optional and not always set.
It is entirely possible, and quite common, to run without that property being set.

[joakime]

Back in Jetty 9, here's how that property was being used.

https://github.com/jetty/jetty.project/blob/jetty-9.4.54.v20240208/jetty-util/src/main/java/org/eclipse/jetty/util/log/Log.java#L113

__logClass = __props.getProperty("org.eclipse.jetty.util.log.class", "org.eclipse.jetty.util.log.Slf4jLog");

As you can see if the property is unset, it defaults to org.eclipse.jetty.util.log.Slf4jLog

Also, the fact that your code is only attempting to access it via a System.getProperty() is also bizarre and broken.
When that property exists, it usually exists entirely within the jetty-logging.properties resource file.
Almost never as a System property.

[joakime]

I'm at a loss on how you got that value set as a System property in the first place.
Nothing in Jetty 9 sets that as a System property.
In fact, nothing in Jetty 9 sets the name org.eclipse.jetty.util.log.class as a property anywhere, it is only ever read from code I listed above (basically meaning it is a read-only property).

Looking at the branches jetty-9.4.x and jetty-12.0.x I can see the following System.setProperty() calls from the Jetty codebase.

Name	Artifact	Jetty 9.4.x	Jetty 12.0.x
jetty.home	jetty-start	yes	yes
jetty.base	jetty-start	yes	yes
jetty.version	jetty-start	yes	yes
jetty.tag.version	jetty-start	yes	yes
jetty.build	jetty-start	yes	yes
org.eclipse.jetty.start.testing	jetty-start	yes	yes
jetty.ant.server.port	jetty-ant	yes	no
jetty.ant.server.host	jetty-ant	yes	no
org.apache.commons.logging.Log	from jetty-util JettyLogHandler	yes	no
com.sun.security.enableCRLDP	from jetty-util's CertificateValidator	yes	yes

In short, aside from the com.sun.security.enableCRLDP all of the rest of the properties only exist if using jetty-start (from a jetty-home / jetty-base setup).
If you are using Jetty embedded then only the com.sun.security.enableCRLDP has a chance to be set, if you are using Certificate validation from jetty-util.

[ludoch]

I understand.
The ask is that

jetty.project/jetty-core/jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/Slf4jEffort.java

Line 167 in 369d9f7

return getLineStream(propFile).anyMatch((line) -> line.startsWith("org.eclipse.jetty.util.log.class="));

code would react nicely with a null or non null but usesless value, so we can keep the presence of this property inside our code without Slf4jEffort.java in head being impacted in terms of current logic today.

[ludoch]

I can as well do a PR in the 3p library, but we would still have GAE customers using java21/jetty12 with possibly old deps and they would see different app behavior, making migration to newer GAE runtime more difficult due to internal changes affecting external app behavior.

[joakime]

If the goal of that line is to produce a warning (in a System.err) if found during the Slf4jEffort utility class (not a runtime class in Jetty).
See https://github.com/jetty/jetty.project/blob/jetty-12.0.7/jetty-core/jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/Slf4jEffort.java#L103-L107

Still doesn't help you with the System.getProperty call you have in your code.
https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L68

  static final String RUNTIME_JETTY_LOGGER = System.getProperty("org.eclipse.jetty.util.log.class");

Now I'm confused in a different way.
Are you wanting the Slf4jEffort changed to also find things like the System.getProperty() call?
Basically, anywhere that string shows up in the code, not just the import statements? (eg: strings, comments, module-info.java, *.mod files, *.ini files, etc)

[joakime]

Also, moving entirely to slf4j is a good thing, even on Jetty 6/7/8/9 as slf4j will work there as well.
And slf4j can be setup to route any existing logging API to slf4j, and then slf4j can further be setup to have that one slf4j point be the logging implementation to send logging events to (eg: java.util.logging, or logback, or log4j2, etc)

Perhaps a better solution would be a new artifact that is the old school Jetty logging, aka org.eclipse.jetty.util.log.* classes as a proper slf4j bridge API. (taking those events and routing them to slf4j)

[ludoch]

I see!

Anyway, what I think I'll do on the GAE side is define this prop with value "unused" if it is not set before.

[joakime]

Here's a quick example of slf4j ...

graph TD;
  lib-using-log4j1-api --> log4j-over-slf4j-bridge;
  lib-using-log4j2-api --> log4j2-to-slf4j-bridge;
  lib-using-commons-logging-api --> jcl-over-slf4j-bridge;
  lib-using-java-util-logging-api --> jul-to-slf4j-bridge;
  lib-using-slf4j-api --> slf4j-api;
  log4j-over-slf4j-bridge --> slf4j-api;
  log4j2-to-slf4j-bridge --> slf4j-api;
  jcl-over-slf4j-bridge --> slf4j-api;
  jul-to-slf4j-bridge --> slf4j-api;
  slf4j-api --> slf4j-jcl-impl;
  slf4j-api --> logback;
  slf4j-api --> jetty-slf4j-impl;
  slf4j-api --> log4j1-impl;
  slf4j-api --> log4j2-impl;
  slf4j-api --> java.util.logging;
  slf4j-api --> android-logging;

What we can do is create a jetty-legacy-logging-bridge artifact, that would sit at the top and pretend to be org.eclipse.jetty.util.log.* classes and just route to slf4j-api.

[lachlan-roberts]

I have opened GoogleCloudPlatform/appengine-java-standard#100 to resolve this, so I will close this issue.

The system property was set by the JettyServletEngineAdapter for the Jetty 9.4 runtimes.

https://github.com/GoogleCloudPlatform/appengine-java-standard/blob/0efe7e0ef41e44c0e550231de8928daa982d0e87/runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyServletEngineAdapter.java#L55-L63