refactor: update test request logic and improve patch

Among a variety of other changes.

- Patch is called earlier and we now have a
ExternalLoadBalancerEndpointAvailableCondition that we can use to convey
failure information.
- Rather than simply checking the OwnerReferences the ClusterName is
used to ensure the owner reference exists.
- Added SetupWithManager method which is called from main.
- Moved test request logic to separate function. The test request now
calls the {ENDPOINT}/livez which should reach the Kubernetes API server

api/v1alpha1/condition_consts.go

@@ -48,3 +48,15 @@ const (
 	// to be available before proceeding.
 	WaitingForBootstrapDataReason = "WaitingForBoostrapData"
 )
+
+const (
+	// ExternalLoadBalancerEndpointAvailableCondition is a condition that indicates that the API server Load Balancer is available
+	ExternalLoadBalancerEndpointAvailableCondition clusterv1.ConditionType = "ExternalLoadBalancerEndpointAvailable"
+
+	// ExternalLoadBalancerEndpointNotAvailableReason is used to indicate any error with the
+	// availability of the load balancer.
+	ExternalLoadBalancerEndpointFailedReason = "ExternalLoadBalancerEndpointFailed"
+
+	// ExternalLoadBalancerEndpointNotAvailableReason is used to indicate that the load balancer isn't available.
+	ExternalLoadBalancerEndpointNotAvailableReason = "ExternalLoadBalancerEndpointNotAvailable"
+)

api/v1alpha1/externalloadbalancer_types.go

@@ -31,7 +31,8 @@ func (ep *ExternalLoadBalancerEndpoint) String() string {
 type ExternalLoadBalancerSpec struct {
 	// Endpoint represents the endpoint for the load balancer. This endpoint will
 	// be tested to see if its available.
-	Endpoint ExternalLoadBalancerEndpoint `json:"endpoint"`
+	Endpoint    ExternalLoadBalancerEndpoint `json:"endpoint"`
+	ClusterName string                       `json:"clusterName"`
 }
 
 type ExternalLoadBalancerStatus struct {

api/v1alpha1/microvmcluster_types.go

@@ -19,8 +19,8 @@ type MicrovmClusterSpec struct {
 	// Placement specifies how machines for the cluster should be placed onto hosts (i.e. where the microvms are created).
 	// +kubebuilder:validation:Required
 	Placement Placement `json:"placement"`
-	// EndpointRef
-	EndpointRef *corev1.ObjectReference `json:"endpointRef,omitempty"`
+	// LoadBalancerRef
+	LoadBalancerRef *corev1.ObjectReference `json:"loadBalancerRef,omitempty"`
 }
 
 // MicrovmClusterStatus defines the observed state of MicrovmCluster.

config/crd/bases/infrastructure.cluster.x-k8s.io_externalloadbalancers.yaml

@@ -50,6 +50,8 @@ spec:
             description: ExternalLoadBalancerSpec defines the desired state for a
               ExternalLoadBalancer.
             properties:
+              clusterName:
+                type: string
               endpoint:
                 description: Endpoint represents the endpoint for the load balancer.
                   This endpoint will be tested to see if its available.
@@ -66,6 +68,7 @@ spec:
                 - host
                 type: object
             required:
+            - clusterName
             - endpoint
             type: object
           status:

config/crd/bases/infrastructure.cluster.x-k8s.io_microvmclusters.yaml

@@ -53,30 +53,8 @@ spec:
           spec:
             description: MicrovmClusterSpec defines the desired state of MicrovmCluster.
             properties:
-              endpointRef:
-                description: 'ObjectReference contains enough information to let you
-                  inspect or modify the referred object. --- New uses of this type
-                  are discouraged because of difficulty describing its usage when
-                  embedded in APIs. 1. Ignored fields.  It includes many fields which
-                  are not generally honored.  For instance, ResourceVersion and FieldPath
-                  are both very rarely valid in actual usage. 2. Invalid usage help.  It
-                  is impossible to add specific help for individual usage.  In most
-                  embedded usages, there are particular restrictions like, "must refer
-                  only to types A and B" or "UID not honored" or "name must be restricted".
-                  Those cannot be well described when embedded. 3. Inconsistent validation.  Because
-                  the usages are different, the validation rules are different by
-                  usage, which makes it hard for users to predict what will happen.
-                  4. The fields are both imprecise and overly precise.  Kind is not
-                  a precise mapping to a URL. This can produce ambiguity during interpretation
-                  and require a REST mapping.  In most cases, the dependency is on
-                  the group,resource tuple and the version of the actual struct is
-                  irrelevant. 5. We cannot easily change it.  Because this type is
-                  embedded in many locations, updates to this type will affect numerous
-                  schemas.  Don''t make new APIs embed an underspecified API type
-                  they do not control. Instead of using this type, create a locally
-                  provided and used type that is well-focused on your reference. For
-                  example, ServiceReferences for admission registration: https://github.com/kubernetes/api/blob/release-1.17/admissionregistration/v1/types.go#L533
-                  .'
+              loadBalancerRef:
+                description: LoadBalancerRef
                 properties:
                   apiVersion:
                     description: API version of the referent.

controllers/errors.go

@@ -12,4 +12,5 @@ var (
 	errMicrovmUnknownState                     = errors.New("microvm is in an unknown/unsupported state")
 	errExpectedMicrovmCluster                  = errors.New("expected microvm cluster")
 	errNoPlacement                             = errors.New("no placement specified")
+	errInvalidLoadBalancerResponseStatusCode   = errors.New("endpoint returned a 5XX status code")
 )

controllers/externalloadbalancer_controller.go

@@ -5,32 +5,45 @@ package controllers
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"net/http"
 	"os"
 	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
+	"sigs.k8s.io/cluster-api/util/predicates"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	infrav1 "github.com/weaveworks/cluster-api-provider-microvm/api/v1alpha1"
+	"github.com/weaveworks/cluster-api-provider-microvm/internal/defaults"
 )
 
 type ExternalLoadBalancerReconciler struct {
 	client.Client
 	Scheme           *runtime.Scheme
 	Recorder         record.EventRecorder
 	WatchFilterValue string
+	HTTPClient       *http.Client
 }
 
 const (
-	httpErrorStatusCode = 500
-	warningLogVerbosity = 2
+	httpErrorStatusCode = 50
 	defaultHTTPTimeout  = 5 * time.Second
 )
 
@@ -58,9 +71,19 @@ func (r *ExternalLoadBalancerReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, err
 	}
 
-	if ownerRef := loadbalancer.GetOwnerReferences(); len(ownerRef) == 0 {
-		// What should we do here if the OwnerReference is empty, simply requeue??
-		return ctrl.Result{RequeueAfter: requeuePeriod}, nil
+	defer func() {
+		if err := r.Patch(ctx, loadbalancer); err != nil {
+			log.Error(err, "attempting to patch loadbalancer object")
+		}
+	}()
+
+	if err := r.ensureClusterOwnerRef(ctx, req, loadbalancer); err != nil {
+		if apierrors.IsNotFound(err) {
+			return ctrl.Result{}, nil
+		}
+		log.Error(err, "retrieving cluster from clusterName", "clusterName", loadbalancer.ClusterName)
+
+		return ctrl.Result{}, err
 	}
 
 	if !loadbalancer.ObjectMeta.DeletionTimestamp.IsZero() {
@@ -69,52 +92,146 @@ func (r *ExternalLoadBalancerReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, nil
 	}
 
-	client := &http.Client{
-		Timeout: defaultHTTPTimeout,
+	if err := r.sendTestRequest(ctx, loadbalancer); err != nil {
+		if os.IsTimeout(err) {
+			log.Error(err, "request timed out attempting to contact endpoint", "endpoint", loadbalancer.Spec.Endpoint.String())
+			conditions.MarkFalse(
+				loadbalancer,
+				infrav1.ExternalLoadBalancerEndpointAvailableCondition,
+				infrav1.ExternalLoadBalancerEndpointNotAvailableReason,
+				clusterv1.ConditionSeverityInfo, "request to loadbalancer endpoint timed out",
+			)
+
+			return ctrl.Result{}, fmt.Errorf("request timed out attempting to contact endpoint: %s: %w", loadbalancer.Spec.Endpoint.String(), err)
+		}
+
+		if errors.Is(err, errInvalidLoadBalancerResponseStatusCode) {
+			log.Error(err, "request to endpoint", "endpoint", loadbalancer.Spec.Endpoint.String())
+			conditions.MarkFalse(
+				loadbalancer,
+				infrav1.ExternalLoadBalancerEndpointAvailableCondition,
+				infrav1.ExternalLoadBalancerEndpointNotAvailableReason,
+				clusterv1.ConditionSeverityInfo, "loadbalancer endpoint responded with error",
+			)
+
+			return ctrl.Result{}, nil
+		}
+
+		log.Error(err, "attempting to contact specified endpoint", "endpoint", loadbalancer.Spec.Endpoint.String())
+		conditions.MarkFalse(
+			loadbalancer,
+			infrav1.ExternalLoadBalancerEndpointAvailableCondition,
+			infrav1.ExternalLoadBalancerEndpointFailedReason,
+			clusterv1.ConditionSeverityInfo, "request to loadbalancer endpoint failed: %s",
+			err.Error(),
+		)
+
+		return ctrl.Result{}, fmt.Errorf("attempting to contact specified endpoint: %s: %w", loadbalancer.Spec.Endpoint.String(), err)
 	}
 
-	epReq, err := http.NewRequestWithContext(ctx, http.MethodGet, loadbalancer.Spec.Endpoint.String(), nil)
-	if err != nil {
-		log.Error(err, "creating endpoint request", "id", req.NamespacedName)
+	loadbalancer.Status.Ready = true
+	conditions.MarkTrue(loadbalancer, infrav1.ExternalLoadBalancerEndpointAvailableCondition)
+
+	return ctrl.Result{}, nil
+}
+
+// Patch persists the resource and status.
+func (r *ExternalLoadBalancerReconciler) Patch(ctx context.Context, lb *infrav1.ExternalLoadBalancer) error {
+	applicableConditions := []clusterv1.ConditionType{
+		infrav1.ExternalLoadBalancerEndpointAvailableCondition,
 	}
 
-	resp, err := client.Do(epReq)
+	conditions.SetSummary(lb,
+		conditions.WithConditions(applicableConditions...),
+		conditions.WithStepCounterIf(lb.DeletionTimestamp.IsZero()),
+		conditions.WithStepCounter(),
+	)
+
+	patchHelper, err := patch.NewHelper(lb, r.Client)
 	if err != nil {
-		if os.IsTimeout(err) {
-			log.Error(err, "request timed out attempting to contact endpoint", "endpoint", loadbalancer.Spec.Endpoint.String())
+		return err
+	}
+	if patchErr := patchHelper.Patch(
+		ctx,
+		lb,
+		patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
+			clusterv1.ReadyCondition,
+			infrav1.LoadBalancerAvailableCondition,
+		}}); patchErr != nil {
+		return err
+	}
 
-			return ctrl.Result{}, err
-		}
-		log.Error(err, "attempting to contact specified endpoint", "endpoint", loadbalancer.Spec.Endpoint.String())
+	return nil
+}
 
-		return ctrl.Result{}, err
+// sendTestRequest makes an HTTP call to ${KUBE_VIP_HOST}:${KUBE_VIP_PORT}/livez, which, if the loadbalancer is live,
+// should reach the /livez endpoint on the Kubernetes API server.
+func (r *ExternalLoadBalancerReconciler) sendTestRequest(ctx context.Context, lb *infrav1.ExternalLoadBalancer) error {
+	endpoint := lb.Spec.Endpoint.String() + "/livez"
+	epReq, err := http.NewRequestWithContext(ctx, http.MethodGet, lb.Spec.Endpoint.String()+"/livez", nil) // use livez endpoint
+	if err != nil {
+		return fmt.Errorf("creating endpoint request: %w", err)
+	}
+
+	log.Log.V(defaults.LogLevelDebug).Info("attempting request to API server livez endpoint via loadbalancer", "endpoint_address", endpoint)
+	resp, err := r.HTTPClient.Do(epReq)
+	if err != nil {
+		return err
 	}
 	defer resp.Body.Close()
+
 	if resp.StatusCode >= httpErrorStatusCode {
-		// Do we requeue here? How do we track retries, or will this be handled automatically (CrashLoopBackoff)
-		log.V(warningLogVerbosity).Info("endpoint returned a 5XX status code", "endpoint", loadbalancer.Spec.Endpoint.String())
+		return errInvalidLoadBalancerResponseStatusCode
+	}
 
-		return ctrl.Result{}, nil
+	return nil
+}
+
+func (r *ExternalLoadBalancerReconciler) ensureClusterOwnerRef(ctx context.Context, req ctrl.Request, lb *infrav1.ExternalLoadBalancer) error {
+	clusterNamespaceName := types.NamespacedName{
+		Namespace: req.NamespacedName.Namespace,
+		Name:      lb.ClusterName,
 	}
 
-	loadbalancer.Status.Ready = true
+	cluster := &clusterv1.Cluster{}
+	if err := r.Get(ctx, clusterNamespaceName, cluster); err != nil {
+		return err
+	}
 
-	defer func() {
-		if err := r.Patch(loadbalancer); err != nil {
-			log.Error(err, "attempting to patch loadbalancer object")
-		}
-	}()
+	lb.OwnerReferences = util.EnsureOwnerRef(lb.OwnerReferences, metav1.OwnerReference{
+		APIVersion: cluster.APIVersion,
+		Kind:       cluster.Kind,
+		Name:       cluster.Name,
+		UID:        cluster.UID,
+	})
 
-	return ctrl.Result{}, nil
+	return nil
 }
 
-func (r *ExternalLoadBalancerReconciler) Patch(lb *infrav1.ExternalLoadBalancer) error {
-	patchHelper, err := patch.NewHelper(lb, r.Client)
-	if err != nil {
-		return err
+func (r *ExternalLoadBalancerReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
+	log := ctrl.LoggerFrom(ctx)
+
+	if r.HTTPClient == nil {
+		r.HTTPClient = &http.Client{Timeout: defaultHTTPTimeout}
 	}
-	if patchErr := patchHelper.Patch(context.TODO(), lb); patchErr != nil {
-		return err
+
+	builder := ctrl.NewControllerManagedBy(mgr).
+		WithOptions(options).
+		For(&infrav1.ExternalLoadBalancer{}).
+		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(log, r.WatchFilterValue)).
+		WithEventFilter(predicates.ResourceIsNotExternallyManaged(log)).
+		Watches(
+			&source.Kind{Type: &clusterv1.Cluster{}},
+			handler.EnqueueRequestsFromMapFunc(
+				util.ClusterToInfrastructureMapFunc(infrav1.GroupVersion.WithKind("ExternalLoadBalancer")),
+			),
+			builder.WithPredicates(
+				predicates.ClusterUnpaused(log),
+			),
+		)
+
+	if err := builder.Complete(r); err != nil {
+		return fmt.Errorf("creating external loadbalancer controller: %w", err)
 	}
 
 	return nil

controllers/microvmcluster_controller.go

@@ -51,7 +51,7 @@ type MicrovmClusterReconciler struct {
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=microvmclusters,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=microvmclusters/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=microvmclusters/finalizers,verbs=update
-// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=externalloadbalancerendpoint,verbs=get;list;watch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=externalloadbalancers,verbs=get;list;watch
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
@@ -128,7 +128,7 @@ func (r *MicrovmClusterReconciler) reconcileDelete(_ context.Context, clusterSco
 func (r *MicrovmClusterReconciler) reconcileNormal(ctx context.Context, clusterScope *scope.ClusterScope) (reconcile.Result, error) {
 	clusterScope.Info("Reconciling MicrovmCluster")
 
-	if clusterScope.MvmCluster.Spec.EndpointRef == nil {
+	if clusterScope.MvmCluster.Spec.LoadBalancerRef == nil {
 		return reconcile.Result{}, errExternalLoadBalancerEndpointRefRequired
 	}
 
@@ -153,7 +153,7 @@ func (r *MicrovmClusterReconciler) isAPIServerAvailable(ctx context.Context, clu
 	endpoint := &infrav1.ExternalLoadBalancer{}
 	eprnn := types.NamespacedName{
 		Namespace: clusterScope.MvmCluster.ObjectMeta.Namespace,
-		Name:      clusterScope.MvmCluster.Spec.EndpointRef.Name,
+		Name:      clusterScope.MvmCluster.Spec.LoadBalancerRef.Name,
 	}
 	if err := r.Get(ctx, eprnn, endpoint); err != nil {
 		clusterScope.Error(err, "get referenced ExternalLoadBalancerEndpoint")

controllers/microvmcluster_controller_test.go

@@ -48,7 +48,7 @@ func TestClusterReconciliationWithMvmClusterEndpoint(t *testing.T) {
 	g := NewWithT(t)
 
 	mvmCluster := createMicrovmCluster(testClusterName, testClusterNamespace)
-	mvmCluster.Spec.EndpointRef = &corev1.ObjectReference{
+	mvmCluster.Spec.LoadBalancerRef = &corev1.ObjectReference{
 		Kind: "ExternalLoadBalancerEndpoint",
 		Name: "tenant1-elb-endpoint",
 	}
@@ -101,7 +101,7 @@ func TestClusterReconciliationWithClusterEndpointAPIServerNotReady(t *testing.T)
 
 	cluster := createCluster(testClusterName, testClusterNamespace)
 	mvmCluster := createMicrovmCluster(testClusterName, testClusterNamespace)
-	mvmCluster.Spec.EndpointRef = &corev1.ObjectReference{
+	mvmCluster.Spec.LoadBalancerRef = &corev1.ObjectReference{
 		Kind: "ExternalLoadBalancerEndpoint",
 		Name: "tenant1-elb-endpoint",
 	}