🐛 Better checks before creating Floating IPs by EmilienM · Pull Request #2261 · kubernetes-sigs/cluster-api-provider-openstack

EmilienM · 2024-11-19T15:16:12Z

What this PR does / why we need it:

When a floating is created, we need to make sure that
OpenStackCluster.Spec.DisableExternalNetwork is not set to True.
Otherwise, we'll have a nil pointer error.

Add a check in reconcileBastion to check that external network is
not disabled before creating the floating IP for the bastion.
Add a check in reconcileControlPlaneEndpoint and
reconcileAPIServerLoadBalancer to check that external
network is not disabled (alongside the DisableAPIServerFloatingIP
check) before creating the floating IP for the API server endpoint.
Add a safeguard in GetOrCreateFloatingIP to return a proper error
(instead of a nil pointer error) when
openStackCluster.Status.ExternalNetwork is nil.
Add API CEL to check that when DisableExternalNetwork is set and true,
the bastion (if defined) doesn't have a floating IP defined and also
that disableAPIServerFloatingIP (when set) is not False.

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

netlify · 2024-11-19T15:16:34Z

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

Name	Link
🔨 Latest commit	`5429b4b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/673eabd665c4a50008dc4345
😎 Deploy Preview	https://deploy-preview-2261--kubernetes-sigs-cluster-api-openstack.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.

EmilienM · 2024-11-19T15:16:40Z

/cc mdbooth

EmilienM · 2024-11-19T15:21:44Z

I initially wanted to do some webhook to just disallow DisableExternalNetwork to be True without also setting DisableAPIServerFloatingIP to True but it's not backward compatible and sucks for our users, so I did that.

EmilienM · 2024-11-19T15:22:13Z

/test pull-cluster-api-provider-openstack-e2e-test
flake

EmilienM · 2024-11-19T16:00:57Z

/cherry-pick release-0.11

k8s-infra-cherrypick-robot · 2024-11-19T16:01:00Z

@EmilienM: once the present PR merges, I will cherry-pick it on top of release-0.11 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-0.11

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-sigs/prow repository.

mdbooth

/lgtm

Any idea how far back we need to backport this? 0.11 at least.

EmilienM · 2024-11-19T18:45:33Z

/lgtm

Any idea how far back we need to backport this? 0.11 at least.

I'll investigate.
EDIT: backport is clean \o/

EmilienM · 2024-11-19T18:45:55Z

On this one I want an eye from @lentzi90.

EmilienM · 2024-11-19T19:05:37Z

/cherry-pick release-0.10

k8s-infra-cherrypick-robot · 2024-11-19T19:05:41Z

@EmilienM: once the present PR merges, I will cherry-pick it on top of release-0.10 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-0.10

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-sigs/prow repository.

lentzi90

I initially wanted to do some webhook to just disallow DisableExternalNetwork to be True without also setting DisableAPIServerFloatingIP to True but it's not backward compatible and sucks for our users

Sure not backwards compatible, but do you mean that it is actually possible to use that configuration? I'm thinking it is ok to break backwards compatibility if all it was doing was cause nil pointer exceptions.

controllers/openstackcluster_controller.go

lentzi90

/lgtm
Looks good, and no need for extra logging now that the webhook immediately tells the user what is wrong 🙂

When a floating is created, we need to make sure that `OpenStackCluster.Spec.DisableExternalNetwork` is not set to `True`. Otherwise, we'll have a nil pointer error. * Add a check in `reconcileBastion` to check that external network is not disabled before creating the floating IP for the bastion. * Add a check in `reconcileControlPlaneEndpoint` and `reconcileAPIServerLoadBalancer` to check that external network is not disabled (alongside the DisableAPIServerFloatingIP check) before creating the floating IP for the API server endpoint. * Add a safeguard in `GetOrCreateFloatingIP` to return a proper error (instead of a nil pointer error) when `openStackCluster.Status.ExternalNetwork` is nil. * Add API CEL to check that when DisableExternalNetwork is set and true, the bastion (if defined) doesn't have a floating IP defined and also that disableAPIServerFloatingIP (when set) is not False.

lentzi90

I'm happy with this, but as mentioned on slack, I have one concern.
Sorry I didn't think of it earlier.

Will the user ever be the one setting the floating IP? Or are we now just going to block the controller trying to add it?

EmilienM · 2024-11-21T20:35:53Z

I'm happy with this, but as mentioned on slack, I have one concern. Sorry I didn't think of it earlier.

Will the user ever be the one setting the floating IP? Or are we now just going to block the controller trying to add it?

yes, a user can set the FIP:

cluster-api-provider-openstack/controllers/openstackcluster_controller.go

Lines 422 to 424 in 5429b4b

    
           case openStackCluster.Spec.Bastion.FloatingIP != nil: 
        
           	// Use floating IP from the spec 
        
           	floatingIP = openStackCluster.Spec.Bastion.FloatingIP

lentzi90

Works as expected. If I try to manually do some invalid combination of disableExternalNetwork + API server floating IP or bastion floating IP, CEL will block it. If I enable the bastion and disable the external network it will be created without floating IP, no errors in the logs. For this situation there isn't anything obvious telling the user why the bastion didn't get a floating IP. That is not ideal, but surely better than nil pointer exceptions, so
/lgtm

EmilienM · 2024-11-22T15:55:41Z

Works as expected. If I try to manually do some invalid combination of disableExternalNetwork + API server floating IP or bastion floating IP, CEL will block it. If I enable the bastion and disable the external network it will be created without floating IP, no errors in the logs. For this situation there isn't anything obvious telling the user why the bastion didn't get a floating IP. That is not ideal, but surely better than nil pointer exceptions, so /lgtm

yeah that's why I updated the doc.

Let's ship it.
/approve

k8s-ci-robot · 2024-11-22T15:55:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM

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

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

k8s-infra-cherrypick-robot · 2024-11-22T15:57:32Z

@EmilienM: new pull request created: #2269

Details

In response to this:

/cherry-pick release-0.11

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-sigs/prow repository.

k8s-infra-cherrypick-robot · 2024-11-22T15:58:04Z

@EmilienM: #2261 failed to apply on top of branch "release-0.10":

Applying: Better conditions for creating Floating IPs
Using index info to reconstruct a base tree...
M	api/v1beta1/openstackcluster_types.go
M	config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml
M	config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml
M	controllers/openstackcluster_controller.go
M	controllers/openstackmachine_controller.go
M	docs/book/src/clusteropenstack/configuration.md
M	pkg/cloud/services/networking/floatingip.go
M	test/e2e/suites/apivalidations/openstackcluster_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/suites/apivalidations/openstackcluster_test.go
CONFLICT (content): Merge conflict in test/e2e/suites/apivalidations/openstackcluster_test.go
Auto-merging pkg/cloud/services/networking/floatingip.go
Auto-merging docs/book/src/clusteropenstack/configuration.md
Auto-merging controllers/openstackmachine_controller.go
Auto-merging controllers/openstackcluster_controller.go
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml
Auto-merging api/v1beta1/openstackcluster_types.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Better conditions for creating Floating IPs

Details

In response to this:

/cherry-pick release-0.10

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-sigs/prow repository.

k8s-ci-robot requested review from jichenjc and lentzi90 November 19, 2024 15:16

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2024

k8s-ci-robot requested a review from mdbooth November 19, 2024 15:16

EmilienM changed the title ~~🐛 Better conditions for creating Floating IPs~~ 🐛 Better checks for creating Floating IPs Nov 19, 2024

EmilienM changed the title ~~🐛 Better checks for creating Floating IPs~~ 🐛 Better checks before creating Floating IPs Nov 19, 2024

mdbooth reviewed Nov 19, 2024

View reviewed changes

k8s-ci-robot assigned mdbooth Nov 19, 2024

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

lentzi90 reviewed Nov 19, 2024

View reviewed changes

controllers/openstackcluster_controller.go Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2024

EmilienM force-pushed the issue_2260 branch from c1844d0 to c746a8a Compare November 19, 2024 20:34

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024

lentzi90 reviewed Nov 20, 2024

View reviewed changes

k8s-ci-robot assigned lentzi90 Nov 20, 2024

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

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

EmilienM changed the title ~~🐛 Better checks before creating Floating IPs~~ 🐛 WIP - Better checks before creating Floating IPs Nov 20, 2024

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 20, 2024

EmilienM force-pushed the issue_2260 branch from d321446 to 5429b4b Compare November 21, 2024 03:41

EmilienM changed the title ~~🐛 WIP - Better checks before creating Floating IPs~~ 🐛 Better checks before creating Floating IPs Nov 21, 2024

EmilienM requested review from lentzi90 and mdbooth November 21, 2024 03:41

lentzi90 reviewed Nov 21, 2024

View reviewed changes

lentzi90 reviewed Nov 22, 2024

View reviewed changes

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

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

k8s-ci-robot merged commit dc71d68 into main Nov 22, 2024

k8s-ci-robot deleted the issue_2260 branch November 22, 2024 15:56

k8s-infra-cherrypick-robot mentioned this pull request Nov 22, 2024

[release-0.11] 🐛 Better checks before creating Floating IPs #2269

Merged

EmilienM mentioned this pull request Nov 22, 2024

[release-0.10] 🐛 Better checks before creating Floating IPs #2270

Merged

Conversation

EmilienM commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

EmilienM commented Nov 19, 2024

Uh oh!

EmilienM commented Nov 19, 2024

Uh oh!

EmilienM commented Nov 19, 2024

Uh oh!

EmilienM commented Nov 19, 2024

Uh oh!

k8s-infra-cherrypick-robot commented Nov 19, 2024

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

EmilienM commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EmilienM commented Nov 19, 2024

Uh oh!

EmilienM commented Nov 19, 2024

Uh oh!

k8s-infra-cherrypick-robot commented Nov 19, 2024

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

EmilienM commented Nov 21, 2024

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

EmilienM commented Nov 22, 2024

Uh oh!

k8s-ci-robot commented Nov 22, 2024

Uh oh!

k8s-infra-cherrypick-robot commented Nov 22, 2024

Uh oh!

k8s-infra-cherrypick-robot commented Nov 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EmilienM commented Nov 19, 2024 •

edited

Loading

netlify bot commented Nov 19, 2024 •

edited

Loading

EmilienM commented Nov 19, 2024 •

edited

Loading