[sc-107107] AKS plugin: fix SDK objects log method#57
[sc-107107] AKS plugin: fix SDK objects log method#57amandineslx wants to merge 4 commits intomasterfrom
Conversation
|
This pull request has been linked to Shortcut Story #107107: AKS plugin: log all API calls. |
nikopol
left a comment
nikopol
left a comment
There was a problem hiding this comment.
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)) |
| 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)) |
| logging.info("Cluster creation results") | ||
| logging.info("Cluster creation results: %s", _print_as_json(cluster_create_op)) |
There was a problem hiding this comment.
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_jsonto handle correctly this object - remove this return from the log
There was a problem hiding this comment.
I have reproduced and will have a look
There was a problem hiding this comment.
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.
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.
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.
redundant and _print_as_json(delete_result) returned null, not useful =x
There was a problem hiding this comment.
indeed, it has been removed
python-lib/dku_utils/access.py
Outdated
| from collections import Mapping, Iterable | ||
| import sys | ||
| import json | ||
| import logging |
| logging.info("Cluster update results") | ||
| logging.info("Cluster update results: %s", _print_as_json(update_result)) |
| logging.info("Cluster deletion results") | ||
| logging.info("Cluster deletion results: %s", _print_as_json(delete_result)) |
| 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.
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.
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)) |
jules-casoli
left a comment
jules-casoli
left a comment
There was a problem hiding this comment.
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)
| 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.
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. |
5 participants
Story details: https://app.shortcut.com/dataiku/story/107107
[sc-107107]