canary: Check canary service for nil elements

pkg/operator/controller/canary/route.go: Check that the canary
service is not nil and that the canary service has a non-empty spec.ports
field when building the canary route via `desiredCanaryRoute` to avoid
nil dereferences.

pkg/operator/controller/canary/route_test.go: Ensure `desiredCanaryRoute`
does not return any errors.

pkg/operator/controller/canary/route.go

@@ -18,7 +18,11 @@ import (
 
 // ensureCanaryRoute ensures the canary route exists
 func (r *reconciler) ensureCanaryRoute(service *corev1.Service) (bool, *routev1.Route, error) {
-	desired := desiredCanaryRoute(service)
+	desired, err := desiredCanaryRoute(service)
+	if err != nil {
+		return false, nil, fmt.Errorf("failed to build canary route: %v", err)
+	}
+
 	haveRoute, current, err := r.currentCanaryRoute()
 	if err != nil {
 		return false, nil, err
@@ -113,14 +117,18 @@ func canaryRouteChanged(current, expected *routev1.Route) (bool, *routev1.Route)
 
 // desiredCanaryRoute returns the desired canary route read in
 // from manifests
-func desiredCanaryRoute(service *corev1.Service) *routev1.Route {
+func desiredCanaryRoute(service *corev1.Service) (*routev1.Route, error) {
 	route := manifests.CanaryRoute()
 
 	name := controller.CanaryRouteName()
 
 	route.Namespace = name.Namespace
 	route.Name = name.Name
 
+	if service == nil {
+		return route, fmt.Errorf("expected non-nil canary service for canary route %s/%s", route.Namespace, route.Name)
+	}
+
 	route.Labels = map[string]string{
 		// associate the route with the canary controller
 		manifests.OwningIngressCanaryCheckLabel: canaryControllerName,
@@ -129,15 +137,18 @@ func desiredCanaryRoute(service *corev1.Service) *routev1.Route {
 	route.Spec.To.Name = controller.CanaryServiceName().Name
 
 	// Set spec.port.targetPort to the first port available in the canary service.
-	// The canary controller will toggle which targetPort the route targets
+	// The canary controller may toggle which targetPort the route targets
 	// to test > 1 endpoint, so it does not matter which port is selected as long
 	// as the canary service has > 1 ports available. If the canary service only has one
 	// available port, then route.Spec.Port.TargetPort will remain unchanged.
+	if len(service.Spec.Ports) == 0 {
+		return route, fmt.Errorf("expected spec.ports to be non-empty for canary service %s/%s", service.Namespace, service.Name)
+	}
 	route.Spec.Port.TargetPort = service.Spec.Ports[0].TargetPort
 
 	route.SetOwnerReferences(service.OwnerReferences)
 
-	return route
+	return route, nil
 }
 
 // checkRouteAdmitted returns true if a given route has been admitted

pkg/operator/controller/canary/route_test.go

@@ -19,7 +19,11 @@ func TestDesiredCanaryRoute(t *testing.T) {
 		Name: "test",
 	}
 	service := desiredCanaryService(daemonsetRef)
-	route := desiredCanaryRoute(service)
+	route, err := desiredCanaryRoute(service)
+
+	if err != nil {
+		t.Fatalf("desiredCanaryService returned an error: %v", err)
+	}
 
 	expectedRouteName := types.NamespacedName{
 		Namespace: "openshift-ingress-canary",
@@ -106,7 +110,10 @@ func TestCanaryRouteChanged(t *testing.T) {
 	service := desiredCanaryService(daemonsetRef)
 
 	for _, tc := range testCases {
-		original := desiredCanaryRoute(service)
+		original, err := desiredCanaryRoute(service)
+		if err != nil {
+			t.Fatalf("desiredCanaryService returned an error: %v", err)
+		}
 		mutated := original.DeepCopy()
 		tc.mutate(mutated)
 		if changed, updated := canaryRouteChanged(original, mutated); changed != tc.expect {