-
Notifications
You must be signed in to change notification settings - Fork 89
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
[security] Update profiler to 0.4.0 #999
[security] Update profiler to 0.4.0 #999
Conversation
I'm not sure how to effectively test this change, so if reviewer(s) could consider carefully, that would be appreciated. |
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.
(reviewing upon request of @barroco)
Given that we can't test automatically, we can look at the changelog of the library. My impression is that:
- there is no breaking change
- there are a few bug fixes that don't look like could impact us
- mostly it is updating dependencies, the most notable one being an upgrade to protobuf-v2
Only the dependencies update could maybe have some red flags.
Now, IIUC with this library the only thing we do is that we spawn a routine to generate and upload profiles, with a behavior that seems to be determined by the server. Given that, as long as the server used supports the new version this upgrade should be OK. Which since this is a hosted cloud service it should.
And also if that change break something, that should be limited to this profiling feature and should in theory not have a broader impact.
Only alternative would be to test manually, but it looks like this service is not enabled in any of the example deployment files:
- --gcp_prof_service_name= |
dss/build/deploy/metadata_base.libsonnet
Line 30 in 7579299
prof_grpc_name: '', |
IMO it is low-risk enough to go ahead with the change.
Tested deployment on GCP using terraform and helm deployment with DSS image built from this PR branch. Note that all test passed except the
|
Note that the problem reported above does not seem to be related to that particular change. The same check fails for the following dss image deployments:
An issue has been created to address this particular problem: #1002 |
Per meeting yesterday, my understanding is that we believe the concurrency latency issue is separate and should not block this PR. |
This is nominally a one-line PR that changes the profiler dependency from 0.2.0 to 0.4.0. The other changes are due to running
go mod tiy -compat=1.17
.