8000 Upgrade Resteasy v4 by pedroigor · Pull Request #10917 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Upgrade Resteasy v4 - #10917

Merged
abstractj merged 1 commit into
keycloak:mainfrom
pedroigor:issue-10916
Jul 11, 2022
Merged

Upgrade Resteasy v4#10917
abstractj merged 1 commit into
keycloak:mainfrom
pedroigor:issue-10916

Conversation

@pedroigor
@pedroigor pedroigor commented Mar 24, 2022 •
edited
Loading
Copy link
Copy Markdown
Contributor

Closes #10916

  • Change version in parent pom to the version used in Wildfly
  • Change poms to use resteasy-core instead of resteasy-jaxrs
  • Wildfly modules do not need to change because Wildfly is still able to resolve org.jboss.resteasy:resteasy-jaxrs module dependency, which points to resteasy-core module.
  • Updates to the list of providers defined for the admin client.

pedroigor
pedroigor commented Mar 24, 2022
View reviewed changes
Comment thread testsuite/model/pom.xml Resolved
@pedroigor pedroigor mentioned this pull request Mar 24, 2022
Closed
@hmlnarik hmlnarik mentioned this pull request Mar 24, 2022
Closed
@pedroigor
pedroigor force-pushed the issue-10916 branch 6 times, most recently from 1581112 to cb1e83c Compare March 26, 2022 00:38
@DGuhr DGuhr mentioned this pull request Mar 29, 2022
Closed
@pedroigor
pedroigor force-pushed the issue-10916 branch 2 times, most recently from c0774b9 to 6f86a57 Compare March 29, 2022 12:42
@pedroigor
pedroigor marked this pull request as ready for review March 29, 2022 13:52
@pedroigor
pedroigor commented Mar 29, 2022
Copy link
Copy Markdown
Contributor Author

@abstractj Looks like test suite is passing now. The last failure was related to the providers defined in the javax.ws.rs.ext.Providers in the keycloak-admin-cli module. This module does not use JAX-RS at all and I removed the file from there.

In v4, enabling the GZIP providers was causing responses to be encoded twice. For instance, serving keycloak.js.

@abstractj
abstractj requested a review from hmlnarik March 29, 2022 20:17
@stianst stianst added this to the 19.0.0 milestone Mar 30, 2022
@stianst stianst added the status/hold PR should not be merged. On hold for later. label Mar 30, 2022
@stianst
stianst commented Mar 30, 2022
Copy link
Copy Markdown
Contributor

This needs to wait to after Keycloak 18, as EAP 7.4 is using RestEasy 3

@pedroigor pedroigor mentioned this pull request Mar 31, 2022
Closed
This was referenced Apr 1, 2022
Closed
Closed
@abstractj abstractj mentioned this pull request Apr 26, 2022
Closed
stianst
stianst reviewed May 4, 2022
View reviewed changes
@stianst stianst left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks okay to me, a few question/comments though:

  • We need to run internal pipelines against this
  • Can this cause some breaking changes to adapters/clients? Do we still support RestEasy 3 at all on the client side, for example in the admin client? Is upgrade to HttpClient a potentially breaking change? I presume we'll at the very least will require some notice around this in migration guide?
  • Unsure about the SPNEGO support in HttpClientBuilder. Do we use this for Kerberos support? If so we need to make sure this stuff is tested
  • Is this really two upgrades in one? HttpClient upgrade as well as RestEasy? I presume that's because RestEasy 4 requires a newer HttpClient?

@stianst stianst removed the status/hold PR should not be merged. On hold for later. label May 4, 2022
@stianst
stianst requested a review from mposolda May 4, 2022 08:17
@pedroigor
pedroigor commented May 5, 2022
Copy link
Copy Markdown
Contributor Author

Looks okay to me, a few question/comments though:

  • We need to run internal pipelines against this
  • Can this cause some breaking changes to adapters/clients? Do we still support RestEasy 3 at all on the client side, for example in the admin client? Is upgrade to HttpClient a potentially breaking change? I presume we'll at the very least will require some notice around this in migration guide?

I don't think it will be a breaking change. The reason is that adapters and clients are wrapping the dependency and we don't really expected people relying on internals.

But yeah, it deserves a release note at the very minimum.

  • Unsure about the SPNEGO support in HttpClientBuilder. Do we use this for Kerberos support? If so we need to make sure this stuff is tested

AFAIK, the testsuite (GHA) also checks SPNEGO. I can double check.

  • Is this really two upgrades in one? HttpClient upgrade as well as RestEasy? I presume that's because RestEasy 4 requires a newer HttpClient?

apache:http-client should be the same: 4.5.13.

@mposolda
mposolda commented May 6, 2022
Copy link
Copy Markdown
Contributor
  • Unsure about the SPNEGO support in HttpClientBuilder. Do we use this for Kerberos support? If so we need to make sure this stuff is tested

AFAIK, the testsuite (GHA) also checks SPNEGO. I can double check.

Yes, this stuff is used in the Kerberos tests (AbstractKerberosTest and it's subclasses). The only thing is, that before this change, the SPNEGO integration with HttpClient was done only in the testsuite. Now some SPNEGO is exposed also in the HttpClientBuilder class inside adapter-core library.

But I don't see any issues with the approach. SPNEGO is not needed by adapters itself, and so in theory shouldn't be needed in the HttpClientBuilder of adapter-core. But at the same time, HttpClientBuilder is an internal class. So I am fine with the changes of this PR.

@pedroigor
pedroigor commented Jun 10, 2022 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@mabartos I was able to reproduce and fix the issue by removing this dependency from the tomcat classpath:

<artifactItem>
    <groupId>org.jboss.resteasy</groupId>
    <artifactId>resteasy-servlet-initializer</artifactId>
    <version>${tomcat.resteasy.version}</version>
</artifactItem>

Would you be able to check it again?

You should also be able to run using the IDE.

mabartos
mabartos reviewed Jun 13, 2022
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pedroigor Thanks for fixing the issue with Tomcat.

I tried to reproduce it and those tests passed for Tomcat 8 and Tomcat 9. I don't think is necessary to execute pipeline tests, as only the Tomcat module was affected. Those changes were locally verified and the correctness is confirmed.

Regarding the mentioned changes for SAML HttpClientBuilder, I'd like to resolve it in some follow-up task in order to speed up the process as it's not a blocking issue.
Could you please create an issue for it?

@mabartos
mabartos commented Jun 24, 2022
Copy link
Copy Markdown
Member

@pedroigor What's the current status? Anything else need to be done?

@pedroigor
pedroigor commented Jun 28, 2022
Copy link
Copy Markdown
Contributor Author

@mabartos For me, it is done. Need someone to review/approve ...

abstractj
abstractj previously approved these changes Jun 28, 2022
View reviewed changes
mabartos
mabartos previously approved these changes Jun 28, 2022
View reviewed changes
@mabartos mabartos left a comment •
edited
Loading
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Haven't got a chance to take a look at the code more deeply, but from a QE perspective, I can approve this PR. Pipeline tests passed and the necessary manual testing of a particular component passed as well.

@pedroigor Please, don't forget to create a follow-up issue to apply the HttpClientBuilder changes also to the representation of HttpClientBuilder in the adapters/saml module. Thanks!

It'd be also good to retry the GH actions for Base testsuite (undertow-map, group1) combination.

@ahus1
ahus1 self-requested a review July 1, 2022 09:22
ahus1
ahus1 previously approved these changes Jul 1, 2022
View reviewed changes
@ahus1 ahus1 left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My previous comments have been addressed.

@abstractj
abstractj commented Jul 1, 2022
Copy link
Copy Markdown
Contributor

@pedroigor could you please double-check the failures on the Keycloak CI / Base testsuite? Not sure if they are related with those changes.

@pedroigor
pedroigor commented Jul 4, 2022
Copy link
Copy Markdown
Contributor Author

@abstractj Does not seem related. Running the failing tests.

@pedroigor
pedroigor dismissed stale reviews from ahus1, mabartos, and abstractj via ca6917d July 4, 2022 19:02
@abstractj
abstractj requested review from ahus1 and mabartos July 5, 2022 14:27
@abstractj
abstractj commented Jul 5, 2022
Copy link
Copy Markdown
Contributor

@pedroigor waiting for the CI feedback for us to merge.

@ahus1
ahus1 commented Jul 8, 2022
Copy link
Copy Markdown
Member

The current run looks a lot better, all undertow tests completed. The quickstart test seems unrelated to this change and flaky. Maybe a maintainer could rerun it.

As I was involved in preparing this PR on the last yards (fixing a double dependency in the test suite), I am hesitant to approve it. Still, I approve the rest of it.

@pedroigor
pedroigor force-pushed the issue-10916 branch 2 times, most recently from 4f9aa5f to 1d262cc Compare July 8, 2022 17:43
@pedroigor
pedroigor commented Jul 8, 2022
Copy link
Copy Markdown
Contributor Author

@ahus1 Thanks for the fix. PR rebased and squashed.

@pedroigor @ahus1
Upgrade Resteasy v4
1d262cc 
Closes keycloak#10916

Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
ahus1
ahus1 approved these changes Jul 8, 2022
View reviewed changes
@ahus1 ahus1 left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving Pedros' parts. Thanks for the PR, happy to join this teamwork!

@mabartos
mabartos commented Jul 11, 2022 •
edited
Loading
Copy link
Copy Markdown
Member

Started a pipeline run to be sure.

https://keycloak-build-pipeline.com/job/keycloak-build-pipeline/809/

@abstractj
abstractj commented Jul 11, 2022
Copy link
Copy Markdown
Contributor

Fantastic work! I'm waiting for @mabartos feedback, so we can merge.

mabartos
mabartos approved these changes Jul 11, 2022
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Failures in the server and adapter pipeline are not related to this PR.

@abstractj
abstractj merged commit 5b48d72 into keycloak:main Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abstractj abstractj abstractj approved these changes
@ahus1 ahus1 ahus1 approved these changes
@mabartos mabartos mabartos approved these changes
@stianst stianst stianst left review comments
@hmlnarik hmlnarik Awaiting requested review from hmlnarik
@mposolda mposolda Awaiting requested review from mposolda

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

19.0.0

Development

Successfully merging this pull request may close these issues.

Upgrade Resteasy v4

7 participants

@pedroigor @stianst @mposolda @abstractj @mabartos @ahus1 @hmlnarik
0