[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #301: GUACAMOLE-1174: Added exec call implementation for kubernetes protocol

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #301:
URL: https://github.com/apache/guacamole-server/pull/301#discussion_r504261540



##########
File path: src/protocols/kubernetes/settings.c
##########
@@ -275,6 +281,11 @@ guac_kubernetes_settings* guac_kubernetes_parse_args(guac_user* user,
         guac_user_parse_args_string(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
                 IDX_CONTAINER, NULL);
 
+    /* Read exec command (optional) */
+    settings->exec_command =
+        guac_user_parse_args_string(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
+                IDX_EXEC_COMMAND, NULL);

Review comment:
       The string returned by `guac_user_parse_args_string()` is dynamically-allocated and will need to eventually be freed. See:
   
   https://github.com/apache/guacamole-server/blob/2aa0218cc761b0810d20d92ba3deccc57c993a63/src/protocols/kubernetes/settings.c#L399-L429
   

##########
File path: src/protocols/kubernetes/url.c
##########
@@ -109,26 +110,52 @@ int guac_kubernetes_endpoint_attach(char* buffer, int length,
                 sizeof(escaped_pod), kubernetes_pod))
         return 1;
 
-    /* Generate attachment endpoint URL */
-    if (kubernetes_container != NULL) {
+    /* Generate endpoint path depending on the call type */
+    char* call = "attach";
+    if (exec_command != NULL)
+        call = "exec";
+
+    char endpoint_path[GUAC_KUBERNETES_MAX_ENDPOINT_LENGTH];
+
+    written = snprintf(endpoint_path, sizeof(endpoint_path), 
+        "/api/v1/namespaces/%s/pods/%s/%s", escaped_namespace, escaped_pod, call);
+        
+    if (written < 0 || written >= sizeof(endpoint_path))
+        return 1;
+
+    /* Generate endpoint params */
+    char endpoint_params[GUAC_KUBERNETES_MAX_ENDPOINT_LENGTH]="";
+
+    if (exec_command != NULL) {
+        /* Escape exec command */
+        if (guac_kubernetes_escape_url_component(escaped_exec_command,
+                    sizeof(escaped_exec_command), exec_command))
+            return 1;
 
+        written = snprintf(endpoint_params, sizeof(endpoint_params), 
+            "command=%s&", escaped_exec_command);
+
+        if (written < 0 || written >= sizeof(endpoint_params))
+            return 1;
+    }

Review comment:
       Perhaps there is a way to clean up these repeated, similar calls to `snprintf()` intended to append parameters to a URI? Seems the sort of thing that would benefit greatly from being moved into its own simple, testable function.

##########
File path: src/protocols/kubernetes/url.c
##########
@@ -109,26 +110,52 @@ int guac_kubernetes_endpoint_attach(char* buffer, int length,
                 sizeof(escaped_pod), kubernetes_pod))
         return 1;
 
-    /* Generate attachment endpoint URL */
-    if (kubernetes_container != NULL) {
+    /* Generate endpoint path depending on the call type */
+    char* call = "attach";
+    if (exec_command != NULL)
+        call = "exec";
+
+    char endpoint_path[GUAC_KUBERNETES_MAX_ENDPOINT_LENGTH];
+
+    written = snprintf(endpoint_path, sizeof(endpoint_path), 
+        "/api/v1/namespaces/%s/pods/%s/%s", escaped_namespace, escaped_pod, call);
+        
+    if (written < 0 || written >= sizeof(endpoint_path))
+        return 1;
+
+    /* Generate endpoint params */
+    char endpoint_params[GUAC_KUBERNETES_MAX_ENDPOINT_LENGTH]="";
+
+    if (exec_command != NULL) {
+        /* Escape exec command */
+        if (guac_kubernetes_escape_url_component(escaped_exec_command,
+                    sizeof(escaped_exec_command), exec_command))
+            return 1;
 
+        written = snprintf(endpoint_params, sizeof(endpoint_params), 
+            "command=%s&", escaped_exec_command);
+
+        if (written < 0 || written >= sizeof(endpoint_params))
+            return 1;
+    }
+
+    if (kubernetes_container != NULL) {
         /* Escape container name */
         if (guac_kubernetes_escape_url_component(escaped_container,
                     sizeof(escaped_container), kubernetes_container))
             return 1;
 
-        written = snprintf(buffer, length,
-                "/api/v1/namespaces/%s/pods/%s/attach"
-                "?container=%s&stdin=true&stdout=true&tty=true",
-                escaped_namespace, escaped_pod, escaped_container);
-    }
-    else {
-        written = snprintf(buffer, length,
-                "/api/v1/namespaces/%s/pods/%s/attach"
-                "?stdin=true&stdout=true&tty=true",
-                escaped_namespace, escaped_pod);
+        written = snprintf(endpoint_params, sizeof(endpoint_params),
+                "container=%s&", escaped_container);

Review comment:
       Won't this overwrite the string previously written to `endpoint_params` if `exec_command` is set?




----------------------------------------------------------------
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.

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