[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r662804747
##
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()
+http_response = http.request(
+'GET',
+url,
+headers=headers,
+timeout=config.agent_timeout()
+)
+return http_response.data.decode('utf-8')
+
except Exception as exception:
raise CLIException("Unable to open url '{url}': {error}"
.format(url=url, error=str(exception)))
-return http_response
-
-def get_json(addr, endpoint, condition=None, timeout=5, query=None):
+def get_json(addr, endpoint, config, condition=None, query=None):
"""
Return the contents of the 'endpoint' at 'addr' as JSON data
subject to the condition specified in 'condition'. If we are
-unable to read the data or unable to meet the condition within
-'timeout' seconds we throw an error.
+unable to read the data we throw an error.
"""
-start_time = time.time()
-while True:
-data = None
+data = read_endpoint(addr, endpoint, config, query)
-try:
-data = read_endpoint(addr, endpoint, query)
-except Exception as exception:
-pass
-
-if data:
-try:
-data = json.loads(data)
-except Exception as exception:
-raise CLIException("Could not load JSON from '{data}': {error}"
- .format(data=data, error=str(exception)))
-
-if not condition:
-return data
+try:
+data = json.loads(data)
+except Exception as exception:
+raise CLIException("Could not load JSON from '{data}': {error}"
+ .format(data=data, error=str(exception)))
-if condition(data):
-return data
+if not condition:
+return data
-if time.time() - start_time > timeout:
-raise CLIException("Failed to get data within {seconds} seconds"
- .format(seconds=str(timeout)))
+if condition(data):
Review comment:
Actually I'm not 100% sure what the idea behind `condition` was. But
yes, `data` have ever be to returned. But if `condition` is set, then through
that function. I will create (for myself) a issue to investigate `condition` to
see if it's needed or if we can remove it. Then we can merge this PR. Is this
procedure ok for everyone? :-)
##
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 +=
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644263295 ## File path: src/python/cli_new/lib/cli/plugins/task/main.py ## @@ -106,12 +108,13 @@ def list(self, argv): # pylint: disable=unused-argument try: master = self.config.master() +config = self.config Review comment: I thinks it's easier to read. -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644258228 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -137,3 +211,15 @@ def plugins(self): return self.data["plugins"] return [] + +def authentication_header(self): +""" +Return the BasicAuth authentication header +""" +if (self.agent_principal() is not None +and self.agent_secret() is not None): Review comment: The linter say it's fine like that. :man_shrugging: And my vim say, it should be like that. :joy: -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r642617105
##
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:
:joy: I was not so brave. But of course, u are right.
--
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:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r641495281
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,94 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+principal = self.data["master"].get("principal")
+if principal is None:
+return None
+elif not principal:
+raise CLIException("The 'principal' field in 'master'"
+ " must be non-empty")
+
+return principal
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+secret = self.data["master"].get("secret")
+if secret is None:
+return None
+elif not secret:
+raise CLIException("The 'secret' field in 'master'"
+ " must be non-empty")
+
+return 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
+elif not agent_ssl:
+if not isinstance(self.data["agent"]["ssl"], bool):
Review comment:
Yeah I didnt commit and pushed just now. I wanted to wait until I get
the feedback to the "def secret". :-) Then I will also squash 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r641474981 ## File path: src/python/cli_new/lib/cli/http.py ## @@ -19,70 +19,63 @@ """ 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 +urllib3.disable_warnings() Review comment: Will do 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r641474881
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,94 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+principal = self.data["master"].get("principal")
+if principal is None:
+return None
+elif not principal:
+raise CLIException("The 'principal' field in 'master'"
+ " must be non-empty")
+
+return principal
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+secret = self.data["master"].get("secret")
+if secret is None:
+return None
+elif not secret:
+raise CLIException("The 'secret' field in 'master'"
+ " must be non-empty")
+
+return 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
+elif not agent_ssl:
+if not isinstance(self.data["agent"]["ssl"], bool):
Review comment:
:+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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r640325483
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,63 @@
"""
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
+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()
+http_response = http.request('GET', url, headers=headers, timeout=5)
Review comment:
:+1: good idea.
Even if it's not easy to see that someone "want" have so many changes in the
code, I feel like the code is getting better and better. :-) The discuss about
some points helping a lot. So, thank's @cf-natali @asekretenko @kamaradclimber
--
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:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r640323131 ## File path: src/python/cli_new/lib/cli/http.py ## @@ -19,70 +19,63 @@ """ 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 +urllib3.disable_warnings() Review comment: I'm getting a lot of non pretty warning if I'm using a self signed certificate. But because of the possibility to set "ssl_verify", didn't think it made sense to issue the warning 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r640321645
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,94 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+principal = self.data["master"].get("principal")
+if principal is None:
+return None
+elif not principal:
+raise CLIException("The 'principal' field in 'master'"
+ " must be non-empty")
+
+return principal
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+secret = self.data["master"].get("secret")
+if secret is None:
+return None
+elif not secret:
+raise CLIException("The 'secret' field in 'master'"
+ " must be non-empty")
+
+return 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
+elif not agent_ssl:
+if not isinstance(self.data["agent"]["ssl"], bool):
Review comment:
Is already change in the current version. But I'm interesting of what
do u think about to change `def secret` and `def principal` to
```
def secret(self):
return self.data["master"].get("secret")
```
No matter secret does not exist, or it's empty, secret would be "None".
Thats why the "elif" is not usefull in these case. And because secret does not
have to be configured, it's also not important to rise a "Exception". What do
u think?
--
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:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r639529665 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -119,6 +119,65 @@ def master(self): return master +def principal(self): +""" +Return the principal in the configuration file +""" +if "principal" not in self.data["master"]: +return None Review comment: Actually, I can also remove the Exception handling. The principal, and secret is not a "must-have". -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r639472838 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -119,6 +119,65 @@ def master(self): return master +def principal(self): +""" +Return the principal in the configuration file +""" +if "principal" not in self.data["master"]: +return None Review comment: Ok, I change it. :-) But I think, the `elif` does not make sense. So, I prefer to remove 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r639472838 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -119,6 +119,65 @@ def master(self): return master +def principal(self): +""" +Return the principal in the configuration file +""" +if "principal" not in self.data["master"]: +return None Review comment: Ok, I change it. :-) Shell I squash the commit again? :joy: -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r639448122 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -119,6 +119,65 @@ def master(self): return master +def principal(self): +""" +Return the principal in the configuration file +""" +if "principal" not in self.data["master"]: +return None Review comment: I didn't feel it's a so big different to before. Thats why I ask! But I have no problem to change it. What do u think? -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r633283705
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,71 @@
"""
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
+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.
"""
+
+data = None
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()
+http_response = http.request('GET', url, headers=headers, timeout=5)
+data = http_response.data.decode('utf-8')
+
except Exception as exception:
raise CLIException("Unable to open url '{url}': {error}"
.format(url=url, error=str(exception)))
-return http_response
+return data
-def get_json(addr, endpoint, condition=None, timeout=5, query=None):
+def get_json(addr, endpoint, config, condition=None, query=None):
"""
Return the contents of the 'endpoint' at 'addr' as JSON data
subject to the condition specified in 'condition'. If we are
-unable to read the data or unable to meet the condition within
-'timeout' seconds we throw an error.
+unable to read the data we throw an error.
"""
-start_time = time.time()
-
-while True:
-data = None
-try:
-data = read_endpoint(addr, endpoint, query)
-except Exception as exception:
-pass
+data = None
-if data:
-try:
-data = json.loads(data)
-except Exception as exception:
-raise CLIException("Could not load JSON from '{data}': {error}"
- .format(data=data, error=str(exception)))
+try:
+data = read_endpoint(addr, endpoint, config, query)
+except Exception as exception:
+pass
Review comment:
So, I just remove the exception handling. Like you said, I make so sense
there. :+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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r633283705
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,71 @@
"""
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
+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.
"""
+
+data = None
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()
+http_response = http.request('GET', url, headers=headers, timeout=5)
+data = http_response.data.decode('utf-8')
+
except Exception as exception:
raise CLIException("Unable to open url '{url}': {error}"
.format(url=url, error=str(exception)))
-return http_response
+return data
-def get_json(addr, endpoint, condition=None, timeout=5, query=None):
+def get_json(addr, endpoint, config, condition=None, query=None):
"""
Return the contents of the 'endpoint' at 'addr' as JSON data
subject to the condition specified in 'condition'. If we are
-unable to read the data or unable to meet the condition within
-'timeout' seconds we throw an error.
+unable to read the data we throw an error.
"""
-start_time = time.time()
-
-while True:
-data = None
-try:
-data = read_endpoint(addr, endpoint, query)
-except Exception as exception:
-pass
+data = None
-if data:
-try:
-data = json.loads(data)
-except Exception as exception:
-raise CLIException("Could not load JSON from '{data}': {error}"
- .format(data=data, error=str(exception)))
+try:
+data = read_endpoint(addr, endpoint, config, query)
+except Exception as exception:
+pass
Review comment:
So, I just remove the exception handling. Like you said, I make so sense
there. :+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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r632179279 ## File path: src/python/cli_new/lib/cli/mesos.py ## @@ -199,11 +199,14 @@ def __init__(self, master, task_id): "This command is only supported for tasks" " launched by the Universal Container Runtime (UCR).") +# Get the scheme of the agent Review comment: Oh yes, that's pretty. Didn't know that. :-) Thanks @cf-natali -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r632178496 ## File path: src/python/cli_new/lib/cli/http.py ## @@ -19,70 +19,71 @@ """ 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 +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. """ + +data = None Review comment: yes, u are right. :+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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r632177344 ## File path: src/python/cli_new/lib/cli/mesos.py ## @@ -114,15 +113,16 @@ def get_container_id(task): " Please try again.") -def get_tasks(master, query=None): +# pylint: disable=dangerous-default-value Review comment: done -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r632176827
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,71 @@
"""
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
+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.
"""
+
+data = None
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()
+http_response = http.request('GET', url, headers=headers, timeout=5)
+data = http_response.data.decode('utf-8')
+
except Exception as exception:
raise CLIException("Unable to open url '{url}': {error}"
.format(url=url, error=str(exception)))
-return http_response
+return data
-def get_json(addr, endpoint, condition=None, timeout=5, query=None):
+def get_json(addr, endpoint, config, condition=None, query=None):
"""
Return the contents of the 'endpoint' at 'addr' as JSON data
subject to the condition specified in 'condition'. If we are
-unable to read the data or unable to meet the condition within
-'timeout' seconds we throw an error.
+unable to read the data we throw an error.
"""
-start_time = time.time()
-
-while True:
-data = None
-try:
-data = read_endpoint(addr, endpoint, query)
-except Exception as exception:
-pass
+data = None
-if data:
-try:
-data = json.loads(data)
-except Exception as exception:
-raise CLIException("Could not load JSON from '{data}': {error}"
- .format(data=data, error=str(exception)))
+try:
+data = read_endpoint(addr, endpoint, config, query)
+except Exception as exception:
+pass
Review comment:
Sorry this part I just removed the loop around. I don't know what the
idea behind this exception handling was. But I think u make a good point. Let
me have a look to see if I can 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r624726591 ## File path: src/python/cli_new/lib/cli/http.py ## @@ -64,7 +73,7 @@ def get_json(addr, endpoint, condition=None, timeout=5, query=None): data = None try: -data = read_endpoint(addr, endpoint, query) +data = read_endpoint(addr, endpoint, config, query) except Exception as exception: Review comment: Well, good that you speak it out. The loop also bother me. So I removed it and add a timeout into the read_endpoint function. :-) Hope it looks good 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r624720898
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -38,20 +38,29 @@ def read_endpoint(addr, endpoint, query=None):
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()
+http_response = http.request('GET', url, headers=headers)
except Exception as exception:
+print(exception)
Review comment:
right, I move it into the try->exception! :-)
--
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:
us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r624718800 ## File path: src/python/cli_new/lib/cli/mesos.py ## @@ -504,13 +508,23 @@ def _attach_container_output(self): client from the agent. """ +# Set authentication header +auth = None +# pylint: disable=line-too-long +if self.config.agent_principal() is not None and self.config.agent_secret() is not None: +auth = requests.auth.HTTPBasicAuth( +self.config.agent_principal(), +self.config.agent_secret() +) + Review comment: U are right. I also moved it into the config.py. but I'm unsteady, maybe it makes more sense in http.py. :thinking: -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r624715879 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -119,6 +119,65 @@ def master(self): return master +def principal(self): +""" +Return the principal in the configuration file +""" +if "principal" not in self.data["master"]: +return None Review comment: I add the check if the parameter set and not empty. Do u have other checks in your mind? :-) -- 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: us...@infra.apache.org
