[KEYCLOAK-11719] - Remove need for servlets/undertow from Quarkus dist by bb-matthewc · Pull Request #6600 · keycloak/keycloak

bb-matthewc · 2019-12-13T11:28:02Z

No description provided.

pedroigor · 2019-12-16T18:18:14Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+public class QuarkusFilter implements javax.ws.rs.container.ContainerRequestFilter,
+        javax.ws.rs.container.ContainerResponseFilter  {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger("QuarkusFilter");


Maybe just use QuarkusFilter.class.getName() here ?

pedroigor · 2019-12-16T18:20:45Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+            String keycloakSession = "";
+            for (String item: list) {
+                //NewCookie adds quotes around the cookie value for the KEYCLOAK_SESSION (due to /), so skipping this one.
+                if (item.contains("KEYCLOAK_SESSION")) {


We have this constant already with the cookie name org.keycloak.services.managers.AuthenticationManager#KEYCLOAK_SESSION_COOKIE.

I think same goes for Set-Cookie which can be grabbed from javax.ws.rs.core.HttpHeaders#SET_COOKIE.

We should wait for fix in Quarkus here, as this stuff is not very nice

pedroigor · 2019-12-16T18:26:20Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+@Provider
+@Priority(1)
+public class QuarkusFilter implements javax.ws.rs.container.ContainerRequestFilter,
+        javax.ws.rs.container.ContainerResponseFilter  {


I'm not sure whether or not a ContainerResponseFilter is invoked if an exception is thrown by applications. If not, we won't be able to perform the ending aspects of this filter such as cleaning up sessions, transactions, etc.

I think back in the early days I did try to use a JAX-RS filter instead of the servlet filter, but it didn't always work. Can't remember what the problems where though, just that I ended up with a servlet filter because I couldn't get JAX-RS filter to work fully

pedroigor · 2019-12-16T18:31:29Z

Two main comments:

Shall we wait for issue #1340: allow multiple Set-Cookie headers vert-x3/vertx-web#1341 to me merged and be available from Quarks ? It seems to me that we would avoid those additional changes. In fact, I told you that I would be checking with Quarkus team their plans for this and I did not.
See my comment about the execution order of ContainerResponseFilter.

Thanks.

pedroigor · 2019-12-17T01:05:02Z

@jonm-bb, this change from Stuart should help with the cookie issue quarkusio/quarkus#6218.

bb-matthewc · 2019-12-18T10:21:54Z

Hi @pedroigor, thanks for the review. Makes sense to wait as the code will be a lot cleaner! Will fix up the other comments and push the changes when the cookie fix is in place.

stianst · 2019-12-16T15:05:15Z

pom.xml

            <modules>
                <module>quarkus</module>
            </modules>
+            <build>


I assume this adds jandex indexes for all modules. Is that needed? Or do we just need it on Hibernate entities and services?

I don't think it will have any side-effect on having jandex indexes included on WildFly, but we'd need to check that since we need to build everything in one go when doing releases.

I remember it complained about a fair number missing from the index and I tried adding them individually. Will see if I can break it down into something more manageable than everything...

stianst · 2019-12-16T15:06:36Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+    public void filter(ContainerRequestContext containerRequestContext) throws IOException {
+
+        if (containerRequestContext.getMediaType() == null) {
+            containerRequestContext.getHeaders().add("Content-Type", MediaType.APPLICATION_FORM_URLENCODED);


This seems like it may have some unforeseen side-effects

stianst · 2019-12-18T11:56:40Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+@Provider
+@Priority(1)
+public class QuarkusFilter implements javax.ws.rs.container.ContainerRequestFilter,
+        javax.ws.rs.container.ContainerResponseFilter  {


I think back in the early days I did try to use a JAX-RS filter instead of the servlet filter, but it didn't always work. Can't remember what the problems where though, just that I ended up with a servlet filter because I couldn't get JAX-RS filter to work fully

stianst · 2019-12-18T11:56:48Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+public class QuarkusFilter implements javax.ws.rs.container.ContainerRequestFilter,
+        javax.ws.rs.container.ContainerResponseFilter  {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger("QuarkusFilter");


stianst · 2019-12-18T11:57:40Z

quarkus/extensions/src/main/java/org/keycloak/provider/quarkus/QuarkusFilter.java

+            String keycloakSession = "";
+            for (String item: list) {
+                //NewCookie adds quotes around the cookie value for the KEYCLOAK_SESSION (due to /), so skipping this one.
+                if (item.contains("KEYCLOAK_SESSION")) {


We should wait for fix in Quarkus here, as this stuff is not very nice

stianst · 2019-12-18T11:58:40Z

quarkus/server/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>io.quarkus</groupId>
+            <artifactId>quarkus-vertx-web</artifactId>


Do we need quarkus-vertx-web? Not sure what it does, but I thought we only needed quarkus-verts and quarkus-resteasy

stianst · 2019-12-18T11:59:30Z

quarkus/server/src/main/resources/application.properties

+
+# Required for jandex index
+quarkus.index-dependency.MultivalueMap.group-id=org.jboss.spec.javax.ws.rs
+quarkus.index-dependency.MultivalueMap.artifact-id=jboss-jaxrs-api_2.1_spec


Seems a bit strange that this is needed and wouldn't then be handled by quarkus-resteasy automatically?

stianst · 2019-12-18T12:01:15Z

services/src/main/java/org/keycloak/services/util/ObjectMapperResolver.java

 public class ObjectMapperResolver implements ContextResolver<ObjectMapper> {
    protected ObjectMapper mapper = new ObjectMapper();

+    public ObjectMapperResolver() {}


Perhaps it would be better to just change the constructor below to use the system property directly instead of getting "indent" passed from somewhere else?

If we just use the ObjectMapper instance from JsonSerialization instead of creating another instance, do we would have any collateral effect?

pedroigor · 2020-05-12T18:55:56Z

@jonm-bb I've opened #7063. Basically, your changes plus others.

Do you want to have those changes here so we can continue discussions on this PR or can we move discussions to that one? Whatever you think is the best as you did most of the work :)

stianst · 2020-05-14T09:21:48Z

Merged in #7063.

Thanks @bb-matthewc

[KEYCLOAK-11719] - Remove need for servlets/undertow from Quarkus dist

c0a2a26

stianst self-requested a review December 16, 2019 14:18

stianst self-assigned this Dec 16, 2019

stianst requested a review from pedroigor December 16, 2019 14:18

pedroigor requested changes Dec 16, 2019

View reviewed changes

stianst reviewed Dec 18, 2019

View reviewed changes

stianst added the Hold label Jan 10, 2020

stianst added this to the 10.0.0 milestone Mar 17, 2020

stianst removed the Hold label Apr 20, 2020

stianst modified the milestones: 10.0.0, 11.0.0 Apr 21, 2020

stianst added the Quarkus label May 12, 2020

pedroigor mentioned this pull request May 12, 2020

[KEYCLOAK-11719] - Remove need for servlets/undertow from Quarkus dist #7063

Merged

stianst closed this May 14, 2020

Conversation

bb-matthewc commented Dec 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Dec 16, 2019

Uh oh!

pedroigor commented Dec 17, 2019

Uh oh!

bb-matthewc commented Dec 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor commented May 12, 2020

Uh oh!

stianst commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants