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<S extends NetconfSession> {
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<S> promise) {
ch.pipeline().addLast(FrameDecoder.HANDLER_NAME, new EOMFrameDecoder());
initializeMessageDecoder(ch);
* 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<S> promise);
}
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+// FIXME: move this class to netconf.common
public abstract class AbstractNetconfSession<S extends NetconfSession, L extends NetconfSessionListener<S>>
+ // 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<Object> 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;
LOG.debug("Session {} created", sessionId);
}
+ @Override
+ public final SessionIdType sessionId() {
+ return sessionId;
+ }
+
protected abstract S thisInstance();
@Override
}
@Override
+ // FIXME: The below comment provides a bit more reasoning why this method should be kept of out end-user APIs.
+ // This method can be invoked from two different contexts:
+ // - a user either publishing a NotificationMessage, a RpcMessage, or similar: we know for sure that the
+ // entrypoint lies outside of the EventLoop's executor
+ // - the server responding with a RpcReplyMessage or similar: we may or may not be executing on the event
+ // loop.
+ //
+ // Examples:
+ //
+ // DeserializerExceptionHandler.exceptionCaught() is calling SendErrorExceptionUtil.sendErrorMessage().
+ // Is that okay?
+ //
+ // NetconfServerSessionListener.onMessage() is catching DocumentedException and calling
+ // SendErrorExceptionUtil.sendErrorMessage(). Is that okay?
+ //
+ // In any case we need to ensure requests complete in the order they arrived, I think, otherwise problems may
+ // occur. Callers of both need to be audited and we should be checking EventLoop.inEventLoop(). Note that we
+ // could be acquiring a ChannelHandlerContext, so we can find something helpful there?
+ //
public ChannelFuture sendMessage(final NetconfMessage netconfMessage) {
// From: https://github.com/netty/netty/issues/3887
// Netty can provide "ordering" in the following situations:
return up;
}
- public final @NonNull SessionIdType sessionId() {
- return sessionId;
- }
-
@Override
@SuppressWarnings("checkstyle:illegalCatch")
public final void channelInactive(final ChannelHandlerContext ctx) {
* Netconf messages after hello should be processed once the negotiation succeeded.
*
*/
+// FIXME: This handler conflates two things:
+// - extracting session metadata
+// - holding onto non-HelloMessages
+//
+// The former is a leftover from initial implementation we had SSH provided with http://www.jcraft.com/jsch/ --
+// where we had piping proxies rather than a live Channel connected to the user, due to JSch being synchronous.
+// See comments about that in NetconfHelloMessageAdditionalHeader.
+//
+// The need for the latter should disappear once we split out
public final class HelloXMLMessageDecoder extends MessageDecoder {
private static final Logger LOG = LoggerFactory.getLogger(HelloXMLMessageDecoder.class);
import io.netty.channel.ChannelFuture;
import java.io.Closeable;
+import org.eclipse.jdt.annotation.NonNullByDefault;
import org.opendaylight.netconf.api.messages.NetconfMessage;
+import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.netconf.base._1._0.rev110601.SessionIdType;
/**
* Protocol Session represents the finite state machine in underlying protocol, including timers and its purpose is to
* <p>
* 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 <kill-session>, https://www.rfc-editor.org/rfc/rfc6241#section-7.9, available to both parties?
+ // - is it <close-session>, 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();
}
* Session-identifier is optional, others mandatory.
* </pre>
* 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 = ";";
/**
* 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.
/**
* 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.
/**
* 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<NetconfMessage> {
private static final Logger LOG = LoggerFactory.getLogger(MessageEncoder.class);
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<NetconfServerSession,
NetconfServerSessionListener> implements NetconfManagementSession {
private static final Logger LOG = LoggerFactory.getLogger(NetconfServerSession.class);
return channelFuture;
}
+ // FIXME: the YANG definition for monitoring says:
+ //
+ // uses common-counters {
+ // description
+ // "Per-session counters. Zero based with following reset behaviour:
+ // - at start of a session
+ // - when max value is reached";
+ // }
+ //
+ // the overflow should checked after increment: if it becomes Uint32#MAX_VALUE + 1, it needs to
+ // be reset to 1.
+ //
+ // We want to isolate the three into a separate class, so that we can share code between here and whoever
+ // is populating the Statistics container. That class should implement CommonCounters to make it super easy
+ // to fill via {Session,Statistics}Builder.fieldsFrom(Grouping).
public void onIncommingRpcSuccess() {
inRpcSuccess++;
}
@Override
public Session toManagementSession() {
final SessionBuilder builder = new SessionBuilder().withKey(new SessionKey(sessionId().getValue()));
+
+ // FIXME: use channel to get this information
final InetAddress address1 = InetAddresses.forString(header.getAddress());
final IpAddress address;
if (address1 instanceof Inet4Address) {
.setInBadRpcs(new ZeroBasedCounter32(Uint32.valueOf(inRpcFail)))
.setInRpcs(new ZeroBasedCounter32(Uint32.valueOf(inRpcSuccess)))
.setOutRpcErrors(new ZeroBasedCounter32(Uint32.valueOf(outRpcError)))
+ // FIXME: a TransportUser from somewhere around TransportChannel
.setUsername(header.getUserName())
+ // FIXME: derive from TransportChannel instead
.setTransport(getTransportForString(header.getTransport()))
.setOutNotifications(new ZeroBasedCounter32(Uint32.valueOf(outNotification)))
+ // FIXME: obsolete this leaf and do not produce it here
.addAugmentation(new Session1Builder().setSessionIdentifier(header.getSessionIdentifier()).build())
.build();
}
Code Review / netconf.git / commitdiff