You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/10/20 22:13:53 UTC
[GitHub] [tinkerpop] cole-bq opened a new pull request, #1838: Add User-Agent request header to java driver
cole-bq opened a new pull request, #1838:
URL: https://github.com/apache/tinkerpop/pull/1838
Adds a user agent to gremlin-driver which is sent during the web socket handshake.
User agent follows the form of [Application Name] [GLV Name]/[Version] [Language Runtime Version] [OS]/[Version] [CPU Architecture]
This behaviour is enabled by default but can be disabled by setting the enableWsHandshakeUserAgent configuration to false.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030802116
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
Review Comment:
Thanks for responding so soon. I agree that all of these changes would provide future extensibility benefits and I especially agree with the naming change to be web socket agnostic. I have pushed a small change where I have moved the UserAgent class to your suggested location and renamed WS_HANDSHAKE_USER_AGENT to just USER_AGENT. I have opted to keep USER_AGENT as a public statically initialized string constant as it is the most lightweight method of accessing the user agent and it meets the current needs. I certainly would not oppose modifying this further in the manner you've suggested sometime in the future if additional extensibility is required.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1019468338
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
Review Comment:
I've updated the name to follow the existing pattern. Unfortunately because the user agent itself is being used as the final part of the metric name, that portion of it will not match the existing format.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1015951820
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java:
##########
@@ -178,10 +180,14 @@ public void configure(final ChannelPipeline pipeline) {
throw new IllegalStateException("To use wss scheme ensure that enableSsl is set to true in configuration");
final int maxContentLength = cluster.connectionPoolSettings().maxContentLength;
+ HttpHeaders httpHeaders = new DefaultHttpHeaders();
Review Comment:
nit: please review changes to make sure variables that can be `final` are marked as such.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030498417
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
+
+ /**
+ * Request header name for user agent
+ */
+ public static final String USER_AGENT_HEADER_NAME = "User-Agent";
+ /**
+ * User Agent body to be sent in web socket handshake
+ * Has the form of:
+ * [Application Name] [GLV Name]/[Version] [Language Runtime Version] [OS]/[Version] [CPU Architecture]
+ */
+ public static final String WS_HANDSHAKE_USER_AGENT;
+
+ static {
+ String applicationName = "";
+ try {
+ applicationName = ((String)(new javax.naming.InitialContext().lookup("java:app/AppName"))).replace(' ', '_');
+ } catch (NamingException e) {
+ applicationName = "NotAvailable";
+ };
+
+ final String glvVersion = Gremlin.version().replace(' ', '_');
+ final String javaVersion = System.getProperty("java.version", "NotAvailable").replace(' ', '_');
Review Comment:
> I'm not sure of any specific use cases for this behavior
Let's remove it in that case. We can always add it later if someone needs it.
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
+
+ /**
+ * Request header name for user agent
+ */
+ public static final String USER_AGENT_HEADER_NAME = "User-Agent";
+ /**
+ * User Agent body to be sent in web socket handshake
+ * Has the form of:
+ * [Application Name] [GLV Name]/[Version] [Language Runtime Version] [OS]/[Version] [CPU Architecture]
+ */
+ public static final String WS_HANDSHAKE_USER_AGENT;
+
+ static {
+ String applicationName = "";
+ try {
+ applicationName = ((String)(new javax.naming.InitialContext().lookup("java:app/AppName"))).replace(' ', '_');
+ } catch (NamingException e) {
+ applicationName = "NotAvailable";
+ };
+
+ final String glvVersion = Gremlin.version().replace(' ', '_');
+ final String javaVersion = System.getProperty("java.version", "NotAvailable").replace(' ', '_');
Review Comment:
> I'm not sure of any specific use cases for this behavior
Let's remove it in that case. We can always add it later if someone needs it.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#issuecomment-1304301174
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030821110
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
Unfortunately in my tests, the user agent handler always triggers before the authentication handler regardless of the order the handlers are added to the pipeline. This is the order of events as I understand it:
The handshake is completely controlled by Netty's `WebSocketServerProtocolHandler`. When the handshake is completed, that handler fires off a `WebSocketServerProtocolHandler.HandshakeComplete` event which contains the request headers with the user agent. This event is the only reasonable place I am aware of that the user agent can be extracted. After this event has fired, the handshake is complete and the connection has been successfully upgraded to a web socket connection. The authentication and authorization handlers then do their work by exchanging messages with the client through the open web socket connection. This cannot be done until after the web socket has been established.
I don't see any good way to delay the user agent handler from triggering until after authentication. We will need to rely on the maxHeaderSize config limiting the size of any incoming header. I have added a change which imposes a hard limit of 10000 to the number of unique user agents being added to the metrics to protect against a malicious user trying to fill memory with millions of garbage user agents.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029790354
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import com.codahale.metrics.MetricRegistry;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.util.MetricManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
+
+ private static final Logger logger = LoggerFactory.getLogger(WsUserAgentHandler.class);
+ public static final AttributeKey<String> USER_AGENT_ATTR_KEY = AttributeKey.valueOf(UserAgent.USER_AGENT_HEADER_NAME);
+
+ @Override
+ public void userEventTriggered(ChannelHandlerContext ctx, java.lang.Object evt){
+ if(evt instanceof WebSocketServerProtocolHandler.HandshakeComplete){
+ final HttpHeaders requestHeaders = ((WebSocketServerProtocolHandler.HandshakeComplete) evt).requestHeaders();
+
+ if(requestHeaders.contains(UserAgent.USER_AGENT_HEADER_NAME)){
+ final String userAgent = requestHeaders.get(UserAgent.USER_AGENT_HEADER_NAME);
+
+ ctx.channel().attr(USER_AGENT_ATTR_KEY).set(userAgent);
+ logger.debug(String.format("New Connection on channel [%s] with user agent [%s]", ctx.channel().id().asShortText(), userAgent));
Review Comment:
Thanks I will incorporate this change
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030522016
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -848,6 +848,14 @@ Server returns for a single request. Again, this description of Gremlin Server'
out-of-the-box configuration. It is quite possible to construct other flows, that might be more amenable to a
particular language or style of processing.
+It is recommended but not required that a driver include a `User-Agent` header as part of any web socket
Review Comment:
There are users of TinkerPop which open one connection per request and hence, they are sensitive to handshake latency. Theoretically the latency incurred will be due to increased size of header (hence longer header parsing time from bytebuffer to Java object + longer time to send larger object over network).
If it is not trivial to calculate this, then we can ignore this since I believe there are larger bottlenecks in handshake than header parsing right now.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk merged pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
vkagamlyk merged PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#issuecomment-1316945305
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029815951
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
Unfortunately I'm not sure this reordering is possible without substantial change. The user agent handler triggers on a HandShake.Complete event right as the handshake is finishing. The authentication and authorization handlers take effect after the handshake is finished. Since the user agent is sent as an http header though, it is limited by MaxHeaderSize which defaults to 8192. This should be sufficient protection against maliciously large headers being sent from unauthenticated users. I will add a test which confirms this behavior.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030516555
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
> The authentication and authorization handlers take effect after the handshake is finished.
AFAIK, AuthN is done as part of the handshake itself. But irrespective of that, I think our new handler only executes when a handshake is complete. In which case our new handler's code will not execute for non authenticated requests, hence, there is no risk (contrary to what I mentioned earlier). Is my understanding right here?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029968748
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
+
+ /**
+ * Request header name for user agent
+ */
+ public static final String USER_AGENT_HEADER_NAME = "User-Agent";
+ /**
+ * User Agent body to be sent in web socket handshake
+ * Has the form of:
+ * [Application Name] [GLV Name]/[Version] [Language Runtime Version] [OS]/[Version] [CPU Architecture]
+ */
+ public static final String WS_HANDSHAKE_USER_AGENT;
+
+ static {
+ String applicationName = "";
+ try {
+ applicationName = ((String)(new javax.naming.InitialContext().lookup("java:app/AppName"))).replace(' ', '_');
+ } catch (NamingException e) {
+ applicationName = "NotAvailable";
+ };
+
+ final String glvVersion = Gremlin.version().replace(' ', '_');
+ final String javaVersion = System.getProperty("java.version", "NotAvailable").replace(' ', '_');
Review Comment:
While implementing this change I didn't have any intentions of making the user agent customizable. It is true that the user agent being constructed from system properties means that they can be overridden in maven with `-D` but this override only applies to that single execution. I'm not sure of any specific use cases for this behavior and such I'm not sure where it should be documented. I was wondering if you have anywhere in mind where it would be convenient, I don't want to dump extra information in places where it is not useful. Thanks.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029789851
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
Review Comment:
Thanks for all of your feedback Divij, could I ask for some more elaboration of your expectations regarding decoupling the driver and server code for the user agent? Keeping things decoupled was something I kept in mind while implementing my changes and I'm not sure what further changes you are looking for in that regard. As it stands all the code which generates and sends the user agent exists in gremlin-driver (which will be re-implemented in each GLV in a future PR). All the code which extracts the user agent, logs it for debugging, and adds it to the server metrics is wrapped into `WsUserAgentHandler` in gremlin-server. The only use of driver code from the server currently (excluding tests) is that the server uses the driver's constant for the "User-Agent" header name. This constant could easily be duplicated in the server to avoid referencing driver code from the server.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030495233
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
Review Comment:
I was thinking in the following direction:
1. UserAgent.java is a model (POJO) that describes the UserAgent. It contains fields such as JavaVersion etc. along with the getter & setters. It contains methods "serializeToString" and "deserializeFromString". This model resides in a place which is common to all drivers, communication protocols and server. I would suggest placing it in https://github.com/apache/tinkerpop/tree/master/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver at same level as Host.java. This POJO/modelling for UserAgent would help us from code extensibility perspective in future if we want to add enums or constraints checks on values of user agent.
2. Both HTTP and WebSocket will create this POJO using a static method in the above file, perhaps, UserAgent#Generate()
The goal of this structure is to ensure that code is extensible to HTTP in future and that individual methods could be unit tested easily. I don't have a strong opinion on this except for things such as WS_HANDSHAKE_USER_AGENT which should be made agnostic of WebSocket (the same could be used for HTTP).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#issuecomment-1331153899
@divijvaidya Do you have any further feedback for this PR? Are there any other changes you would like to see included? Thanks.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] alexey-temnikov commented on pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
alexey-temnikov commented on PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#issuecomment-1288259491
@spmallette, absolutely, this is intent. We also wanted to explore options if we can add client version information to the exception that occurred in the server to help with troubleshooting.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1017281136
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
Review Comment:
I agree, best to keep logs clearer and aggregated user agents will be more useful too. I have incorporated these suggestions.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1017281136
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
Review Comment:
I agree, best to keep logs clearer and aggregated user agents will be more useful too.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029966286
##########
gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/WebSocketClientBehaviorIntegrateTest.java:
##########
@@ -88,6 +89,44 @@ public void shutdown() {
rootLogger.removeAppender(recordingAppender);
}
+ /**
+ * Tests that client is correctly sending user agent during web socket handshake by having the server return
+ * the captured user agent.
+ */
+ @Test
+ public void shouldIncludeUserAgentInHandshakeRequest() {
Review Comment:
On further inspection I'm less sure that any test of this nature would be useful. Since the user agent is sent as an http header during the handshake, it is by definition only sent once per connection. Once the handshake is done and the connection established, there isn't really anywhere to check for a user agent being sent again.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1015960954
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
Review Comment:
is `info` the right log level here? seems we would trigger this for each connection made by a driver. maybe that would flood the logs with too much stuff? maybe we `debug` logging is better and some sort of aggregation of agent types reported as metrics (which by default only periodically writes to the logs)? thoughts?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030522552
##########
gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/WebSocketClientBehaviorIntegrateTest.java:
##########
@@ -88,6 +89,44 @@ public void shutdown() {
rootLogger.removeAppender(recordingAppender);
}
+ /**
+ * Tests that client is correctly sending user agent during web socket handshake by having the server return
+ * the captured user agent.
+ */
+ @Test
+ public void shouldIncludeUserAgentInHandshakeRequest() {
Review Comment:
sure, feel free to ignore the suggestion of this new test.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030804465
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -848,6 +848,14 @@ Server returns for a single request. Again, this description of Gremlin Server'
out-of-the-box configuration. It is quite possible to construct other flows, that might be more amenable to a
particular language or style of processing.
+It is recommended but not required that a driver include a `User-Agent` header as part of any web socket
Review Comment:
I would like to ignore this for now. It seems that designing a good benchmark would take a reasonable amount of time and it is not clear to me how I would normalize results to account for various network conditions.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1030821110
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
Unfortunately in my tests, the user agent handler always triggers before the authentication handler regardless of the order the handlers are added to the pipeline. This is the order of events as I understand it:
The handshake is completely controlled by Netty's `WebSocketServerProtocolHandler`. When the handshake is completed, that handler fires off a `WebSocketServerProtocolHandler.HandshakeComplete` event which contains the request headers with the user agent. This event is the only reasonable place I am aware of that the user agent can be extracted. After this event has fired, the handshake is complete and the connection has been successfully upgraded to a web socket connection. The authentication and authorization handlers then do their work by exchanging messages with the client through the open web socket connection. This cannot be done until after the web socket has been established.
I don't see any good way to delay the user agent handler from triggering until after authentication. We will need to rely on the maxHeaderSize config limiting the size of any incoming header. I am also working on a change which will impose a hard limit on the number of unique user agents being added to the metrics to protect against a malicious user trying to fill memory with millions of garbage user agents.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] divijvaidya commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1023972230
##########
gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/WebSocketClientBehaviorIntegrateTest.java:
##########
@@ -88,6 +89,44 @@ public void shutdown() {
rootLogger.removeAppender(recordingAppender);
}
+ /**
+ * Tests that client is correctly sending user agent during web socket handshake by having the server return
+ * the captured user agent.
+ */
+ @Test
+ public void shouldIncludeUserAgentInHandshakeRequest() {
Review Comment:
Please add the following test scenarios:
1. Validate that the user agent is sent only once per WebSocket connection.
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import com.codahale.metrics.MetricRegistry;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.apache.tinkerpop.gremlin.server.GremlinServer;
+import org.apache.tinkerpop.gremlin.server.util.MetricManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
+
+ private static final Logger logger = LoggerFactory.getLogger(WsUserAgentHandler.class);
+ public static final AttributeKey<String> USER_AGENT_ATTR_KEY = AttributeKey.valueOf(UserAgent.USER_AGENT_HEADER_NAME);
+
+ @Override
+ public void userEventTriggered(ChannelHandlerContext ctx, java.lang.Object evt){
+ if(evt instanceof WebSocketServerProtocolHandler.HandshakeComplete){
+ final HttpHeaders requestHeaders = ((WebSocketServerProtocolHandler.HandshakeComplete) evt).requestHeaders();
+
+ if(requestHeaders.contains(UserAgent.USER_AGENT_HEADER_NAME)){
+ final String userAgent = requestHeaders.get(UserAgent.USER_AGENT_HEADER_NAME);
+
+ ctx.channel().attr(USER_AGENT_ATTR_KEY).set(userAgent);
+ logger.debug(String.format("New Connection on channel [%s] with user agent [%s]", ctx.channel().id().asShortText(), userAgent));
Review Comment:
I would recommend to use [slf4j's parameterized format for logging with](https://www.slf4j.org/faq.html#logging_performance) `{}`. That form ensures that the arguments in the log line are not evaluated is the log level is not satisfied. In the correct scenario, String.format() will always be calculated irrespective of the logging level set for the server.
Same comment for other log lines.
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -848,6 +848,14 @@ Server returns for a single request. Again, this description of Gremlin Server'
out-of-the-box configuration. It is quite possible to construct other flows, that might be more amenable to a
particular language or style of processing.
+It is recommended but not required that a driver include a `User-Agent` header as part of any web socket
Review Comment:
What are the side effects of enabling it? We should help the users make this decision. Please add a jmh benchmark for handshake w/ and w/o the user agent. User agent will add additional latency to the handshake and hence, connection setup will be slower but by how much?
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
+
+ /**
+ * Request header name for user agent
+ */
+ public static final String USER_AGENT_HEADER_NAME = "User-Agent";
+ /**
+ * User Agent body to be sent in web socket handshake
+ * Has the form of:
+ * [Application Name] [GLV Name]/[Version] [Language Runtime Version] [OS]/[Version] [CPU Architecture]
+ */
+ public static final String WS_HANDSHAKE_USER_AGENT;
+
+ static {
+ String applicationName = "";
+ try {
+ applicationName = ((String)(new javax.naming.InitialContext().lookup("java:app/AppName"))).replace(' ', '_');
+ } catch (NamingException e) {
+ applicationName = "NotAvailable";
+ };
+
+ final String glvVersion = Gremlin.version().replace(' ', '_');
+ final String javaVersion = System.getProperty("java.version", "NotAvailable").replace(' ', '_');
Review Comment:
The information on how to configure the user agent is missing in documentation. We should add the information that the user agent is constructed from System Properties and could be overridden/set by using `-D`.
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/util/UserAgent.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.tinkerpop.gremlin.driver.util;
+
+import org.apache.tinkerpop.gremlin.util.Gremlin;
+import javax.naming.NamingException;
+
+public class UserAgent {
Review Comment:
From an implementation perspective, we need to decouple the code which generates the UserAgent from the code that models the UserAgent. The former will reside in `gremlin-driver` and other drivers will write a similar implementation for themselves but the latter will reside in server and server will use it to represent and parse UserAgent coming from different language drivers on both WS and HTTP.
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
This handler should go "after" authN has taken place. Else an attacker may perform a DoS attack by sending unauthenticated large string in user agent.
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
Please create a JIRA to implement this for HTTP calls as well.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] codecov-commenter commented on pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#issuecomment-1286229442
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1838?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1838](https://codecov.io/gh/apache/tinkerpop/pull/1838?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3ff47a) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/468bc4870977f7c9be89106e55366962fd4badab?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (468bc48) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1838 +/- ##
========================================
Coverage 63.58% 63.58%
========================================
Files 23 23
Lines 3636 3636
========================================
Hits 2312 2312
Misses 1145 1145
Partials 179 179
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#issuecomment-1288201585
i realize this feature is focused on the Java driver, but perhaps we could make the server do something with the agent in the PR? could we at least have a debug level log message output a line with the requestid/agent value? or perhaps we dont need a discrete message? maybe just update the server logging that currently exists for writing a `RequestMessage` to include some of that information where it might be helpful in debugging?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1017827021
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
Review Comment:
you went with `UserAgent` for the metric name, but please double check to see if that is consistent with patterns already established for naming metrics in Gremlin Server. i think naming prefixes are all lower case with a hyphen between words if there are more than one. also, since you added a metric, please document it [here](https://tinkerpop.apache.org/docs/current/reference/#metrics).
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1019466399
##########
docs/src/reference/gremlin-variants.asciidoc:
##########
@@ -682,6 +683,14 @@ This command will generate a new Maven project in a directory called "app" with
`com.my`. Please see the `README.asciidoc` in the root of each generated project for information on how to build and
execute it.
+[[gremlin-java-user-agent]]
+=== User Agent
Review Comment:
I agree, I have updated the docs accordingly.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1019477780
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsUserAgentHandler.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.handler.codec.http.websocketx.WebSocketServerProtocolHandler;
+import io.netty.util.AttributeKey;
+import org.apache.tinkerpop.gremlin.driver.util.UserAgent;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Channel handler which extracts a user agent header from a web socket handshake if present
+ * then logs the user agent and stores it as a channel attribute for future reference.
+ */
+public class WsUserAgentHandler extends ChannelInboundHandlerAdapter {
Review Comment:
> that portion of it will not match the existing format.
I believe that is fine as I think other metrics work that way as well. as long as the static prefix part of the metric is named according to standard the dynamic right-hand-side can do as it wishes.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029974456
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/channel/WebSocketChannelizer.java:
##########
@@ -108,7 +109,7 @@ public void configure(final ChannelPipeline pipeline) {
closeOnProtocolViolation(false).allowExtensions(true).maxFramePayloadLength(settings.maxContentLength).build();
pipeline.addLast(PIPELINE_REQUEST_HANDLER, new WebSocketServerProtocolHandler(GREMLIN_ENDPOINT,
null, false, false, 10000L, wsDecoderConfig));
-
+ pipeline.addLast("ws-user-agent-handler", new WsUserAgentHandler());
Review Comment:
I have made this JIRA for the HTTP implementation. https://issues.apache.org/jira/browse/TINKERPOP-2830
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to java driver
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1015956869
##########
docs/src/reference/gremlin-variants.asciidoc:
##########
@@ -329,6 +329,7 @@ The following table describes the various configuration options for the Gremlin
|serializer.config |A `Map` of configuration settings for the serializer. |_none_
|username |The username to submit on requests that require authentication. |_none_
|workerPoolSize |Size of the pool for handling background work. |available processors * 2
+|enableWsHandshakeUserAgent |Configures the channelizer to send a user agent during web socket handshakes. |true
Review Comment:
1. It doesn't necessarily have to be here in this table, but i think the user will need more details as to what effect this setting has.
2. `enableWsHandshakeUserAgent` says what it is but isn't really clear at first read? maybe `enableUserAgentOnConnect` would be more descriptive?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1017821994
##########
docs/src/reference/gremlin-variants.asciidoc:
##########
@@ -682,6 +683,14 @@ This command will generate a new Maven project in a directory called "app" with
`com.my`. Please see the `README.asciidoc` in the root of each generated project for information on how to build and
execute it.
+[[gremlin-java-user-agent]]
+=== User Agent
Review Comment:
i wouldn't make this a separate top-level section as GLVs have a set form. i was going to suggest you could make it a fourth level section under `gremlin-java-configuration` but i feel like this the "user agent" is just going to then have to be replicated for each GLV. I'm starting to think about this differently now. This new use of user agent is technically a standard that drivers should adhere to. I think you should document it as such in the [Provider Documentation](https://github.com/apache/tinkerpop/blob/master/docs/src/dev/provider/index.asciidoc#graph-driver-provider-requirements) where you should include the expected format, examples, etc. Then you can just add a link to that from your little configuration description if folks want to know what the "use agent" is. Finally in the Upgrade Documentation you should make mention of this new feature in the Provider section to call attention to it. how does that sound?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1017823259
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java:
##########
@@ -993,6 +1002,16 @@ public Builder connectionSetupTimeoutMillis(final long connectionSetupTimeoutMil
return this;
}
+ /**
+ * Configures whether cluster will send a user agent during
+ * web socket handshakes
+ * @param enableUserAgentOnConnect true enables the useragent. false disables the useragent.
+ */
+ public Builder enableUserAgentOnConnect(boolean enableUserAgentOnConnect) {
+ this.wsHandshakeUserAgentEnabled = enableUserAgentOnConnect;
Review Comment:
it seems like you went with the rename i suggested - please rename the underlying variables as well to keep things consistent.
nit: `final`
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029806753
##########
docs/src/dev/provider/index.asciidoc:
##########
@@ -848,6 +848,14 @@ Server returns for a single request. Again, this description of Gremlin Server'
out-of-the-box configuration. It is quite possible to construct other flows, that might be more amenable to a
particular language or style of processing.
+It is recommended but not required that a driver include a `User-Agent` header as part of any web socket
Review Comment:
How much value do you see in having such a benchmark? My understanding is that the jmh benchmarks aren't currently setup for measuring anything like the driver handshake latency. I also have concerns about how usefulness of any result as the latency will be highly dependent on network conditions. A typical user agent in the format we are using is under 100 bytes and it is only sent once per connection in the handshake. Personally I don't foresee there being any noticeable side effects other than the obvious added logging and metrics.
Some sort of benchmark can certainty be added but I don't believe it will be trivial to add and I worry it won't offer much value for the investment put into creating it.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] cole-bq commented on a diff in pull request #1838: TINKERPOP-2480 Add User-Agent request header to Java Driver and Server
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1838:
URL: https://github.com/apache/tinkerpop/pull/1838#discussion_r1029791005
##########
gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/WebSocketClientBehaviorIntegrateTest.java:
##########
@@ -88,6 +89,44 @@ public void shutdown() {
rootLogger.removeAppender(recordingAppender);
}
+ /**
+ * Tests that client is correctly sending user agent during web socket handshake by having the server return
+ * the captured user agent.
+ */
+ @Test
+ public void shouldIncludeUserAgentInHandshakeRequest() {
Review Comment:
That sounds like a good test to have, I will add it.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org