OCPBUGS-63133: PowerVS: Error out if VPC does not exist #10137
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@AshwinHIBM: This pull request references Jira Issue OCPBUGS-63133, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[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 |
|
/jira refresh |
|
@prb112: This pull request references Jira Issue OCPBUGS-63133, which is invalid:
Comment 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@prb112: This pull request references Jira Issue OCPBUGS-63133, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @mjturek |
|
@AshwinHIBM: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
barbacbd
left a comment
barbacbd
left a comment
There was a problem hiding this comment.
I don't know that this is truly solving the nature of the bug. Why is the segmentation fault happening with an unknown vpc.
| } else if vpc, err = client.GetVPCByName(ctx, vpcNameOrID); err == nil { | ||
| vpcExists = true | ||
| } else { | ||
| return err |
There was a problem hiding this comment.
This will only ever return the error from the else-if directly above. This may take some reworking to get this to where the functionality is correct.
if vpcNameOrID == "" {
vpcNameOrID = fmt.Sprintf("vpc-%s", clusterID.InfraID)
} else {
if vpc, err = client.GetVPCByID(ctx, vpcNameOrID, vpcRegion); err == nil {
vpcNameOrID = *vpc.Name
vpcExists = true
} else {
return err << maybe want to override or add to the error here with fmt.Errorf
}
if vpc, err = client.GetVPCByName(ctx, vpcNameOrID); err == nil {
vpcExists = true
} else {
return err << add to error for more info with fmt.Errorf
}
}
Logically it could be something like that but there is probably a cleaner way of doing this
There was a problem hiding this comment.
GetVPCByID() errors out if there's no VPC with an ID with the value in vpcNameOrID. But vpcNameOrID could be the VPC's name so we can't exit out if GetVPCByID() returns an error since we need to check whether vpcNameOrID is a valid, existing VPC's name. And if GetVPCByName() fails too, I think it's fine to cascade the error returned by GetVPCByName() which is "failed to find VPC vpcNameOrID. Available VPCs: list of available VPC names in the resource group".
Hopefully that makes sense. Please let me know if it's still a concern.
There was a problem hiding this comment.
💡 Make sense to me. I guess we can further refactor (if you'd like later on) this chain of if into a function like GetVPCByIDOrName as the same chain is being used in other places.
Because a non-existent but non-empty |
Ahh, I see. The segmentation fault is elsewhere in other components (i.e. not within the installer). The fix is to ensure an invalid non-existing VPC ID/Name is rejected and the install process stops early. Though, I think such check should already be performed in pkg/asset/installconfig/powervs/validation.go before even reaching this far... Am I missing something? |
6 participants
Currently if an invalid VPC name is provided in platform.powervs.vpcName, the installer ends up in a segmentation violation. This PR causes the installer to instead error out which is the expected behaviour.