Use native error-wrapping instead of pkg/errors

[thaJeztah]

follow-up to #3, so opening as draft. Only the last two commits are new

This removes the dependency on pkg/errors, in favour of Go's native wrapping

1. IsNotExist(): rewrite to use errors.Is(). This requires Go 1.13 or up
2. Use native error-wrapping instead of pkg/errors

[kolyshkin]

I'm not sure what kind of errors are supposed to be supplied as an argument here, but in case the error was obtained using errors.Wrap() from pkg/errors version > 0.9.0, the new code won't work as the Unwrap method only appeared in pkg/errors 0.9.0.

So, this either needs to be documented. Alternatively, we can do a runtime check, something like "if this is an error which has Cause method, it should also have Unwrap method, otherwise paniс saying the caller is using old pkg/errors". Complicated...

[thaJeztah]

Ah, good one; I wanted to answer "but go mod enforces pkg/errors 0.9.1, but we remove that now of course 🤔

We can do a quick check which projects use this projects as a dependency (I think that list is limited, so we can at least make sure that they use a current pkg/errors)

[thaJeztah]

Doing a quick scan using https://grep.app/search?q=github.com/cyphar/filepath-securejoin

Consumers (using pkg/errors v0.9.1))

• https://github.com/bcicen/ctop
• https://github.com/cloudfoundry/cli
• https://github.com/containers/podman
• https://github.com/cri-o/cri-o
• https://github.com/genuinetools/img
• https://github.com/google/cadvisor
• https://github.com/hashicorp/nomad
• https://github.com/helm/helm
• https://github.com/kubernetes/autoscaler
• https://github.com/kubernetes/kubernetes
• https://github.com/kubesphere/kubesphere
• https://github.com/moby/moby
• https://github.com/opencontainers/runc
• https://github.com/opencontainers/umoci
• https://github.com/openshift/origin
• https://github.com/lunarway/release-manager
• https://github.com/goharbor/harbor
• https://github.com/rancher/k3s
• https://github.com/elastic/beats
• https://github.com/kubeedge/kubeedge
• https://github.com/istio/istio
• https://github.com/kubeapps/kubeapps
• https://github.com/hpcng/singularity
• https://github.com/kubernetes/minikube
• https://github.com/datawire/ambassador
• https://github.com/rancher/rancher

Indirect

• https://github.com/linkerd/linkerd2 (indirect)
• https://github.com/databus23/helm-diff (indirect)
• https://github.com/derailed/k9s (indirect)
• https://github.com/operator-framework/operator-sdk (indirect)

Indirect / potentially affected

• https://github.com/fluxcd/flux (old pkg/errors, but using go 1.15)
• https://github.com/flynn/flynn (old pkg/errors)
• https://github.com/bblfsh/bblfshd (indirect dependency, uses older pkg/errors)
• https://github.com/ContainerSolutions/helm-monitor (indirect dependency, uses older pkg/errors)
• https://github.com/deepmind/kapitan (indirect dependency, uses older pkg/errors)
• https://github.com/helm/monocular (indirect dependency, uses older pkg/errors)

[kolyshkin]

https://github.com/fluxcd/flux (old pkg/errors, but using go 1.15)

Updated to 0.9.1

https://github.com/flynn/flynn (old pkg/errors)

No longer maintained.

https://github.com/bblfsh/bblfshd (indirect dependency, uses older pkg/errors)

Appears to be no longer maintained (last commits are about removing maintainers)

https://github.com/ContainerSolutions/helm-monitor (indirect dependency, uses older pkg/errors)

Appears to be no longer maintained (last commit is from 2019)

https://github.com/deepmind/kapitan (indirect dependency, uses older pkg/errors)
https://github.com/helm/monocular (indirect dependency, uses older pkg/errors)

These to appear to be alive but uses older pkg/errors; can be updated though.

So I guess we can proceed with the change.

[kolyshkin]

I would do this instead:

 // ErrSymlinkLoop is returned by SecureJoinVFS when too many symlinks have been
 // evaluated in attempting to securely join the two given paths.
-var ErrSymlinkLoop = errors.Wrap(syscall.ELOOP, "secure join")
+//
+// Deprecated: use errors.Is(err, syscall.ELOOP) instead.
+var ErrSymlinkLoop = syscall.ELOOP
 
 // IsNotExist tells you if err is an error that implies that either the path
 // accessed does not exist (or path components don't exist). This is

@@ -68,7 +59,7 @@ func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) {
        n := 0
        for unsafePath != "" {
                if n > 255 {
-                       return "", ErrSymlinkLoop
+                       return "", &os.PathError{Op: "SecureJoin", Path: root + "/" + unsafePath, Err: syscall.ELOOP}
                }
 
                // Next path component, p.

[kolyshkin]

Ah, I just did a similar patch and then found this one :) which also needs some updating.

@thaJeztah do you think you can revive this?

@cyphar do you think it can be merged? It should be good to ditch pkg/errors

[cyphar]

Yeah I'd be happy to take something like this now (the lack of backtraces is sad, but ultimately error messages are usually enough in practice).

[kolyshkin]

the lack of backtraces is sad

It was part of the proposal that ended up in adding %w, errors.Is and errors.As, but dropped later (golang/go#29934 (comment)). The "beta" version of stdlib
errors package, https://pkg.go.dev/golang.org/x/xerrors, still provides the backtraces.

Practically I saw a need for a backtrace once or twice, when the error itself is not good enough
(like when a bare unix errno was returned), so proper error wrapping (or adding context by using
e.g. os.PathError, os.SyscallError and the likes) should eliminate the need for backtraces.
The only problem here is without auditing all the source code (or having some linter to do so)
to make sure it does not return bare errors (such as ones from unix and syscall) we can't
be sure that we're good. Alas, I don't know about such a linter.

[cyphar]

Seems like something that errorlint should be updated to have. But in any case, switching to native wrapping is fine by me.

[thaJeztah]

@kolyshkin rebased, and pushed a commit with your suggestion

Let me know if you want to do #3 first / separately, and/or if you want some commits to be squashed.

[kolyshkin]

I would do

return os.IsNotExist(err) || errors.Is(err, syscall.ENOTDIR) || errors.Is(err, syscall.ENOENT)

(mostly because os.IsNotExist could do more than errors.Is(err, os.ErrNotExist).

[kolyshkin]

I take it back; as @thaJeztah pointed out elsewhere, the official docs call for using errors.Is(err, os.ErrNotExist).

[cyphar]

Closing in favour of #7.