✨ Allow adding custom HA proxy config for CAPD load balancer #8785

alexander-demicev · 2023-06-05T09:00:55Z

What this PR does / why we need it:

Allow adding custom HA proxy config for CAPD load balancer. With this change, an additional load balancer configuration can be added using a config map. For more details see #7684

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

alexander-demicev · 2023-06-05T09:43:45Z

/test pull-cluster-api-e2e-full-main

fabriziopandini · 2023-06-14T13:39:29Z

test/infrastructure/docker/api/v1beta1/dockercluster_types.go

+	// should be equal to 'value'.
+	// The content of the config map will be appended to the HAProxy config file. Please use it with caution,
+	// as there are no checks to ensure the validity of the configuration.
+	AdditionalProxyConfigRef *corev1.LocalObjectReference `json:"additionalProxyConfigRef,omitempty"`


What about

Suggested change

AdditionalProxyConfigRef *corev1.LocalObjectReference `json:"additionalProxyConfigRef,omitempty"`

UnsafeHAProxyConfigTemplateRef *corev1.LocalObjectReference `json:"additionalProxyConfigRef,omitempty"`

So we are explicity about "use at your own risk"; also, dropped additional according to latest discussion in the issue (it should be replace)

test/infrastructure/docker/internal/docker/loadbalancer.go

alexander-demicev · 2023-06-22T11:49:04Z

/test pull-cluster-api-e2e-full-main

richardcase · 2023-06-22T12:21:13Z

test/infrastructure/docker/api/v1beta1/dockercluster_types.go

+	// where the key is the server name and the value is the address. This map is dynamic and is updated every time a new control plane
+	// node is added or removed. The template will also support the JoinHostPort function to join the host and port of the backend server.
+	// +optional
+	UnsafeHAProxyConfigTemplateRef *corev1.LocalObjectReference `json:"additionalProxyConfigRef,omitempty"`


Not sure i like the use of Unsafe as that sounds scary 💀 Could we use something like CustomHAProxyConfigTemplateRef or CustomLoadBalancerConfigRef

richardcase · 2023-06-22T12:21:39Z

test/infrastructure/docker/api/v1beta1/dockercluster_types.go

@@ -53,6 +54,17 @@ type DockerClusterSpec struct {
 type DockerLoadBalancer struct {
 	// ImageMeta allows customizing the image used for the cluster load balancer.
 	ImageMeta `json:",inline"`
+
+	// UnsafeHAProxyConfigTemplateRef allows you to replace the HAProxy config file.


Suggested change

// UnsafeHAProxyConfigTemplateRef allows you to replace the HAProxy config file.

// UnsafeHAProxyConfigTemplateRef allows you to replace the default HAProxy config file.

test/infrastructure/docker/internal/loadbalancer/config.go

fabriziopandini

Just two nits from my side

test/infrastructure/docker/internal/controllers/dockermachine_controller.go

test/infrastructure/docker/internal/loadbalancer/config.go

fabriziopandini · 2023-07-04T19:27:14Z

/lgtm
@chrischdi @elmiko PTAL

k8s-ci-robot · 2023-07-04T19:27:20Z

LGTM label has been added.

Git tree hash: 3a72949a4180f9981c207ba1a3e11e0ee06455e0

chrischdi

Some smaller nits :-)

test/infrastructure/docker/internal/loadbalancer/config.go

test/infrastructure/docker/internal/docker/loadbalancer.go

test/infrastructure/docker/internal/controllers/dockermachine_controller.go

alexander-demicev · 2023-07-10T17:43:32Z

@chrischdi Thanks! all fixed

fabriziopandini · 2023-07-10T18:14:14Z

/test pull-cluster-api-e2e-full-main

fabriziopandini · 2023-07-10T18:16:13Z

/lgtm
Waiting for a final test run before approving

k8s-ci-robot · 2023-07-10T18:16:19Z

LGTM label has been added.

Git tree hash: 166a5864042e1fdafd44a0bb9313586112cb1747

chrischdi

One last nit but also ok to do it in a follow up :-)

As of that lgtm 👍

test/infrastructure/docker/internal/controllers/dockermachine_controller.go

richardcase · 2023-07-11T14:00:16Z

test/infrastructure/docker/api/v1beta1/dockercluster_types.go

+	// where the key is the server name and the value is the address. This map is dynamic and is updated every time a new control plane
+	// node is added or removed. The template will also support the JoinHostPort function to join the host and port of the backend server.
+	// +optional
+	UnsafeHAProxyConfigTemplateRef *corev1.LocalObjectReference `json:"unsafeHAProxyConfigTemplateRef,omitempty"`


Re-raising this as i think it got lost.

Not sure i like the use of Unsafe as that sounds scary 💀 Could we use something like CustomHAProxyConfigTemplateRef or CustomLoadBalancerConfigRef ?

I don't have a strong opinion here, not sure what other think

No big opinions on my side either :-) I think the unsafe comes from:

Let's [...] make sure to clearly document the contract about it: e.g. no validation, use at your own risk, etc.

[0]

There are a number of areas where there is no/little validation, and you can do lots of damage. For example PreKubeadmCommands.

If we have a warning in the documentation of the field that states "you better know what you are doing" then that's probably enough.

I agree with Richard. I think we don't need Unsafe in the field name

There are so many ways to misconfigure Cluster API easily in which it's totally impossible for us to validate. I think it's not necessary to additional communicate it here by adding an Unsafe prefix to the field.

chrischdi

/lgtm

pending the naming discussion :-)

k8s-ci-robot · 2023-07-12T13:38:24Z

LGTM label has been added.

Git tree hash: 6b2485fb36b25688ff1f397857d201b8b0c177e2

sbueringer · 2023-07-13T13:49:38Z

/lgtm

pending getting consensus on the field name (no strong opinion from my side)

fabriziopandini · 2023-07-19T10:42:35Z

/unassign

richardcase · 2023-07-19T15:32:15Z

Thanks @alexander-demicev for the name change :) For me:

/lgtm

k8s-ci-robot · 2023-07-19T15:32:21Z

LGTM label has been added.

Git tree hash: 616f0c5b39a6ea9067b2d43db9842dc741bd4f5f

fabriziopandini · 2023-07-21T09:57:23Z

/lgtm

sbueringer · 2023-07-21T12:07:06Z

Thank you very much!

/lgtm
/approve

k8s-ci-robot · 2023-07-21T12:07:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

g-gaston · 2023-10-23T15:47:35Z

/area provider/infrastructure-docker

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2023

k8s-ci-robot requested review from stmcginnis and ykakarap June 5, 2023 09:01

fabriziopandini reviewed Jun 14, 2023

View reviewed changes

richardcase reviewed Jun 22, 2023

View reviewed changes

fabriziopandini reviewed Jun 26, 2023

View reviewed changes

test/infrastructure/docker/internal/controllers/dockermachine_controller.go Outdated Show resolved Hide resolved

test/infrastructure/docker/internal/loadbalancer/config.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned fabriziopandini Jul 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 4, 2023

chrischdi reviewed Jul 6, 2023

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 8, 2023

k8s-ci-robot requested a review from fabriziopandini July 10, 2023 17:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2023

chrischdi reviewed Jul 11, 2023

View reviewed changes

richardcase reviewed Jul 11, 2023

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2023

chrischdi reviewed Jul 12, 2023

View reviewed changes

k8s-ci-robot assigned chrischdi Jul 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2023

k8s-ci-robot assigned sbueringer Jul 13, 2023

k8s-ci-robot unassigned fabriziopandini Jul 19, 2023

alexander-demicev added 2 commits July 19, 2023 17:18

Add custom ha proxy config to CAPD API

dd5d210

Replace HA proxy config with custom one if specified

6ac336a

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2023

k8s-ci-robot requested review from chrischdi and sbueringer July 19, 2023 15:25

k8s-ci-robot assigned richardcase Jul 19, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2023

k8s-ci-robot assigned fabriziopandini Jul 21, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2023

k8s-ci-robot merged commit de0ed1e into kubernetes-sigs:main Jul 21, 2023
19 checks passed

k8s-ci-robot added this to the v1.6 milestone Jul 21, 2023

alexander-demicev deleted the capdloadbalancer branch July 21, 2023 12:34

k8s-ci-robot added the area/provider/infrastructure-docker Issues or PRs related to the docker infrastructure provider label Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Allow adding custom HA proxy config for CAPD load balancer #8785

✨ Allow adding custom HA proxy config for CAPD load balancer #8785

alexander-demicev commented Jun 5, 2023

alexander-demicev commented Jun 5, 2023

fabriziopandini Jun 14, 2023

alexander-demicev commented Jun 22, 2023

richardcase Jun 22, 2023

richardcase Jun 22, 2023

fabriziopandini left a comment

fabriziopandini commented Jul 4, 2023

k8s-ci-robot commented Jul 4, 2023

chrischdi left a comment

alexander-demicev commented Jul 10, 2023

fabriziopandini commented Jul 10, 2023

fabriziopandini commented Jul 10, 2023

k8s-ci-robot commented Jul 10, 2023

chrischdi left a comment

richardcase Jul 11, 2023

alexander-demicev Jul 12, 2023

chrischdi Jul 12, 2023

richardcase Jul 12, 2023 •

edited

sbueringer Jul 13, 2023 •

edited

chrischdi left a comment

k8s-ci-robot commented Jul 12, 2023

sbueringer commented Jul 13, 2023

fabriziopandini commented Jul 19, 2023

richardcase commented Jul 19, 2023

k8s-ci-robot commented Jul 19, 2023

fabriziopandini commented Jul 21, 2023

sbueringer commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

g-gaston commented Oct 23, 2023

	AdditionalProxyConfigRef *corev1.LocalObjectReference `json:"additionalProxyConfigRef,omitempty"`
	UnsafeHAProxyConfigTemplateRef *corev1.LocalObjectReference `json:"additionalProxyConfigRef,omitempty"`

	// UnsafeHAProxyConfigTemplateRef allows you to replace the HAProxy config file.
	// UnsafeHAProxyConfigTemplateRef allows you to replace the default HAProxy config file.

✨ Allow adding custom HA proxy config for CAPD load balancer #8785

✨ Allow adding custom HA proxy config for CAPD load balancer #8785

Conversation

alexander-demicev commented Jun 5, 2023

alexander-demicev commented Jun 5, 2023

fabriziopandini Jun 14, 2023

Choose a reason for hiding this comment

alexander-demicev commented Jun 22, 2023

richardcase Jun 22, 2023

Choose a reason for hiding this comment

richardcase Jun 22, 2023

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini commented Jul 4, 2023

k8s-ci-robot commented Jul 4, 2023

chrischdi left a comment

Choose a reason for hiding this comment

alexander-demicev commented Jul 10, 2023

fabriziopandini commented Jul 10, 2023

fabriziopandini commented Jul 10, 2023

k8s-ci-robot commented Jul 10, 2023

chrischdi left a comment

Choose a reason for hiding this comment

richardcase Jul 11, 2023

Choose a reason for hiding this comment

alexander-demicev Jul 12, 2023

Choose a reason for hiding this comment

chrischdi Jul 12, 2023

Choose a reason for hiding this comment

richardcase Jul 12, 2023 • edited

Choose a reason for hiding this comment

sbueringer Jul 13, 2023 • edited

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 12, 2023

sbueringer commented Jul 13, 2023

fabriziopandini commented Jul 19, 2023

richardcase commented Jul 19, 2023

k8s-ci-robot commented Jul 19, 2023

fabriziopandini commented Jul 21, 2023

sbueringer commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

g-gaston commented Oct 23, 2023

richardcase Jul 12, 2023 •

edited

sbueringer Jul 13, 2023 •

edited