Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accomodated review comments from JHutar #106

Closed
wants to merge 21 commits into from
Closed

accomodated review comments from JHutar #106

wants to merge 21 commits into from

Conversation

sarathbrp
Copy link
Contributor

No description provided.

@sarathbrp sarathbrp requested a review from jhutar January 11, 2024 14:10
@sarathbrp sarathbrp self-assigned this Jan 11, 2024
@@ -75,6 +75,10 @@ def load_config(conf, fp):
assert not conf.history_es_server.endswith("/")
conf.history_es_index = data["history"]["es_index"]
conf.history_es_query = data["history"]["es_query"]
if "es_server_user" in data["history"]:
conf.es_server_user = data["history"]["es_server_user"]
conf.es_server_pass = data["history"]["es_server_pass"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
conf.es_server_pass = data["history"]["es_server_pass"]
conf.es_server_pass_env_var = data["history"]["es_server_pass_env_var"]

@@ -88,6 +92,10 @@ def load_config(conf, fp):
conf.decisions_es_server = data["decisions"]["es_server"]
assert not conf.decisions_es_server.endswith("/")
conf.decisions_es_index = data["decisions"]["es_index"]
if "es_server_user" in data["decisions"]:
conf.decisions_es_server_user = data["decisions"]["es_server_user"]
conf.decisions_es_server_pass = data["decisions"]["es_server_pass"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, just env var

opl/http.py Outdated
@@ -12,6 +12,7 @@ def disable_insecure_request_warnings(disable_it):
if disable_it:
logging.debug("Disabling insecure request warnings")
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
session.verify = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not put it even here. I would leave it on user to explicitly do http.session.verify = False or so. I admit this function name in unfortunate. Also this http module should have been a class or so, this sucks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe just create function like this:

def insecure():
    session.verify = False
    disable_insecure_request_warnings(True)

else:
response = opl.http.get(url, headers=headers, json=data)

if response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this if response: required?

@@ -114,12 +114,18 @@ def main():
if args.history_type == "csv":
history = opl.investigator.csv_loader.load(args.history_file, args.sets)
elif args.history_type == "elasticsearch":
if hasattr(args, "es_server_user"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if hasattr(args, "es_server_user"):
if hasattr(args, "es_server_verify"):

@@ -200,8 +206,17 @@ def main():
if not args.dry_run:
for d_type in args.decisions_type:
if d_type == "elasticsearch":
if hasattr(args, "es_server_user"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if hasattr(args, "es_server_user"):
if hasattr(args, "es_server_verify"):

@sarathbrp sarathbrp closed this Jan 12, 2024
@sarathbrp sarathbrp reopened this Jan 12, 2024
opl/http.py Outdated
@@ -8,6 +8,12 @@
session = requests.Session()


def insecure():
session.verify = False
logging.debug("Disabling insecure request warnings")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed, it will be printed in that disable_insecure_request_warnings. Or maybe you meant:

Suggested change
logging.debug("Disabling insecure request warnings")
logging.debug("Disabling SSL verifications for this session")

history = opl.investigator.elasticsearch_loader.load(
args.history_es_server,
args.history_es_index,
args.history_es_query,
args.sets,
es_server_user=getattr(args, "es_server_user", None),
es_server_pass=getattr(args, "es_server_pass_env_var", None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls rename es_server_pass as well to be consistent.

args.decisions_es_index,
info_all,
es_server_user=getattr(args, "decisions_es_server_user", None),
es_server_pass=getattr(args, "decisions_es_server_pass_env_var", None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Pls either use es_server_pass_env_var or call os.environ.get(...) here.

@@ -114,12 +114,18 @@ def main():
if args.history_type == "csv":
history = opl.investigator.csv_loader.load(args.history_file, args.sets)
elif args.history_type == "elasticsearch":
if hasattr(args, "es_server_verify"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you need this, or am I missing something?

Suggested change
if hasattr(args, "es_server_verify"):
if hasattr(args, "es_server_verify") and args.es_server_verify == False:

@@ -200,8 +206,17 @@ def main():
if not args.dry_run:
for d_type in args.decisions_type:
if d_type == "elasticsearch":
if hasattr(args, "es_server_verify"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

args.decisions_es_index,
info_all,
es_server_user=getattr(args, "decisions_es_server_user", None),
decisions_es_server_pass_env_var=getattr(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tihs is not a requirement, but to align with es_server_user I would rename decisions_es_server_pass_env_var to es_server_pass_env_var. Or rename es_server_user to decisions_es_server_user

if "es_server_user" in data["history"]:
conf.es_server_user = data["history"]["es_server_user"]
conf.es_server_pass_env_var = data["history"]["es_server_pass_env_var"]
conf.es_server_verify = data["history"]["es_server_verify"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls make this option optional, defaulting to True. Also this option is valid for setups without auth, so should be outside of this if.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have removed If condition then most of services fail at this line because investigator.conf file is not updated with username, password and verify flag. The plan is to start with front-end service and slowly propagate changes to other services, once it is complete then If condition is not longer required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep it for now until all investigator.conf files are updated across the services.

@jhutar
Copy link
Contributor

jhutar commented Jan 15, 2024

Pls also add these new fields (commented out) to the sample config, maybe with some explanation.

@@ -75,6 +75,9 @@ def load_config(conf, fp):
assert not conf.history_es_server.endswith("/")
conf.history_es_index = data["history"]["es_index"]
conf.history_es_query = data["history"]["es_query"]
conf.history_es_server_user = data["history"]["es_server_user"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! conf.history_es_server_user and pass needs to stay in that if because they will not be configured every time. Just conf.history_es_server_verify should be excluded from it because it can be used for unauthenticated ES.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this:

    if conf.history_type == "elasticsearch":
        conf.history_es_server = data["history"]["es_server"]
        assert not conf.history_es_server.endswith("/")
        conf.history_es_index = data["history"]["es_index"]
        conf.history_es_query = data["history"]["es_query"]
        if "es_server_user" in data["history"]:
            conf.history_es_server_user = data["history"]["es_server_user"]
            conf.history_es_server_pass_env_var = data["history"]["es_server_pass_env_var"]
        if "es_server_verify" in data["history"]:
            conf.history_es_server_verify = data["history"]["es_server_verify"]
        else:
            conf.history_es_server_verify = True

@@ -75,6 +75,9 @@ def load_config(conf, fp):
assert not conf.history_es_server.endswith("/")
conf.history_es_index = data["history"]["es_index"]
conf.history_es_query = data["history"]["es_query"]
conf.history_es_server_user = data["history"]["es_server_user"]
conf.history_es_server_pass_env_var = data["history"]["es_server_pass_env_var"]
conf.history_es_server_verify = data["history"]["es_server_verify"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it so that if es_server_verify is not configured in the config, conf.history_es_server_verify is set to true.

conf.decisions_es_server_pass_env_var = data["decisions"][
"decisions_es_server_pass_env_var"
]
conf.decisions_es_server_verify = data["decisions"]["es_server_verify"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Comment on lines 216 to 217
decisions_es_server_user=getattr(args, "decisions_es_server_user", None),
es_server_pass_env_var=getattr(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either call both with decisions_* or without.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably without, as you are doing that bit above in call to history = opl.investigator.elasticsearch_loader.load(...)

@jhutar
Copy link
Contributor

jhutar commented Jan 15, 2024

Pls also fix linter complaints and add commented example to https://github.com/redhat-performance/opl/blob/main/opl/investigator/sample_config.yaml

Copy link
Contributor

@jhutar jhutar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@@ -1,22 +1,42 @@
import argparse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello. This file should not be in this PR. Pls remove it.

@sarathbrp sarathbrp closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants