Wrong parameter for a user provided network crashes the controller

[pierrecregut]

/kind bug

What steps did you take and what happened:
[A clear and concise description of what the bug is.]
Tried to specify in openstack cluster a network (which was wrong). Not only the deployment failed but the container crashed.

What did you expect to happen:
Just a failed deployment.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]
In openstackcluster_controller line 300, an or (||) was probably intended on the check of err and the size of the network list.

Environment:

• Cluster-api version:
• Minikube/KIND version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[jichenjc]

@pierrecregut do you have any logs can be shared?

[hidekazuna]

@pierrecregut @jichenjc

I investigated from
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/master/controllers/openstackcluster_controller.go#L300

If wrong network name was given,

if err != nil && len(networkList) == 0 {
    return errors.Errorf("failed to find network: %v", err)
}

did not return error, so reached

openStackCluster.Status.Network = &infrav1.Network{
    ID:   networkList[0].ID,
    Name: networkList[0].Name,
}

networkList[0].ID can not be found and crashed.

[pierrecregut]

Yes exactly, sorry if it was not clear.

[hidekazuna]

Yes exactly, sorry if it was not clear.

@pierrecregut Don't worry. Thanks for raising the issue!

[sbueringer]

Yup this should be fixed. I would propose checking it separately with two if conditions because in case of err != nil we want to append the error in case of len(networkList) == 0 there is no error to append

[hidekazuna]

/good-first-issue

[k8s-ci-robot]

@hidekazuna:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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.

[pramchan]

/assign pramchan

[pramchan]

I edited and split the if len zero and err separate with two ifs to avoid the crash as asked. Is this good enough to create pull request or do i need to uses existing pull pr#590 to submit the patch orked and edited in my github https://github.com/pramchan/cluster-api-provider-openstack/blob/master/controllers/openstackcluster_controller.go#L300 listed below relevant code lines.
if len(networkList) == 0 {
return errors.Errorf("failed to find any network: %v", err)
}
if err != nil {
return errors.Errorf("failed to find network: %v", err)
}
if len(networkList) > 1 {
return errors.Errorf("failed to find only one network (result: %v): %v", networkList, err)
}
openStackCluster.Status.Network = &infrav1.Network{
ID: networkList[0].ID,
Name: networkList[0].Name,
}

[hidekazuna]

@pramchan Please submit the patch as PR so that we can review.

[pramchan]

I though I did but seems something is wrong, it did not connect to right PR590 some mistake - may be need to resubmit
a3188f4