Add loadbalancer field to Power VS machine provider spec

[Karthik-K-N]

This PR adds LoadBalancers filed to Power VS spec.
Usage:

1. Installer will set the loadbalaner name in machine spec for control-plane machines
2. Power VS machine api provider will use it to add or remove created/deleted instance to loadbalancer pool
3. This will help us in providing support for cluster-control-plane-machine-set-operator

[openshift-ci]

Hello @Karthik-K-N! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

[Karthik-K-N]

@mkumatag @JoelSpeed could you please take a look and help in reviewing this.

[JoelSpeed]

Should start with the json tag name not the Go field name, is there any way to restrict this? For example can we explain what character set, minimum/maximum lengths are required for this field?

Suggested change

	// Name of the LoadBalancer in IBM Cloud VPC
	// It is a reference to existing LoadBalancer created by openshift installer component.
	// +kubebuilder:validation:Required
	Name string `json:"name"`
	// name of the LoadBalancer in IBM Cloud VPC
	// It is a reference to existing LoadBalancer created by openshift installer component.
	// +kubebuilder:validation:Required
	Name string `json:"name"`

[Karthik-K-N]

sure , not sure how can we restrict it, Currently the loadbalancer name will be of format "{cluster-id}-loadbalancer" for public and "{cluster-id}-loadbalancer-int" for internal loadbalancer

[JoelSpeed]

Is cluster-id restricted at all by the installer? We can probably restrict this to alphanumeric characters, hyphens and underscores, but we should check what restrictions are imposed by the installer

[Karthik-K-N]

Apologies we are using infraID not clusterid, Infra ID is generated like this: https://github.com/Karthik-K-N/installer/blob/e91df626c10e569e7613249053b7b9b264db42df/pkg/asset/installconfig/clusterid.go#L60-L79

I think we can add validation for alphanumeric with hyphen,

[JoelSpeed]

Yeah looks like it can be limited to alphanum and hyphen, great!

[JoelSpeed]

Suggested change

	// Currently only Application LoadBalancer is supported
	// More details about Application LoadBalancer
	// https://cloud.ibm.com/docs/vpc?topic=vpc-load-balancers-about&interface=ui
	// Currently only the Application LoadBalancer is supported.
	// More details about Application LoadBalancer
	// https://cloud.ibm.com/docs/vpc?topic=vpc-load-balancers-about&interface=ui.
	// Supported values are Application.

[Karthik-K-N]

Updated.

[JoelSpeed]

Lets add a kubebuilder validation for the alpha numeric characters and hyphen, then we will want to explain this limitation within the godoc, and also, we should probably set a maximum length, is there anything in IBM that limits the length or the characters?

[JoelSpeed]

I found the following in the docs, how wil we handle this? Convert any hyphen to an underscore internally? How will we limit the name length given the infra-id may be 27 characters, any thoughts?

Names may not be empty. A valid load balancer name starts with a letter, followed by letters, digits, or underscores. The length of the name may not exceed 40 characters.

[Karthik-K-N]

Loadbalancer name may contain hyphens, Currently this is how the installer creates the loadbalancer rdr-kn08031125-vhwtg-loadbalancer.
From api doc I can see this description for name

The globally unique name for this load balancer profile

Possible values: 1 ≤ length ≤ 63, Value must match regular expression ^([a-z]|[a-z][-a-z0-9]*[a-z0-9]|[0-9][-a-z0-9]*([a-z]|[-a-z][-a-z0-9]*[a-z0-9]))$

Example: network-fixed

[JoelSpeed]

Great, thanks for finding that, can you explain this in prose within the godoc please? We need to say something along the lines of

// The name should be between 1 and 63 characters long and may consist of lowercase alphanumeric characters and hyphens only.
// The value must not end with a hyphen and integer values are not allowed.

I think I've understood that regex correctly anyway

[Karthik-K-N]

If we can just add a touch more documentation to this name field as per my suggestion I think this is good to go

Updated accordingly, Please take a look. Thank you.

[JoelSpeed]

If we can just add a touch more documentation to this name field as per my suggestion I think this is good to go

[JoelSpeed]

Great, thanks for finding that, can you explain this in prose within the godoc please? We need to say something along the lines of

// The name should be between 1 and 63 characters long and may consist of lowercase alphanumeric characters and hyphens only.
// The value must not end with a hyphen and integer values are not allowed.

I think I've understood that regex correctly anyway

[JoelSpeed]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, Karthik-K-N

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [JoelSpeed]

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

[openshift-ci]

@Karthik-K-N: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.