davsclaus commented on code in PR #28991: URL: https://github.com/apache/camel/pull/28991#discussion_r4448886243
########## components/camel-netty/src/test/java/org/apache/camel/component/netty/NettyTransferExchangeConnectionHeadersTest.java: ########## @@ -0,0 +1,157 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.netty; + +import java.io.File; +import java.net.InetSocketAddress; + +import io.netty.channel.ChannelHandler; +import io.netty.handler.codec.serialization.ClassResolvers; +import org.apache.camel.BindToRegistry; +import org.apache.camel.Exchange; +import org.apache.camel.Message; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.component.mock.MockEndpoint; +import org.apache.camel.component.netty.codec.ObjectDecoder; +import org.apache.camel.component.netty.codec.ObjectEncoder; +import org.apache.camel.test.AvailablePortFinder; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledIfSystemProperty; +import org.junit.jupiter.api.extension.RegisterExtension; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * With transferExchange the consumer keeps its own connection headers: the connection it accepted is the source of + * truth, so the connection headers and properties in the transferred exchange are not used. + */ +@DisabledIfSystemProperty(named = "java.vendor", matches = ".*ibm.*") +public class NettyTransferExchangeConnectionHeadersTest extends BaseNettyTest { + + private static final String CLIENT_CERT_SUBJECT + = "SERIALNUMBER=1234567890, CN=CommonName, OU=Unit, O=Org, L=Loc, ST=State, C=US"; + private static final String CODECS = "&encoders=#encoder&decoders=#decoder"; + private static final String SSL_OPTIONS = "&ssl=true&passphrase=changeit&keyStoreResource=#ksf&trustStoreResource=#tsf"; + + @RegisterExtension + AvailablePortFinder.Port sslPort = AvailablePortFinder.find(); + + @BindToRegistry("ksf") + public File loadKeystoreKsf() { + return new File("src/test/resources/keystore.jks"); + } + + @BindToRegistry("tsf") + public File loadKeystoreTsf() { + return new File("src/test/resources/keystore.jks"); + } + + @BindToRegistry("encoder") + public ChannelHandler getEncoder() { + return new ShareableChannelHandlerFactory(new ObjectEncoder()); + } + + @BindToRegistry("decoder") + public ChannelHandler getDecoder() { + return new DefaultChannelHandlerFactory() { + @Override + public ChannelHandler newChannelHandler() { + return new ObjectDecoder(ClassResolvers.weakCachingResolver(null)); + } + }; + } + + @Test + public void testConsumerKeepsItsConnectionHeaders() throws Exception { + MockEndpoint mock = getMockEndpoint("mock:plain"); + mock.expectedBodiesReceived("Hello World"); + mock.expectedHeaderReceived("cheese", "gouda"); + + Exchange reply = template.request("netty:tcp://localhost:{{port}}?sync=true&transferExchange=true" + CODECS, e -> { + e.getIn().setBody("Hello World"); + e.getIn().setHeader("cheese", "gouda"); + e.getIn().setHeader(NettyConstants.NETTY_REMOTE_ADDRESS, "10.1.2.3:4567"); + e.getIn().setHeader(NettyConstants.NETTY_LOCAL_ADDRESS, "10.1.2.3:8910"); + e.getIn().setHeader(NettyConstants.NETTY_SSL_CLIENT_CERT_SUBJECT_NAME, "CN=Sender"); + e.getIn().setHeader(NettyConstants.NETTY_SSL_CLIENT_CERT_SERIAL_NO, "42"); + e.setProperty(NettyConstants.NETTY_REMOTE_ADDRESS, "10.1.2.3:4567"); + }); + assertNull(reply.getException()); + + MockEndpoint.assertIsSatisfied(context); + + Exchange received = mock.getReceivedExchanges().get(0); + Message in = received.getIn(); + // the addresses of the connection the consumer accepted + assertInstanceOf(InetSocketAddress.class, in.getHeader(NettyConstants.NETTY_REMOTE_ADDRESS)); + InetSocketAddress local = assertInstanceOf(InetSocketAddress.class, in.getHeader(NettyConstants.NETTY_LOCAL_ADDRESS)); + assertEquals(getPort(), local.getPort()); + // no client certificate on this connection, so no client certificate headers + assertNull(in.getHeader(NettyConstants.NETTY_SSL_CLIENT_CERT_SUBJECT_NAME)); + assertNull(in.getHeader(NettyConstants.NETTY_SSL_CLIENT_CERT_SERIAL_NO)); + assertNull(received.getProperty(NettyConstants.NETTY_REMOTE_ADDRESS)); + + // the producer keeps its own connection headers too: the reply has none from the consumer + Message message = reply.getMessage(); + assertEquals("Bye World", message.getBody()); + assertEquals("cheddar", message.getHeader("cheese")); + assertNull(message.getHeader(NettyConstants.NETTY_SSL_CLIENT_CERT_SUBJECT_NAME)); Review Comment: Yes, both assertions pass, and both tests fail without the fix. The reply does get `Bye World` because `transform()` puts its result on the OUT message: `TransformProcessor` calls `exchange.setOut(exchange.getIn())` when the exchange has no OUT yet (or `ExchangeHelper.replaceMessage(exchange, msg, true)` for a specialized message). So on the consumer side `exchange.hasOut()` is true when the reply is written, `ServerChannelHandler.getResponseBody` calls `NettyPayloadHelper.getOut`, and `DefaultExchangeHolder.marshal` fills `outBody` and `outHeaders` (`cheese=cheddar` was set on the same message just before). On the producer side `DefaultExchangeHolder.unmarshal` then sets them on the OUT of the producer exchange. A temporary print in the test showed `hasOut=true in=Bye World out=Bye World` for `reply`, so `reply.getMessage().getBody()` is the OUT body `Bye World`. `NettyTransferExchangeOptionTest` sets the OUT body explicitly; with `transform()` Camel does that for you. Evidence on 66146d92193c: - `mvn -o test -Dtest=NettyTransferExchangeConnectionHeadersTest`: 2 tests, 0 failures. - With `NettyPayloadHelper` taken from `main` (without this fix), both fail: `testConsumerKeepsItsConnectionHeaders:102` "expected: <java.net.InetSocketAddress> but was: <null>" (the String header from the sender replaced the consumer's address), and `testConsumerKeepsItsClientCertHeaders` "mock://ssl Header with name CamelNettySSLClientCertSubjectName ... Expected: <SERIALNUMBER=1234567890, CN=CommonName, ...> but was: <CN=Sender>". - Full `camel-netty` build with `-Psourcecheck`: 151 tests, 0 failures, 9 skipped. No change to the test. _Claude Code on behalf of Claus Ibsen (davsclaus)_ ########## components/camel-netty/src/main/java/org/apache/camel/component/netty/NettyConfiguration.java: ########## @@ -97,7 +97,10 @@ public class NettyConfiguration extends NettyServerBootstrapConfiguration implem description = "Only used for TCP. You can transfer the exchange over the wire instead of just the body. The following fields are" + " transferred: In body, Out body, fault body, In headers, Out headers, fault headers, exchange properties, exchange" + " exception. This requires that the objects are serializable. Camel will exclude any non-serializable objects and" - + " log it at WARN level.") + + " log it at WARN level. The connection headers (CamelNettyRemoteAddress, CamelNettyLocalAddress," + + " CamelNettyChannelHandlerContext, CamelNettySSLSession and the CamelNettySSLClientCert headers) are always" Review Comment: Done in 66146d92193c. The `transferExchange` `@UriParam` description and the `setTransferExchange` javadoc now say that the connection headers (`CamelNettyRemoteAddress`, `CamelNettyLocalAddress`, `CamelNettyChannelHandlerContext`, `CamelNettySSLSession` and the `CamelNettySSLClientCert` headers) **and the `CamelNettyRemoteAddress` and `CamelNettyChannel` exchange properties** are always the ones of the receiving side. That matches the upgrade guide entry. I regenerated `netty.json` and `netty-http.json` (component and catalog) and the endpoint and component DSL builders for netty and netty-http, and committed them. _Claude Code on behalf of Claus Ibsen (davsclaus)_ ########## components/camel-netty/src/main/java/org/apache/camel/component/netty/NettyPayloadHelper.java: ########## @@ -151,4 +153,47 @@ public static void setOut(Exchange exchange, Object payload) { } } + /** + * The headers and exchange properties that describe the Netty connection of this side. + */ + private static final String[] CONNECTION_KEYS = { + NettyConstants.NETTY_CHANNEL_HANDLER_CONTEXT, NettyConstants.NETTY_MESSAGE_EVENT, + NettyConstants.NETTY_REMOTE_ADDRESS, NettyConstants.NETTY_LOCAL_ADDRESS, NettyConstants.NETTY_CHANNEL, + NettyConstants.NETTY_SSL_SESSION, NettyConstants.NETTY_SSL_CLIENT_CERT_SUBJECT_NAME, + NettyConstants.NETTY_SSL_CLIENT_CERT_ISSUER_NAME, NettyConstants.NETTY_SSL_CLIENT_CERT_SERIAL_NO, + NettyConstants.NETTY_SSL_CLIENT_CERT_NOT_BEFORE, NettyConstants.NETTY_SSL_CLIENT_CERT_NOT_AFTER }; + + /** + * Unmarshals the transferred exchange into the exchange, but keeps the connection headers and exchange properties + * of this side: the connection this side accepted (or opened) is the source of truth for them. A connection header + * or property in the transferred exchange is never used, also when this side has no value for it (for example the + * SSL client certificate headers when the connection has no client certificate). Review Comment: Confirmed: on this branch (with main merged in), `NETTY_MESSAGE_EVENT` is only declared in `NettyConstants` and listed in the key arrays of `NettyPayloadHelper`. Nothing in camel-netty or camel-netty-http sets it as a header or an exchange property. In 66146d92193c I removed it from `CONNECTION_KEYS`, and I dropped `CamelNettyMessageEvent` from the key list in the PR description. The upgrade guide entry of this PR never listed it. `TRANSPORT_KEYS` (the send-side list from main's CAMEL-26504 change) still lists it, and so does main's upgrade-guide paragraph about that change. I left both alone in this PR because that is main's code and it does no harm. They could be cleaned up when the two key lists are merged into one. _Claude Code on behalf of Claus Ibsen (davsclaus)_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
