Re: [PR] CASSANDRA-21756: Add configurable authentication timeout for native transport connections [cassandra]

["gandherva (via GitHub)" <gi...@apache.org>]

gandherva commented on code in PR #5261:
URL: https://github.com/apache/cassandra/pull/5261#discussion_r4289139629


##########
src/java/org/apache/cassandra/transport/ServerConnection.java:
##########
@@ -120,11 +131,19 @@ else if (responseType == Message.Type.READY)
                 if (responseType == Message.Type.READY || responseType == Message.Type.AUTH_SUCCESS)
                 {
                     stage = ConnectionStage.READY;
+                    authenticationComplete();
                     // we won't use the authenticator again, null it so that it can be GC'd
                     saslNegotiator = null;
                 }
                 break;
             case READY:
+                if (responseType == Message.Type.AUTHENTICATE)
+                    stage = ConnectionStage.AUTHENTICATING;
+                else if (responseType == Message.Type.READY)

Review Comment:
   Do we need these lines when READY is the final state and the authenticationComplete is already called for stage transitions from ESTABLED and AUTHENTICATING to READY (Assuming my other comment about calling `authenticationComplete()` on the `ESTABLISHED → READY` transition is addressed ). Also in this code we are going from the final state READY to AUTHENTICATING which would move the connection back if this code path ever runs. Do we want to just revert `case READY:` to be just `break;` ? 
   
   
   "                if (responseType == Message.Type.AUTHENTICATE)
                       stage = ConnectionStage.AUTHENTICATING;
                   else if (responseType == Message.Type.READY)
                  {
                       stage = ConnectionStage.READY;
                       authenticationComplete();
                   }"
   
   
   
   



##########
src/java/org/apache/cassandra/transport/PipelineConfigurator.java:
##########
@@ -275,6 +275,11 @@ protected void channelIdle(ChannelHandlerContext ctx, IdleStateEvent evt)
             });
         }
 
+        long authTimeout = DatabaseDescriptor.nativeTransportAuthTimeout();
+        if (authTimeout > 0)
+            pipeline.addLast("authenticationTimeout",
+                             new AuthenticationTimeoutHandler(authTimeout));
+

Review Comment:
   Once authentication is complete, can we remove `AuthenticationTimeoutHandler` from the pipeline, since it's no longer needed? Right now it stays attached for the whole life of the connection. The handler's job ends once the connection reaches `READY`, and this would match how `PipelineConfigurator` already removes `INITIAL_HANDLER` once negotiation is done.
   
   A few things to watch for with the removal from AI
   - The pending timeout should be cancelled as part of removing the handler.
   - The timer should never close a connection that has just finished authenticating. If removal and the timeout both run on the event loop, they can't overlap, which might also make the `AtomicBoolean`/`volatile` fields unnecessary. (verify before removing )
   -   Removal should be safe if the handler is already gone, was never installed, or the pipeline has been reconfigured.
   - Every path into `READY` should trigger it.
   -  Removing the handler after successful authentication should not affect the active connection.
   - Ensure removal is safe during pipeline reconfiguration
   - A test showing an authenticated connection stays open past the timeout and the handler is removed
   



##########
src/java/org/apache/cassandra/transport/AuthenticationTimeoutHandler.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.cassandra.transport;
+
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+
+final class AuthenticationTimeoutHandler extends ChannelInboundHandlerAdapter
+{
+    private static final Logger logger = LoggerFactory.getLogger(AuthenticationTimeoutHandler.class);
+
+    private final long timeoutMillis;
+    private final AtomicBoolean completed = new AtomicBoolean();

Review Comment:
   I think `completed` can be removed. All access to it seems to be from single Netty channel's event loop thread which runs sequentially , so `timeoutFuture.cancel(false)` is enough without atomic boolean. The re-check in `channelActive()` also appears unncessary.  Verify before removal



##########
src/java/org/apache/cassandra/transport/ServerConnection.java:
##########


Review Comment:
   We might need to call  authenticationComplete() here for the state transition to READY, check and confirm before adding. 



##########
src/java/org/apache/cassandra/transport/AuthenticationTimeoutHandler.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.cassandra.transport;
+
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+
+final class AuthenticationTimeoutHandler extends ChannelInboundHandlerAdapter
+{
+    private static final Logger logger = LoggerFactory.getLogger(AuthenticationTimeoutHandler.class);
+
+    private final long timeoutMillis;
+    private final AtomicBoolean completed = new AtomicBoolean();
+    private volatile ScheduledFuture<?> timeoutFuture;

Review Comment:
   volatile might not be needed here all access to 'timeoutFuture' seems to be on the single Netty event loop thread which executes sequentially.  Verify before removal



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org