-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
cleanup: rsa pin #714
cleanup: rsa pin #714
Conversation
🚲 PR staged at http://34.123.56.89 |
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 taking care of this, @ckim328!
This one seems pretty complicated.
I was going to create a GitHub issue titled "Remove rsa
as a Direct Dependency" similar to #713, which I made for #711 — so that we eventually clean up the requirements.in
file. But I think this situation might be too messy to worry about (i.e., might not be worth the effort).
I've tested the staging URL, and things work! Approved. :)
I've left a few questions to clarify some things for myself and any future passerby.
Feel free to merge. :)
grpcio-health-checking==1.33.2 | ||
grpcio==1.33.2 | ||
opencensus==0.7.11 | ||
opencensus==0.8.0 |
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.
Question: Why do we manually upgrade opencensus
as well? Is it because the older version of opencensus
doesn't work with rsa==4.7
?
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 bumpedopencensus
since I saw it was an older lib
I thought it would be best if we update these while we can (instead of letting them age), but I can bump this back down to 0.7.11
!
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.
Awesome. Nope, don't bump it back down.
I just thought it was related to the rsa
stuff and was confused.
But yes, thanks for updating opencensus
as well! 👍
@@ -1,12 +1,13 @@ | |||
google-api-core==1.23.0 | |||
google-python-cloud-debugger==2.18 | |||
google-cloud-profiler==3.0.7 |
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.
Question: Why do we manually upgrade google-cloud-profiler
as well? Is it because the older version of google-cloud-profiler
doesn't work with rsa==4.7
?
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.
We will eventually have to bump google-cloud-profiler
so that it's compatible with v2.x of google-api-core
While this isn't completely necessary right now, we will have to bump it in the future
(also google-cloud-profiler v1.1.2 is 2 years old 😅 )
I tested and looked at the documentation for how we use google-cloud-profiler, no changes to the code is needed for how we use this module
* Bumping cloud-profiler * Bumping api-core * Pinned rsa to 4.7 * Updated opencensus lib
Background
Dependabot alerts for
rsa
indicating moderate severity in our recommendation serviceFixes
CVE descriptions can be found in the dependabot alerts
Change Summary
I had to pin rsa==4.7.0. I wanted to bump the modules that rely on rsa (ie
# via google-auth
), butgoogle-auth
is relies on a few more modules. I also bumpedopencensus
Additional Notes
The
opencensus-ext-stackdriver
is pinned to an oldgoogle-cloud-trace
, which disallows usage ofgoogle-core 2.x
.We should watch for this issue in the opencensus-python repo, and bump google-api-core when it's resolved
Testing Procedure
Tested the recommendation service bumps by deploying a cluster with
TRACING
,PROFILER
, andDEBUGGER
enabledRelated PRs or Issues
n/a