OCPBUGS-62892: openstack: Handle empty MAPO network subnets #381
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@stephenfin: This pull request references Jira Issue OCPBUGS-62892, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
WalkthroughMAPI↔CAPO OpenStack conversion logic was made stricter: network, filter, subnet, fixed IP, and port tag fields are only populated when corresponding input data exists; nil checks and guarded assignments were added in both conversion directions; fuzz test network case adjusted to use an empty UUID with a Filter and SubnetParam. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant M2C as convertMAPONetworksToCAPO
participant Builder as CAPO Port Builder
Caller->>M2C: convert MAPI network(s)
activate M2C
M2C->>M2C: iterate networks
alt network.UUID present
M2C->>Builder: set capoNetwork.ID
end
alt network.Filter non-empty
M2C->>M2C: compute ProjectID
M2C->>Builder: set capoNetwork.Filter
else Filter empty
note right of M2C: skip Filter population
end
alt no network but subnets present
M2C->>M2C: for each subnet build capoSubnet and capoFixedIP only if fields exist
M2C->>Builder: append capoFixedIP(s)
else network present
M2C->>Builder: attach capoNetwork and capoFixedIPs (if any)
end
M2C-->>Caller: CAPO Port/Network objects
deactivate M2C
sequenceDiagram
autonumber
participant Caller
participant C2M as convertCAPOPortOptsToMAPO
participant Builder as MAPI Network Builder
Caller->>C2M: convert CAPO PortOpts
activate C2M
C2M->>C2M: inspect capoPort.Network
alt capoPort.Network != nil
opt Network.ID
C2M->>Builder: set mapoNetwork.UUID from ID
end
opt Network.Filter
C2M->>Builder: set mapoNetwork.Filter (tags, not-tags, projectID...)
end
alt neither ID nor Filter
C2M->>Caller: record error "A network must be referenced by a UUID or filter"
end
else capoPort.Network == nil
alt capoPort.FixedIPs present
C2M->>C2M: set PortTags, build mapoSubnet(s) from FixedIPs (UUID or Filter)
C2M->>Builder: attach subnet params
else
C2M->>Caller: record error "A network must be referenced by a UUID or filter, else a subnet must be referenced"
end
end
C2M-->>Caller: MAPI network/port spec or errors
deactivate C2M
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@stephenfin: This pull request references Jira Issue OCPBUGS-62892, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 @mdbooth |
|
/test ? |
|
@damdo: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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. |
|
/test e2e-openstack-capi-techpreview |
|
[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 |
stephenfin
force-pushed
the
OCPBUGS-62892
branch
from
9e36388 to
c6b925d
Compare
Signed-off-by: Stephen Finucane <[email protected]>
stephenfin
force-pushed
the
OCPBUGS-62892
branch
from
c6b925d to
c8b86ab
Compare
Signed-off-by: Stephen Finucane <[email protected]>
stephenfin
force-pushed
the
OCPBUGS-62892
branch
from
c8b86ab to
6004d19
Compare
|
/test e2e-openstack-capi-techpreview |
|
@stephenfin @mdbooth It looks like E2Es passed on ci/prow/e2e-openstack-capi-techpreview but the overall job failed due to a gather step failure (which might be recurrent and worth fixing, although not in this PR :) ) @mdbooth if you are happy with the latest changes I think we should be ok overriding and merging. |
Ah, yes, we fixed that this morning openshift/release#70212. This job must have started before that patch merged. It's merged now though so let's re-run. /test e2e-openstack-capi-techpreview |
There was a problem hiding this comment.
If you have time I still think there's a bunch of cleanup to do here. In particular I think this change highlights a latent panic which ideally we would fix.
However, given that it fixes an immediate problem I wouldn't want to hold it up for the issues here. Please consider having somebody on the team follow up, though.
/lgtm
| // TenantID is deprecated and covered by ProjectID so we don't set it | ||
| if capoPort.Network != nil { | ||
| switch { | ||
| case capoPort.Network != nil && capoPort.Network.ID != nil: |
There was a problem hiding this comment.
nit: the capoPort.Network != nil is redundant now we're guarding the whole switch.
| } else if len(capoPort.FixedIPs) == 0 { | ||
| // TODO(OSASINFRA-3779): Add VAP to prevent usage of the below field. | ||
| errors = append(errors, field.Invalid(fldPath.Child("network"), capoPort.Network, "A network must be referenced by a UUID or filter")) | ||
| errors = append(errors, field.Invalid(fldPath.Child("network"), capoPort.Network, "A network must be referenced by a UUID or filter, else a subnet must be referenced")) |
There was a problem hiding this comment.
nit: I think it would be less confusing to say 'Either network or fixedIPs must be specified on a port'. The bit about UUID and filter doesn't have any context here.
| mapoSubnet := mapiv1alpha1.SubnetParam{ | ||
| UUID: *capoFixedIP.Subnet.ID, | ||
| Filter: mapiv1alpha1.SubnetFilter{ | ||
| UUID: *capoFixedIP.Subnet.ID, |
There was a problem hiding this comment.
I appreciate this isn't new, but what stops us panicing here if the subnet is specified by filter instead of ID? Do we have test coverage of this?
| if projectID == "" { | ||
| projectID = mapoNetwork.Filter.TenantID | ||
| } | ||
| if (mapoNetwork.Filter != mapiv1alpha1.Filter{}) { |
There was a problem hiding this comment.
It's an error for a network to specify both ID and filter. Filter is redundant if we specified ID, so we could add an additional guard here that networkID is not set.
|
|
||
| // convert .Subnets | ||
| if networkID == "" && (mapoNetwork.Filter == mapiv1alpha1.Filter{}) { //nolint:nestif | ||
| if (capoNetwork == openstackv1.NetworkParam{}) { //nolint:nestif |
There was a problem hiding this comment.
Code structure observation. We have:
capoNetwork := switch {
case mapoNetwork.UUID != "":
addCAPOSubnets(capoNetwork{ID = &mapoNetwork.UUID}, ...)
case mapoNetwork.Filter != nil:
addCAPOSubnets(capoNetworkFromFilter(), ...)
default:
capoNetworkFromSubnets() (this if statement, which is the default/else of the above)
}
addCAPOSubnets() is the else branch starting line 483 below.
| } | ||
| } | ||
|
|
||
| capoFixedIP := openstackv1.FixedIP{} |
There was a problem hiding this comment.
nit: This doesn't seem to be doing anything outside the if block below. You could move it in there.
|
|
||
| //nolint:funlen | ||
| func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.NetworkParam) ([]openstackv1.PortOpts, []string, field.ErrorList) { //nolint:gocognit,cyclop | ||
| func convertMAPONetworksToCAPO(fldPath *field.Path, mapoNetworks []mapiv1alpha1.NetworkParam) ([]openstackv1.PortOpts, []string, field.ErrorList) { //nolint:gocognit,cyclop,gocyclo |
There was a problem hiding this comment.
I have to agree with the linter here: this function is getting hard to read!
|
Scheduling tests matching the |
|
@stephenfin: The following tests 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. |
|
Hey @stephenfin what's the plan for this one? TY |
4 participants
Summary by CodeRabbit
Bug Fixes
Stability
Tests