vdk-control-cli: allow cli users to explicitly set the user agent tag #1403
Conversation
Signed-off-by: murphp15 <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: murphp15 <[email protected]>
…sts' into person/murphp15/vdk_client_requests
|
I've created corresponding issue in Github for the work explaining the problem and the solution - #1404 |
projects/vdk-control-cli/src/vdk/internal/control/configuration/vdk_config.py
Outdated
Show resolved
Hide resolved
Signed-off-by: murphp15 <[email protected]>
for more information, see https://pre-commit.ci
|
For the purposes of this change, you can use a local server with telemetry endpoint like https://webhook.site/ or https://requestbin.com/ Thought it is a good point. Our CICD deployment uses vcsa.vmware.com currently for telemetry - https://github.com/vmware/versatile-data-kit/blob/main/projects/control-service/cicd/deploy-testing-pipelines-service.sh#L97 |
|
LGTM |
| @@ -18,6 +19,11 @@ class VDKConfig: | |||
|
|
|||
| _op_id = os.environ.get("VDK_OP_ID_OVERRIDE", f"{uuid.uuid4().hex}"[:16]) | |||
|
|
|||
| _user_agent = os.environ.get( | |||
| "VDK_CONTROL_SERVICE_USER_AGENT", | |||
| f"vdk-control-cli/1.2 ({os.name}; {sys.platform})" + sys.version.split(" ")[0], | |||
There was a problem hiding this comment.
You can get version from projects/vdk-control-cli/src/vdk/internal/control/command_groups/version_group/version.py
But how about this
| f"vdk-control-cli/1.2 ({os.name}; {sys.platform})" + sys.version.split(" ")[0], | |
| f"vdk-control-cli/{version.__version__} ({sys.platform}; {platform.platform()}; ) Python {platform.python_version()} Node {platform.node()} User {getpass.getuser()}", |
There was a problem hiding this comment.
Is that revealing too much info?
I'm not sure I want my username being sent in a user-agent tag?
There was a problem hiding this comment.
I agree with adding {version.__version__}.
I also don't think we want to include the node name.
my node id is paulmxxxxxx.vmware.com'.
Seems like too much info to include in my opinion.
@tozka Please let me know what you think.
There was a problem hiding this comment.
I like this 'vdk-control-cli/1.3.0 (darwin; macOS-12.6.1-arm64-arm-64bit; ) Python 3.10.8'
There was a problem hiding this comment.
Ok. I am OK with this.
AWS allows hostname though it contains user information. assuming it's used by admin team of the control service, it might be ok to have host name ... But I understand the having user data could be a bad precedent and lead to incidental PII violation. So I am ok to default without node and user info
There was a problem hiding this comment.
Something seems wrong. Did it auto-merge? This was not supposed to be the change to be merged ?
There was a problem hiding this comment.
My bad.
Here is the fix #1412.
Can you please take a look at it.
# Why? This PR is including a small fix that was supposed to be in this(#1403) PR. It's tested as described in the linked PR. # What? Corrections to the tag format # How has this been tested? Tested as part of the previous ticket. # What type of change are you making? Bug fix Signed-off-by: murphp15 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Why
Users are looking to be able to determine where requests originated from when browsing the data.
How
We are going to let cli users explicitly set the user-agent header.
How has this been tested
sent requests to vdk aas and made sure they have the correct user agent in super collider.
The results of this test can only be validated by VMware employees which isn't ideal for an open source project. https://sql.supercollider.vmware.com/stg/history/index.html?sql=select%20*%20from%20history_staging.taurus_httptrace%20where%20request_headers_user_agent%20like%20%27%25paul%25%27%20limit%2010&ddlOutputType=table&version=1#
Closes: #1404
Signed-off-by: murphp15 [email protected]