[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli

[GitBox]

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



##
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:
   Maybe make `timeout` a parameter with default 5 instead of hard-coding 
it, so it could be specified if needed.




-- 
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] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli

[GitBox]

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



##
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:
   Why is this necessary?




-- 
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] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli

[GitBox]

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



##
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:
   ```
   isinstance(agent_ssl, bool)
   ```




-- 
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] cf-natali commented on pull request #386: Fixed a LevelDBStorage bug where positions would overflow.

[GitBox]

cf-natali commented on pull request #386:
URL: https://github.com/apache/mesos/pull/386#issuecomment-849169050


   > @cf-natali @bmahler It strikes me that no backward conversion (from the 
string key to an integer position) is ever used by the replicated log code 
Do we have a good understanding what the actual contract for `encode()` should 
look like?
   > 
   > From what I can tell by quickly looking at the code, the main requirement 
is that `encode()` results, when ordered lexicographically, should be in the 
order of positions. This means that, **_if I haven't missed something_**, for 
example, prefixing integers larger than `9'999'999'999` by a single letter `A` 
will result in the correct order of keys:
   > 
   > ```
   > 00
   > ...
   > 98
   > 99
   > A00100
   > A00101
   > ```
   
   Nice!
   Indeed, that's also my understanding, as far as I can tell the only 
properties needed for `encode()` are that it is:
   - bijective
   - monotonic - the ordering is checked using `leveldb::BytewiseComparator`, 
which does a lexicographic comparison, see 
https://github.com/google/leveldb/blob/f57513a1d6c99636fc5b710150d0b93713af4e43/util/comparator.cc#L28
 and 
https://github.com/google/leveldb/blob/5d94ad4d95c09d3ac203ddaf9922e55e730706a8/include/leveldb/slice.h#L100
   
   So yes, it looks like this could work - I'll have a play.


-- 
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] asekretenko commented on pull request #386: Fixed a LevelDBStorage bug where positions would overflow.

[GitBox]

asekretenko commented on pull request #386:
URL: https://github.com/apache/mesos/pull/386#issuecomment-849077997


   Hmmm... what the approach in this patch definitely helps against, is the 
replicated log corruption when the position wraps.
   Resulting state will not be recoverable without erasing the replicated log; 
however, this is cleaner than the effects of the position wrap.  I'm not sure 
whether the 4X gain of possible position count makes a lot of difference for 
affected users or not.
   
   @cf-natali @bmahler It strikes me that no backward conversion (from the 
string key to an integer position) is ever used by the replicated log code 
Do we have a good understanding what the actual contract for `encode()` should 
look like?
   
   From what I can tell by quickly looking at the code, the main requirement is 
that `encode()` results, when ordered lexicographically, should be in the order 
of positions. This means that, **_if I haven't missed something_**, for 
example, prefixing integers larger than `9'999'999'999` by a single letter `A` 
will result in the correct order of keys:
   ```
   00
   ...
   98
   99
   A00100
   A00101
   ```


-- 
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] asekretenko merged pull request #387: Fixed compilation error on CAP_(PERFMON,BPF,CHECKPOINT_RESTORE).

[GitBox]

asekretenko merged pull request #387:
URL: https://github.com/apache/mesos/pull/387


   


-- 
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] cf-natali commented on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.

[GitBox]

cf-natali commented on pull request #388:
URL: https://github.com/apache/mesos/pull/388#issuecomment-849017166


   Yes.
   
   Unless the involvement of existing committers significantly improves over
   the next week or so, I'll contact the various users who manifested interest
   in order to start a fork.
   
   
   
   On Wed, 26 May 2021, 19:12 Renán I. Del Valle, ***@***.***>
   wrote:
   
   > This looks sane to me, and barring any opposition, this should probably be
   > shipped. 9 days for such a small patch is already a lot. Can a Mesos
   > committer either merge this or ask for improvements? This silence is
   > exactly what I was afraid of when I voiced my concerns in the e-mail thread
   > re: putting the project into the Attic.
   >
   > —
   > You are receiving this because you authored the thread.
   > Reply to this email directly, view it on GitHub
   > , or
   > unsubscribe
   > 

   > .
   >
   


-- 
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] ridv edited a comment on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.

[GitBox]

ridv edited a comment on pull request #388:
URL: https://github.com/apache/mesos/pull/388#issuecomment-849008518


   This looks sane to me, and barring any opposition, this should probably be 
shipped. 9 days for such a small patch is already a lot. Can a Mesos committer 
either merge this or ask for improvements? This silence is exactly what I was 
afraid of when I voiced my concerns about the current state of Mesos in the 
e-mail thread re: putting the project into the Attic.


-- 
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] ridv commented on pull request #388: Fixed a bug where the cgroup task killer leaves the cgroup frozen.

[GitBox]

ridv commented on pull request #388:
URL: https://github.com/apache/mesos/pull/388#issuecomment-849008518


   This looks sane to me, and barring any opposition, this should probably be 
shipped. 9 days for such a small patch is already a lot. Can a Mesos committer 
either merge this or ask for improvements? This silence is exactly what I was 
afraid of when I voiced my concerns in the e-mail thread re: putting the 
project into the Attic.


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

[GitBox]

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

[GitBox]

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

[GitBox]

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

[GitBox]

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