-
Notifications
You must be signed in to change notification settings - Fork 27
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
Specify User Agent for Console CLI requests #952
Specify User Agent for Console CLI requests #952
Conversation
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #952 +/- ##
============================================
- Coverage 80.88% 78.98% -1.90%
Complexity 2723 2723
============================================
Files 402 375 -27
Lines 15586 13869 -1717
Branches 970 970
============================================
- Hits 12607 10955 -1652
+ Misses 2405 2340 -65
Partials 574 574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
...tion/src/main/docker/migrationConsole/lib/console_link/console_link/models/client_options.py
Show resolved
Hide resolved
@@ -1,6 +1,7 @@ | |||
from enum import Enum | |||
from typing import Dict, Optional | |||
|
|||
from console_link.models.client_options import ClientOptions |
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.
Okay, I think I slightly regret YAML vs env variable decision because we could have kept env variable stuff much more limited to only grabbing it at the level of each client, instead of having to pass it all the way through the code -- it feels like conceptually this shouldn't have to be woven through all these factories and lots of unrelated code and it should just stay low level, but I don't really have any way in mind to implement that if it comes from the YAML.
Which is all to say, I'm not sure if there's a better solution or not, and definitely not sure whether it's worth refactoring this, because I think it's well-implemented for this route. Thoughts?
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.
(Circling back to add that after looking at the tests, this services.yaml approach is definitely clearer and easier to test, so that's a point in its favor)
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.
From some discussion with Mikayla there are pros and cons with the current approach versus setting something more globally instead of passing through yaml. I'm comfortable with the current approach for now, and think it can be improved so there are less touch points next time we want some global settings but let me know if you have concerns @mikaylathompson
@@ -217,9 +216,14 @@ def get_metrics(self, recent=False) -> Dict[str, List[str]]: | |||
raise NotImplementedError("Recent metrics are not implemented for Prometheus") | |||
for c in Component: | |||
exported_job = prometheus_component_names(c) | |||
headers = None | |||
if self.client_options and self.client_options.user_agent_extra: | |||
headers = append_user_agent_header_for_requests(headers=None, |
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.
Out of curiosity, what's the reasoning for adding it to the headers here? I guess that if a user is using this field for something other than what we are, they would expect it to be applied for more than just aws services?
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.
Yes exactly, I want this user-agent feature to be valuable for other use cases cx may have as well
if source_cluster.auth_details and source_cluster.auth_details["password_from_secret_arn"]: | ||
source_auth_secret = source_cluster.auth_details["password_from_secret_arn"] |
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.
this is just the arn
-- should it be the real password? If so, it needs to use the method that pulls it from secrets manager. Additionally, if password_from_secret_arn
doesn't exist, line 288 will throw a key error (python vs js semantics)
if source_cluster.auth_details and source_cluster.auth_details["password_from_secret_arn"]: | |
source_auth_secret = source_cluster.auth_details["password_from_secret_arn"] | |
if source_cluster.auth_details and "password_from_secret_arn" in source_cluster.auth_details: | |
source_auth_secret = source_cluster.get_basic_auth_password() |
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.
Oh, actually, source_auth_secret
does sound like you just want the arn. So ignore the second line!
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.
Thanks for catching, have updated now. And yes we want the arn here 👍
adjusted_headers = dict(headers) if headers else {} | ||
if "User-Agent" in adjusted_headers: | ||
adjusted_headers["User-Agent"] = f"{adjusted_headers['User-Agent']} {user_agent_extra}" |
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 annotation says these aren't covered by a test, but I think this would be a very good condition to have an explicit check on. Can you add a quick one?
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.
Added now
adjusted_headers["User-Agent"] = f"{requests.utils.default_user_agent()} {user_agent_extra}" | ||
return adjusted_headers |
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.
Actually, these lines aren't covered either, so maybe two tests that cover this whole function?
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.
Added now
@@ -122,6 +122,10 @@ export class KafkaYaml { | |||
standard?: string | null; | |||
} | |||
|
|||
export class ClientOptions { | |||
user_agent_extra?: string | null; |
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, but doesn't the ?
mean that you don't have to add | null
?
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.
This is a good point, I've seen us use this pattern but do we ever use null? Just having the ?
is sufficient for undefined
. I have removed the null and not seeing any issues with tests currently
# Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/metrics_source.py # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/utils.py
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
# Conflicts: # deployment/cdk/opensearch-service-migration/bin/app.ts # deployment/cdk/opensearch-service-migration/lib/stack-composer.ts
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
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 one more test request for something that includes a cluster call_api
. Otherwise LGTM
...kerSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/cluster.py
Show resolved
Hide resolved
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Description
Allows setting a global user agent on CDK deployments that will be configured in a
client_options
section in theservices.yaml
. This user agent will currently be provided to the Traffic Replayer for its replayed requests (existing logic) and to the Console CLI for all of its boto3 requests and python requests library requests. This can also be extended to other migrations as support is added.Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-1835
Testing
Unit testing and Cloud deployment testing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.