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

feat: add configurable loadbalancer network #1922

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

oblazek
Copy link
Contributor

@oblazek oblazek commented 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.

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha8
kind: OpenStackCluster
metadata:
  name: {{ capi_cluster_name }}
spec:
  cloudName: {{ capi_cluster_os_cloud }}
  identityRef:
    kind: Secret
    name: test3-cloud-config
  managedSecurityGroups: true
  apiServerLoadBalancer:
    enabled: true
    network:
      name: vip-net-ulab1
    subnets:
      - name: ulab1-ipv4-1
status:
  apiServerLoadBalancer:
    id: 808239dc-064f-402e-bd7d-ca667157a76c
    internalIP: 10.248.11.161
    ip: ""
    loadBalancerNetwork:
      id: 65f6a910-3968-46c0-8f72-e4f171476670
      name: vip-net-ulab1
      subnets:
      - id: 5b4e2c9b-1ab9-4510-87a4-6d2301e16e77
    name: k8s-clusterapi-cluster-test3-test3-kubeapi

Fixes: #1809

Signed-off-by: Ondrej Blazek [email protected]

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Mar 5, 2024
Copy link

linux-foundation-easycla bot commented Mar 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: oblazek / name: Ondrej Blazek (e2897fb)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 5, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @oblazek!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 5, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @oblazek. 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.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 5, 2024
Copy link

netlify bot commented Mar 5, 2024

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit e2897fb
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6613961c7573080009e8254f
😎 Deploy Preview https://deploy-preview-1922--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot k8s-ci-robot requested review from dulek and EmilienM March 5, 2024 10:15
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 5, 2024
@dulek
Copy link
Contributor

dulek commented Mar 5, 2024

/ok-to-test

@k8s-ci-robot 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 Mar 5, 2024
api/v1alpha6/conversion_test.go Outdated Show resolved Hide resolved
Comment on lines 126 to 128
// None of these fields exist in v1alpha6
spec.APIServerLoadBalancer.Network = nil
spec.APIServerLoadBalancer.Subnets = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

api/v1beta1/types.go Outdated Show resolved Hide resolved
api/v1beta1/types.go Outdated Show resolved Hide resolved
pkg/cloud/services/loadbalancer/loadbalancer.go Outdated Show resolved Hide resolved
controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved
controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved
// Network defines which network should the load balancer be allocated on.
//+optional
Network *NetworkFilter `json:"network,omitempty"`
// Subnet defines which network(s) should the load balancer be allocated on.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that all these subnets must be in the same network, which is described by Network?

Is it permitted to specify Subnets without network?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's a good point, tbh we will most likely end up in specifying just the Network (by its name) and it won't matter to us which subnet will be chosen as far as I get 1 IPv4 and 1 IPv6 VIP

Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good in principal, but there are a few things to straighten out.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2024
@oblazek oblazek force-pushed the ob-loadbalancer-network branch 2 times, most recently from cd7ab5b to 1b68d37 Compare March 13, 2024 13:02
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2024
@oblazek oblazek force-pushed the ob-loadbalancer-network branch from 1b68d37 to 96c0bb0 Compare March 13, 2024 14:06
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Mar 13, 2024
@oblazek
Copy link
Contributor Author

oblazek commented Mar 13, 2024

this should be ready for another round

@oblazek oblazek requested review from dulek and mdbooth March 13, 2024 14:58
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

This looks pretty good to me, some small remarks inline.

api/v1alpha6/openstackcluster_conversion.go Outdated Show resolved Hide resolved
api/v1alpha7/openstackcluster_conversion.go Outdated Show resolved Hide resolved
api/v1beta1/types.go Show resolved Hide resolved
controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2024
Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, but this is close. Thanks!

controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved
controllers/openstackcluster_controller.go Outdated Show resolved Hide resolved
return fmt.Errorf("no subnet match was found in the specified network (specified subnet: %v, available subnets: %v)", s, lbNetList[0].Subnets)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making the default explicit and copying in the cluster network in the else branch? Then we could just make the creation code assume it's set. We could also shortcut the lookup at the top of this function if it's already set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks better now.

Comment on lines 273 to 275
var vipNetworkID, vipSubnetID string
if openStackCluster.Status.APIServerLoadBalancer != nil {
if openStackCluster.Status.APIServerLoadBalancer.LoadBalancerNetwork != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we made the suggested change above where we copy in the cluster network when resolving the vip network, this block becomes unconditional. Also, when we add dual stack support here we get it in the non-custom case too, for free. You'll just want to return an error if APIServerLB or APIServerLB.Network are nil in the status, which will also mean we don't need all the nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, should be fixed now if I am not mistaken

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2024
@oblazek oblazek force-pushed the ob-loadbalancer-network branch from 96c0bb0 to af1eccb Compare April 3, 2024 14:34
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 3, 2024
@oblazek oblazek force-pushed the ob-loadbalancer-network branch from af1eccb to 884c175 Compare April 3, 2024 20:23
@oblazek oblazek requested review from mdbooth and dulek April 3, 2024 21:46
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question inline.

go.mod Outdated
@@ -16,7 +16,7 @@ require (
github.com/onsi/gomega v1.30.0
github.com/prometheus/client_golang v1.17.0
github.com/spf13/pflag v1.0.5
golang.org/x/crypto v0.16.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the deps bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably by running some go mod tidy .. will remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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.

Signed-off-by: Ondrej Blazek <[email protected]>
@oblazek oblazek force-pushed the ob-loadbalancer-network branch from 884c175 to e2897fb Compare April 8, 2024 07:00
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2024
@mdbooth
Copy link
Contributor

mdbooth commented Apr 9, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek, mdbooth

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024
@k8s-ci-robot k8s-ci-robot merged commit 9aaf481 into kubernetes-sigs:main Apr 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add the ability to specify a configurable VIP network for loadbalancer
4 participants