From a3118cebde19fd2b481d60af08cfe1748425d094 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sun, 11 Aug 2024 04:17:38 +0200 Subject: [PATCH] Deprecate NetconfHelloMessageAdditionalHeader This class should not exist, but getting rid of it requires us to do quite some work, which in turn has impacts in netconf-{codec,netty-util} and perhaps somewhere else as well. Deprecate this class for removal to drive its replacements. Also provide a lot of FIXMEs explaining the various changes to netconf-{api,codec,client,server} that need to happen to solidify the pipeline layout, the fool-proof the end user APIs and provide reliable operation. Change-Id: I8ec055dc6f032df3171cd0dddc78f33498956fde Signed-off-by: Robert Varga --- .../nettyutil/AbstractChannelInitializer.java | 11 ++++++ .../nettyutil/AbstractNetconfSession.java | 34 ++++++++++++++++--- .../handler/HelloXMLMessageDecoder.java | 9 +++++ .../netconf/api/NetconfSession.java | 29 ++++++++++++++++ .../NetconfHelloMessageAdditionalHeader.java | 21 ++++++++++++ .../netconf/codec/FrameDecoder.java | 13 +++++++ .../netconf/codec/MessageDecoder.java | 10 ++++++ .../netconf/codec/MessageEncoder.java | 4 +++ .../netconf/server/NetconfServerSession.java | 22 ++++++++++++ 9 files changed, 149 insertions(+), 4 deletions(-) diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractChannelInitializer.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractChannelInitializer.java index 56753acf61..17db5e177f 100644 --- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractChannelInitializer.java +++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractChannelInitializer.java @@ -17,9 +17,18 @@ import org.opendaylight.netconf.codec.MessageDecoder; import org.opendaylight.netconf.codec.MessageEncoder; import org.opendaylight.netconf.nettyutil.handler.HelloXMLMessageDecoder; +// FIXME: This is not related Netty's'ChannelInitializer' and is pretty much unused. +// Perhaps a NetconfConnectionFactory? It would: +// - take a TransportChannel +// - initialize the pipeline with MessageEncoder +// - produce a NetconfConnection, which has: +// - TransportChannel transport() +// - MessageEncoder messageEncoder() public abstract class AbstractChannelInitializer { public static final String NETCONF_SESSION_NEGOTIATOR = "negotiator"; + // FIXME: this should be taking only a TransportChannel and should be factory method producing NetconfConnections, + // without the 'attach a negotiator with some promise' part public void initialize(final Channel ch, final Promise promise) { ch.pipeline().addLast(FrameDecoder.HANDLER_NAME, new EOMFrameDecoder()); initializeMessageDecoder(ch); @@ -40,5 +49,7 @@ public abstract class AbstractChannelInitializer { * Insert session negotiator into the pipeline. It must be inserted after message decoder * identified by {@link MessageDecoder#HANDLER_NAME}, (or any other custom decoder processor) */ + // FIXME: This should be somewhere else. It should take an already-initialized NetconfConnection and perform + // the listener magic of eventually giving out the negotiated NetconfSession. protected abstract void initializeSessionNegotiator(Channel ch, Promise promise); } diff --git a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSession.java b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSession.java index c5fe45f224..ece47d8d30 100644 --- a/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSession.java +++ b/netconf/netconf-netty-util/src/main/java/org/opendaylight/netconf/nettyutil/AbstractNetconfSession.java @@ -28,12 +28,18 @@ import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.netconf.base._1._0.re import org.slf4j.Logger; import org.slf4j.LoggerFactory; +// FIXME: move this class to netconf.common public abstract class AbstractNetconfSession> + // FIXME: This is fugly: we receive either NetconfNotification or Exception, routing it to listener. + // It would be much better if we communicated Exception via something else than channelRead(), for + // example via userEventTriggered(). The contract of what can actually be seen is dictated by + // MessageDecoder in general. extends SimpleChannelInboundHandler implements NetconfSession { private static final Logger LOG = LoggerFactory.getLogger(AbstractNetconfSession.class); private final @NonNull SessionIdType sessionId; private final @NonNull L sessionListener; + // FIXME: we should have a TransportChannel available private final @NonNull Channel channel; private boolean up; @@ -45,6 +51,11 @@ public abstract class AbstractNetconfSession * This interface should be implemented by a final class representing a protocol specific session. */ +@NonNullByDefault public interface NetconfSession extends Closeable { + /** + * Return the {@code session-id} of this session. + * + * @return the {@code session-id} of this session + */ + SessionIdType sessionId(); + // FIXME: This is overly generalized leading to potential bad use: + // - HelloMessage cannot be sent as it is used only before NetconfSession is established + // - RpcMessage can only be sent via ClientSession + // - RpcReply can only be sent on a ServerSession + // - NotificationMessage can only be sent on a ServerSession which has seen a subscription request + // + // There are further state management issues: without interleave capability the client gives up the right + // to execute most operations. + // + // At the end of the day, this method should be exposed from AbstractNetconfSession only, with perhaps a + // different signature. ChannelFuture sendMessage(NetconfMessage message); + // FIXME: this is ambiguous w.r.t.: + // - protocol interactions: + // - is it , https://www.rfc-editor.org/rfc/rfc6241#section-7.9, available to both parties? + // - is it , https://www.rfc-editor.org/rfc/rfc6241#section-7.8, available to client? + // - state transitions: + // - what happens sendMessage()'s still in the output TCP buffer? + // - what happens to those that are on the wire? + // - synchronicity: is this a non-blocking operation? + // - if it is blocking: what is its non-blocking equivalent? @Override void close(); } diff --git a/protocol/netconf-api/src/main/java/org/opendaylight/netconf/api/messages/NetconfHelloMessageAdditionalHeader.java b/protocol/netconf-api/src/main/java/org/opendaylight/netconf/api/messages/NetconfHelloMessageAdditionalHeader.java index 17374ead4d..d45a3e05ea 100644 --- a/protocol/netconf-api/src/main/java/org/opendaylight/netconf/api/messages/NetconfHelloMessageAdditionalHeader.java +++ b/protocol/netconf-api/src/main/java/org/opendaylight/netconf/api/messages/NetconfHelloMessageAdditionalHeader.java @@ -28,7 +28,28 @@ import java.util.regex.Pattern; * Session-identifier is optional, others mandatory. * * This header is inserted in front of a netconf hello message followed by a newline. + * + * @deprecated This class is a leftover from initial implementation where SSH was done by http://www.jcraft.com/jsch/ + * and piped channels to get around the sync nature of JSch. We are now using a directly-connected Netty + * channel via transport-ssh. */ +// FIXME: session-identifier is unused +// FIXME: hostAddress, and port is readily available from Channel +// FIXME: transport should be visible in the form of TransportChannel in NetconfServerSession, the actual String here +// is a leak towards netconf-server-'s ietf-netconf-monitoring operational state. That piece of code should use +// a switch expression to go from 'transportChannel instanceof SSHTransportChannel' to 'NetconfSsh.VALUE' without +// the intermediate random string -- which does not support TLS anyway! +// FIXME: userName is coming from: +// - authentication in case of SSH/TLS +// - from wire in case of TCP +// We should propagate these via transport-api: +// - sealed interface TransportUser permits AuthenticatedUser, UnauthenticatedUser +// - non-sealed interface AuthenticatedUser +// - final class UnauthenticatedUser +// A TransportUser and TransportChannel via a netconf.codec.NetconfChannel exposed to +// ServerSessionNegotiator. On ClientSessionNegotiator side we want to have TransportServer exposing server's +// credentials, like the result of server key authentication. +@Deprecated(since = "8.0.0", forRemoval = true) public class NetconfHelloMessageAdditionalHeader { private static final String SC = ";"; diff --git a/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/FrameDecoder.java b/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/FrameDecoder.java index a00f2a2a36..4dace0a2d2 100644 --- a/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/FrameDecoder.java +++ b/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/FrameDecoder.java @@ -13,6 +13,19 @@ import org.eclipse.jdt.annotation.NonNull; /** * An channel handler assembling inbound messages from specified framing. */ +// TODO: Reconsider use of ByteToMessageDecoder here. While the Cumulator use is nice, I think we can do better if we +// do our buffer management ourselves: +// - we already use a CompositeByteBuf in ChunkedFrameDecoder with a buch of state tracking +// - we have searchIndex in order to optimize resumed searches, so taking ownership of the buffer should not be +// a big problem +// +// ChannelInboundHandlerAdapter should be a good first step, allowing us to focus on the implementation rather +// than the relationship to MessageDecoder. +// +// Ultimately this should become a base class disconnected from ChannelHandler, instances of which are given out +// by FramingSupport to MessageEncoder, which then handles interactions between those objects and the Netty +// channel, becoming also ChannelInboundHandler. Note that when that happens, channel initialization (now in +// AbstractChannelInitializer) must be updated to place MessageEncoder *before* MessageDecoder. public abstract sealed class FrameDecoder extends ByteToMessageDecoder permits ChunkedFrameDecoder, EOMFrameDecoder { /** * The name of the handler providing frame decoding. diff --git a/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageDecoder.java b/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageDecoder.java index dc80a7cc1d..d882808019 100644 --- a/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageDecoder.java +++ b/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageDecoder.java @@ -14,6 +14,16 @@ import org.opendaylight.netconf.api.messages.NetconfMessage; /** * A decoder from a series of bytes to a {@link NetconfMessage}. */ +// FIXME: 'NetconfMesssage' is not quite accurate: we can also produce Exceptions and the handling of those needs to be +// specific to the negotiator or attached client/server session. This class should probably be producting: +// - sealed interface MessageEvent permits MessageException, MessageDocument +// - record DecoderError(Exception cause), or similar +// - record DecoderMessage(Document document), to be converted to NetconfMessage +// There should be another abstract class for the above: +// - abstract sealed class MessageHandler extends ChannelInboundHandlerAdapter +// permits AbstractNetconfSesssion, AbstractSessionNegotiator +// - with it having a codified channelRead() method which does not tolerate unknown messages and dispatches to +// the abstract equivalent of AbstractNetconfSesssion.handleError()/handleMessage() public abstract class MessageDecoder extends ByteToMessageDecoder { /** * The name of the handler providing message decoding. diff --git a/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageEncoder.java b/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageEncoder.java index 3d5069b3a9..d76e77af94 100644 --- a/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageEncoder.java +++ b/protocol/netconf-codec/src/main/java/org/opendaylight/netconf/codec/MessageEncoder.java @@ -23,6 +23,10 @@ import org.slf4j.LoggerFactory; /** * An encoder of {@link NetconfMessage} to a series of bytes. */ +// TODO: MessageToByteEncoder is forcing us to encode the entire NetconfMessage into a single buffer before it can be +// passed downstream. We should switch to a plain ChannelOutboundHandlerAdapter and send multiple ByteBufs down +// the pipeline, completing the Promise with the result of Future returned by the last write(ByteBuf) -- which +// we expect underlying Channel to handle as required by https://www.rfc-editor.org/rfc/rfc6241#section-2.1 public final class MessageEncoder extends MessageToByteEncoder { private static final Logger LOG = LoggerFactory.getLogger(MessageEncoder.class); diff --git a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/NetconfServerSession.java b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/NetconfServerSession.java index 0a491d6d5d..aed26ad2af 100644 --- a/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/NetconfServerSession.java +++ b/protocol/netconf-server/src/main/java/org/opendaylight/netconf/server/NetconfServerSession.java @@ -48,6 +48,8 @@ import org.opendaylight.yangtools.yang.common.Uint32; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +// FIXME: separate out API and implementation, because at it currently stands we are leaking all of +// ChannelInboundHandlerAdapter, potentially leading to surprises. public final class NetconfServerSession extends AbstractNetconfExiSession implements NetconfManagementSession { private static final Logger LOG = LoggerFactory.getLogger(NetconfServerSession.class); @@ -106,6 +108,21 @@ public final class NetconfServerSession extends AbstractNetconfExiSession