hack/ci,*: check all returned errors (#912)

AlexNPavel · web-flow · commit 3b86d1a9ad4e · 2019-01-17T14:11:05.000-08:00
**Description of the change:** Check all returned errors in functions used by the `operator-sdk`. **Motivation for the change:** We should make sure that all errors are checked in general. Some of these issues don't _really_ affect the code much (specifically the deferred `file.Close()` calls), but some of these could cause difficult to diagnose problems. To find all unchecked errors, the [errcheck](https://github.com/kisielk/errcheck) program can be used. There is a PR to add that to our travis CI, but currently it segfaults when run under travis, so that will need more work.
diff --git a/commands/operator-sdk/cmd/add/api.go b/commands/operator-sdk/cmd/add/api.go
@@ -54,9 +54,13 @@ Example:
 	}
 
 	apiCmd.Flags().StringVar(&apiVersion, "api-version", "", "Kubernetes APIVersion that has a format of $GROUP_NAME/$VERSION (e.g app.example.com/v1alpha1)")
-	apiCmd.MarkFlagRequired("api-version")
+	if err := apiCmd.MarkFlagRequired("api-version"); err != nil {
+		log.Fatalf("Failed to mark `api-version` flag for `add api` subcommand as required")
+	}
 	apiCmd.Flags().StringVar(&kind, "kind", "", "Kubernetes resource Kind name. (e.g AppService)")
-	apiCmd.MarkFlagRequired("kind")
+	if err := apiCmd.MarkFlagRequired("kind"); err != nil {
+		log.Fatalf("Failed to mark `kind` flag for `add api` subcommand as required")
+	}
 
 	return apiCmd
 }
diff --git a/commands/operator-sdk/cmd/add/controller.go b/commands/operator-sdk/cmd/add/controller.go
@@ -24,7 +24,7 @@ import (
 )
 
 func NewControllerCmd() *cobra.Command {
-	apiCmd := &cobra.Command{
+	controllerCmd := &cobra.Command{
 		Use:   "controller",
 		Short: "Adds a new controller pkg",
 		Long: `operator-sdk add controller --kind=<kind> --api-version=<group/version> creates a new
@@ -47,12 +47,16 @@ Example:
 		Run: controllerRun,
 	}
 
-	apiCmd.Flags().StringVar(&apiVersion, "api-version", "", "Kubernetes APIVersion that has a format of $GROUP_NAME/$VERSION (e.g app.example.com/v1alpha1)")
-	apiCmd.MarkFlagRequired("api-version")
-	apiCmd.Flags().StringVar(&kind, "kind", "", "Kubernetes resource Kind name. (e.g AppService)")
-	apiCmd.MarkFlagRequired("kind")
+	controllerCmd.Flags().StringVar(&apiVersion, "api-version", "", "Kubernetes APIVersion that has a format of $GROUP_NAME/$VERSION (e.g app.example.com/v1alpha1)")
+	if err := controllerCmd.MarkFlagRequired("api-version"); err != nil {
+		log.Fatalf("Failed to mark `api-version` flag for `add controller` subcommand as required")
+	}
+	controllerCmd.Flags().StringVar(&kind, "kind", "", "Kubernetes resource Kind name. (e.g AppService)")
+	if err := controllerCmd.MarkFlagRequired("kind"); err != nil {
+		log.Fatalf("Failed to mark `kind` flag for `add controller` subcommand as required")
+	}
 
-	return apiCmd
+	return controllerCmd
 }
 
 func controllerRun(cmd *cobra.Command, args []string) {
diff --git a/commands/operator-sdk/cmd/add/crd.go b/commands/operator-sdk/cmd/add/crd.go
@@ -43,9 +43,13 @@ Generated CR  filename: <project-name>/deploy/crds/<group>_<version>_<kind>_cr.y
 		Run: crdFunc,
 	}
 	crdCmd.Flags().StringVar(&apiVersion, "api-version", "", "Kubernetes apiVersion and has a format of $GROUP_NAME/$VERSION (e.g app.example.com/v1alpha1)")
-	crdCmd.MarkFlagRequired("api-version")
+	if err := crdCmd.MarkFlagRequired("api-version"); err != nil {
+		log.Fatalf("Failed to mark `api-version` flag for `add crd` subcommand as required")
+	}
 	crdCmd.Flags().StringVar(&kind, "kind", "", "Kubernetes CustomResourceDefintion kind. (e.g AppService)")
-	crdCmd.MarkFlagRequired("kind")
+	if err := crdCmd.MarkFlagRequired("kind"); err != nil {
+		log.Fatalf("Failed to mark `kind` flag for `add crd` subcommand as required")
+	}
 	return crdCmd
 }
 
diff --git a/commands/operator-sdk/cmd/test/cluster.go b/commands/operator-sdk/cmd/test/cluster.go
@@ -20,6 +20,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
 	k8sInternal "github.com/operator-framework/operator-sdk/internal/util/k8sutil"
 	"github.com/operator-framework/operator-sdk/internal/util/projutil"
 	"github.com/operator-framework/operator-sdk/pkg/scaffold"
@@ -168,7 +169,11 @@ func testClusterFunc(cmd *cobra.Command, args []string) error {
 			if err != nil {
 				return fmt.Errorf("test failed and failed to get error logs")
 			}
-			defer readCloser.Close()
+			defer func() {
+				if err := readCloser.Close(); err != nil && !fileutil.IsClosedError(err) {
+					log.Errorf("Failed to close pod log reader: (%v)", err)
+				}
+			}()
 			buf := new(bytes.Buffer)
 			_, err = buf.ReadFrom(readCloser)
 			if err != nil {
diff --git a/commands/operator-sdk/cmd/test/local.go b/commands/operator-sdk/cmd/test/local.go
@@ -240,7 +240,9 @@ func replaceImage(manifestPath, image string) error {
 		foundDeployment = true
 		scheme := runtime.NewScheme()
 		// scheme for client go
-		cgoscheme.AddToScheme(scheme)
+		if err := cgoscheme.AddToScheme(scheme); err != nil {
+			log.Fatalf("Failed to add client-go scheme to runtime client: (%v)", err)
+		}
 		dynamicDecoder := serializer.NewCodecFactory(scheme).UniversalDeserializer()
 
 		obj, _, err := dynamicDecoder.Decode(yamlSpec, nil, nil)
diff --git a/internal/util/yamlutil/manifest.go b/internal/util/yamlutil/manifest.go
@@ -58,7 +58,11 @@ func GenerateCombinedNamespacedManifest() (*os.File, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil && !fileutil.IsClosedError(err) {
+			log.Errorf("Failed to close file %s: (%v)", file.Name(), err)
+		}
+	}()
 
 	sa, err := ioutil.ReadFile(filepath.Join(scaffold.DeployDir, scaffold.ServiceAccountYamlFile))
 	if err != nil {
@@ -98,7 +102,11 @@ func GenerateCombinedGlobalManifest() (*os.File, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil && !fileutil.IsClosedError(err) {
+			log.Errorf("Failed to close file %s: (%v)", file.Name(), err)
+		}
+	}()
 
 	files, err := ioutil.ReadDir(scaffold.CrdsDir)
 	if err != nil {
diff --git a/pkg/ansible/controller/reconcile.go b/pkg/ansible/controller/reconcile.go
@@ -133,7 +133,11 @@ func (r *AnsibleOperatorReconciler) Reconcile(request reconcile.Request) (reconc
 	if err != nil {
 		return reconcileResult, err
 	}
-	defer os.Remove(kc.Name())
+	defer func() {
+		if err := os.Remove(kc.Name()); err != nil {
+			logger.Error(err, "Failed to remove generated kubeconfig file")
+		}
+	}()
 	result, err := r.Runner.Run(ident, u, kc.Name())
 	if err != nil {
 		return reconcileResult, err
diff --git a/pkg/ansible/controller/reconcile_test.go b/pkg/ansible/controller/reconcile_test.go
@@ -501,10 +501,13 @@ func TestReconcile(t *testing.T) {
 			if tc.ExpectedObject != nil {
 				actualObject := &unstructured.Unstructured{}
 				actualObject.SetGroupVersionKind(tc.ExpectedObject.GroupVersionKind())
-				tc.Client.Get(context.TODO(), types.NamespacedName{
+				err := tc.Client.Get(context.TODO(), types.NamespacedName{
 					Name:      tc.ExpectedObject.GetName(),
 					Namespace: tc.ExpectedObject.GetNamespace(),
 				}, actualObject)
+				if err != nil {
+					t.Fatalf("Failed to get object: (%v)", err)
+				}
 				if !reflect.DeepEqual(actualObject.GetAnnotations(), tc.ExpectedObject.GetAnnotations()) {
 					t.Fatalf("Annotations are not the same\nexpected: %v\nactual: %v", tc.ExpectedObject.GetAnnotations(), actualObject.GetAnnotations())
 				}
diff --git a/pkg/ansible/controller/status/types.go b/pkg/ansible/controller/status/types.go
@@ -78,7 +78,9 @@ func NewAnsibleResultFromMap(sm map[string]interface{}) *AnsibleResult {
 	}
 	if v, ok := sm["completion"]; ok {
 		s := v.(string)
-		a.TimeOfCompletion.UnmarshalJSON([]byte(s))
+		if err := a.TimeOfCompletion.UnmarshalJSON([]byte(s)); err != nil {
+			log.Error(err, "Failed to unmarshal time of completion for ansible result")
+		}
 	}
 	return a
 }
@@ -189,6 +191,8 @@ func (status *Status) GetJSONMap() map[string]interface{} {
 		log.Error(err, "Unable to marshal json")
 		return status.CustomStatus
 	}
-	json.Unmarshal(b, &status.CustomStatus)
+	if err := json.Unmarshal(b, &status.CustomStatus); err != nil {
+		log.Error(err, "Unable to unmarshal json")
+	}
 	return status.CustomStatus
 }
diff --git a/pkg/ansible/proxy/kubeconfig/kubeconfig.go b/pkg/ansible/proxy/kubeconfig/kubeconfig.go
@@ -23,9 +23,14 @@ import (
 	"net/url"
 	"os"
 
+	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
 
+var log = logf.Log.WithName("kubeconfig")
+
 // kubectl, as of 1.10.5, only does basic auth if the username is present in
 // the URL. The python client used by ansible, as of 6.0.0, only does basic
 // auth if the username and password are provided under the "user" key within
@@ -80,7 +85,9 @@ func Create(ownerRef metav1.OwnerReference, proxyURL string, namespace string) (
 	var parsed bytes.Buffer
 
 	t := template.Must(template.New("kubeconfig").Parse(kubeConfigTemplate))
-	t.Execute(&parsed, v)
+	if err := t.Execute(&parsed, v); err != nil {
+		return nil, err
+	}
 
 	file, err := ioutil.TempFile("", "kubeconfig")
 	if err != nil {
@@ -89,7 +96,11 @@ func Create(ownerRef metav1.OwnerReference, proxyURL string, namespace string) (
 	// multiple calls to close file will not hurt anything,
 	// but we don't want to lose the error because we are
 	// writing to the file, so we will call close twice.
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil && !fileutil.IsClosedError(err) {
+			log.Error(err, "Failed to close generated kubeconfig file")
+		}
+	}()
 
 	if _, err := file.WriteString(parsed.String()); err != nil {
 		return nil, err
diff --git a/pkg/ansible/proxy/kubectl.go b/pkg/ansible/proxy/kubectl.go
@@ -148,7 +148,9 @@ func (f *FilterServer) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 	}
 	log.Info("Filter rejection", "Request.Method", req.Method, "Request.URL", req.URL.Path, "Host", host)
 	rw.WriteHeader(http.StatusForbidden)
-	rw.Write([]byte("<h3>Unauthorized</h3>"))
+	if _, err := rw.Write([]byte("<h3>Unauthorized</h3>")); err != nil {
+		log.Error(err, "Failed to write response body")
+	}
 }
 
 // Server is a http.Handler which proxies Kubernetes APIs to remote API server.
@@ -233,7 +235,9 @@ func (s *server) ListenUnix(path string) (net.Listener, error) {
 	// Remove any socket, stale or not, but fall through for other files
 	fi, err := os.Stat(path)
 	if err == nil && (fi.Mode()&os.ModeSocket) != 0 {
-		os.Remove(path)
+		if err := os.Remove(path); err != nil {
+			return nil, err
+		}
 	}
 	// Default to only user accessible socket, caller can open up later if desired
 	oldmask := syscall.Umask(0077)
diff --git a/pkg/ansible/proxy/proxy.go b/pkg/ansible/proxy/proxy.go
@@ -169,7 +169,9 @@ func CacheResponseHandler(h http.Handler, informerCache cache.Cache, restMapper
 			w.Header().Set("Content-Type", "application/json")
 			// Set X-Cache header to signal that response is served from Cache
 			w.Header().Set("X-Cache", "HIT")
-			json.Indent(&i, resp, "", "  ")
+			if err := json.Indent(&i, resp, "", "  "); err != nil {
+				log.Error(err, "Failed to indent json")
+			}
 			_, err = w.Write(i.Bytes())
 			if err != nil {
 				log.Error(err, "Failed to write response")
@@ -222,7 +224,12 @@ func InjectOwnerReferenceHandler(h http.Handler, cMap *ControllerMap, restMapper
 				return
 			}
 			owner := metav1.OwnerReference{}
-			json.Unmarshal(authString, &owner)
+			if err := json.Unmarshal(authString, &owner); err != nil {
+				m := "Could not unmarshal auth string"
+				log.Error(err, m)
+				http.Error(w, m, http.StatusInternalServerError)
+				return
+			}
 			log.Info(fmt.Sprintf("Owner: %#v", owner))
 
 			body, err := ioutil.ReadAll(req.Body)
@@ -359,7 +366,9 @@ func Run(done chan error, o Options) error {
 		}
 		stop := make(chan struct{})
 		go func() {
-			informerCache.Start(stop)
+			if err := informerCache.Start(stop); err != nil {
+				log.Error(err, "Failed to start informer cache")
+			}
 			defer close(stop)
 		}()
 		log.Info("Waiting for cache to sync...")
diff --git a/pkg/ansible/proxy/proxy_test.go b/pkg/ansible/proxy/proxy_test.go
@@ -21,6 +21,8 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
+
 	kcorev1 "k8s.io/api/core/v1"
 	kmetav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -56,7 +58,11 @@ func TestHandler(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error getting pod from proxy: %v", err)
 	}
-	defer resp.Body.Close()
+	defer func() {
+		if err := resp.Body.Close(); err != nil && !fileutil.IsClosedError(err) {
+			t.Errorf("Failed to close response body: (%v)", err)
+		}
+	}()
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		t.Fatalf("Error reading response body: %v", err)
diff --git a/pkg/ansible/runner/eventapi/eventapi.go b/pkg/ansible/runner/eventapi/eventapi.go
@@ -25,6 +25,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
+
 	"github.com/go-logr/logr"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
@@ -95,7 +97,9 @@ func (e *EventReceiver) Close() {
 	e.stopped = true
 	e.mutex.Unlock()
 	e.logger.V(1).Info("Event API stopped")
-	e.server.Close()
+	if err := e.server.Close(); err != nil && !fileutil.IsClosedError(err) {
+		e.logger.Error(err, "Failed to close event receiver")
+	}
 	close(e.Events)
 }
 
@@ -116,7 +120,9 @@ func (e *EventReceiver) handleEvents(w http.ResponseWriter, r *http.Request) {
 	if strings.Split(ct, ";")[0] != "application/json" {
 		e.logger.Info("Wrong content type", "code", "415", "Request.Content-Type", ct)
 		w.WriteHeader(http.StatusUnsupportedMediaType)
-		w.Write([]byte("The content-type must be \"application/json\""))
+		if _, err := w.Write([]byte("The content-type must be \"application/json\"")); err != nil {
+			e.logger.Error(err, "Failed to write response body")
+		}
 		return
 	}
 
@@ -132,7 +138,9 @@ func (e *EventReceiver) handleEvents(w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		e.logger.Info("Could not deserialize body.", "code", "400", "Error", err)
 		w.WriteHeader(http.StatusBadRequest)
-		w.Write([]byte("Could not deserialize body as JSON"))
+		if _, err := w.Write([]byte("Could not deserialize body as JSON")); err != nil {
+			e.logger.Error(err, "Failed to write response body")
+		}
 		return
 	}
 
diff --git a/pkg/ansible/runner/internal/inputdir/inputdir.go b/pkg/ansible/runner/internal/inputdir/inputdir.go
@@ -21,6 +21,8 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
+
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 )
 
@@ -117,7 +119,11 @@ func (i *InputDir) Write() error {
 			log.Error(err, "Failed to open playbook file", "Path", i.PlaybookPath)
 			return err
 		}
-		defer f.Close()
+		defer func() {
+			if err := f.Close(); err != nil && !fileutil.IsClosedError(err) {
+				log.Error(err, "Failed to close playbook file")
+			}
+		}()
 
 		playbookBytes, err := ioutil.ReadAll(f)
 		if err != nil {
diff --git a/pkg/helm/internal/types/types.go b/pkg/helm/internal/types/types.go
@@ -81,7 +81,9 @@ func (s *HelmAppStatus) ToMap() (map[string]interface{}, error) {
 	if err != nil {
 		return nil, err
 	}
-	json.Unmarshal(jsonObj, &out)
+	if err := json.Unmarshal(jsonObj, &out); err != nil {
+		return nil, err
+	}
 	return out, nil
 }
 
diff --git a/pkg/test/framework.go b/pkg/test/framework.go
@@ -74,10 +74,14 @@ func setup(kubeconfigPath, namespacedManPath *string, localOperator bool) error
 			if err != nil {
 				return fmt.Errorf("failed to get service host: %v", err)
 			}
-			os.Setenv("KUBERNETES_SERVICE_HOST", addrs[0])
+			if err := os.Setenv("KUBERNETES_SERVICE_HOST", addrs[0]); err != nil {
+				return fmt.Errorf("failed to set kubernetes host env var: (%v)", err)
+			}
 		}
 		if len(os.Getenv("KUBERNETES_SERVICE_PORT")) == 0 {
-			os.Setenv("KUBERNETES_SERVICE_PORT", "443")
+			if err := os.Setenv("KUBERNETES_SERVICE_PORT", "443"); err != nil {
+				return fmt.Errorf("failed to set kubernetes port env var: (%v)", err)
+			}
 		}
 		kubeconfig, err = rest.InClusterConfig()
 		*singleNamespace = true
@@ -100,8 +104,12 @@ func setup(kubeconfigPath, namespacedManPath *string, localOperator bool) error
 		return fmt.Errorf("failed to build the kubeclient: %v", err)
 	}
 	scheme := runtime.NewScheme()
-	cgoscheme.AddToScheme(scheme)
-	extscheme.AddToScheme(scheme)
+	if err := cgoscheme.AddToScheme(scheme); err != nil {
+		return fmt.Errorf("failed to add cgo scheme to runtime scheme: (%v)", err)
+	}
+	if err := extscheme.AddToScheme(scheme); err != nil {
+		return fmt.Errorf("failed to add api extensions scheme to runtime scheme: (%v)", err)
+	}
 	cachedDiscoveryClient := cached.NewMemCacheClient(kubeclient.Discovery())
 	restMapper = restmapper.NewDeferredDiscoveryRESTMapper(cachedDiscoveryClient)
 	restMapper.Reset()
diff --git a/pkg/test/resource_creator.go b/pkg/test/resource_creator.go
diff --git a/test/e2e/memcached_test.go b/test/e2e/memcached_test.go
diff --git a/test/test-framework/cmd/manager/main.go b/test/test-framework/cmd/manager/main.go
diff --git a/test/test-framework/pkg/controller/memcached/memcached_controller.go b/test/test-framework/pkg/controller/memcached/memcached_controller.go

Original file line number	Diff line number	Diff line change
`@@ -54,9 +54,13 @@ Example:`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	`apiCmd.Flags().StringVar(&apiVersion, "api-version", "", "Kubernetes APIVersion that has a format of $GROUP_NAME/$VERSION (e.g app.example.com/v1alpha1)")`
`57`		`- apiCmd.MarkFlagRequired("api-version")`
	`57`	`+ if err := apiCmd.MarkFlagRequired("api-version"); err != nil {`
	`58`	+ log.Fatalf("Failed to mark `api-version` flag for `add api` subcommand as required")
	`59`	`+ }`
`58`	`60`	`apiCmd.Flags().StringVar(&kind, "kind", "", "Kubernetes resource Kind name. (e.g AppService)")`
`59`		`- apiCmd.MarkFlagRequired("kind")`
	`61`	`+ if err := apiCmd.MarkFlagRequired("kind"); err != nil {`
	`62`	+ log.Fatalf("Failed to mark `kind` flag for `add api` subcommand as required")
	`63`	`+ }`
`60`	`64`
`61`	`65`	`return apiCmd`
`62`	`66`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,9 @@ func NewAnsibleResultFromMap(sm map[string]interface{}) *AnsibleResult {`
`78`	`78`	`}`
`79`	`79`	`if v, ok := sm["completion"]; ok {`
`80`	`80`	`s := v.(string)`
`81`		`- a.TimeOfCompletion.UnmarshalJSON([]byte(s))`
	`81`	`+ if err := a.TimeOfCompletion.UnmarshalJSON([]byte(s)); err != nil {`
	`82`	`+ log.Error(err, "Failed to unmarshal time of completion for ansible result")`
	`83`	`+ }`
`82`	`84`	`}`
`83`	`85`	`return a`
`84`	`86`	`}`
`@@ -189,6 +191,8 @@ func (status *Status) GetJSONMap() map[string]interface{} {`
`189`	`191`	`log.Error(err, "Unable to marshal json")`
`190`	`192`	`return status.CustomStatus`
`191`	`193`	`}`
`192`		`- json.Unmarshal(b, &status.CustomStatus)`
	`194`	`+ if err := json.Unmarshal(b, &status.CustomStatus); err != nil {`
	`195`	`+ log.Error(err, "Unable to unmarshal json")`
	`196`	`+ }`
`193`	`197`	`return status.CustomStatus`
`194`	`198`	`}`