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

[2.29.x] Java 21 upgrades #6835

Open
wants to merge 12 commits into
base: 2.29.x
Choose a base branch
from

Conversation

malmgrens4
Copy link
Collaborator

What does this PR do?

Who is reviewing it?

Select relevant component teams:

Ask 2 committers to review/merge the PR and tag them here.

How should this be tested?

Any background context you want to provide?

What are the relevant tickets?

Fixes: #____

Screenshots

Checklist:

  • Documentation Updated
  • Update / Add Threat Dragon models
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Notes on Review Process

Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.

Review Comment Legend:

  • ✏️ (Pencil) This comment is a nitpick or style suggestion, no action required for approval. This comment should provide a suggestion either as an in line code snippet or a gist.
  • ❓ (Question Mark) This comment is to gain a clearer understanding of design or code choices, clarification is required but action may not be necessary for approval.
  • ❗ (Exclamation Mark) This comment is critical and requires clarification or action before approval.

@@ -182,7 +182,7 @@
<configuration>
<rules>
<requireFilesSize>
<maxsize>280000000</maxsize>
<maxsize>350000000</maxsize>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See if this can be reduced again

@@ -24,6 +24,7 @@
<!-- A list of blacklisted feature identifiers that can't be installed in Karaf and are not part of the distribution -->
<blacklistedFeatures>
<feature>pax-jetty-http2</feature>
<feature>pax-web-jetty-http2</feature>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The pax web upgrade comes with this (which we don't use)

@@ -139,7 +139,7 @@
description="Contains the modules that should be installed after the installer is finished.">
<feature>admin-modules-configuration</feature>
<feature>admin-modules-application</feature>
<feature>admin-modules-docs</feature>
<!-- <feature>admin-modules-docs</feature>-->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reintegrate

@@ -178,8 +178,9 @@
#######################################

# Set the authmethod/realmname so that our JettyAuthenticator is used
org.ops4j.pax.web.default.authmethod=DDF
org.ops4j.pax.web.default.realmname=DDF
org.ops4j.pax.web.default.authMethod=DDF
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Delete - these aren't being picked up anymore

<entry key="alias" value="/admin/jolokia"/>
<entry key="osgi.http.whiteboard.servlet.pattern" value="/jolokia/*"/>
<entry key="osgi.http.whiteboard.servlet.name" value="jolokia"/>
<entry key="osgi.http.whiteboard.context.select" value="(osgi.http.whiteboard.context.path=/*)"/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ddf-admin-ui apps register based on their Context-Paths. Previously pax-web/ jetty would still register all servlets under each context. Now any servlets that need to work with those WABs apps will need to register under /* for the context path (believe it defaults to just /).

@@ -27,7 +27,7 @@
<module>admin-modules-installer</module>
<module>admin-modules-configuration</module>
<module>admin-modules-application</module>
<module>admin-modules-docs</module>
<!-- <module>admin-modules-docs</module>-->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reintegrate

@@ -82,6 +82,7 @@ public void event(ServiceEvent event, Map<BundleContext, Collection<ListenerInfo

public void init() {
checkForMissedServletContexts();
LOGGER.trace("skipping error page initialization.");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The class reflection the error page injector used no longer works under j21. But registering a global error page is now easier with the new pax web version. So that should be done instead of this method.

import org.osgi.framework.BundleContext;
import org.osgi.framework.ServiceRegistration;

public class Activator implements BundleActivator {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was placed fairly arbitrarily so can probably go somewhere better.
Previously we were able to do this via config but those config values aren't supported anymore. So using this bundle activator we can set the auth method and realm name we need for triggering out JettyAuthenticator

@Override
public void start(BundleContext bundleContext) throws Exception {
DefaultSecurityConfigurationMapping security = new DefaultSecurityConfigurationMapping();
security.setAuthMethod("DDF");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switch to constants

@@ -37,20 +37,24 @@

<reference id="securityManager" interface="ddf.security.service.SecurityManager"/>

<service interface="javax.servlet.Servlet">

<bean id="locallogout" class="org.codice.ddf.security.servlet.logout.LocalLogoutServlet">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something worth noting: You can no longer create the bean in the service tags. Simply does not work with the latest upgrades.

@@ -62,6 +62,38 @@
<artifactId>security-core-services</artifactId>
<scope>test</scope>
</dependency>
<!-- &lt;!&ndash; Provided dependencies &ndash;&gt;-->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

delete

@@ -58,6 +65,9 @@
<configuration>
<instructions>
<Bundle-SymbolicName>${project.artifactId}</Bundle-SymbolicName>
<Import-Package>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needs to be investigated further

@malmgrens4
Copy link
Collaborator Author

build now

@malmgrens4 malmgrens4 changed the title [WIP] Java 21 upgrades Java 21 upgrades Jan 24, 2025
@malmgrens4
Copy link
Collaborator Author

build now

1 similar comment
@malmgrens4
Copy link
Collaborator Author

build now

@malmgrens4 malmgrens4 force-pushed the no-camel-with-updates branch from 85674b0 to 3ba0bb7 Compare January 24, 2025 02:55
@alexabird
Copy link
Contributor

alexabird commented Jan 24, 2025

Not sure why it "doesnt meet criteria" when it is the same as other branches.. I've seen jenkins do weird things when it doesnt like branch names, maybe that? Think we want to update the java version in the jenkins file too
image

@jlcsmith jlcsmith changed the title Java 21 upgrades [2.29.x] Java 21 upgrades Jan 31, 2025
@alexabird
Copy link
Contributor

The PR is showing up in Jenkins now, however is not buildable because of the merge conflicts
image

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.

2 participants