Add validation for loadbalancer port against 6443 and to avoid duplicate names #1746

KeerthanaAP · 2024-04-24T09:58:30Z

Added validation for loadbalancer port against 6443 as its already used, kubebuilder tag to avoid duplicate ports and validation to avoid duplicate loadbalancer, vpcsubnet names.

Validation output:

The IBMPowerVSCluster "capi-powervs-keerthana" is invalid:
* spec.vpcSubnets[1]: Duplicate value: map[string]interface {}{"Name":"capi-powervs-keerthana-vpcsubnet"}
* spec.loadbalancers[2]: Duplicate value: map[string]interface {}{"Name":"capi-powervs-keerthana-loadbalancer2"}
* spec.loadbalancers[3]: Duplicate value: map[string]interface {}{"Name":"capi-powervs-keerthana-loadbalancer1"}
* spec.loadbalancers[0].additionalListeners[0].port: Invalid value: 6443: value of port should not be equal to APIServerPort (6443)

What this PR does / why we need it:

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 #1663

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Add validation for loadbalancer port against 6443 and to avoid duplicate names.

k8s-ci-robot · 2024-04-24T09:58:39Z

Hi @KeerthanaAP. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2024-04-24T09:58:45Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`643d878`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/665d7583ed04cf00084fddf6
😎 Deploy Preview	https://deploy-preview-1746--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Karthik-K-N

/ok-to-test

KeerthanaAP · 2024-04-25T05:19:13Z

/retest-required

Prajyot-Parab · 2024-04-29T05:13:39Z

ptal @Karthik-K-N

Karthik-K-N · 2024-04-29T05:49:53Z

api/v1beta2/ibmvpccluster_types.go

@@ -81,6 +81,7 @@ type VPCLoadBalancerSpec struct {
 	// +listType=map
 	// +listMapKey=port
 	// +optional
+	// ++kubebuilder:validation:UniqueItems=true


Do mind sharing some reference about this tag usage.

Since its slice and for now we have only Port within it. what should be unique, Is port? in future we add another field name say PortName along with port then what should be unique is it port or port and portName combination?

// +kubebuilder:validation:UniqueItems = <bool> specifies that all items in this list must be unique.

As mentioned it validates the ports. All the ports should be unique. if we add another field like PortName, it will fail only if both the fields (portName and port) are same.

api/v1beta2/ibmpowervscluster_webhook.go

Karthik-K-N

Overall LGTM Thanks, Is there any way to improve the error message

spec.vpcSubnets[1]: Duplicate value: map[string]interface {}{"Name":"capi-powervs-keerthana-vpcsubnet"}

to remove map[string]interface {}

KeerthanaAP · 2024-05-06T17:41:23Z

Overall LGTM Thanks, Is there any way to improve the error message
spec.vpcSubnets[1]: Duplicate value: map[string]interface {}{"Name":"capi-powervs-keerthana-vpcsubnet"}
to remove map[string]interface {}

I followed the same error msg format that we get when we apply the kubebuilder tag ++kubebuilder:validation:UniqueItems=true

Example:
The IBMPowerVSCluster "capi-powervs-keerthana" is invalid: spec.loadBalancers[1].additionalListeners[1]: Duplicate value: map[string]interface {}{"port":6442}

or
we can print it as a string as mentioned below

The IBMPowerVSCluster "capi-powervs-keerthana" is invalid: 
* spec.vpcSubnets[1]: Duplicate value: "{Name: capi-powervs-keerthana-vpcsubnet}"

Karthik-K-N · 2024-05-07T03:07:07Z

Thanks for clarifying
/lgtm

KeerthanaAP · 2024-05-08T05:57:02Z

@mkumatag @Prajyot-Parab ptal.

Prajyot-Parab · 2024-05-08T09:18:13Z

@mkumatag @Prajyot-Parab ptal.

update release notes field please.

KeerthanaAP · 2024-05-10T05:31:09Z

@mkumatag @Prajyot-Parab ptal.

update release notes field please.

Done. Updated the release notes.

mkumatag · 2024-05-14T04:23:21Z

api/v1beta2/ibmpowervscluster_webhook.go

+func (r *IBMPowerVSCluster) validateIBMPowerVSClusterLoadBalancerPort() (allErrs field.ErrorList) {
+	for i, loadbalancer := range r.Spec.LoadBalancers {
+		for j, listerner := range loadbalancer.AdditionalListeners {
+			if listerner.Port == int64(DefaultAPIServerPort) {


what if this port is set Cluster.Spec.ClusterNetwork.APIServerPort ? will this comparison with DefaultAPIServerPort still hold good?

No, it won't hold good, if user has set Cluster.Spec.ClusterNetwork.APIServerPort we should make sure that that port is not set here in listner.
We usually find the apiserver port like this

cluster-api-provider-ibmcloud/cloud/scope/powervs_cluster.go

Lines 336 to 342 in 81e93e2

// APIServerPort returns the APIServerPort to use when creating the ControlPlaneEndpoint.

func (s *PowerVSClusterScope) APIServerPort() int32 {

if s.Cluster.Spec.ClusterNetwork != nil && s.Cluster.Spec.ClusterNetwork.APIServerPort != nil {

return *s.Cluster.Spec.ClusterNetwork.APIServerPort

}

return infrav1beta2.DefaultAPIServerPort

}

we dont have access to cluster resource here in webhook, So we need verify this one scenario in controller code only.

then adding this check here for the DefaultAPIServerPort may mislead the user, lets drop this change here and introduce in the controller itself

I think most of the time the apiserver will be running in 6443 only, so i think we can keep it here and can add a check in controller only if the apiserver port is not 6443.

Sure. Will add this as error msg.

Modified the error msg.
Updated output:

Conditions: Last Transition Time: 2024-05-27T17:01:33Z Message: capi-powervs-keerthana-new-loadbalancer2 port (6443) cannot be used as additional Listener port value, as its already configured by system for default listener Reason: LoadBalancerReconciliationFailed

Nice, Lets hear others opinion on as well.

how about this one - #1746 (comment)?

Looks good. As the error strings should not be capitalized, Can we update it as below.

Conditions: Last Transition Time: 2024-05-29T07:03:15Z Message: port 6443 for the capi-powervs-keerthana-new-loadbalancer2 load balancer cannot be used as an additional listener port, as it is already assigned to the API server Reason: LoadBalancerReconciliationFailed

mkumatag · 2024-05-21T15:01:21Z

@KeerthanaAP can you make the changes by tomorrow sometime? we are planning to cut a release soon

cloud/scope/powervs_cluster.go

…o avoid duplicates.

mkumatag

/lgtm

k8s-ci-robot · 2024-06-04T05:53:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KeerthanaAP, mkumatag

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~~ [mkumatag]

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

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Apr 24, 2024

k8s-ci-robot requested review from Karthik-K-N and mkumatag April 24, 2024 09:58

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 24, 2024

Karthik-K-N reviewed Apr 24, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2024

KeerthanaAP force-pushed the loadbalancer_validation branch from 433e48c to 1f97063 Compare April 25, 2024 05:18

Karthik-K-N reviewed Apr 29, 2024

View reviewed changes

KeerthanaAP force-pushed the loadbalancer_validation branch from 1f97063 to 5079456 Compare April 30, 2024 15:42

Karthik-K-N reviewed May 2, 2024

View reviewed changes

KeerthanaAP force-pushed the loadbalancer_validation branch from 5079456 to bb43dd2 Compare May 6, 2024 17:22

k8s-ci-robot assigned Karthik-K-N May 7, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2024

mkumatag requested changes May 14, 2024

View reviewed changes

mkumatag added this to the v0.8 milestone May 17, 2024

KeerthanaAP force-pushed the loadbalancer_validation branch from bb43dd2 to 94dbb22 Compare May 20, 2024 08:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2024

KeerthanaAP force-pushed the loadbalancer_validation branch 2 times, most recently from 572b09e to d8e1605 Compare May 22, 2024 09:48

mkumatag modified the milestones: v0.8, v0.8.x May 24, 2024

KeerthanaAP force-pushed the loadbalancer_validation branch 2 times, most recently from c58e4da to af434d1 Compare May 27, 2024 16:59

mkumatag requested changes May 29, 2024

View reviewed changes

cloud/scope/powervs_cluster.go Outdated Show resolved Hide resolved

Added webhook for validating port against 6443 and kubebuilder tags t…

643d878

…o avoid duplicates.

KeerthanaAP force-pushed the loadbalancer_validation branch from af434d1 to 643d878 Compare June 3, 2024 07:49

mkumatag approved these changes Jun 4, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag Jun 4, 2024

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2024

k8s-ci-robot merged commit ff78637 into kubernetes-sigs:main Jun 4, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for loadbalancer port against 6443 and to avoid duplicate names #1746

Add validation for loadbalancer port against 6443 and to avoid duplicate names #1746

KeerthanaAP commented Apr 24, 2024 •

edited

k8s-ci-robot commented Apr 24, 2024

netlify bot commented Apr 24, 2024 •

edited

Karthik-K-N left a comment

KeerthanaAP commented Apr 25, 2024

Prajyot-Parab commented Apr 29, 2024

Karthik-K-N Apr 29, 2024

KeerthanaAP May 2, 2024

Karthik-K-N left a comment

KeerthanaAP commented May 6, 2024 •

edited

Karthik-K-N commented May 7, 2024

KeerthanaAP commented May 8, 2024

Prajyot-Parab commented May 8, 2024

KeerthanaAP commented May 10, 2024

mkumatag May 14, 2024

Karthik-K-N May 14, 2024

Karthik-K-N May 15, 2024

mkumatag May 15, 2024

Karthik-K-N May 16, 2024

KeerthanaAP May 27, 2024

KeerthanaAP May 27, 2024

Karthik-K-N May 28, 2024

mkumatag May 29, 2024

KeerthanaAP May 29, 2024

mkumatag commented May 21, 2024

mkumatag left a comment

k8s-ci-robot commented Jun 4, 2024

	// APIServerPort returns the APIServerPort to use when creating the ControlPlaneEndpoint.
	func (s *PowerVSClusterScope) APIServerPort() int32 {
	if s.Cluster.Spec.ClusterNetwork != nil && s.Cluster.Spec.ClusterNetwork.APIServerPort != nil {
	return *s.Cluster.Spec.ClusterNetwork.APIServerPort
	}
	return infrav1beta2.DefaultAPIServerPort
	}

Add validation for loadbalancer port against 6443 and to avoid duplicate names #1746

Add validation for loadbalancer port against 6443 and to avoid duplicate names #1746

Conversation

KeerthanaAP commented Apr 24, 2024 • edited

k8s-ci-robot commented Apr 24, 2024

netlify bot commented Apr 24, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Karthik-K-N left a comment

Choose a reason for hiding this comment

KeerthanaAP commented Apr 25, 2024

Prajyot-Parab commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karthik-K-N left a comment

Choose a reason for hiding this comment

KeerthanaAP commented May 6, 2024 • edited

Karthik-K-N commented May 7, 2024

KeerthanaAP commented May 8, 2024

Prajyot-Parab commented May 8, 2024

KeerthanaAP commented May 10, 2024

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

mkumatag commented May 21, 2024

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 4, 2024

KeerthanaAP commented Apr 24, 2024 •

edited

netlify bot commented Apr 24, 2024 •

edited

KeerthanaAP commented May 6, 2024 •

edited