-
Notifications
You must be signed in to change notification settings - Fork 6
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
[sc-107107] AKS plugin: fix SDK objects log method #57
base: master
Are you sure you want to change the base?
Conversation
This pull request has been linked to Shortcut Story #107107: AKS plugin: log all API calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some stuff to change or discuss. btw, i think _print_as_json
is not an accurate name, something like _json_dump
would be more appropriate.
logging.info("Information retrieved for cluster") | ||
logging.info("Information retrieved for cluster %s in %s: %s", cluster_name, resource_group, _print_as_json(get_cluster_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant
logging.info("Kubeconfig retrieved for cluster") | ||
logging.info("Kubeconfig retrieved for cluster %s in %s: %s", cluster_name, resource_group, _print_as_json(get_credentials_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant
logging.info("Cluster creation results") | ||
logging.info("Cluster creation results: %s", _print_as_json(cluster_create_op)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant and _print_as_json(cluster_create_op)
result to {}
for me.
three options:
- try to log this at the return of this function since it returns
cluster_create_op.result()
which may be more serializable - improve
_object_to_json
to handle correctly this object - remove this return from the log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reproduced and will have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not a bug, this object is empty
removed log line
logging.info("Kubeconfig retrieved for cluster") | ||
logging.info("Kubeconfig retrieved for cluster %s in %s: %s", self.cluster_name, resource_group, _print_as_json(get_credentials_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant and do we really want log out sensitive info such as the kubeconfig ? that allows anyone who can read the log the ability to manage the cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the persons able to display the cluster information will see the cluster configuration in the page
and the persons able to see the logs also can display this page
so I don't think it is a problem logging it
logging.info("Cluster deletion results") | ||
logging.info("Cluster deletion results: %s", _print_as_json(delete_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant and _print_as_json(delete_result)
returned null
, not useful =x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, it has been removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still there
python-lib/dku_utils/access.py
Outdated
@@ -2,6 +2,7 @@ | |||
from collections import Mapping, Iterable | |||
import sys | |||
import json | |||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
logging.info("Cluster update results") | ||
logging.info("Cluster update results: %s", _print_as_json(update_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant
logging.info("Cluster deletion results") | ||
logging.info("Cluster deletion results: %s", _print_as_json(delete_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still there
@@ -440,6 +442,7 @@ def do_delete(): | |||
future = clusters_client.managed_clusters.begin_delete(resource_group, cluster_name) | |||
return future.result() | |||
delete_result = run_and_process_cloud_error(do_delete) | |||
logging.info("Cluster deletion results") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no results a this point
logging.info("Cluster deletion results") | |
logging.info("Cluster deletion requested") |
logging.info("Cluster update results: %s", _print_as_json(update_result)) | ||
logging.info("Cluster updated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant
logging.info("Cluster update results: %s", _print_as_json(update_result)) | |
logging.info("Cluster updated") | |
logging.info("Cluster updated with results: %s", _print_as_json(update_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is printed with rather ugly literal \n
, I don't remember if that was what we wanted ? (it is not what we do on e.g. gke)
@@ -320,6 +320,7 @@ def start(self): | |||
logging.info("Start creation of cluster") | |||
def do_creation(): | |||
cluster_create_op = cluster_builder.build() | |||
logging.info("Cluster creation results: %s", _print_as_json(cluster_create_op.__dict__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit : I only get an empty dict on success, maybe no print in that case ?
Putting this PR on hold until we have decided what we want to do with it. |
This pull request has been linked to Shortcut Story #105893: [AKS/EKS/GKE] Identify and hide secrets in plugin logs. |
Story details: https://app.shortcut.com/dataiku/story/107107
[sc-107107]