introduce load balancer API for capv #538
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yastij The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-rebase
yastij
force-pushed
the
vmc-v1a2
branch
3 times, most recently
from
8910a60 to
7dbaea2
Compare
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
size/XL
yastij
force-pushed
the
vmc-v1a2
branch
2 times, most recently
from
450b125 to
85f60e9
Compare
|
/retest |
|
Hi @yastij, I started my review yesterday, but didn't have time to complete it. One thing I noticed was that the types are in Example 1 ├── api
│ └── v1alpha2
│ ├── cloud
│ └── load-balancer
│ └── vmc
│ └── awsor Example 2 ├── api
│ ├── load-balancer
│ │ └── v1alpha1
│ │ └── vmc
│ │ └── aws
│ └── vsphere
│ └── v1alpha2
│ └── cloudNow obviously the second pattern would require not insignificant refactoring since it changes the location of the existing vSphere types, but, the second pattern does adhere to the Kubebuilder recommended guidelines for multi-group projects. Thoughts? |
api/v1alpha2/cloud/types.go
Outdated
| // LoadBalancerConfig describes the supported Load balancer providers | ||
| type LoadBalancerConfig struct { | ||
| // AwsProvider specifies the information needed to run VMC on AWS | ||
| AwsProvider *AwsProviderSpec `json:"awsProvider,omitempty"` |
There was a problem hiding this comment.
Hi @yastij,
Any field that is omitempty should also have a godoc of // +optional according to the OpenAPI guidelines.
api/v1alpha2/cloud/types.go
Outdated
| // VpcID is the id of the VPC used to create loadBalancers | ||
| VpcID string `json:"vpcID"` | ||
|
|
||
| // Subnets is the list of subnets where |
There was a problem hiding this comment.
Hi @yastij,
The godoc for Subnets is incomplete. Also, maybe rename the field to SubnetIDs or SubnetARNs?
| // LoadBalancerFinalizer allows to clean up the load balancer | ||
| // associated with VSphereCluster before removing it from the | ||
| // API server. | ||
| LoadBalancerFinalizer = "loadbalancer.infrastructure.cluster.x-k8s.io" |
There was a problem hiding this comment.
Hi @yastij,
There could technically be multiple load balancers, right? Should the LB finalizer be specific to the type of LB used?
There was a problem hiding this comment.
I'm not sure, the finalizer is added to the cluster object. The cluster object only supports one provider.
api/v1alpha2/vspherecluster_types.go
Outdated
|
|
||
| // LoadBalancerConfiguration holds a provider-specific configuration to provision | ||
| // a Load balancer as a control plane endpoint | ||
| LoadBalancerConfiguration *cloud.LoadBalancerConfig `json:"loadBalancerConfiguration,omitempty"` |
There was a problem hiding this comment.
Hi @yastij,
Any field that is omitempty should also have a godoc of // +optional according to the OpenAPI guidelines. The reason the CloudProviderConfiguration doesn't is because it's not a pointer, and thus is never technically empty.
| @@ -0,0 +1,142 @@ | |||
| package controllers | |||
There was a problem hiding this comment.
Hi @yastij,
I'm wondering if the load balancer controller shouldn't be in a new sub-directory:
├── controllers
└── load-balancerI'm just thinking of things to make it easier to move the work in this PR to a separate module or repository at a later date. If we're already importing the LB types/controller from inside this repo, it makes it easier to move it later since we'll just rewrite imports.
| } | ||
|
|
||
| func (svc *Service) reconcileLoadBalancer(clusterName string, subnets []string) (*string, *string, error) { | ||
| var loadBalancerArn *string |
There was a problem hiding this comment.
Hi @yastij,
Please simplify as:
var (
loadBalancerArn *string
loadBalancerDNS *string
describeLoadBalancersInput = &elbv2.DescribeLoadBalancersInput{
Names: []*string{aws.String(generateELBName(clusterName))},
}
)| } | ||
|
|
||
| func (svc *Service) reconcileListeners(loadBalancerArn *string, targetGroupArn *string) (*int64, error) { | ||
| var listenerPort *int64 |
There was a problem hiding this comment.
Hi @yastij,
Please simplify as:
var (
listenerPort *int64
describeListnerInput = &elbv2.DescribeListenersInput{
LoadBalancerArn: loadBalancerArn,
}
)| } | ||
|
|
||
| func (svc *Service) deleteTargetGroup(clusterName string) error { | ||
| describeTargetGroupInput := &elbv2.DescribeTargetGroupsInput{} |
There was a problem hiding this comment.
Hi @yastij,
Please simplify as:
describeTargetGroupInput := &elbv2.DescribeTargetGroupsInput{
Names: []*string{aws.String(clusterName+"-controlPlane")},
}Also, please note that I recommended above to extract the building of this name into a distinct function.
|
|
||
| func (svc *Service) reconcileTargetGroup(clusterName string, vpcID string, controlPlaneIPs []string) (*string, error) { | ||
| describeTargetGroupInput := &elbv2.DescribeTargetGroupsInput{} | ||
| describeTargetGroupInput.Names = append(describeTargetGroupInput.Names, aws.String(clusterName+"-controlPlane")) |
There was a problem hiding this comment.
Hi @yastij,
I've seen aws.String(clusterName+"-controlPlane") at least twice. Due to the fact that this is required to find a resource, let's extract this into a function:
// GetTargetGroupNameForCluster returns the name of a target group for the provided cluster.
func GetTargetGroupNameForCluster(clusterName string) *string {
return aws.String(clusterName+"-controlPlane")
}| return err | ||
| } | ||
|
|
||
| if err := svc.deleteTargetGroup(clusterName); err != nil { |
There was a problem hiding this comment.
Hi @yastij,
This could be simplified as:
return svc.deleteTargetGroup(clusterName)Or please feel free to leave as-is if you think we may include additional logic beneath this call one day.
k8s-ci-robot
added
the
do-not-merge/hold
yastij
force-pushed
the
vmc-v1a2
branch
4 times, most recently
from
c0e10a0 to
90a023e
Compare
k8s-ci-robot
removed
the
needs-rebase
yastij
force-pushed
the
vmc-v1a2
branch
2 times, most recently
from
9b7c7a3 to
cbb90be
Compare
|
/retest |
yastij
force-pushed
the
vmc-v1a2
branch
6 times, most recently
from
d2f8fb6 to
d8a9817
Compare
api/v1alpha2/loadbalancer/types.go
Outdated
|
|
||
| const ( | ||
| // AwsProvider is the name of the aws provider | ||
| AwsProvider = "aws" |
There was a problem hiding this comment.
I think the provider name should be explicit to the LB implementation, not the cloud provider, so this should be "ELB", thoughts?
There was a problem hiding this comment.
actually this should be removed. As the current PR uses Kind to store/retrieve the provider
api/v1alpha2/loadbalancer/types.go
Outdated
| // +kubebuilder:subresource:status | ||
|
|
||
| // AWSLoadBalancer is the schema for the AWS Load balancer API | ||
| type AWSLoadBalancer struct { |
There was a problem hiding this comment.
Shouldn't the API types be generic and the implementations be provider specific?
There was a problem hiding this comment.
No. Think of this as a KubeadmBootstrap provider. There will be a diff controller / API model for each implementation of the LB.
There was a problem hiding this comment.
The issue is that there's no common API for loadbalancers, as each provider needs its own information. I don't want us to endup implementing annotations on-top to satisfy each provider's specifics.
Another way could be store the information on-disk/cm implement something Service (e.g. MachineService or MachineLoadbalancer) which would select machines based on a the selector and add them to the load balancing pool. A drawback is that a management cluster is restricted to provision against one Loadbalancing provider. Thoughts @akutz @andrewsykim ?
There was a problem hiding this comment.
I don't think it's that complicated. Just like each Cluster resource today has a ConfigRef to an Infrastructure provider's Cluster, so to will there be a ConfigRef from a Cluster to a load balancer config, in this case, AWSLoadBalancerConfig.
There's some questions around how to get the IP information from the machines, but that's something we can work out.
The benefit of this decoupled model is that it enables the introduction of a load balancer provider of any kind.
There was a problem hiding this comment.
Yes, plus this wouldn't change much the implementation itself. thoughts @andrewsykim ?
There was a problem hiding this comment.
So my thoughts around this was that we introduce a generic LoadBalancer type with a ProviderRef field similar to what @akutz mentioned re: Cluster with a ConfigRef. That way, for each implementation you only have to implement a new ProviderRef and not the entire LoadBalancer API again. Maybe we already agree on this but it's not clear to me yet.
yastij
force-pushed
the
vmc-v1a2
branch
2 times, most recently
from
6f6718f to
794c59c
Compare
|
/retest |
Signed-off-by: Yassine TIJANI <[email protected]>
Signed-off-by: Yassine TIJANI <[email protected]>
Signed-off-by: Yassine TIJANI <[email protected]>
Signed-off-by: Yassine TIJANI <[email protected]>
| // APIEndpoint represents the endpoint to communicate with the load | ||
| // balancer. | ||
| // +optional | ||
| APIEndpoint APIEndpoint `json:"apiEndpoint,omitempty"` |
There was a problem hiding this comment.
Not doing a full review on this right now (unless you'd like), but one thing I wanted to mention was the API endpoint needs to be in spec, not status. If you have a provider that generates an endpoint (e.g. a DNS name) and the endpoint is not something you can determine by querying, we would lose the data if we store it in status and we're trying to move the resource from one management cluster to another.
There was a problem hiding this comment.
@ncdc - I'll rebase and ping you for an API review
|
@yastij: PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
@yastij: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
/close |
|
@akutz: Closed this PR. In response to this:
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. |
* Make sure ubuntu gets proper version of cloud-init * Add Goss test * Use packer-goss provisioner to execute Goss tests * Add packer docs * Add link to packer documentation in README file. * Add link to packer-goss * Add Ansible as prerequisite for packer
5 participants
What this PR does / why we need it: This PR introduces a load balancing API to implement in-tree load balacing providers. This PR ships AWS as a first provider
Which issue(s) this PR fixes : Fixes #468
Special notes for your reviewer:
what is missing
loadbalancerRefkustomization and anAWSLoadBalancerresource/assign @akutz
/cc @andrewsykim
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: