Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to specify a configurable VIP network for loadbalancer #1809

Closed
oblazek opened this issue Jan 3, 2024 · 13 comments · Fixed by #1922
Closed

Add the ability to specify a configurable VIP network for loadbalancer #1809

oblazek opened this issue Jan 3, 2024 · 13 comments · Fixed by #1922
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@oblazek
Copy link
Contributor

oblazek commented Jan 3, 2024

/kind feature

Describe the solution you'd like

Based on an API documentation of openstack/octavia it should be possible to specify a VIP network for the loadbalancer:

Provide a vip_port_id.
Provide a vip_network_id.
Provide a vip_subnet_id.

which is possible in the openstack API

and used in CAPO here, but is strictly filled by this -> openStackCluster.Status.Network.Subnets[0].ID.

Unfortunately this is expecting that loadbalancer is supposed to be created on the same network/subnet as the VM itself which is not always the correct assumption. In our case we have a separate network for the loadbalancer from which the VIP should be allocated and a separate network for the control plane and worker nodes (VMs).
This is a setup with octavia and our own specific cloud provider, so there are no amphoras nor floating IPs (the openstack is using calico as network driver).

So either OpenStackCluster and/or OpenStackMachineTemplate should have the possibility to specify which network/subnet ID should be used just for the VIP/loadbalancer separately from OpenStackMachineSpec.network or OpenStackMachineSpec.subnet. E.g. something like, but not strictly in this way:

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha7
kind: OpenStackCluster
metadata:
  name: test2
  namespace: test2
spec:
  cloudName: ulab1-test-infra8
  controlPlaneAvailabilityZones:
  - az1
  - az2
  - az3
  identityRef:
    kind: Secret
    name: test2-cloud-config
  apiServerLoadBalancer:
    enabled: true
+    network: <network_id_or_tag_or_name>
  disableAPIServerFloatingIP: true
  managedSecurityGroups: true
  network:
    name: ulab1
  subnet:
    name: ulab1-ipv4-1

Anything else you would like to add:
Partly similar issue is: #1558 and related thread https://kubernetes.slack.com/archives/CFKJB65G9/p1702562005631379

cc @mdbooth

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 3, 2024
@oblazek
Copy link
Contributor Author

oblazek commented Jan 3, 2024

similarly to this feature request this is possible in OCCM https://github.com/kubernetes/cloud-provider-openstack/blob/fdba36babb2c4b46e759c99cca50ac7eba2ee06f/pkg/openstack/openstack.go#L103 via configuration:

[LoadBalancer]
lb-provider=labrador
subnet-id=5b4e2c9b-1ab9-4510-87a4-6d2301e16e77
internal-lb=true
provider-requires-serial-api-calls=true

@mdbooth
Copy link
Contributor

mdbooth commented Jan 3, 2024

To be clear, you want the control plane machines to have 2 attached interfaces:

  • Loadbalancer VIP network
  • Control plane network

You want the API servers to listen on the LB VIP network. You want kubelet to listen on the control plane network.

Or do you mean that the control plane machines should only be connected to the control plane network and the LB on the VIP network should add the port on the control plane network, which is a different network, as a member? Does that work?

@oblazek
Copy link
Contributor Author

oblazek commented Jan 3, 2024

Basically the VMs only need 1 interface as they are routable across the whole DC. The VIP network is only a subnet within the routable network.

To be clear, you want the control plane machines to have 2 attached interfaces:

No, they only need to be attached on the Control plane network

Or do you mean that the control plane machines should only be connected to the control plane network

yes

and the LB on the VIP network should add the port on the control plane network, which is a different network as a member

yes, because both networks are routable within the DC (we have a flat L3 network)

@oblazek
Copy link
Contributor Author

oblazek commented Jan 3, 2024

the VIP network (in our case) is not meant to be used as VM interface, but is announced by our L4 balancer (standalone Cilium L4LB) and is BGP routable just like control plane network

@oblazek
Copy link
Contributor Author

oblazek commented Jan 5, 2024

@mdbooth any chance you would accept support for this? If we agree on an implementation I can start right away.

@mdbooth
Copy link
Contributor

mdbooth commented Jan 9, 2024

I think the API impact will be adding a vipNetwork field taking a network filter under apiServerLoadBalancer. The VIP would be allocated from this network, but members would be added as they are now. That doesn't sound too invasive, or significant additional complexity for the loadbalancer api.

@dulek What are your thoughts on this?

@dulek
Copy link
Contributor

dulek commented Jan 9, 2024

I think the API impact will be adding a vipNetwork field taking a network filter under apiServerLoadBalancer. The VIP would be allocated from this network, but members would be added as they are now. That doesn't sound too invasive, or significant additional complexity for the loadbalancer api.

@dulek What are your thoughts on this?

This looks sane with two modifications I'd suggest:

  1. Ability to choose a subnet too, in case network has multiple subnets.
  2. We should most likely make both fields an array in order to support dual stack LBs use case using additonal_vips functionality. @MaysaMacedo, you might have more thoughts here.

@mdbooth
Copy link
Contributor

mdbooth commented Jan 9, 2024

I think the API impact will be adding a vipNetwork field taking a network filter under apiServerLoadBalancer. The VIP would be allocated from this network, but members would be added as they are now. That doesn't sound too invasive, or significant additional complexity for the loadbalancer api.
@dulek What are your thoughts on this?

This looks sane with two modifications I'd suggest:

  1. Ability to choose a subnet too, in case network has multiple subnets.
  2. We should most likely make both fields an array in order to support dual stack LBs use case using additonal_vips functionality. @MaysaMacedo, you might have more thoughts here.

Good idea! I think only subnets needs to be a list though as from the docs:

Additional VIP subnets must all belong to the same network as the primary VIP.

@dulek
Copy link
Contributor

dulek commented Jan 9, 2024

I think the API impact will be adding a vipNetwork field taking a network filter under apiServerLoadBalancer. The VIP would be allocated from this network, but members would be added as they are now. That doesn't sound too invasive, or significant additional complexity for the loadbalancer api.
@dulek What are your thoughts on this?

This looks sane with two modifications I'd suggest:

  1. Ability to choose a subnet too, in case network has multiple subnets.
  2. We should most likely make both fields an array in order to support dual stack LBs use case using additonal_vips functionality. @MaysaMacedo, you might have more thoughts here.

Good idea! I think only subnets needs to be a list though as from the docs:

Additional VIP subnets must all belong to the same network as the primary VIP.

Oh good, it's purely for dual stack, so this simplifies it a lot.

@oblazek
Copy link
Contributor Author

oblazek commented Jan 11, 2024

great thanks, will start with the implementation then

@oblazek
Copy link
Contributor Author

oblazek commented Jan 12, 2024

btw regarding:

Additional VIP subnets must all belong to the same network as the primary VIP.

This is not yet possible as additional_vips are not yet part of gophercloud loadbalancer interface (see gophercloud/gophercloud#2700)

is it ok to do 1x networkID and 1x subnetID for now?

@dulek
Copy link
Contributor

dulek commented Jan 12, 2024

btw regarding:

Additional VIP subnets must all belong to the same network as the primary VIP.

This is not yet possible as additional_vips are not yet part of gophercloud loadbalancer interface (see gophercloud/gophercloud#2700)

is it ok to do 1x networkID and 1x subnetID for now?

We can work to add the required stuff to gophercloud, a single field should be a fairly simple addition and we should be able to get @EmilienM help with merging it.

Nevertheless if you'd like to pursue the simpler case first, I'd still insist that API should have the subnet field as an array even if documented that secondary values are ignored.

@oblazek
Copy link
Contributor Author

oblazek commented Jan 12, 2024

I'd still insist that API should have the subnet field as an array even if documented that secondary values are ignored

yeah, totally agree

oblazek added a commit to oblazek/cluster-api-provider-openstack that referenced this issue Mar 5, 2024
Previously when loadbalacer was created it used the same network/subnet as the
control plane nodes for the VIP. This was not always the right assumption as some
users might want to be able to customize this according to their env.

This commit fixes the above by adding two fields into
OpenStackClusterSpec/Status two fields `network` and `subnets` under
`APIServerLoadBalancer` so that user can define which network/subnet
to use for allocation of the loadbalancer.

Fixes: kubernetes-sigs#1809

Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
oblazek added a commit to oblazek/cluster-api-provider-openstack that referenced this issue Mar 13, 2024
Previously when loadbalacer was created it used the same network/subnet as the
control plane nodes for the VIP. This was not always the right assumption as some
users might want to be able to customize this according to their env.

This commit fixes the above by adding two fields into
OpenStackClusterSpec/Status two fields `network` and `subnets` under
`APIServerLoadBalancer` so that user can define which network/subnet
to use for allocation of the loadbalancer.

Fixes: kubernetes-sigs#1809

Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
oblazek added a commit to oblazek/cluster-api-provider-openstack that referenced this issue Mar 13, 2024
Previously when loadbalacer was created it used the same network/subnet as the
control plane nodes for the VIP. This was not always the right assumption as some
users might want to be able to customize this according to their env.

This commit fixes the above by adding two fields into
OpenStackClusterSpec/Status two fields `network` and `subnets` under
`APIServerLoadBalancer` so that user can define which network/subnet
to use for allocation of the loadbalancer.

Fixes: kubernetes-sigs#1809

Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants