Lint common error wrapping issues, update README (microsoft#1969)

* Lint common error wrapping issues, update README

Enable `errorlint` to catch common issues with wrapping and testing for errors.

Wherever possible, switched to using `errors.Is` and `errors.As`.
Exceptions:
- function is defined in the same package and explicitly returns a
  know error variable
- returns from functions in `io`, `binary`, `context`, `syscall`,
  `golang.org/x/sys/windows`, or `golang.org/x/sys/unix` that are
  (relatively) stable in error return value and type
- conversion would interact with with `github.com/pkg/errors`
- conversion would be non-trivial and require additional
  testing/validation
  - specifically, legacy code in `runhcs` and the root of the repo

Rename `context` to `ctx` in `pkg\go-runhcs\*.go` to avoid
overshadowing `context` package.

Update `README.md`:
- run markdown formatter (spaces around code blocks and headers, raw link URLS)
- add section on linter and go generate (similar to go-winio's)

Signed-off-by: Hamza El-Saawy <[email protected]>

* PR: hcserrors(+tests), README

Signed-off-by: Hamza El-Saawy <[email protected]>

---------

Signed-off-by: Hamza El-Saawy <[email protected]>

.github/workflows/ci.yml

@@ -41,7 +41,6 @@ jobs:
             --modules-download-mode=readonly
             --timeout=10m
             --config=${{ github.workspace }}/.golangci.yml
-          skip-cache: true # even if a cache is found, don't use it
           working-directory: ${{ github.workspace }}/${{ matrix.root }}
         env:
           GOOS: ${{ matrix.goos }}

.golangci.yml

@@ -20,6 +20,7 @@ linters:
     # - typecheck
     # - unused
 
+    - errorlint # error wrapping (eg, not using `errors.Is`, using `%s` instead of `%w` in `fmt.Errorf`)
     - gofmt # whether code was gofmt-ed
     - govet # enabled by default, but just to be sure
     - nolintlint # ill-formed or insufficient nolint directives
@@ -53,6 +54,12 @@ issues:
       text: "^ST1003: should not use underscores in package names$"
       source: "^package cri_containerd$"
 
+    # don't bother with propper error wrapping in test code
+    - path: cri-containerd
+      linters:
+        - errorlint
+      text: "non-wrapping format verb for fmt.Errorf"
+
     # This repo has a LOT of generated schema files, operating system bindings, and other
     # things that ST1003 from stylecheck won't like (screaming case Windows api constants for example).
     # There's also some structs that we *could* change the initialisms to be Go friendly

README.md

@@ -9,15 +9,18 @@ It is primarily used in the [Moby](https://github.com/moby/moby) and [Containerd
 ## Building
 
 While this repository can be used as a library of sorts to call the HCS apis, there are a couple binaries built out of the repository as well. The main ones being the Linux guest agent, and an implementation of the [runtime v2 containerd shim api](https://github.com/containerd/containerd/blob/master/runtime/v2/README.md).
+
 ### Linux Hyper-V Container Guest Agent
 
 To build the Linux guest agent itself all that's needed is to set your GOOS to "Linux" and build out of ./cmd/gcs.
+
 ```powershell
 C:\> $env:GOOS="linux"
 C:\> go build .\cmd\gcs\
 ```
 
 or on a Linux machine
+
 ```sh
 > go build ./cmd/gcs
 ```
@@ -33,13 +36,15 @@ make all
 ```
 
 If the build is successful, in the `./out` folder you should see:
+
 ```sh
 > ls ./out/
 delta.tar.gz  initrd.img  rootfs.tar.gz
 ```
 
 ### Containerd Shim
-For info on the Runtime V2 API: https://github.com/containerd/containerd/blob/master/runtime/v2/README.md.
+
+For info on the [Runtime V2 API](https://github.com/containerd/containerd/blob/master/runtime/v2/README.md).
 
 Contrary to the typical Linux architecture of shim -> runc, the runhcs shim is used both to launch and manage the lifetime of containers.
 
@@ -48,14 +53,17 @@ C:\> $env:GOOS="windows"
 C:\> go build .\cmd\containerd-shim-runhcs-v1
 ```
 
-Then place the binary in the same directory that Containerd is located at in your environment. A default Containerd configuration file can be generated by running:
+Then place the binary in the same directory that Containerd is located at in your environment.
+A default Containerd configuration file can be generated by running:
+
 ```powershell
 .\containerd.exe config default | Out-File "C:\Program Files\containerd\config.toml" -Encoding ascii
 ```
 
 This config file will already have the shim set as the default runtime for cri interactions.
 
 To trial using the shim out with ctr.exe:
+
 ```powershell
 C:\> ctr.exe run --runtime io.containerd.runhcs.v1 --rm mcr.microsoft.com/windows/nanoserver:2004 windows-test cmd /c "echo Hello World!"
 ```
@@ -64,16 +72,69 @@ C:\> ctr.exe run --runtime io.containerd.runhcs.v1 --rm mcr.microsoft.com/window
 
 This project welcomes contributions and suggestions. Most contributions require you to agree to a
 Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us
-the rights to use your contribution. For details, visit https://cla.microsoft.com.
+the rights to use your contribution. For details, visit [Microsoft CLA](https://cla.microsoft.com).
 
 When you submit a pull request, a CLA-bot will automatically determine whether you need to provide
 a CLA and decorate the PR appropriately (e.g., label, comment). Simply follow the instructions
 provided by the bot. You will only need to do this once across all repos using our CLA.
 
-We also require that contributors [sign their commits](https://git-scm.com/docs/git-commit) using `git commit -s` or `git commit --signoff` to
-certify they either authored the work themselves or otherwise have permission to use it in this project. Please see https://developercertificate.org/ for
-more info, as well as to make sure that you can attest to the rules listed. Our CI uses the [DCO Github app](https://github.com/apps/dco) to ensure
-that all commits in a given PR are signed-off.
+We require that contributors sign their commits
+to certify they either authored the work themselves or otherwise have permission to use it in this project.
+
+We also require that contributors sign their commits using  using [`git commit --signoff`][git-commit-s]
+to certify they either authored the work themselves or otherwise have permission to use it in this project.
+A range of commits can be signed off using [`git rebase --signoff`][git-rebase-s].
+
+Please see  [the developer certificate](https://developercertificate.org) for more info,
+as well as to make sure that you can attest to the rules listed.
+Our CI uses the [DCO Github app](https://github.com/apps/dco) to ensure that all commits in a given PR are signed-off.
+
+### Linting
+
+Code must pass a linting stage, which uses [`golangci-lint`][lint].
+Since `./test` is a separate Go module, the linter is run from both the root and the
+`test` directories. Additionally, the linter is run with `GOOS` set to both `windows` and
+`linux`.
+
+The linting settings are stored in [`.golangci.yaml`](./.golangci.yaml), and can be run
+automatically with VSCode by adding the following to your workspace or folder settings:
+
+```json
+    "go.lintTool": "golangci-lint",
+    "go.lintOnSave": "package",
+```
+
+Additional editor [integrations options are also available][lint-ide].
+
+Alternatively, `golangci-lint` can be [installed][lint-install] and run locally:
+
+```shell
+# use . or specify a path to only lint a package
+# to show all lint errors, use flags "--max-issues-per-linter=0 --max-same-issues=0"
+> golangci-lint run
+```
+
+To run across the entire repo for both `GOOS=windows` and `linux`:
+
+```powershell
+> foreach ( $goos in ('windows', 'linux') ) {
+    foreach ( $repo in ('.', 'test') ) {
+        pwsh -Command "cd $repo && go env -w GOOS=$goos && golangci-lint.exe run --verbose"
+    }
+}
+```
+
+### Go Generate
+
+The pipeline checks that auto-generated code, via `go generate`, are up to date.
+Similar to the [linting stage](#linting), `go generate` is run in both the root and test Go modules.
+
+This can be done via:
+
+```shell
+> go generate ./...
+> cd test && go generate ./...
+```
 
 ## Code of Conduct
 
@@ -83,7 +144,7 @@ contact [[email protected]](mailto:[email protected]) with any additio
 
 ## Dependencies
 
-This project requires Golang 1.17 or newer to build.
+This project requires Golang 1.18 or newer to build.
 
 For system requirements to run this project, see the Microsoft docs on [Windows Container requirements](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/system-requirements).
 
@@ -100,3 +161,10 @@ For additional details, see [Report a Computer Security Vulnerability](https://t
 
 ---------------
 Copyright (c) 2018 Microsoft Corp.  All rights reserved.
+
+[lint]: https://golangci-lint.run/
+[lint-ide]: https://golangci-lint.run/usage/integrations/#editor-integration
+[lint-install]: https://golangci-lint.run/usage/install/#local-installation
+
+[git-commit-s]: https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--s
+[git-rebase-s]: https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---signoff

cmd/containerd-shim-runhcs-v1/delete.go

@@ -119,7 +119,7 @@ The delete command will be executed in the container's bundle as its cwd.
 		if err := winapi.NetUserDel(
 			"",
 			username,
-		); err != nil && err != winapi.NERR_UserNotFound {
+		); err != nil && !errors.Is(err, winapi.NERR_UserNotFound) {
 			fmt.Fprintf(os.Stderr, "failed to delete user %q: %v", username, err)
 		}
 

cmd/containerd-shim-runhcs-v1/exec_wcow_podsandbox_test.go

@@ -4,13 +4,13 @@ package main
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
 	task "github.com/containerd/containerd/api/runtime/task/v2"
 	containerd_v1_types "github.com/containerd/containerd/api/types/task"
 	"github.com/containerd/containerd/errdefs"
-	"github.com/pkg/errors"
 )
 
 func verifyWcowPodSandboxExecStatus(t *testing.T, wasStarted bool, es containerd_v1_types.Status, status *task.StateResponse) {
@@ -194,7 +194,7 @@ func Test_newWcowPodSandboxExec_Kill_Created(t *testing.T) {
 
 	// Call Kill again
 	err = wpse.Kill(context.TODO(), 0x0)
-	if errors.Cause(err) != errdefs.ErrNotFound {
+	if !errors.Is(err, errdefs.ErrNotFound) {
 		t.Fatalf("Kill should fail with `ErrNotFound` in the exited state got: %v", err)
 	}
 }
@@ -219,7 +219,7 @@ func Test_newWcowPodSandboxExec_Kill_Started(t *testing.T) {
 
 	// Call Kill again
 	err = wpse.Kill(context.TODO(), 0x0)
-	if errors.Cause(err) != errdefs.ErrNotFound {
+	if !errors.Is(err, errdefs.ErrNotFound) {
 		t.Fatalf("Kill should fail with `ErrNotFound` in the exited state got: %v", err)
 	}
 }

cmd/containerd-shim-runhcs-v1/rootfs.go

@@ -63,6 +63,8 @@ func parseLegacyRootfsMount(m *types.Mount) (string, []string, error) {
 		if strings.HasPrefix(option, mount.ParentLayerPathsFlag) {
 			err := json.Unmarshal([]byte(option[len(mount.ParentLayerPathsFlag):]), &parentLayerPaths)
 			if err != nil {
+				// TODO (go1.20): use multierror via fmt.Errorf("...: %w; ...: %w", ...)
+				//nolint:errorlint // non-wrapping format verb for fmt.Errorf
 				return "", nil, fmt.Errorf("unmarshal parent layer paths from mount: %v: %w", err, errdefs.ErrFailedPrecondition)
 			}
 			// Would perhaps be worthwhile to check for unrecognized options and return an error,

cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go

@@ -4,6 +4,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"strconv"
@@ -628,7 +629,7 @@ func Test_PodShim_updateInternal_Error(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected to get an error for incorrect resource's type")
 	}
-	if err != errNotSupportedResourcesRequest {
+	if !errors.Is(err, errNotSupportedResourcesRequest) {
 		t.Fatalf("expected to get errNotSupportedResourcesRequest, instead got %v", err)
 	}
 }

cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go

@@ -4,6 +4,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"os"
@@ -615,7 +616,7 @@ func Test_TaskShim_updateInternal_Error(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected to get an error for incorrect resource's type")
 	}
-	if err != errNotSupportedResourcesRequest {
+	if !errors.Is(err, errNotSupportedResourcesRequest) {
 		t.Fatalf("expected to get errNotSupportedResourcesRequest, instead got %v", err)
 	}
 }

cmd/containerd-shim-runhcs-v1/service_internal_test.go

@@ -17,7 +17,7 @@ import (
 
 func verifyExpectedError(t *testing.T, resp interface{}, actual, expected error) {
 	t.Helper()
-	if actual == nil || errors.Cause(actual) != expected || !errors.Is(actual, expected) {
+	if actual == nil || errors.Cause(actual) != expected || !errors.Is(actual, expected) { //nolint:errorlint
 		t.Fatalf("expected error: %v, got: %v", expected, actual)
 	}
 

cmd/gcstools/generichook.go

@@ -43,7 +43,7 @@ func runGenericHook() error {
 
 	out, err := hookCmd.CombinedOutput()
 	if err != nil {
-		return fmt.Errorf("failed to run nvidia cli tool with: %v, %v", string(out), err)
+		return fmt.Errorf("failed to run nvidia cli tool with: %v, %w", string(out), err)
 	}
 
 	return nil

cmd/ncproxy/hcn.go

@@ -162,7 +162,7 @@ func createHCNNetwork(ctx context.Context, req *ncproxygrpc.HostComputeNetworkSe
 		// searching for here. If the network exists, the vSwitch with this name exists.
 		extSwitch, err := hcn.GetNetworkByName(req.SwitchName)
 		if err != nil {
-			if _, ok := err.(hcn.NetworkNotFoundError); ok {
+			if _, ok := err.(hcn.NetworkNotFoundError); ok { //nolint:errorlint
 				return nil, errors.Errorf("no network/switch with name `%s` found", req.SwitchName)
 			}
 			return nil, errors.Wrapf(err, "failed to get network/switch with name %q", req.SwitchName)
@@ -203,7 +203,7 @@ func createHCNNetwork(ctx context.Context, req *ncproxygrpc.HostComputeNetworkSe
 	if len(req.SubnetIpaddressPrefixIpv6) != 0 {
 		if err := hcn.IPv6DualStackSupported(); err != nil {
 			// a request was made for an IPv6 address on a system that doesn't support IPv6
-			return nil, fmt.Errorf("IPv6 address requested but not supported: %v", err)
+			return nil, fmt.Errorf("IPv6 address requested but not supported: %w", err)
 		}
 	}
 
@@ -341,7 +341,7 @@ func createHCNEndpoint(ctx context.Context, network *hcn.HostComputeNetwork, req
 	if req.Ipv6Address != "" && req.Ipv6AddressPrefixlength != 0 {
 		if err := hcn.IPv6DualStackSupported(); err != nil {
 			// a request was made for an IPv6 address on a system that doesn't support IPv6
-			return nil, fmt.Errorf("IPv6 address requested but not supported: %v", err)
+			return nil, fmt.Errorf("IPv6 address requested but not supported: %w", err)
 		}
 		ipv6Config := hcn.IpConfig{
 			IpAddress:    req.Ipv6Address,

cmd/ncproxy/ncproxy.go

@@ -110,7 +110,7 @@ func (s *grpcService) AddNIC(ctx context.Context, req *ncproxygrpc.AddNICRequest
 		}
 		ep, err := hcn.GetEndpointByName(req.EndpointName)
 		if err != nil {
-			if _, ok := err.(hcn.EndpointNotFoundError); ok {
+			if _, ok := err.(hcn.EndpointNotFoundError); ok { //nolint:errorlint
 				return nil, status.Errorf(codes.NotFound, "no endpoint with name `%s` found", req.EndpointName)
 			}
 			return nil, errors.Wrapf(err, "failed to get endpoint with name `%s`", req.EndpointName)
@@ -180,7 +180,7 @@ func (s *grpcService) ModifyNIC(ctx context.Context, req *ncproxygrpc.ModifyNICR
 
 	ep, err := hcn.GetEndpointByName(req.EndpointName)
 	if err != nil {
-		if _, ok := err.(hcn.EndpointNotFoundError); ok {
+		if _, ok := err.(hcn.EndpointNotFoundError); ok { //nolint:errorlint
 			return nil, status.Errorf(codes.NotFound, "no endpoint with name `%s` found", req.EndpointName)
 		}
 		return nil, errors.Wrapf(err, "failed to get endpoint with name `%s`", req.EndpointName)
@@ -281,7 +281,7 @@ func (s *grpcService) DeleteNIC(ctx context.Context, req *ncproxygrpc.DeleteNICR
 		}
 		ep, err := hcn.GetEndpointByName(req.EndpointName)
 		if err != nil {
-			if _, ok := err.(hcn.EndpointNotFoundError); ok {
+			if _, ok := err.(hcn.EndpointNotFoundError); ok { //nolint:errorlint
 				return nil, status.Errorf(codes.NotFound, "no endpoint with name `%s` found", req.EndpointName)
 			}
 			return nil, errors.Wrapf(err, "failed to get endpoint with name `%s`", req.EndpointName)
@@ -299,7 +299,7 @@ func (s *grpcService) DeleteNIC(ctx context.Context, req *ncproxygrpc.DeleteNICR
 			Endpoint:    protobuf.FromAny(anyEndpoint),
 		}
 		if _, err := agent.DeleteNIC(ctx, caReq); err != nil {
-			if err == uvm.ErrNICNotFound || err == uvm.ErrNetNSNotFound {
+			if errors.Is(err, uvm.ErrNICNotFound) || errors.Is(err, uvm.ErrNetNSNotFound) {
 				return nil, status.Errorf(codes.NotFound, "failed to remove endpoint %q from namespace %q", req.EndpointName, req.NicID)
 			}
 			return nil, err
@@ -384,7 +384,7 @@ func (s *grpcService) CreateEndpoint(ctx context.Context, req *ncproxygrpc.Creat
 
 		network, err := hcn.GetNetworkByName(reqEndpoint.NetworkName)
 		if err != nil {
-			if _, ok := err.(hcn.NetworkNotFoundError); ok {
+			if _, ok := err.(hcn.NetworkNotFoundError); ok { //nolint:errorlint
 				return nil, status.Errorf(codes.NotFound, "no network with name `%s` found", reqEndpoint.NetworkName)
 			}
 			return nil, errors.Wrapf(err, "failed to get network with name %q", reqEndpoint.NetworkName)
@@ -461,7 +461,7 @@ func (s *grpcService) AddEndpoint(ctx context.Context, req *ncproxygrpc.AddEndpo
 		}
 		ep, err := hcn.GetEndpointByName(req.Name)
 		if err != nil {
-			if _, ok := err.(hcn.EndpointNotFoundError); ok {
+			if _, ok := err.(hcn.EndpointNotFoundError); ok { //nolint:errorlint
 				return nil, status.Errorf(codes.NotFound, "no endpoint with name `%s` found", req.Name)
 			}
 			return nil, errors.Wrapf(err, "failed to get endpoint with name `%s`", req.Name)
@@ -513,7 +513,7 @@ func (s *grpcService) DeleteEndpoint(ctx context.Context, req *ncproxygrpc.Delet
 		}
 		ep, err := hcn.GetEndpointByName(req.Name)
 		if err != nil {
-			if _, ok := err.(hcn.EndpointNotFoundError); ok {
+			if _, ok := err.(hcn.EndpointNotFoundError); ok { //nolint:errorlint
 				return nil, status.Errorf(codes.NotFound, "no endpoint with name `%s` found", req.Name)
 			}
 			return nil, errors.Wrapf(err, "failed to get endpoint with name %q", req.Name)
@@ -548,7 +548,7 @@ func (s *grpcService) DeleteNetwork(ctx context.Context, req *ncproxygrpc.Delete
 		}
 		network, err := hcn.GetNetworkByName(req.Name)
 		if err != nil {
-			if _, ok := err.(hcn.NetworkNotFoundError); ok {
+			if _, ok := err.(hcn.NetworkNotFoundError); ok { //nolint:errorlint
 				return nil, status.Errorf(codes.NotFound, "no network with name `%s` found", req.Name)
 			}
 			return nil, errors.Wrapf(err, "failed to get network with name %q", req.Name)
@@ -615,7 +615,7 @@ func (s *grpcService) GetEndpoint(ctx context.Context, req *ncproxygrpc.GetEndpo
 
 	ep, err := hcn.GetEndpointByName(req.Name)
 	if err != nil {
-		if _, ok := err.(hcn.EndpointNotFoundError); ok {
+		if _, ok := err.(hcn.EndpointNotFoundError); ok { //nolint:errorlint
 			return nil, status.Errorf(codes.NotFound, "no endpoint with name `%s` found", req.Name)
 		}
 		return nil, errors.Wrapf(err, "failed to get endpoint with name %q", req.Name)
@@ -694,7 +694,7 @@ func (s *grpcService) GetNetwork(ctx context.Context, req *ncproxygrpc.GetNetwor
 
 	network, err := hcn.GetNetworkByName(req.Name)
 	if err != nil {
-		if _, ok := err.(hcn.NetworkNotFoundError); ok {
+		if _, ok := err.(hcn.NetworkNotFoundError); ok { //nolint:errorlint
 			return nil, status.Errorf(codes.NotFound, "no network with name `%s` found", req.Name)
 		}
 		return nil, errors.Wrapf(err, "failed to get network with name %q", req.Name)