OSAC-2350, OSAC-2482: create MetalLB IPAddressPool at subnet creation and stamp VIP range for DHCP exclusion - #258
Conversation
|
@ori-amizur: This pull request references OSAC-2350 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. This pull request references OSAC-2482 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn 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. |
WalkthroughThe operator now calculates VIP ranges from NetworkClass settings, annotates subnets, and manages corresponding MetalLB ChangesVIP pool provisioning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SubnetReconciler
participant NetworkClassesClient
participant VIPRangeHelpers
participant TargetClusterClient
participant MetalLB
SubnetReconciler->>NetworkClassesClient: fetch NetworkClass VIP prefix
NetworkClassesClient-->>SubnetReconciler: return configuration
SubnetReconciler->>VIPRangeHelpers: calculate VIP CIDR
VIPRangeHelpers-->>SubnetReconciler: return VIP range
SubnetReconciler->>TargetClusterClient: obtain target-cluster client
SubnetReconciler->>MetalLB: create or update IPAddressPool
MetalLB-->>SubnetReconciler: return resource state
SubnetReconciler->>MetalLB: delete IPAddressPool during deprovisioning
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 7❌ Failed checks (1 warning, 6 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
🤖 Finished Review · ✅ Success · Started 11:02 AM UTC · Completed 11:21 AM UTC Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@osac-operator/internal/controller/subnet_controller.go`:
- Around line 215-219: The error from resolveVIPCIDR in handleUpdate must be
returned so reconciliation retries instead of continuing without VIP
configuration; update osac-operator/internal/controller/subnet_controller.go
lines 215-219 accordingly. Also handle the NestedStringSlice error before
comparing or updating spec.addresses, returning it rather than treating invalid
data as an empty list; update lines 415-421 in the same file.
- Around line 230-232: Update the subnet reconciliation logic around
resolveVIPCIDR to handle an empty vipCIDR by deleting the existing
osacVIPCIDRAnnotation and removing the corresponding MetalLB IPAddressPool for
the configured NetworkClass. Preserve the current annotation update behavior
when vipCIDR is non-empty, and ensure annotationsChanged reflects deletion as
well as modification.
- Around line 430-435: Update deleteMetalLBIPAddressPool to return nil before
calling getTargetClient when subnet.Annotations[osacVIPCIDRAnnotation] is empty,
while preserving the existing r.mgr nil guard and normal target-client cleanup
for Subnets with a VIP annotation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a91a76d4-3c21-43d8-b526-0da0fe72e972
📒 Files selected for processing (6)
osac-operator/cmd/main.goosac-operator/config/rbac/role.yamlosac-operator/helpers/helpers_suite_test.goosac-operator/helpers/viprange.goosac-operator/helpers/viprange_test.goosac-operator/internal/controller/subnet_controller.go
| // Resolve VIP prefix length from NetworkClass (if gRPC is available) | ||
| vipCIDR := "" | ||
| if r.NetworkClassesClient != nil && vnet.Spec.NetworkClass != "" && subnet.Spec.IPv4CIDR != "" { | ||
| vipCIDR, _ = r.resolveVIPCIDR(ctx, vnet.Spec.NetworkClass, subnet.Spec.IPv4CIDR) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle the returned errors.
Line 218 can continue provisioning without the required VIP CIDR and MetalLB pool when NetworkClass lookup or CIDR calculation fails. Line 416 can treat an invalid spec.addresses field as an empty list and overwrite the external resource.
osac-operator/internal/controller/subnet_controller.go#L215-L219: Return theresolveVIPCIDRerror fromhandleUpdateso reconciliation retries instead of marking the Subnet ready without VIP configuration.osac-operator/internal/controller/subnet_controller.go#L415-L421: Return theNestedStringSliceerror before comparing or updatingspec.addresses.
As per path instructions, Go code must never ignore error returns.
📍 Affects 1 file
osac-operator/internal/controller/subnet_controller.go#L215-L219(this comment)osac-operator/internal/controller/subnet_controller.go#L415-L421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-operator/internal/controller/subnet_controller.go` around lines 215 -
219, The error from resolveVIPCIDR in handleUpdate must be returned so
reconciliation retries instead of continuing without VIP configuration; update
osac-operator/internal/controller/subnet_controller.go lines 215-219
accordingly. Also handle the NestedStringSlice error before comparing or
updating spec.addresses, returning it rather than treating invalid data as an
empty list; update lines 415-421 in the same file.
Source: Path instructions
| if vipCIDR != "" && subnet.Annotations[osacVIPCIDRAnnotation] != vipCIDR { | ||
| subnet.Annotations[osacVIPCIDRAnnotation] = vipCIDR | ||
| annotationsChanged = true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove obsolete VIP state when VIP configuration is absent.
When resolveVIPCIDR returns an empty value because vip_prefix_length is removed, Line 230 preserves the old osac.openshift.io/vip-cidr annotation. The existing MetalLB IPAddressPool then remains configured for a range that the NetworkClass no longer defines.
Delete the annotation and remove the corresponding pool when a configured NetworkClass no longer has a VIP prefix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-operator/internal/controller/subnet_controller.go` around lines 230 -
232, Update the subnet reconciliation logic around resolveVIPCIDR to handle an
empty vipCIDR by deleting the existing osacVIPCIDRAnnotation and removing the
corresponding MetalLB IPAddressPool for the configured NetworkClass. Preserve
the current annotation update behavior when vipCIDR is non-empty, and ensure
annotationsChanged reflects deletion as well as modification.
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 2:03 PM UTC · Completed 2:40 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Auto-dismissed: only Prow labels gate merging
|
🤖 Finished Review · ✅ Success · Started 10:14 AM UTC · Completed 10:33 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 10:47 AM UTC · Completed 11:05 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
Auto-dismissed: only Prow labels gate merging
… and stamp VIP range for DHCP exclusion At subnet creation, the operator now resolves vip_prefix_length from the NetworkClass and creates a MetalLB IPAddressPool on the target cluster covering the VIP sub-range of the subnet CIDR. On subnet deletion the IPAddressPool is removed before AAP deprovisioning. The VIP range is also stamped as an osac.openshift.io/vip-cidr annotation on the Subnet CR so the fabric manager Ansible role can read it and exclude that range from DHCP. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ori Amizur <oamizur@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 12:40 PM UTC · Completed 1:00 PM UTC Commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
osac-operator/internal/controller/subnet_controller.go (1)
250-262: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInclude VIP CIDR changes in the provisioning version.
A NetworkClass VIP prefix change updates the annotation at Line 250.
ComputeDesiredConfigVersiononly receivessubnet.SpecandimplementationStrategy. AAP provisioning can therefore remain skipped while DHCP exclusion retains the previous VIP range.Include the resolved VIP CIDR, or a NetworkClass revision, in the desired configuration input. Add coverage for a changed
vip_prefix_length.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osac-operator/internal/controller/subnet_controller.go` around lines 250 - 262, Update ComputeDesiredConfigVersion and its call sites to include the resolved vipCIDR or NetworkClass revision in the desired configuration input, so VIP prefix changes produce a new provisioning version and trigger AAP reprovisioning. Add coverage verifying that changing vip_prefix_length changes the computed version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@osac-operator/internal/controller/subnet_controller.go`:
- Around line 229-255: The VIP annotation cleanup currently treats skipped
resolution as an empty result. In the reconciliation flow around resolveVIPCIDR,
track whether VIP resolution completed successfully, and only delete
osacVIPCIDRAnnotation when resolution completed and returned no CIDR; preserve
the existing annotation when networkClassesClient is unavailable or resolution
is skipped.
- Line 258: Remove customer network data from the logging calls in
subnet_controller.go: at lines 258-258, update the log in the annotation update
flow to omit the vipCIDR field; at lines 438-438 and 453-453, update the
corresponding logs to omit the addresses field. Preserve non-sensitive fields
such as implementationStrategy and existing log behavior.
---
Outside diff comments:
In `@osac-operator/internal/controller/subnet_controller.go`:
- Around line 250-262: Update ComputeDesiredConfigVersion and its call sites to
include the resolved vipCIDR or NetworkClass revision in the desired
configuration input, so VIP prefix changes produce a new provisioning version
and trigger AAP reprovisioning. Add coverage verifying that changing
vip_prefix_length changes the computed version.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5245663f-72cb-4ba3-a2dc-3b6876805bf4
📒 Files selected for processing (6)
osac-operator/cmd/main.goosac-operator/config/rbac/role.yamlosac-operator/helpers/viprange.goosac-operator/helpers/viprange_test.goosac-operator/internal/controller/subnet_controller.goosac-operator/internal/controller/subnet_names.go
🚧 Files skipped from review as they are similar to previous changes (2)
- osac-operator/cmd/main.go
- osac-operator/helpers/viprange.go
| // Resolve VIP prefix length from NetworkClass (if gRPC is available) | ||
| vipCIDR := "" | ||
| if r.networkClassesClient != nil && vnet.Spec.NetworkClass != "" && subnet.Spec.IPv4CIDR != "" { | ||
| var resolveErr error | ||
| vipCIDR, resolveErr = r.resolveVIPCIDR(ctx, vnet.Spec.NetworkClass, subnet.Spec.IPv4CIDR) | ||
| if resolveErr != nil { | ||
| log.Error(resolveErr, "failed to resolve VIP CIDR from NetworkClass, requeueing", | ||
| "networkClass", vnet.Spec.NetworkClass) | ||
| return ctrl.Result{RequeueAfter: defaultPreconditionRequeueInterval}, nil | ||
| } | ||
| } | ||
|
|
||
| // Stamp annotations for AAP playbooks (implementation strategy + VIP CIDR) | ||
| if subnet.Annotations == nil { | ||
| subnet.Annotations = make(map[string]string) | ||
| } | ||
| annotationsChanged := false | ||
| if subnet.Annotations[osacImplementationStrategyAnnotation] != implementationStrategy { | ||
| subnet.Annotations[osacImplementationStrategyAnnotation] = implementationStrategy | ||
| log.Info("setting implementation-strategy annotation", "strategy", implementationStrategy) | ||
| annotationsChanged = true | ||
| } | ||
| if vipCIDR != "" && subnet.Annotations[osacVIPCIDRAnnotation] != vipCIDR { | ||
| subnet.Annotations[osacVIPCIDRAnnotation] = vipCIDR | ||
| annotationsChanged = true | ||
| } else if vipCIDR == "" && subnet.Annotations[osacVIPCIDRAnnotation] != "" { | ||
| delete(subnet.Annotations, osacVIPCIDRAnnotation) | ||
| annotationsChanged = true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not delete a VIP CIDR when resolution was skipped.
Line 230 initializes vipCIDR as empty. If networkClassesClient is nil, Line 231 skips resolution. Line 253 then deletes an existing VIP annotation without confirming that the NetworkClass removed vip_prefix_length.
Track whether resolution completed. Delete osacVIPCIDRAnnotation only after a successful resolution returns no VIP CIDR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-operator/internal/controller/subnet_controller.go` around lines 229 -
255, The VIP annotation cleanup currently treats skipped resolution as an empty
result. In the reconciliation flow around resolveVIPCIDR, track whether VIP
resolution completed successfully, and only delete osacVIPCIDRAnnotation when
resolution completed and returned no CIDR; preserve the existing annotation when
networkClassesClient is unavailable or resolution is skipped.
| annotationsChanged = true | ||
| } | ||
| if annotationsChanged { | ||
| log.Info("updating annotations", "strategy", implementationStrategy, "vipCIDR", vipCIDR) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not log VIP CIDR values. VIP CIDRs are customer network configuration. Remove the CIDR values from controller logs.
osac-operator/internal/controller/subnet_controller.go#L258-L258: Remove thevipCIDRlog field.osac-operator/internal/controller/subnet_controller.go#L438-L438: Remove theaddresseslog field.osac-operator/internal/controller/subnet_controller.go#L453-L453: Remove theaddresseslog field.
As per coding guidelines, “Do not log ... customer data.”
📍 Affects 1 file
osac-operator/internal/controller/subnet_controller.go#L258-L258(this comment)osac-operator/internal/controller/subnet_controller.go#L438-L438osac-operator/internal/controller/subnet_controller.go#L453-L453
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osac-operator/internal/controller/subnet_controller.go` at line 258, Remove
customer network data from the logging calls in subnet_controller.go: at lines
258-258, update the log in the annotation update flow to omit the vipCIDR field;
at lines 438-438 and 453-453, update the corresponding logs to omit the
addresses field. Preserve non-sensitive fields such as implementationStrategy
and existing log behavior.
Source: Coding guidelines
fullsend-ai-review
Bot
added
the
requires-manual-review
Auto-dismissed: only Prow labels gate merging
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, ori-amizur The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
Re-triggered failed runs:
|
931bcea
4 participants
At subnet creation, the operator now resolves vip_prefix_length from the NetworkClass and creates a MetalLB IPAddressPool on the target cluster covering the VIP sub-range of the subnet CIDR. On subnet deletion the IPAddressPool is removed before AAP deprovisioning.
The VIP range is also stamped as an osac.openshift.io/vip-cidr annotation on the Subnet CR so the fabric manager Ansible role can read it and exclude that range from DHCP.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests