feat: making control plane endpoint fields optional #106

prometherion · 2024-02-09T11:28:56Z

Issue #95

Description of changes:

Skipping webhook validation for the ProxmoxCluster Control Plane endpoint field.

The ProxmoxCluster controller will wait for a valid IP before proceeding to mark the infrastructure ready.

Testing performed:

N.A.

sonarcloud · 2024-02-09T11:31:22Z

Quality Gate failed

Failed conditions
20.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

prometherion · 2024-02-09T14:41:54Z

Before providing more test coverage, may I ask for a simple review of the proposed changes, from a workflow perspective?

It's not clear to me if the maintainers are open to skipping the validation of the ProxmoxCluster.spec.controlPlaneEnpdoint field.

avorima · 2024-02-19T17:00:44Z

Hi @prometherion, thank you for your contribution. Skipping validation for optional fields is fine, but please make sure your changes don't alter the validation behavior of other fields. Users should see errors as early as possible.
The controller checks for empty control plane endpoint should be done after IPAM was reconciled, so that they're not waiting for the external process to set the control plane endpoint.

Does Kamaji set the port and address separately? I'm asking, because my understanding was that the endpoint is always written in full or not at all. This would mean that you could merge the two conditions into one MissingControlPlaneEndpoint.

prometherion · 2024-02-22T14:12:40Z

Does Kamaji set the port and address separately?

Kamaji is doing in a single transaction, yes, I can uniform those checks.

mcbenjemaa · 2024-02-28T15:43:06Z

Anything needed here?

prometherion · 2024-02-28T16:46:22Z

Thanks for the heads up @mcbenjemaa, I'm planning to work on this to make the PR ready for review, by the end of the week or the following one.

Pretty busy days, sorry.

mcbenjemaa · 2024-02-29T12:24:19Z

take your time mate

mcbenjemaa · 2024-04-30T08:07:49Z

@prometherion
Are there any updates on this, or can I try it

prometherion · 2024-05-27T15:30:34Z

Finally, I'm revamping it, feeling sorry for being late @mcbenjemaa.

Let me know if we're ready to get this tested.

mcbenjemaa · 2024-05-28T09:27:13Z

can you provide a use-case to test it with Kamaji?

internal/controller/proxmoxcluster_controller.go

Signed-off-by: Dario Tranchitella <dario@tranchitella.eu>

sonarcloud · 2024-06-07T14:35:20Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
86.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

wikkyk

I think this is close to done. The core is there but I'm not sold on the details.. This could use some test cases which would've uncovered the inconsistency I pointed out inline.

However, I don't think I like the 'magic' behaviour of empty host and(?) port meaning externally managed. Someone could omit host/port by accident without actually intending to use Kamaji or so. This needs very clear documentation. (Technically, this doesn't actually make the fields optional (-:)

Personally, I would prefer an optional bool field in ProxmoxClusterSpec like ControlPlaneEndpointExternallyManaged and to require that either ControlPlaneEndpointExternallyManaged or ControlPlaneEndpoint is set. This would make the intent clear.

That said, I would be fine with just clearly, explicitly documenting that setting host="" and port=0 means we'll wait for an externally managed endpoint. The check in proxmoxcluster_controller.go would need to be exactly the same as in the validation func i.e. host=="" && port==0.

wikkyk · 2024-06-10T07:33:40Z

internal/webhook/proxmoxcluster_webhook.go

-	infrav1 "github.com/ionos-cloud/cluster-api-provider-proxmox/api/v1alpha1"
 	"github.com/pkg/errors"
 	"go4.org/netipx"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	infrav1 "github.com/ionos-cloud/cluster-api-provider-proxmox/api/v1alpha1"


I think this is a spurious change.

It's my IDE sorting go imports, I'll amend it.

wikkyk · 2024-06-10T07:36:56Z

internal/webhook/proxmoxcluster_webhook.go

+	// Skipping the validation of the Control Plane endpoint in case of an empty value:
+	// this is the case of externally managed Control Plane which eventually provides the LB.
+	if ep.Host == "" && ep.Port == 0 {
+		return nil
+	}


What about the case where someone accidentally doesn't provide the control plane endpoint?

wikkyk · 2024-06-10T07:48:50Z

internal/webhook/proxmoxcluster_webhook.go

@@ -94,6 +95,11 @@ func (*ProxmoxCluster) ValidateUpdate(_ context.Context, _ runtime.Object, newOb

 func validateControlPlaneEndpoint(cluster *infrav1.ProxmoxCluster) error {
 	ep := cluster.Spec.ControlPlaneEndpoint
+	// Skipping the validation of the Control Plane endpoint in case of an empty value:
+	// this is the case of externally managed Control Plane which eventually provides the LB.
+	if ep.Host == "" && ep.Port == 0 {


This could use some test cases.

Also, default Port is 6443.

wikkyk · 2024-06-10T07:50:41Z

internal/controller/proxmoxcluster_controller.go

@@ -168,6 +168,22 @@ func (r *ProxmoxClusterReconciler) reconcileNormal(ctx context.Context, clusterS
 	// If the ProxmoxCluster doesn't have our finalizer, add it.
 	ctrlutil.AddFinalizer(clusterScope.ProxmoxCluster, infrav1alpha1.ClusterFinalizer)

+	cpe := clusterScope.ControlPlaneEndpoint()
+	switch {
+	case cpe.Host == "":


This only checks host but not port, yet the validation func checks both.

prometherion · 2024-06-10T10:07:38Z

However, I don't think I like the 'magic' behaviour of empty host and(?) port meaning externally managed. Someone could omit host/port by accident without actually intending to use Kamaji or so. This needs very clear documentation. (Technically, this doesn't actually make the fields optional (-:)

Documentation is a great place, of course, wondering if we could implement this kind of check in a different way.

The Cluster API has a contract for an externally managed control plane thanks to the status key externalManagedControlPlane.

When a user is open to using Kamaji, or any other Control Plane provider which is satisfying that status key contract, we could skip the validation requiring a filled ControlPlaneEndpoint struct. With this, we're creating more guardrails for users that otherwise could shoot themselves in the foot.

wikkyk · 2024-06-11T05:28:06Z

Two points, then:

We should set externalManagedControlPlane in the status when selected
I like it when things are explicit (or I don't like magic values) so I'd opt for a new key in ProxmoxClusterSpec (we can name it ExternalManagedControlPlane to stay consistent) and the user either sets it to true or populates ControlPlaneEndpoint and we validate that only one of those is set.

prometherion requested review from 65278, avorima, lubedacht, pborn-ionos, mcbenjemaa and wikkyk as code owners February 9, 2024 11:28

wikkyk added this to the v0.4.0 milestone Feb 22, 2024

wikkyk force-pushed the main branch from fd38117 to 72d5796 Compare April 3, 2024 13:22

wikkyk removed this from the v0.4.0 milestone Apr 17, 2024

wikkyk force-pushed the main branch from f16632d to e057b80 Compare April 29, 2024 10:42

prometherion force-pushed the issues/95 branch from 60f2608 to bdb982a Compare May 27, 2024 15:25

prometherion temporarily deployed to e2e May 27, 2024 15:25 — with GitHub Actions Inactive

avorima reviewed May 28, 2024

View reviewed changes

internal/controller/proxmoxcluster_controller.go Show resolved Hide resolved

prometherion force-pushed the issues/95 branch from bdb982a to 63dd136 Compare May 31, 2024 12:14

prometherion requested a deployment to e2e May 31, 2024 12:14 — with GitHub Actions Waiting

feat: making control plane endpoint fields optional

e2b7a89

Signed-off-by: Dario Tranchitella <dario@tranchitella.eu>

wikkyk force-pushed the issues/95 branch from 63dd136 to e2b7a89 Compare June 7, 2024 14:33

wikkyk temporarily deployed to e2e June 7, 2024 14:33 — with GitHub Actions Inactive

wikkyk requested changes Jun 10, 2024

View reviewed changes

wikkyk added this to the v0.6.0 milestone Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: making control plane endpoint fields optional #106

feat: making control plane endpoint fields optional #106

prometherion commented Feb 9, 2024

sonarcloud bot commented Feb 9, 2024

prometherion commented Feb 9, 2024

avorima commented Feb 19, 2024

prometherion commented Feb 22, 2024

mcbenjemaa commented Feb 28, 2024

prometherion commented Feb 28, 2024

mcbenjemaa commented Feb 29, 2024

mcbenjemaa commented Apr 30, 2024

prometherion commented May 27, 2024

mcbenjemaa commented May 28, 2024

sonarcloud bot commented Jun 7, 2024

wikkyk left a comment •

edited

wikkyk Jun 10, 2024

prometherion Jun 10, 2024

wikkyk Jun 10, 2024

wikkyk Jun 10, 2024

wikkyk Jun 10, 2024

wikkyk Jun 10, 2024

prometherion commented Jun 10, 2024

wikkyk commented Jun 11, 2024

feat: making control plane endpoint fields optional #106

Are you sure you want to change the base?

feat: making control plane endpoint fields optional #106

Conversation

prometherion commented Feb 9, 2024

sonarcloud bot commented Feb 9, 2024

Quality Gate failed

prometherion commented Feb 9, 2024

avorima commented Feb 19, 2024

prometherion commented Feb 22, 2024

mcbenjemaa commented Feb 28, 2024

prometherion commented Feb 28, 2024

mcbenjemaa commented Feb 29, 2024

mcbenjemaa commented Apr 30, 2024

prometherion commented May 27, 2024

mcbenjemaa commented May 28, 2024

sonarcloud bot commented Jun 7, 2024

Quality Gate passed

wikkyk left a comment • edited

Choose a reason for hiding this comment

wikkyk Jun 10, 2024

Choose a reason for hiding this comment

prometherion Jun 10, 2024

Choose a reason for hiding this comment

wikkyk Jun 10, 2024

Choose a reason for hiding this comment

wikkyk Jun 10, 2024

Choose a reason for hiding this comment

wikkyk Jun 10, 2024

Choose a reason for hiding this comment

wikkyk Jun 10, 2024

Choose a reason for hiding this comment

prometherion commented Jun 10, 2024

wikkyk commented Jun 11, 2024

wikkyk left a comment •

edited