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

Camel Quarkus CXF Extension #3931

Merged
merged 1 commit into from Aug 7, 2022
Merged

Camel Quarkus CXF Extension #3931

merged 1 commit into from Aug 7, 2022

Conversation

javaduke
Copy link
Contributor

This PR adds Camel Quarkus CXF Extension, addressing the issue #764 . Both SOAP client and SOAP server supported, in JVM and native modes.

@zhfeng zhfeng mentioned this pull request Jul 22, 2022
<artifactId>camel-cxf-transport</artifactId>
</dependency>

<!-- CXF WS-* dependencies -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if these should be direct dependencies of the extension or not.

I guess they're just here for convenience so that if somebody wants to use them, they are automatically available. If that's the case, I think it'd be better moving them to the BOM, so that folks can add them manually to their project if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, you can't really use cxf without a transport, so I'm inclined to leave it here, but if you insist, I can move it back to the BOM, it's no big deal really.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not so much the transport. But this stuff:

        <dependency>
            <groupId>org.apache.cxf</groupId>
            <artifactId>cxf-rt-ws-rm</artifactId>
        </dependency>
        <dependency>
            <groupId>org.apache.cxf</groupId>
            <artifactId>cxf-rt-ws-addr</artifactId>
        </dependency>
        <dependency>
            <groupId>org.apache.cxf</groupId>
            <artifactId>cxf-rt-ws-security</artifactId>
        </dependency>
        <dependency>
            <groupId>org.apache.cxf</groupId>
            <artifactId>cxf-rt-ws-policy</artifactId>
        </dependency>

Presumably ws-security is not strictly required in every app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see, sure, I can move it out as well.


<dependency>
<groupId>org.apache.cxf</groupId>
<artifactId>cxf-rt-transports-http-jetty</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for the WS server part, right?

If so, similar to my previous comment, I don't think this should be a dependency of the extension by default. So it should be moved to the BOM.

However, I do not want us to be supporting HTTP transports like Jetty. We should be integrating with the Quarkus Vert.x HTTP server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I can move it out. I agree that ideally we want to support Vert.x, but CXF doesn't provide transport implementation for it (and I'm not sure if they will). I can try to implement one, but it may take a while. So as an interim solution let's support it and I'll see what I can do about supporting other transports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a ticket here: https://issues.apache.org/jira/browse/CXF-8742
Please feel free to add your comments and/or corrections.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it's mandatory that the endpoints are run on the Quarkus Vert.x HTTP server. Otherwise we're just making the app performance worse and exposing ourselves to more CVEs.

AFAIK quarkiverse-cxf has already solved this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried it, for some reason CXF does not recognize the VertxDestinationFactory that is provided by quarkiverse-cxf. I'll keep investigating.

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe we can consider to leverage on quarkus-cxf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to figure out how it's done there, but it seems like we'd have to rewrite Camel CXF completely, because there seems to be no way to pass the factories defined in the quarkus-cxf to Camel...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I think I know what's going on - currently Camel does not allow creating custom JaxWsServerFactroryBean: https://github.com/apache/camel/blob/main/components/camel-cxf/camel-cxf-soap/src/main/java/org/apache/camel/component/cxf/jaxws/CxfEndpoint.java#L683
This means that destination factories are initialized in the Camel CXF code, and apparently custom destination factories are ignored. Not sure if there's any way around it, apparently not...

Copy link
Contributor

Choose a reason for hiding this comment

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

@ffang can you take a look?

@javaduke
Copy link
Contributor Author

javaduke commented Aug 3, 2022

OK, I think we're done - this PR now uses Quarkus CXF for SOAP services and WS-Security, all tests pass correctly.

@javaduke
Copy link
Contributor Author

javaduke commented Aug 3, 2022

Apparently the Quarkus CXF uses a newer version of the xmlsec which causes test failures in the xmlsecurity extension, but I have a fix for that. See https://santuario.apache.org/faq.html 4. Secure Validation

@javaduke
Copy link
Contributor Author

javaduke commented Aug 5, 2022

Any updates? Comments?

@jamesnetherton
Copy link
Contributor

Any updates? Comments?

We should probably wait until Camel 3.18.1 is available (hopefully next week) because it fixes the dependency that was not properly test scoped:

https://github.com/apache/camel/blob/main/components/camel-cxf/camel-cxf-common/pom.xml#L104

So we wont need to pull in a bunch of redundant dependencies.

@jamesnetherton
Copy link
Contributor

Thinking also we should have a 'code first' test for the consumer. Right now we're just feeding the endpoint a WSDL file. Would be good to also test a JAX-WS annotated service interface.

@javaduke
Copy link
Contributor Author

javaduke commented Aug 5, 2022

Sure, I can work on code first test, although I see it a bit redundant, since the service interface is generated from the WSDL by the cxf-codegen-plugin during the build and then configured in the endpoint: camel.beans.soapServiceEndpoint.serviceClass=com.helloworld.service.HelloPortType

@javaduke
Copy link
Contributor Author

javaduke commented Aug 5, 2022

OK, code-first test added

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

Very nice @javaduke! Some nitpicks inline which I can try to fix myself tomorrow. Thank you very much for your endurance!

"org.apache.wss4j.dom.handler.WSHandler",
"org.apache.cxf.phase.AbstractPhaseInterceptor",
"org.apache.cxf.binding.soap.interceptor.AbstractSOAPInterceptor",
"org.apache.cxf.phase.PhaseInterceptor")
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these a bit overlapping with what we collect bellow via getAllKnownImplementors() ?
I mean AbstractFeature implements org.apache.cxf.feature.Feature,
AbstractPhaseInterceptor implements org.apache.cxf.phase.PhaseInterceptor, etc.
so each subclass if the given abstract class will implement the interface and will inevitably be caught by the getAllKnownImplementors() query, no?

Comment on lines +36 to +37
<camel.quarkus.jvmSince>2.11.0</camel.quarkus.jvmSince>
<camel.quarkus.nativeSince>2.11.0</camel.quarkus.nativeSince>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<camel.quarkus.jvmSince>2.11.0</camel.quarkus.jvmSince>
<camel.quarkus.nativeSince>2.11.0</camel.quarkus.nativeSince>
<camel.quarkus.jvmSince>2.12.0</camel.quarkus.jvmSince>
<camel.quarkus.nativeSince>2.12.0</camel.quarkus.nativeSince>

A bit too late for 2.11.0

@ppalaga ppalaga merged commit 2449a39 into apache:main Aug 7, 2022
@zbendhiba
Copy link
Contributor

Many thanks @javaduke for this great work! Happy to find out this has been merged.

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.

None yet

5 participants