mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
Date Mon, 31 May 2021 17:15:30 GMT

cf-natali commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r642607447



##########
File path: src/python/cli_new/lib/cli/mesos.py
##########
@@ -413,6 +417,7 @@ def _wait(self):
             'wait_container': {
                 'container_id': self.container_id}}
         req_extra_args = {
+            'verify': self.config.agent_ssl_verify(),

Review comment:
       What happens if `elf.config.agent_ssl_verify()` is `None`, will it work?

##########
File path: src/python/cli_new/lib/cli/plugins/task/main.py
##########
@@ -75,11 +75,11 @@ def attach(self, argv):
         """
         try:
             master = self.config.master()
+            config = self.config
         except Exception as exception:
             raise CLIException("Unable to get leading master address: {error}"
                                .format(error=exception))
-
-        task_io = TaskIO(master, argv["<task-id>"])
+        task_io = TaskIO(master, config, argv["<task-id>"])

Review comment:
       Nit: I think the indentation with the newline before was more readable.

##########
File path: src/python/cli_new/lib/cli/config.py
##########
@@ -119,6 +120,92 @@ def master(self):
 
         return master
 
+    def principal(self):
+        """
+        Return the principal in the configuration file
+        """
+        return self.data["master"].get("principal")
+
+    def secret(self):
+        """
+        Return the secret in the configuration file
+        """
+        return self.data["master"].get("secret")
+
+    def agent_ssl(self):
+        """
+        Return if the agent support ssl
+        """
+        if "agent" in self.data:
+            agent_ssl = self.data["agent"].get("ssl")
+            if agent_ssl is None:
+                return None
+            if not isinstance(agent_ssl, bool):
+                raise CLIException("The 'agent->ssl' field"
+                                   " must be True/False")
+
+            return agent_ssl
+
+        return None
+
+    def agent_ssl_verify(self):
+        """
+        Return if the ssl certificate should be verified
+        """
+        if "agent" in self.data:
+            ssl_verify = self.data["agent"].get("ssl_verify")
+            if ssl_verify is None:
+                return None
+            if not isinstance(ssl_verify, bool):
+                raise CLIException("The 'agent->ssl_verify' field"
+                                   " must be True/False")
+
+            return ssl_verify
+
+        return None
+
+    def agent_timeout(self):
+        """
+        Return the connection timeout of the agent
+        """
+        if "agent" in self.data:
+            timeout = self.data["agent"].get("timeout")
+            if timeout is None:
+                return 5
+            if not isinstance(timeout, int):
+                raise CLIException("The 'agent->timeout' field"
+                                   " must be a number in seconds")
+
+            return timeout
+
+        return 5
+
+    def agent_principal(self):
+        """
+        Return the principal in the configuration file
+        """
+        if "agent" in self.data:
+            principal = self.data["agent"].get("principal")
+            if principal is None:
+                return None
+
+            return principal
+
+        return None
+
+    def agent_secret(self):
+        """
+        Return the secret in the configuration file
+        """
+        if "agent" in self.data:
+            secret = self.data["agent"].get("secret")
+            if secret is None:
+                return None

Review comment:
       This seems equivalent to just:
   ```
   return self.data["agent"].get("secret")
   ```

##########
File path: src/python/cli_new/lib/cli/config.py
##########
@@ -119,6 +120,92 @@ def master(self):
 
         return master
 
+    def principal(self):
+        """
+        Return the principal in the configuration file
+        """
+        return self.data["master"].get("principal")
+
+    def secret(self):
+        """
+        Return the secret in the configuration file
+        """
+        return self.data["master"].get("secret")
+
+    def agent_ssl(self):
+        """
+        Return if the agent support ssl
+        """
+        if "agent" in self.data:
+            agent_ssl = self.data["agent"].get("ssl")
+            if agent_ssl is None:
+                return None
+            if not isinstance(agent_ssl, bool):
+                raise CLIException("The 'agent->ssl' field"
+                                   " must be True/False")
+
+            return agent_ssl
+
+        return None
+
+    def agent_ssl_verify(self):
+        """
+        Return if the ssl certificate should be verified
+        """
+        if "agent" in self.data:
+            ssl_verify = self.data["agent"].get("ssl_verify")
+            if ssl_verify is None:
+                return None
+            if not isinstance(ssl_verify, bool):
+                raise CLIException("The 'agent->ssl_verify' field"
+                                   " must be True/False")
+
+            return ssl_verify
+
+        return None
+
+    def agent_timeout(self):
+        """
+        Return the connection timeout of the agent
+        """
+        if "agent" in self.data:
+            timeout = self.data["agent"].get("timeout")
+            if timeout is None:
+                return 5

Review comment:
       I think you can do instead:
   ```
               timeout = self.data["agent"].get("timeout", 5)
   ```
   
   Also, maybe better than hard-coding the default could be moved to the function prototype?
   ```
       def agent_timeout(self, default=5):
   ```

##########
File path: src/python/cli_new/lib/cli/config.py
##########
@@ -119,6 +120,92 @@ def master(self):
 
         return master
 
+    def principal(self):
+        """
+        Return the principal in the configuration file
+        """
+        return self.data["master"].get("principal")
+
+    def secret(self):
+        """
+        Return the secret in the configuration file
+        """
+        return self.data["master"].get("secret")
+
+    def agent_ssl(self):
+        """
+        Return if the agent support ssl
+        """
+        if "agent" in self.data:
+            agent_ssl = self.data["agent"].get("ssl")
+            if agent_ssl is None:
+                return None
+            if not isinstance(agent_ssl, bool):
+                raise CLIException("The 'agent->ssl' field"
+                                   " must be True/False")
+
+            return agent_ssl
+
+        return None
+
+    def agent_ssl_verify(self):
+        """
+        Return if the ssl certificate should be verified
+        """
+        if "agent" in self.data:
+            ssl_verify = self.data["agent"].get("ssl_verify")
+            if ssl_verify is None:
+                return None
+            if not isinstance(ssl_verify, bool):
+                raise CLIException("The 'agent->ssl_verify' field"
+                                   " must be True/False")
+
+            return ssl_verify
+
+        return None
+
+    def agent_timeout(self):
+        """
+        Return the connection timeout of the agent
+        """
+        if "agent" in self.data:
+            timeout = self.data["agent"].get("timeout")
+            if timeout is None:
+                return 5
+            if not isinstance(timeout, int):
+                raise CLIException("The 'agent->timeout' field"
+                                   " must be a number in seconds")
+
+            return timeout
+
+        return 5
+
+    def agent_principal(self):
+        """
+        Return the principal in the configuration file
+        """
+        if "agent" in self.data:
+            principal = self.data["agent"].get("principal")
+            if principal is None:
+                return None
+
+            return principal

Review comment:
       This seems equivalent to just:
   ```
   return self.data["agent"].get("principal")
   ```

##########
File path: src/python/cli_new/lib/cli/http.py
##########
@@ -19,70 +19,70 @@
 """
 
 import json
-import urllib.request
-import urllib.error
-import urllib.parse
-import time
+from urllib.parse import urlencode
+import urllib3
 
 import cli
 
 from cli.exceptions import CLIException
 
+# Disable all SSL warnings. These are not necessary, as the user has
+# the option to disable SSL verification.
+urllib3.disable_warnings()
 
-def read_endpoint(addr, endpoint, query=None):
+def read_endpoint(addr, endpoint, config, query=None):
     """
     Read the specified endpoint and return the results.
     """
+
     try:
         addr = cli.util.sanitize_address(addr)
     except Exception as exception:
         raise CLIException("Unable to sanitize address '{addr}': {error}"
                            .format(addr=addr, error=str(exception)))
-
     try:
         url = "{addr}/{endpoint}".format(addr=addr, endpoint=endpoint)
         if query is not None:
-            url += "?{query}".format(query=urllib.parse.urlencode(query))
-        http_response = urllib.request.urlopen(url).read().decode("utf-8")
+            url += "?{query}".format(query=urlencode(query))
+        if config.principal() is not None and config.secret() is not None:
+            headers = urllib3.make_headers(
+                basic_auth=config.principal() + ":" + config.secret()
+            )
+        else:
+            headers = None
+        http = urllib3.PoolManager()

Review comment:
       I'm not familiar with `urllib3` - what does `Poolmanager` bring over just using  `utllib.request`?
   Does it not support custom headers?




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



Mime
View raw message