🐛 Better checks before creating Floating IPs #2261
Conversation
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.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/cc mdbooth |
EmilienM
changed the title
EmilienM
changed the title
|
I initially wanted to do some webhook to just disallow |
|
/test pull-cluster-api-provider-openstack-e2e-test |
|
/cherry-pick release-0.11 |
|
@EmilienM: once the present PR merges, I will cherry-pick it on top of 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-sigs/prow repository. |
k8s-ci-robot
added
the
lgtm
I'll investigate. |
|
On this one I want an eye from @lentzi90. |
|
/cherry-pick release-0.10 |
|
@EmilienM: once the present PR merges, I will cherry-pick it on top of 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-sigs/prow repository. |
There was a problem hiding this comment.
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.
| if !ptr.Deref(openStackCluster.Spec.DisableExternalNetwork, false) { | ||
| return bastionAddFloatingIP(openStackCluster, clusterResourceName, port, networkingService) | ||
| } | ||
|
|
||
| return nil, nil |
There was a problem hiding this comment.
What do you think about doing this the other way? If disabled -> log something and return. Otherwise continue and add it.
There was a problem hiding this comment.
IIUC what you're proposing is
...
if ptr.Deref(openStackCluster.Spec.DisableExternalNetwork, false) {
// log something? what exactly?
return nil, nil
}
return bastionAddFloatingIP(openStackCluster, clusterResourceName, port, networkingService)If yes, I think it's functionally the same and is fairly consistent with other Deref calls for that field. No strong opinion here, happy to make the change.
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
added
size/M
* Don't allow a bastion to have a floating IP if external network is disabled. * Don't allow the APIserver to have a floating IP if external network is disabled.
k8s-ci-robot
removed
the
size/M
k8s-ci-robot
added
the
size/L
What this PR does / why we need it:
When a floating is created, we need to make sure that
OpenStackCluster.Spec.DisableExternalNetworkis not set toTrue.Otherwise, we'll have a nil pointer error.
reconcileBastionto check that external network isnot disabled before creating the floating IP for the bastion.
reconcileControlPlaneEndpointandreconcileAPIServerLoadBalancerto check that externalnetwork is not disabled (alongside the DisableAPIServerFloatingIP
check) before creating the floating IP for the API server endpoint.
GetOrCreateFloatingIPto return a proper error(instead of a nil pointer error) when
openStackCluster.Status.ExternalNetworkis nil.Also includes a change to the webhooks:
disabled.
disabled.
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