Use errors.Is for error equality checks

[ianlancetaylor]

Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
• This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• You have read the etcd bug reporting guidelines.
• Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

When using the current Go HEAD which will be the future Go 1.24 release, in the directory etcd-io/etcd/server, running go test ./embed fails. The test error messages are:

--- FAIL: TestLogRotation (0.00s)
    --- FAIL: TestLogRotation/invalid_logger_config (0.00s)
        config_test.go:736: test "invalid logger config", expected error: invalid log rotation config: json: cannot unmarshal bool into Go struct field logRotationConfig.maxsize of type int, got: invalid log rotation config: json: cannot unmarshal bool into Go struct field logRotationConfig.Logger.maxsize of type int

What did you expect to happen?

I expected the tests to pass.

How can we reproduce it (as minimally and precisely as possible)?

Build Go from HEAD as outlined at https://go.dev/doc/install/source. Use the newly built Go to run the etcd-io/server tests.

Anything else we need to know?

In general we recommend against testing for exact error strings. The Go standard library reserves the right to change error strings. It's better to simply test whether an error occurred or not. If it's necessary to check for a particular error, it's better to use errors.Is or errors.As to check the error. If that is infeasible, it's better to use strings.Contains to check that the error message contains some identifying string. Thanks.

Etcd version (please run commands below)

$ etcd --version
# paste output here

$ etcdctl version
# paste output here

Etcd configuration (command line flags or environment variables)

linux/amd64

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

$ etcdctl member list -w table
# paste output here

$ etcdctl --endpoints=<member list> endpoint status -w table
# paste output here

Relevant log output

No response

[ivanvc]

Hi @ianlancetaylor, Thanks for raising this issue. We aim to have the main branch working with the latest Go version released. When a new minor is released. We work to adopt the new version (refer to issues like #18443 and #18548) and plan/log the work in a version bump issue.

Thanks for raising awareness about comparing errors with errors.Is(); we currently have #18510 and #18551 open to track this change.

I think the best action item for this issue is to close it for now, as we'll work on the Go 1.24.0 update when it gets released.

Thanks again, Ian.

[ivanvc]

We discussed this issue and #18510 in our fortnightly etcd triage meeting. We'll keep this issue open and use it as an umbrella to track the adoption of errors.Is.

[ivanvc]

/retitle Use errors.Is for error equality checks

[redwrasse]

/assign

[redwrasse]

Will try to get time tomorrow to submit PRs for this issue.

[redwrasse]

Created a number of pull requests just now. These should constitute all changes originally in #18551 for the remaining errors.Is conversions.

[redwrasse]

@ianlancetaylor, I'm a culprit here, but it occurred to me- are there possible performance concerns with errors.Is to be considered against the benefits of robustness to error wrapping, that might merit not putting them in place of error equality/inequality checks in certain parts of the etcd codebase?

[ivanvc]

@ianlancetaylor, I'm a culprit here, but it occurred to me- are there possible performance concerns with errors.Is to be considered against the benefits of robustness to error wrapping, that might merit not putting them in place of error equality/inequality checks in certain parts of the etcd codebase?

@serathius, WDYT?

[ianlancetaylor]

If the choice is between using errors.Is or calling Error and comparing the error string to some value, then ordinarily errors.Is will be faster, because it does not have to construct an error string. In cases where no error string needs to be constructed, then errors.Is will also be fast.

For testing purposes a better choice is often just err != nil, which is of course faster still. You should only reach for errors.Is when it is important to know which specific error occurred. For many tests it suffices to know that some error occurred.

[ivanvc]

Once we finish addressing this, let's enable https://golangci-lint.run/usage/linters/#errorlint as part of this effort, as suggested by @mmorel-35.

[redwrasse]

@ivanvc @mmorel-35, let me know if there's anything more you need me to do here for this issue.

@mmorel-35, it looks like you are handling adding in errorlint, which is great.

[mmorel-35]

You can activate errorlint through golangci config and pass it on your modified files. If there is nothing left to do you are good to go .

What I'm doing is exactly this for each errorlint. Once the project is ready I will persist the configuration

[ivanvc]

Closing as all tasks are complete, and the errorlint linter is now enabled. Thanks for your work on this, @mmorel-35 and @redwrasse! 🙇