Make logging in admission controllers consistent with controller-manager (#1519)

Author: pmorie

plugin/pkg/admission/broker/authsarcheck/admission.go

@@ -74,23 +74,23 @@ func (s *sarcheck) Admit(a admission.Attributes) error {
 	if a.GetResource().Group != servicecatalog.GroupName || a.GetResource().GroupResource() != servicecatalog.Resource("clusterservicebrokers") {
 		return nil
 	}
-	clusterClusterServiceBroker, ok := a.GetObject().(*servicecatalog.ClusterServiceBroker)
+	clusterServiceBroker, ok := a.GetObject().(*servicecatalog.ClusterServiceBroker)
 	if !ok {
 		return errors.NewBadRequest("Resource was marked with kind ClusterServiceBroker, but was unable to be converted")
 	}
-	glog.V(5).Infof("Retrieved clusterClusterServiceBroker %v", clusterClusterServiceBroker)
 
-	if clusterClusterServiceBroker.Spec.AuthInfo == nil {
+	if clusterServiceBroker.Spec.AuthInfo == nil {
 		// no auth secret to check
 		return nil
 	}
 
 	var secretRef *servicecatalog.ObjectReference
-	if clusterClusterServiceBroker.Spec.AuthInfo.Basic != nil {
-		secretRef = clusterClusterServiceBroker.Spec.AuthInfo.Basic.SecretRef
-	} else if clusterClusterServiceBroker.Spec.AuthInfo.Bearer != nil {
-		secretRef = clusterClusterServiceBroker.Spec.AuthInfo.Bearer.SecretRef
+	if clusterServiceBroker.Spec.AuthInfo.Basic != nil {
+		secretRef = clusterServiceBroker.Spec.AuthInfo.Basic.SecretRef
+	} else if clusterServiceBroker.Spec.AuthInfo.Bearer != nil {
+		secretRef = clusterServiceBroker.Spec.AuthInfo.Bearer.SecretRef
 	}
+	glog.V(5).Infof("ClusterServiceBroker %q: evaluating auth secret ref", clusterServiceBroker)
 	userInfo := a.GetUserInfo()
 
 	sar := &authorizationapi.SubjectAccessReview{

plugin/pkg/admission/namespace/lifecycle/admission.go

@@ -106,7 +106,7 @@ func (l *lifecycle) Admit(a admission.Attributes) error {
 			exists = true
 		}
 		if exists {
-			glog.V(4).Infof("found %s in cache after waiting", a.GetNamespace())
+			glog.V(4).Infof("found namespace %q in cache after waiting", a.GetNamespace())
 		}
 	}
 
@@ -120,7 +120,7 @@ func (l *lifecycle) Admit(a admission.Attributes) error {
 		case err != nil:
 			return errors.NewInternalError(err)
 		}
-		glog.V(4).Infof("found %s via storage lookup", a.GetNamespace())
+		glog.V(4).Infof("found namespace %q via storage lookup", a.GetNamespace())
 	}
 
 	// ensure that we're not trying to create objects in terminating namespaces
@@ -129,7 +129,7 @@ func (l *lifecycle) Admit(a admission.Attributes) error {
 			return nil
 		}
 
-		return admission.NewForbidden(a, fmt.Errorf("unable to create new content in namespace %s because it is being terminated", a.GetNamespace()))
+		return admission.NewForbidden(a, fmt.Errorf("unable to create new content in namespace %q because it is being terminated", a.GetNamespace()))
 	}
 
 	return nil

plugin/pkg/admission/servicebindings/lifecycle/admission.go

@@ -73,15 +73,15 @@ func (b *enforceNoNewCredentialsForDeletedInstance) Admit(a admission.Attributes
 
 	credentials, ok := a.GetObject().(*servicecatalog.ServiceBinding)
 	if !ok {
-		return apierrors.NewBadRequest("Resource was marked with kind ServiceBindings but was unable to be converted")
+		return apierrors.NewBadRequest("Resource was marked with kind ServiceBinding but was unable to be converted")
 	}
 
 	instanceRef := credentials.Spec.ServiceInstanceRef
 	instance, err := b.instanceLister.ServiceInstances(credentials.Namespace).Get(instanceRef.Name)
 
 	// block the credentials operation if the ServiceInstance is being deleted
 	if err == nil && instance.DeletionTimestamp != nil {
-		warning := fmt.Sprintf("ServiceBindings %s/%s references an instance that is being deleted: %s/%s",
+		warning := fmt.Sprintf("ServiceBinding %s/%s references a ServiceInstance that is being deleted: %s/%s",
 			credentials.Namespace,
 			credentials.Name,
 			credentials.Namespace,
@@ -101,7 +101,7 @@ func (b *enforceNoNewCredentialsForDeletedInstance) SetInternalServiceCatalogInf
 
 func (b *enforceNoNewCredentialsForDeletedInstance) Validate() error {
 	if b.instanceLister == nil {
-		return fmt.Errorf("missing instanceLister")
+		return fmt.Errorf("missing serviceInstanceLister")
 	}
 	return nil
 }

plugin/pkg/admission/servicebindings/lifecycle/admission_test.go

@@ -100,7 +100,7 @@ func TestBlockNewCredentialsForDeletedInstance(t *testing.T) {
 	if err == nil {
 		t.Errorf("Unexpected error: %v", err.Error())
 	} else {
-		if err.Error() != "servicebindings.servicecatalog.k8s.io \"test-cred\" is forbidden: ServiceBindings test-ns/test-cred references an instance that is being deleted: test-ns/test-instance" {
+		if err.Error() != "servicebindings.servicecatalog.k8s.io \"test-cred\" is forbidden: ServiceBinding test-ns/test-cred references a ServiceInstance that is being deleted: test-ns/test-instance" {
 			t.Fatalf("admission controller blocked the request but not with expected error, expected a forbidden error, got %q", err.Error())
 		}
 	}

plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go

@@ -68,7 +68,7 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 	}
 	instance, ok := a.GetObject().(*servicecatalog.ServiceInstance)
 	if !ok {
-		return apierrors.NewBadRequest("Resource was marked with kind Instance but was unable to be converted")
+		return apierrors.NewBadRequest("Resource was marked with kind ServiceInstance but was unable to be converted")
 	}
 
 	// If the plan is specified, let it through and have the controller
@@ -83,7 +83,7 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 		if !apierrors.IsNotFound(err) {
 			return admission.NewForbidden(a, err)
 		}
-		msg := fmt.Sprintf("ServiceClass %q does not exist, can not figure out the default Service Plan.", instance.Spec.ClusterServiceClassExternalName)
+		msg := fmt.Sprintf("ClusterServiceClass %q does not exist, can not figure out the default ClusterServicePlan.", instance.Spec.ClusterServiceClassExternalName)
 		glog.V(4).Info(msg)
 		return admission.NewForbidden(a, errors.New(msg))
 	}
@@ -98,30 +98,30 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 
 	plans, err := d.getClusterServicePlansByClusterServiceClassName(sc.Name)
 	if err != nil {
-		msg := fmt.Sprintf("Error listing plans for service class (K8S: %v ExternalName: %v) - retry and specify desired ClusterServicePlan", sc.Name, instance.Spec.ClusterServiceClassExternalName)
-		glog.V(4).Info(msg)
+		msg := fmt.Sprintf("Error listing ClusterServicePlans for ClusterServiceClass (K8S: %v ExternalName: %v) - retry and specify desired ClusterServicePlan", sc.Name, instance.Spec.ClusterServiceClassExternalName)
+		glog.V(4).Infof(`ServiceInstance "%s/%s": %s`, instance.Namespace, instance.Name, msg)
 		return admission.NewForbidden(a, errors.New(msg))
 	}
 
 	// check if there were any service plans
 	// TODO: in combination with not allowing classes with no plans, this should be impossible
 	if len(plans) <= 0 {
-		msg := fmt.Sprintf("no plans found at all for service class %q", instance.Spec.ClusterServiceClassExternalName)
-		glog.V(4).Info(msg)
+		msg := fmt.Sprintf("no ClusterServicePlans found at all for ClusterServiceClass %q", instance.Spec.ClusterServiceClassExternalName)
+		glog.V(4).Infof(`ServiceInstance "%s/%s": %s`, instance.Namespace, instance.Name, msg)
 		return admission.NewForbidden(a, errors.New(msg))
 	}
 
 	// check if more than one service plan was specified and error
 	if len(plans) > 1 {
 		msg := fmt.Sprintf("ServiceClass %q has more than one plan, PlanName must be specified", instance.Spec.ClusterServiceClassExternalName)
-		glog.V(4).Info(msg)
+		glog.V(4).Infof(`ServiceInstance "%s/%s": %s`, instance.Namespace, instance.Name, msg)
 		return admission.NewForbidden(a, errors.New(msg))
 	}
 	// otherwise, by default, pick the only plan that exists for the service class
 
 	p := plans[0]
-	glog.V(4).Infof("Using default plan %q (K8S: %q) for Service Class %q for instance %s",
-		p.Spec.ExternalName, p.Name, sc.Spec.ExternalName, instance.Name)
+	glog.V(4).Infof(`ServiceInstance "%s/%s": Using default plan %q (K8S: %q) for Service Class %q`,
+		instance.Namespace, instance.Name, p.Spec.ExternalName, p.Name, sc.Spec.ExternalName)
 	if instance.Spec.ClusterServiceClassExternalName != "" {
 		instance.Spec.ClusterServicePlanExternalName = p.Spec.ExternalName
 	} else {
@@ -147,10 +147,10 @@ func (d *defaultServicePlan) SetInternalServiceCatalogClientSet(f internalclient
 
 func (d *defaultServicePlan) Validate() error {
 	if d.scClient == nil {
-		return errors.New("missing service class interface")
+		return errors.New("missing clusterserviceclass interface")
 	}
 	if d.spClient == nil {
-		return errors.New("missing serviceplan interface")
+		return errors.New("missing clusterserviceplan interface")
 	}
 	return nil
 }
@@ -163,27 +163,27 @@ func (d *defaultServicePlan) getClusterServiceClassByPlanReference(a admission.A
 }
 
 func (d *defaultServicePlan) getClusterServiceClassByK8SName(a admission.Attributes, scK8SName string) (*servicecatalog.ClusterServiceClass, error) {
-	glog.V(4).Infof("Fetching serviceclass by class k8s name %q", scK8SName)
+	glog.V(4).Infof("Fetching ClusterServiceClass by k8s name %q", scK8SName)
 	return d.scClient.Get(scK8SName, apimachineryv1.GetOptions{})
 }
 
 func (d *defaultServicePlan) getClusterServiceClassByExternalName(a admission.Attributes, scName string) (*servicecatalog.ClusterServiceClass, error) {
-	glog.V(4).Infof("Fetching serviceclass filtered by class external name %q", scName)
+	glog.V(4).Infof("Fetching ClusterServiceClass filtered by external name %q", scName)
 	fieldSet := fields.Set{
 		"spec.externalName": scName,
 	}
 	fieldSelector := fields.SelectorFromSet(fieldSet).String()
 	listOpts := apimachineryv1.ListOptions{FieldSelector: fieldSelector}
 	serviceClasses, err := d.scClient.List(listOpts)
 	if err != nil {
-		glog.V(4).Infof("List failed %q", err)
+		glog.V(4).Infof("Listing ClusterServiceClasses failed: %q", err)
 		return nil, err
 	}
 	if len(serviceClasses.Items) == 1 {
-		glog.V(4).Infof("Found Single item as %+v", serviceClasses.Items[0])
+		glog.V(4).Infof("Found single ClusterServiceClass as %+v", serviceClasses.Items[0])
 		return &serviceClasses.Items[0], nil
 	}
-	msg := fmt.Sprintf("Could not find a single ServiceClass with name %q, found %v", scName, len(serviceClasses.Items))
+	msg := fmt.Sprintf("Could not find a single ClusterServiceClass with name %q, found %v", scName, len(serviceClasses.Items))
 	glog.V(4).Info(msg)
 	return nil, admission.NewNotFound(a)
 }
@@ -199,10 +199,10 @@ func (d *defaultServicePlan) getClusterServicePlansByClusterServiceClassName(scN
 	listOpts := apimachineryv1.ListOptions{FieldSelector: fieldSelector}
 	servicePlans, err := d.spClient.List(listOpts)
 	if err != nil {
-		glog.Infof("List failed %q", err)
+		glog.Infof("Listing ClusterServicePlans failed: %q", err)
 		return nil, err
 	}
-	glog.V(4).Infof("plans fetched by filtering classname: %+v", servicePlans.Items)
+	glog.V(4).Infof("ClusterServicePlans fetched by filtering classname: %+v", servicePlans.Items)
 	r := servicePlans.Items
 	return r, err
 }