feat: add controller for external load balancer #101

richardcase · 2022-01-19T09:04:58Z

What this PR does / why we need it:

The controller does a simple HTTP get on the endpoint.
Any non-5xx response is acceptable.
The MvmCluster checks the endpointRef for it's ready status
If it is not ready, it requeues the reconciliation.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Relates #90

Special notes for your reviewer:

Checklist:

adds unit tests

richardcase · 2022-01-19T09:05:03Z

Current dependencies on/for this PR:

main
- PR feat: add api types for external load balancer #102
  - PR feat: add controller for external load balancer #101 (this PR)

This comment was auto-generated by Graphite.

- The controller does a simple HTTP get on the endpoint. - Any non-5xx response is acceptable. - The MvmCluster checks the endpointRef for it's ready status - If it is not ready, it requeues the reconciliation.

Callisto13 · 2022-01-25T11:44:19Z

templates/cluster-template.yaml

@@ -20,19 +20,30 @@ spec:
    apiVersion: controlplane.cluster.x-k8s.io/v1beta1
    name: "${CLUSTER_NAME}-control-plane"
 ---
+# ExternalLoadBalancer Definition


do we need to do the same in cluster-template-cilium.yaml?

richardcase · 2022-01-25T21:13:44Z

api/v1alpha1/microvmcluster_types.go

@@ -25,6 +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"`


This should probably be LoadBalancerRef or something more descriptive.

richardcase · 2022-01-25T21:14:53Z

controllers/externalloadbalancer_controller.go

+	}
+
+	if ownerRef := loadbalancer.GetOwnerReferences(); len(ownerRef) == 0 {
+		// What should we do here if the OwnerReference is empty, simply requeue??


I would do a debug log and then requeue.

richardcase · 2022-01-25T21:16:22Z

controllers/externalloadbalancer_controller.go

+		Timeout: defaultHTTPTimeout,
+	}
+
+	epReq, err := http.NewRequestWithContext(ctx, http.MethodGet, loadbalancer.Spec.Endpoint.String(), nil)


Could we move the actual testing of the endpoint to a separate function?

richardcase · 2022-01-25T21:18:01Z

controllers/externalloadbalancer_controller.go

+		return ctrl.Result{}, nil
+	}
+
+	client := &http.Client{


Should we be passing in a interface of some sort for the http client so that we can mock it for tests?

I've been thinking a bit more about this, perhaps we could use the remote client and the secret to actually connect....this would require a small change....lets chat today.

Haha, we did talk about this before. It's ok I know how to make this change!

richardcase · 2022-01-25T21:19:11Z

controllers/externalloadbalancer_controller.go

+
+	epReq, err := http.NewRequestWithContext(ctx, http.MethodGet, loadbalancer.Spec.Endpoint.String(), nil)
+	if err != nil {
+		log.Error(err, "creating endpoint request", "id", req.NamespacedName)


We should return from reconciliation with the error as well.

richardcase · 2022-01-25T21:29:55Z

controllers/externalloadbalancer_controller.go

+	infrav1 "github.com/weaveworks/cluster-api-provider-microvm/api/v1alpha1"
+)
+
+type ExternalLoadBalancerReconciler struct {


We could do with some unit tests to cover this controller.

I thought about this and mentioned as much in slack. Happy to add tests but there isn't a huge amount to actually test. Outside of the HTTP call it's mostly basic logic. Probably worth doing it anyway - will add some.

richardcase · 2022-01-25T21:31:16Z

controllers/microvmcluster_controller.go

@@ -51,6 +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


I think this should be externalloadbalancers instead?

richardcase · 2022-01-25T21:38:55Z

controllers/microvmcluster_controller.go

-	remoteClient, err := r.RemoteClientGetter(ctx, clusterScope.ClusterName(), r.Client, clusterKey)
-	if err != nil {
-		clusterScope.Error(err, "creating remote cluster client")
+	if err := r.Get(ctx, eprnn, endpoint); err != nil {


If we get the endpoint i think we need to ensure the owner reference is set on it using EnsureOwnerRef. This would then require we patch the loadbalancer kind.

That could also be done in the ExternalLoadBalancer controller but that would required we have a cluster name field (or something similar)

richardcase · 2022-01-25T21:39:31Z

templates/cluster-template.yaml

@@ -20,19 +20,30 @@ spec:
    apiVersion: controlplane.cluster.x-k8s.io/v1beta1
    name: "${CLUSTER_NAME}-control-plane"
 ---
+# ExternalLoadBalancer Definition


richardcase · 2022-01-25T21:42:35Z

controllers/externalloadbalancer_controller.go

+
+type ExternalLoadBalancerReconciler struct {
+	client.Client
+	Scheme           *runtime.Scheme


We need a SetupWithManager for this new controller and that will need to be called from main

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

Signed-off-by: Richard Case <richard@weave.works>

github-actions · 2022-03-13T07:19:16Z

This PR is stale because it has been open 30 days with no activity.

richardcase · 2022-03-15T14:52:10Z

We still need to finish this.

feat: add controller for external load balancer

f53a477

richardcase mentioned this pull request Jan 19, 2022

feat: add api types for external load balancer #102

Draft

4 tasks

github-actions bot added the size/s label Jan 19, 2022

richardcase added the kind/feature New feature or request label Jan 19, 2022

feat: add external loadbalancer endpoint types and controller

050ad6c

- The controller does a simple HTTP get on the endpoint. - Any non-5xx response is acceptable. - The MvmCluster checks the endpointRef for it's ready status - If it is not ready, it requeues the reconciliation.

github-actions bot added size/m and removed size/s labels Jan 25, 2022

jmickey added 2 commits January 25, 2022 10:36

fix: lint errors

4f59f7c

fix: gofumpt not gofmt

1173a2b

jmickey marked this pull request as ready for review January 25, 2022 11:01

Callisto13 reviewed Jan 25, 2022

View reviewed changes

richardcase commented Jan 25, 2022

View reviewed changes

jmickey force-pushed the rcfeat_add_controller_for_external_load_ branch from b277f16 to 66b8c53 Compare February 9, 2022 12:37

richardcase added 2 commits February 9, 2022 15:08

wip: changes as part of testing

1b9fab3

Signed-off-by: Richard Case <richard@weave.works>

wip

675c283

Signed-off-by: Richard Case <richard@weave.works>

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 13, 2022

Callisto13 removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 23, 2022

richardcase mentioned this pull request Oct 26, 2023

📖 CAEP: Flexible Managed Kubernetes Endpoints kubernetes-sigs/cluster-api#8500

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add controller for external load balancer #101

feat: add controller for external load balancer #101

richardcase commented Jan 19, 2022 •

edited by jmickey

Loading

richardcase commented Jan 19, 2022

Callisto13 Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 26, 2022

jmickey Jan 26, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

jmickey Jan 26, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

richardcase Jan 25, 2022

github-actions bot commented Mar 13, 2022

richardcase commented Mar 15, 2022

feat: add controller for external load balancer #101

Are you sure you want to change the base?

feat: add controller for external load balancer #101

Conversation

richardcase commented Jan 19, 2022 • edited by jmickey Loading

richardcase commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2022

richardcase commented Mar 15, 2022

richardcase commented Jan 19, 2022 •

edited by jmickey

Loading