review feedback

Author: kenjenkins

cmd/all_in_one.go

@@ -338,7 +338,7 @@ func (s *allCmdParam) applyBootstrapIngress(
 	// Fetch the bootstrap ingress as a one-off.
 	ns, n, ok := strings.Cut(name, "/")
 	if !ok {
-		return fmt.Errorf("expected ingress name in namespace/name formt")
+		return fmt.Errorf("expected ingress name in namespace/name format, got %q", name)
 	}
 	nn := types.NamespacedName{Namespace: ns, Name: n}
 	scheme, err := getScheme()
@@ -362,14 +362,16 @@ func (s *allCmdParam) applyBootstrapIngress(
 		return fmt.Errorf("fetch bootstrap ingress data: %w", err)
 	}
 
-	// Force the route 'From' URL to use the service proxy URL (so we don't need
+	// Force the route 'To' URL to use the service proxy URL (so we don't need
 	// to watch for changes to the service endpoints).
 	ic.Ingress = ic.Ingress.DeepCopy()
-	ic.Ingress.Annotations[annotationPrefix+"/"+model.UseServiceProxy] = "true"
+	util.SetAnnotation(ic.Ingress, annotationPrefix+"/"+model.UseServiceProxy, "true")
 
 	routes, err := pomerium.IngressToRoutes(ctx, ic)
 	if err != nil {
 		return fmt.Errorf("convert bootstrap ingress to routes: %w", err)
+	} else if len(routes) == 0 {
+		return fmt.Errorf("bootstrap ingress %q has no rules", name)
 	}
 	s.cfg.Options.Policies = append(s.cfg.Options.Policies, routes...)
 

pomerium/sync_api.go

@@ -260,7 +260,7 @@ func (r *APIReconciler) upsertOneIngress(
 			return changed, err
 		}
 		if ic.Annotations[k] != *route.Id {
-			setAnnotation(ic, k, *route.Id)
+			util.SetAnnotation(ic, k, *route.Id)
 		}
 		changed = changed || changedRoute
 	}
@@ -332,7 +332,7 @@ func (r *APIReconciler) syncOneSecret(
 	if err != nil {
 		return false, err
 	} else if changed {
-		setAnnotation(secret, apiKeyPairIDAnnotation, keyPair.GetId())
+		util.SetAnnotation(secret, apiKeyPairIDAnnotation, keyPair.GetId())
 		controllerutil.AddFinalizer(secret, apiFinalizer)
 		err = r.k8sClient.Patch(ctx, secret, client.MergeFrom(originalSecret))
 	}
@@ -502,7 +502,7 @@ func (r *APIReconciler) SetGatewayConfig(
 				}
 				changes = changes || routeChanged
 				if gr.Annotations[k] != *route.Id {
-					setAnnotation(gr, k, *route.Id)
+					util.SetAnnotation(gr, k, *route.Id)
 				}
 			}
 			controllerutil.AddFinalizer(gr, apiFinalizer)
@@ -570,7 +570,7 @@ func (r *APIReconciler) syncGatewayPolicies(
 			return changes, nil, err
 		} else if changedPolicy {
 			changes = true
-			setAnnotation(obj, apiPolicyIDAnnotation, policy.GetId())
+			util.SetAnnotation(obj, apiPolicyIDAnnotation, policy.GetId())
 			controllerutil.AddFinalizer(obj, apiFinalizer)
 		}
 
@@ -770,7 +770,7 @@ func (r *APIReconciler) syncIngressPolicy(
 		return false, "", fmt.Errorf("couldn't update ingress policy: %w", err)
 	}
 	updatedPolicyID = apiPolicy.GetId()
-	setAnnotation(ingress, apiPolicyIDAnnotation, updatedPolicyID)
+	util.SetAnnotation(ingress, apiPolicyIDAnnotation, updatedPolicyID)
 	return changed, updatedPolicyID, nil
 }
 
@@ -1040,15 +1040,6 @@ func convertProto[Dst, Src proto.Message](msg Src) (Dst, error) {
 	return newMsg, err
 }
 
-func setAnnotation(object client.Object, key, value string) {
-	m := object.GetAnnotations()
-	if m == nil {
-		m = make(map[string]string)
-	}
-	m[key] = value
-	object.SetAnnotations(m)
-}
-
 func falseToNil(x *bool) *bool {
 	if x != nil && !*x {
 		return nil

util/object.go

@@ -0,0 +1,14 @@
+package util
+
+import "sigs.k8s.io/controller-runtime/pkg/client"
+
+// SetAnnotation sets the given annotation key/value, making a new Annotations
+// map if one does not already exist.
+func SetAnnotation(object client.Object, key, value string) {
+	m := object.GetAnnotations()
+	if m == nil {
+		m = make(map[string]string)
+	}
+	m[key] = value
+	object.SetAnnotations(m)
+}