Repository: knative/pkg
Pull request #1839 (2020-10-23 17:47:25)

Fix the error/warn log usage
zap doesn't do this well, when errors are not presented without zap.Error

/assign @dprotaso mattmoor

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1839#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/webhook/certificates/certificates.go b/webhook/certificates/certificates.go
index 03b393c05..209403331 100644
--- a/webhook/certificates/certificates.go
+++ b/webhook/certificates/certificates.go
@@ -22,6 +22,7 @@ import (
 	"crypto/x509"
 	"time"
 
+	"go.uber.org/zap"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -83,11 +84,11 @@ func (r *reconciler) reconcileCertificate(ctx context.Context) error {
 		// Check the expiration date of the certificate to see if it needs to be updated
 		cert, err := tls.X509KeyPair(secret.Data[certresources.ServerCert], secret.Data[certresources.ServerKey])
 		if err != nil {
-			logger.Warn("Error creating pem from certificate and key: ", err)
+			logger.Warnw("Error creating pem from certificate and key", zap.Error(err))
 		} else {
 			certData, err := x509.ParseCertificate(cert.Certificate[0])
 			if err != nil {
-				logger.Error("Error parsing certificate: ", err)
+				logger.Errorw("Error parsing certificate", zap.Error(err))
 			} else if time.Now().Add(oneWeek).Before(certData.NotAfter) {
 				return nil
 			}
diff --git a/webhook/configmaps/configmaps.go b/webhook/configmaps/configmaps.go
index 5599148e1..e7f62de90 100644
--- a/webhook/configmaps/configmaps.go
+++ b/webhook/configmaps/configmaps.go
@@ -24,6 +24,7 @@ import (
 	"fmt"
 	"reflect"
 
+	"go.uber.org/zap"
 	admissionv1 "k8s.io/api/admission/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -77,7 +78,7 @@ func (ac *reconciler) Reconcile(ctx context.Context, key string) error {
 
 	secret, err := ac.secretlister.Secrets(system.Namespace()).Get(ac.secretName)
 	if err != nil {
-		logger.Error("Error fetching secret: ", err)
+		logger.Errorw("Error fetching secret ", zap.Error(err))
 		return err
 	}
 
diff --git a/webhook/psbinding/psbinding.go b/webhook/psbinding/psbinding.go
index dc21b8b66..741859058 100644
--- a/webhook/psbinding/psbinding.go
+++ b/webhook/psbinding/psbinding.go
@@ -26,6 +26,7 @@ import (
 	"sync"
 
 	"github.com/markbates/inflect"
+	"go.uber.org/zap"
 	admissionv1 "k8s.io/api/admission/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
@@ -164,7 +165,7 @@ func (ac *Reconciler) Reconcile(ctx context.Context, key string) error {
 	// Look up the webhook secret, and fetch the CA cert bundle.
 	secret, err := ac.SecretLister.Secrets(system.Namespace()).Get(ac.SecretName)
 	if err != nil {
-		logging.FromContext(ctx).Error("Error fetching secret: ", err)
+		logging.FromContext(ctx).Errorw("Error fetching secret", zap.Error(err))
 		return err
 	}
 	caCert, ok := secret.Data[certresources.CACert]
diff --git a/webhook/psbinding/reconciler.go b/webhook/psbinding/reconciler.go
index 10f42dac9..5d05fe6ac 100644
--- a/webhook/psbinding/reconciler.go
+++ b/webhook/psbinding/reconciler.go
@@ -435,19 +435,19 @@ func (r *BaseReconciler) ReconcileSubject(ctx context.Context, fb Bindable, muta
 func (r *BaseReconciler) UpdateStatus(ctx context.Context, desired Bindable) error {
 	actual, err := r.Get(desired.GetNamespace(), desired.GetName())
 	if err != nil {
-		logging.FromContext(ctx).Error("Error fetching actual: ", err)
+		logging.FromContext(ctx).Errorw("Error fetching actual", zap.Error(err))
 		return err
 	}
 
 	// Convert to unstructured for use with the dynamic client.
 	ua, err := duck.ToUnstructured(actual)
 	if err != nil {
-		logging.FromContext(ctx).Error("Error converting actual: ", err)
+		logging.FromContext(ctx).Errorw("Error converting actual", zap.Error(err))
 		return err
 	}
 	ud, err := duck.ToUnstructured(desired)
 	if err != nil {
-		logging.FromContext(ctx).Error("Error converting desired: ", err)
+		logging.FromContext(ctx).Errorw("Error converting desired", zap.Error(err))
 		return err
 	}
 
diff --git a/webhook/resourcesemantics/conversion/conversion.go b/webhook/resourcesemantics/conversion/conversion.go
index 19e9d291e..d8dcde744 100644
--- a/webhook/resourcesemantics/conversion/conversion.go
+++ b/webhook/resourcesemantics/conversion/conversion.go
@@ -56,7 +56,7 @@ func (r *reconciler) Convert(
 	for _, obj := range req.Objects {
 		converted, err := r.convert(ctx, obj, req.DesiredAPIVersion)
 		if err != nil {
-			logging.FromContext(ctx).Error("Conversion failed: ", err)
+			logging.FromContext(ctx).Errorw("Conversion failed", zap.Error(err))
 			res.Result.Status = metav1.StatusFailure
 			res.Result.Message = err.Error()
 			break
diff --git a/webhook/resourcesemantics/conversion/reconciler.go b/webhook/resourcesemantics/conversion/reconciler.go
index 99f805bd5..e1f3be597 100644
--- a/webhook/resourcesemantics/conversion/reconciler.go
+++ b/webhook/resourcesemantics/conversion/reconciler.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 
+	"go.uber.org/zap"
 	apixv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apixlisters "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1"
@@ -71,7 +72,7 @@ func (r *reconciler) Reconcile(ctx context.Context, key string) error {
 	// Look up the webhook secret, and fetch the CA cert bundle.
 	secret, err := r.secretLister.Secrets(system.Namespace()).Get(r.secretName)
 	if err != nil {
-		logger.Error("Error fetching secret: ", err)
+		logger.Errorw("Error fetching secret", zap.Error(err))
 		return err
 	}
 
diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go
index 33f29a772..69d0e9ae8 100644
--- a/webhook/resourcesemantics/defaulting/defaulting.go
+++ b/webhook/resourcesemantics/defaulting/defaulting.go
@@ -87,7 +87,7 @@ func (ac *reconciler) Reconcile(ctx context.Context, key string) error {
 	// Look up the webhook secret, and fetch the CA cert bundle.
 	secret, err := ac.secretlister.Secrets(system.Namespace()).Get(ac.secretName)
 	if err != nil {
-		logger.Error("Error fetching secret: ", err)
+		logger.Errorw("Error fetching secret", zap.Error(err))
 		return err
 	}
 	caCert, ok := secret.Data[certresources.CACert]
@@ -149,7 +149,7 @@ func (ac *reconciler) reconcileMutatingWebhook(ctx context.Context, caCert []byt
 			Rule: admissionregistrationv1.Rule{
 				APIGroups:   []string{gvk.Group},
 				APIVersions: []string{gvk.Version},
-				Resources:   []string{plural + "/*"},
+				Resources:   []string{plural, plural + "/status"},
 			},
 		})
 	}
diff --git a/webhook/resourcesemantics/validation/reconcile_config.go b/webhook/resourcesemantics/validation/reconcile_config.go
index 2408e28c2..281a12e43 100644
--- a/webhook/resourcesemantics/validation/reconcile_config.go
+++ b/webhook/resourcesemantics/validation/reconcile_config.go
@@ -112,7 +112,7 @@ func (ac *reconciler) reconcileValidatingWebhook(ctx context.Context, caCert []b
 			Rule: admissionregistrationv1.Rule{
 				APIGroups:   []string{gvk.Group},
 				APIVersions: []string{gvk.Version},
-				Resources:   []string{plural + "/*"},
+				Resources:   []string{plural, plural + "/status"},
 			},
 		})
 	}


Test patch:
diff --git a/webhook/resourcesemantics/defaulting/table_test.go b/webhook/resourcesemantics/defaulting/table_test.go
index cd7329448..c9770f2c6 100644
--- a/webhook/resourcesemantics/defaulting/table_test.go
+++ b/webhook/resourcesemantics/defaulting/table_test.go
@@ -78,28 +78,28 @@ func TestReconcile(t *testing.T) {
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1alpha1"},
-			Resources:   []string{"innerdefaultresources/*"},
+			Resources:   []string{"innerdefaultresources", "innerdefaultresources/status"},
 		},
 	}, {
 		Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE"},
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1alpha1"},
-			Resources:   []string{"resources/*"},
+			Resources:   []string{"resources", "resources/status"},
 		},
 	}, {
 		Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE"},
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1beta1"},
-			Resources:   []string{"resources/*"},
+			Resources:   []string{"resources", "resources/status"},
 		},
 	}, {
 		Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE"},
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.io"},
 			APIVersions: []string{"v1alpha1"},
-			Resources:   []string{"innerdefaultresources/*"},
+			Resources:   []string{"innerdefaultresources", "innerdefaultresources/status"},
 		},
 	}}
 
@@ -213,7 +213,7 @@ func TestReconcile(t *testing.T) {
 						Rule: admissionregistrationv1.Rule{
 							APIGroups:   []string{"pkg.knative.dev"},
 							APIVersions: []string{"v1alpha1"},
-							Resources:   []string{"innerdefaultresources/*"},
+							Resources:   []string{"innerdefaultresources", "innerdefaultresources/status"},
 						},
 					}},
 				}},
@@ -272,7 +272,7 @@ func TestReconcile(t *testing.T) {
 						Rule: admissionregistrationv1.Rule{
 							APIGroups:   []string{"pkg.knative.dev"},
 							APIVersions: []string{"v1alpha1"},
-							Resources:   []string{"innerdefaultresources/*"},
+							Resources:   []string{"innerdefaultresources", "innerdefaultresources/status"},
 						},
 					}},
 				}},
diff --git a/webhook/resourcesemantics/validation/reconcile_config_test.go b/webhook/resourcesemantics/validation/reconcile_config_test.go
index 7bcb33f9c..27562f423 100644
--- a/webhook/resourcesemantics/validation/reconcile_config_test.go
+++ b/webhook/resourcesemantics/validation/reconcile_config_test.go
@@ -78,28 +78,28 @@ func TestReconcile(t *testing.T) {
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1alpha1"},
-			Resources:   []string{"innerdefaultresources/*"},
+			Resources:   []string{"innerdefaultresources", "innerdefaultresources/status"},
 		},
 	}, {
 		Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1alpha1"},
-			Resources:   []string{"resources/*"},
+			Resources:   []string{"resources", "resources/status"},
 		},
 	}, {
 		Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.dev"},
 			APIVersions: []string{"v1beta1"},
-			Resources:   []string{"resources/*"},
+			Resources:   []string{"resources", "resources/status"},
 		},
 	}, {
 		Operations: []admissionregistrationv1.OperationType{"CREATE", "UPDATE", "DELETE"},
 		Rule: admissionregistrationv1.Rule{
 			APIGroups:   []string{"pkg.knative.io"},
 			APIVersions: []string{"v1alpha1"},
-			Resources:   []string{"innerdefaultresources/*"},
+			Resources:   []string{"innerdefaultresources", "innerdefaultresources/status"},
 		},
 	}}