Open nathanjsweet opened 5 months ago
Calling deprecated functions is a low-level security risk. It would be better if Cilium kept track of its deprecated function calls somehow (maybe a periodic CI job?) and periodically fixed them.
Hi @nathanjsweet , I noticed that golangci-lint staticcheck
for deprecated method is disabled in .golnagci.yaml
.
Would it be a good idea to enable staticcheck SA1019
?
https://github.com/cilium/cilium/blob/5cb8a54a58fe700ac41b64711d3e921e7a55d8a0/.golangci.yaml#L86
Would it be a good idea to enable staticcheck SA1019 ?
No. Even if we magically addressed every deprecated function call right now, at some point the check will break CI. It's better to think of this as a periodic health check that gets addressed. I'll assign this to myself for the time being.
Can I work on this issue, can you guide me through this, where to start from
Hey @yogesh1801 , We would love to have your help! The first thing is to create a PR that removes all of these deprecated function calls (maybe one at a time). The second would be to make a workflow for CI that either runs on a monthly schedule to publish a report or to have it be a part of pull request CI. The more I think about this problem, the more I think that we could potentially have it be a part of the pull request workflow, because pull requests will always introduce these problems (even if is a simple library upgrade). However, we need to be careful, because perhaps there are other avenues for deprecated function calls to creep in without a pull request, in which case, we would have broken CI for everyone.
@nathanjsweet i was trying to remove the depreacted calls, i tried fixing the strings.Title deprecated call using cases, can you tell me how can i test if my fixes are right or not, or should i create a PR straight away. I am planning to remove all deprecated function one by one and then create a CI pipeline for the same.
this is what i did for reference
Hi,
I was looking at a first task, any help needed on this issue ?
Regards
Hi @davchos , I'm not sure whether all of the individual deprecation warnings were addressed in the codebase, but you could try pulling the latest main
branch and verify that. If there are any remaining warnings, we would encourage pull requests to address those warnings.
Additionally, as far as I know, the staticcheck for deprecation warnings isn't integrated into the .github/workflows CI yet. The last part of the description requests for this check to be added into the CI. This could be a separate PR submission.
Hi @joestringer,
Some deprecation warnings are present, I will work on it.
David
Hi @joestringer,
I have some deprecated calls that will be supported throughout 1.x.x, especially for grpc. Can I remove them, when possible ?
I suppose they should be removed.
operator/auth/spire/client.go:179:15: grpc.Dial is deprecated: use NewClient instead. Will be supported throughout 1.x. (SA1019)
Thanks in advance for your help.
David
Good question, let me cc @cilium/sig-servicemesh specifically since that codepath is related to SPIRE.
I'm not sure how much work it is to overhaul that logic to use NewClient
, but if you wanted to try switching it over then you're welcome to give it a go. If you think that it seems viable to switch this code over then we can go through review for that PR to confirm it's a path we want to use. As a backup we could always add nolint
tags, but that will only kick the can further down the road until we eventually do want to switch the library over. Presumably the warning is there to give us notice that 1.x will eventually be retired and then we'll need to switch the library over to continue receiving updates.
@joestringer, ok I'll try the switch.
David
Running staticcheck on Cilium gives the following list:
Cilium should fix these issues as well as create CI that reports deprecated calls.