Use errors.Is for error equality checks

[redwrasse]

Update err == * expressions to errors.Is(err, *), as a preferred means of error equality checks that can handle wrapping (https://pkg.go.dev/errors#pkg-overview).

This PR updates all cases to be fixed in a number of files, but not all files. The rest of the cases should be addressed in a separate PR.

[k8s-ci-robot]

Hi @redwrasse. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[henrybear327]

/ok-to-test

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 28.26087% with 33 lines in your changes missing coverage. Please review.

Project coverage is 68.90%. Comparing base (2c53be7) to head (503946f).
Report is 18 commits behind head on main.

❗ Current head 503946f differs from pull request most recent head d4df7a9

Please upload reports for the commit d4df7a9 to get more accurate results.

Files with missing lines	Patch %	Lines
client/v3/kubernetes/client.go	0.00%	27 Missing ⚠️
server/etcdserver/api/v3discovery/discovery.go	0.00%	1 Missing and 1 partial ⚠️
server/etcdserver/server.go	66.66%	2 Missing ⚠️
pkg/proxy/server.go	50.00%	1 Missing ⚠️
server/etcdserver/v3_server.go	87.50%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/api/v3compactor/periodic.go	90.24% <100.00%> (ø)
pkg/proxy/server.go	60.95% <50.00%> (ø)
server/etcdserver/v3_server.go	72.90% <87.50%> (ø)
server/etcdserver/api/v3discovery/discovery.go	67.80% <0.00%> (ø)
server/etcdserver/server.go	81.26% <66.66%> (ø)
client/v3/kubernetes/client.go	0.00% <0.00%> (ø)

... and 18 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18510      +/-   ##
==========================================
+ Coverage   68.79%   68.90%   +0.11%     
==========================================
  Files         420      420              
  Lines       35489    35470      -19     
==========================================
+ Hits        24413    24439      +26     
+ Misses       9646     9609      -37     
+ Partials     1430     1422       -8     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c53be7...d4df7a9. Read the comment docs.

[redwrasse]

Looks like a failed lint test, I'll update the PR.

[redwrasse]

Hmm, failed integration tests https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18510/pull-etcd-integration-1-cpu-amd64/1828914519779315712

• go.etcd.io/etcd/tests/v3/common: TestLeaseGrantKeepAliveOnce/PeerAutoTLS
• go.etcd.io/etcd/tests/v3/common: TestLeaseGrantKeepAliveOnce

Guessing this is just test flakiness? I'm not reproducing locally.

[henrybear327]

/retest

[ivanvc]

Hi @redwrasse, thanks for your pull request. I did a grep 'err ==' in your branch, and excluding err == nil, I still see some instances of comparisons using ==. Is there a particular reason why your PR addresses only some of them? (I may be missing something)

[redwrasse]

@ivanvc, nope, I had hoped to be thorough. Let me try to find the ones I missed.

[redwrasse]

I found three additional errors.Is conversions. Let me know if there's others still present. I see two additional but those are in a proto generated file rpc.pb.gw.go.

There are a handful of cases in tests of the pattern if err == nil || !strings.Contains(err.Error(), rpctypes.ErrRoleEmpty.Error()) that could be changed to errors.Is, but I haven't changed those here.

[ivanvc]

@redwrasse, sorry for the back and forth sweat_smile: I should have probably given you the list of files since the beginning. So here are some others that I see after pulling your branch:

• client/pkg/fileutil/lock_linux.go:67
• client/internal/v2/client.go:371
• client/internal/v2/keys.go:462
• client/v3/experimental/recipes/key.go:158
• client/v3/retry_interceptor.go:257
• client/v3/client.go:297
• client/v3/client.go:630
• client/v3/client.go:648
• client/v3/watch.go:398
• etcdctl/ctlv3/command/auth_command.go:85
• tests/e2e/ctl_v3_member_test.go:196
• tests/e2e/ctl_v3_member_test.go:224
• tests/integration/clientv3/connectivity/network_partition_test.go:38
• tests/integration/clientv3/connectivity/network_partition_test.go:325
• tests/integration/clientv3/concurrency/example_mutex_test.go:67
• tests/integration/clientv3/connectivity/black_hole_test.go:116
• tests/integration/clientv3/connectivity/black_hole_test.go:126
• tests/integration/clientv3/connectivity/black_hole_test.go:139
• tests/integration/clientv3/connectivity/black_hole_test.go:149
• tests/integration/clientv3/connectivity/black_hole_test.go:210
• tests/integration/clientv3/lease/leasing_test.go:2051 -> I'm not sure about this one
• tests/integration/clientv3/connectivity/server_shutdown_test.go:104
• tests/integration/clientv3/experimental/recipes/v3_lock_test.go:142
• tests/integration/v3_grpc_test.go:117 -> Not sure either
• server/etcdserver/api/v3compactor/revision.go:92
• server/etcdserver/api/rafthttp/pipeline.go:168
• server/etcdserver/api/rafthttp/snapshot_sender.go:113
• server/etcdserver/api/rafthttp/stream.go:431
• server/etcdserver/api/snap/snapshotter_test.go:83
• server/etcdserver/api/v2discovery/discovery.go:190
• server/etcdserver/api/v2discovery/discovery.go:230
• server/etcdserver/api/v3rpc/lease.go:103
• server/etcdserver/api/v3rpc/lease.go:113
• server/etcdserver/api/v3rpc/lease.go:136
• server/etcdserver/api/v3rpc/util.go:98
• server/etcdserver/api/v3rpc/watch.go:214
• server/etcdserver/api/v3rpc/watch.go:220
• server/etcdserver/api/v3rpc/watch.go:244
• server/etcdserver/apply/uber_applier.go:126
• server/lease/leasehttp/http.go:78
• server/proxy/grpcproxy/health.go:55
• server/proxy/grpcproxy/maintenance.go:53
• server/proxy/grpcproxy/lease.go:248
• server/storage/wal/decoder.go:92
• server/storage/wal/decoder.go:117

And matching err !=:

• server/storage/wal/wal_test.go:155
• server/storage/wal/wal_test.go:967
• server/storage/wal/repair_test.go:195
• server/storage/mvcc/kvstore_test.go:521
• server/storage/schema/actions_test.go:138
• server/storage/mvcc/key_index_test.go:216
• server/storage/mvcc/index_test.go:133
• server/storage/mvcc/index_test.go:137
• server/storage/mvcc/kv_test.go:206
• server/storage/mvcc/kv_test.go:629
• server/storage/backend/batch_tx.go:128
• server/lease/lessor_test.go:459
• server/lease/lessor_test.go:512
• server/etcdmain/config_test.go:227
• server/etcdmain/config_test.go:270
• server/etcdmain/config_test.go:313
• server/embed/etcd_test.go:35
• server/auth/jwt_test.go:142
• and many in server/auth/store_test.go

And the list is way longer than I expected. I didn't continue checking all of the err !=, as there are many matches (other than the proto buffer generated files, as you mentioned). Because of the size of the list and changes, it would be better to do it in a follow-up pull request(s).

Please let me know if the instances I found are not relevant to this change.

[redwrasse]

@ivanvc thanks, my IDE search was not pulling these up for some reason.

I'm happy to try to make the remaining changes in a separate follow-up PR, if you're fine with the scope of this current one.

[ivanvc]

@redwrasse, what do you think if we scope it to the files you have already edited in this pull request? In that case, here's the list of missing ones (basically the err != case):

• tests/integration/clientv3/lease/lease_test.go:42
• tests/integration/clientv3/lease/lease_test.go:58
• tests/integration/clientv3/lease/lease_test.go:94
• tests/integration/clientv3/lease/lease_test.go:118
• server/etcdserver/server.go:1984
• server/etcdserver/v3_server.go:300
• server/etcdserver/v3_server.go:531
• tests/integration/clientv3/kv_test.go:54
• tests/integration/clientv3/kv_test.go:59
• tests/integration/clientv3/kv_test.go:122
• tests/integration/clientv3/kv_test.go:161
• tests/integration/clientv3/kv_test.go:203
• tests/integration/clientv3/kv_test.go:417
• tests/integration/clientv3/kv_test.go:422
• tests/integration/clientv3/kv_test.go:754

Thanks again, Samir!

[redwrasse]

Sounds good! I'll do that.

[redwrasse]

Alright, I believe the latest commit with the err != cases covers all files changed in this PR.

Hopefully, can get through the rest of them shortly in a separate PR.

[ivanvc]

Thanks for addressing, @redwrasse. Can you squash your commits, please? Thanks again.

[ivanvc]

LGTM. Thanks, Samir.

[redwrasse]

Perfect, I'll work on the rest of the cases as well as possibly the miscellaneous strings.Contains(err.Error(), substring) instances.

[jmhbnz]

LGTM - Thanks for tidying these up @redwrasse. Happy to complete the remainder in subsequent pr's

cc @serathius, @ahrtr

[redwrasse]

Great, I tried to hopefully get the rest of the cases in #18551.

[ivanvc]

Link to #18576.

Do we have enough approvals to merge this?

[ahrtr]

@serathius any comment or concern on this PR?

[serathius]

No concerns.

Replacing err == with errors.Is seems correct and safe enough that we can do it in bolk like this. Would be interesting if there is any static analysis that would highlight outdated comparison method.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ivanvc, jmhbnz, redwrasse, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ahrtr,jmhbnz,serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ivanvc]

No concerns.

Replacing err == with errors.Is seems correct and safe enough that we can do it in bolk like this. Would be interesting if there is any static analysis that would highlight outdated comparison method.

That's a good point. Without a static check, I'm sure we'll soon have newly written code checking errors with ==/!=. I checked golangci-lint and revive, but there's nothing yet for it. We'll need to wait and keep an eye on it.