This repository was archived by the owner on Sep 9, 2026. It is now read-only.
MGMT-23908: Public IP feedback controller - #200
Merged
openshift-merge-bot[bot] merged 1 commit intoApr 29, 2026
Merged
openshift-merge-bot[bot] merged 1 commit into
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@DakCrowder: This pull request references MGMT-23908 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 sub-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. |
📝 WalkthroughWalkthroughAdds a new PublicIPFeedbackReconciler that synchronizes PublicIP hub CR status/finalizers with fulfillment gRPC records, maps CR phases to fulfillment states, issues Update/Signal calls, and is registered in main when a fulfillment gRPC connection is present. Changes
Sequence DiagramsequenceDiagram
participant K as Kubernetes API
participant M as Manager / Reconciler
participant G as Fulfillment gRPC
participant H as Hub Client
activate M
M->>H: Get PublicIP CR
H-->>M: CR (phase, address, finalizers, labels)
alt missing `publicip-uuid` label
M-->>M: Skip reconcile
else has `publicip-uuid`
M->>G: Get fulfillment PublicIP by id
G-->>M: Fulfillment or NotFound
alt Fulfillment exists
M-->>M: Map CR phase → fulfillment state
alt mapped state differs
M->>G: Update fulfillment (state, address)
G-->>M: Update response
end
alt CR is deleting and feedback finalizer last
M->>G: Signal fulfillment (id)
G-->>M: Signal response (logged if failed)
M->>K: Remove feedback finalizer from CR
K-->>M: Updated CR
else CR deleting but not last finalizer
M->>K: Ensure feedback finalizer present
K-->>M: Updated CR
end
else Fulfillment NotFound
alt CR deleting
M->>K: Remove feedback finalizer from CR
K-->>M: Updated CR
else not deleting
M-->>M: Return ErrPublicIPNotFound
end
end
end
deactivate M
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/controller/publicip_feedback_controller.go (2)
160-178: Defensive:response.GetObject()assumed non-nil.If the fulfillment service ever returns a response with a nil
Object(contract violation or a future change),publicIP.HasSpec()is nil-safe on generated protobuf getters, but the subsequentpublicIP.SetSpec(...)on a nil pointer will panic. A quick nil check (returningErrPublicIPNotFoundor a distinct error) would make this path more resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/publicip_feedback_controller.go` around lines 160 - 178, In fetchPublicIP, guard against a nil response.GetObject() to avoid a panic when calling publicIP.SetSpec/SetStatus; after calling response := r.publicIPsClient.Get(...) and before using response.GetObject(), check if response.GetObject() is nil and return a clear error (e.g., wrap ErrPublicIPNotFound or a new sentinel) so callers get a safe, explicit error instead of a panic; update the fetchPublicIP function to perform this nil check and return early when publicIP == nil.
128-155: Signal fires only if the k8s finalizer update succeeded; fire-and-forget after that is intentional.The order here is: (1) update the hub to drop the feedback finalizer; (2) send
Signal. If step 1 fails you return and retry; if step 2 fails you only log and rely on "periodic sync" — fine per the comment. One minor tidy: because you already verifiedContainsFinalizerat line 128,controllerutil.RemoveFinalizerat 134 will always returntrue, so the nestedifis dead-branching. Inverting the check or dropping the guard makes the flow clearer.♻️ Proposed tidy-up
- if controllerutil.RemoveFinalizer(object, osacPublicIPFeedbackFinalizer) { - if err := r.hubClient.Update(ctx, object); err != nil { - return ctrl.Result{}, err - } - } + controllerutil.RemoveFinalizer(object, osacPublicIPFeedbackFinalizer) + if err := r.hubClient.Update(ctx, object); err != nil { + return ctrl.Result{}, err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/publicip_feedback_controller.go` around lines 128 - 155, The code redundantly checks the boolean return of controllerutil.RemoveFinalizer after already confirming controllerutil.ContainsFinalizer(object, osacPublicIPFeedbackFinalizer); remove the dead nested guard and always call controllerutil.RemoveFinalizer(object, osacPublicIPFeedbackFinalizer) before updating via r.hubClient.Update(ctx, object), or invert the logic to call RemoveFinalizer only when ContainsFinalizer is true but without an additional if on RemoveFinalizer's return value; keep the subsequent r.publicIPsClient.Signal call and the same logging behavior unchanged.internal/controller/publicip_names.go (1)
27-30: Optional: drop redundantstringtype on the two vars.
fmt.Sprintfalready returnsstring, so the explicit type declaration is redundant and commonly flagged bystylecheck(ST1023).♻️ Proposed tidy-up
var ( - osacPublicIPIDLabel string = fmt.Sprintf("%s/publicip-uuid", osacPrefix) - osacPublicIPFeedbackFinalizer string = fmt.Sprintf("%s/publicip-feedback", osacPrefix) + osacPublicIPIDLabel = fmt.Sprintf("%s/publicip-uuid", osacPrefix) + osacPublicIPFeedbackFinalizer = fmt.Sprintf("%s/publicip-feedback", osacPrefix) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/publicip_names.go` around lines 27 - 30, The two vars osacPublicIPIDLabel and osacPublicIPFeedbackFinalizer declare an explicit string type despite being initialized with fmt.Sprintf (which returns string); remove the redundant "string" type from both declarations so they are simply: var osacPublicIPIDLabel = fmt.Sprintf(...) and var osacPublicIPFeedbackFinalizer = fmt.Sprintf(...).internal/controller/publicip_feedback_controller_test.go (1)
574-622: Optional: guard test reads ofmockServer.updates/signalswith the mutex, or snapshot them under the lock.
Update/Signal/addPublicIPwrite underm.mu, but assertions readmockServer.updatesandmockServer.signalsdirectly. In practice, these reads happen afterreconciler.Reconcilereturns (so the gRPC handlers have completed), butgo test -racemay still flag them because the synchronization goes through gRPC internals rather than the same mutex. A tinysnapshotUpdates()/snapshotSignals()helper that takesm.muand returns a copy would make the suite race-clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/publicip_feedback_controller_test.go` around lines 574 - 622, The test reads mockPublicIPsServer.updates and .signals without locking while the server methods (Update, Signal, addPublicIP) write under m.mu, which can trigger race detector warnings; fix by adding snapshot helpers (e.g., snapshotUpdates() and snapshotSignals() on mockPublicIPsServer) that lock m.mu, copy m.updates/m.signals and return the copies, and then use those snapshot helpers in assertions (or alternatively acquire m.mu in the test when reading these fields) to ensure reads are performed under the same mutex used by Update, Signal, and addPublicIP.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/main.go`:
- Around line 526-535: The PR wires the feedback controller
(NewPublicIPFeedbackReconciler) but never registers the corresponding resource
controller; add and register PublicIPReconciler so the operator implements the
required dual-controller pattern: create a PublicIPReconciler (implementing
provisioning logic to create/update MetalLB resources and set
PublicIP.status.phase, and emit AAP/EDA events as needed) and call its
SetupWithManager(localMgr) in the same conditional where
NewPublicIPFeedbackReconciler is registered (i.e., alongside the existing block
that checks grpcConn and uses localMgr.GetClient()); ensure the reconciler type
is named PublicIPReconciler and exposes a SetupWithManager method so the manager
starts it.
In `@internal/controller/publicip_feedback_controller_test.go`:
- Around line 62-100: The tests currently instantiate a fake Kubernetes client
via fake.NewClientBuilder() in BeforeEach; replace this with an envtest
environment so reconciliation runs against a real API server: create an
envtest.Environment, Start() it to obtain a rest.Config (cfg), register the
scheme with runtime.NewScheme() and use client.New(cfg, client.Options{Scheme:
scheme}) to build k8sClient, then pass that client into
NewPublicIPFeedbackReconciler; also ensure teardown in AfterEach stops the
envtest Environment (env.Stop()) and removes any fake.NewClientBuilder
references so the test uses envtest.Environment, client.New, and cfg instead of
the fake client.
---
Nitpick comments:
In `@internal/controller/publicip_feedback_controller_test.go`:
- Around line 574-622: The test reads mockPublicIPsServer.updates and .signals
without locking while the server methods (Update, Signal, addPublicIP) write
under m.mu, which can trigger race detector warnings; fix by adding snapshot
helpers (e.g., snapshotUpdates() and snapshotSignals() on mockPublicIPsServer)
that lock m.mu, copy m.updates/m.signals and return the copies, and then use
those snapshot helpers in assertions (or alternatively acquire m.mu in the test
when reading these fields) to ensure reads are performed under the same mutex
used by Update, Signal, and addPublicIP.
In `@internal/controller/publicip_feedback_controller.go`:
- Around line 160-178: In fetchPublicIP, guard against a nil
response.GetObject() to avoid a panic when calling publicIP.SetSpec/SetStatus;
after calling response := r.publicIPsClient.Get(...) and before using
response.GetObject(), check if response.GetObject() is nil and return a clear
error (e.g., wrap ErrPublicIPNotFound or a new sentinel) so callers get a safe,
explicit error instead of a panic; update the fetchPublicIP function to perform
this nil check and return early when publicIP == nil.
- Around line 128-155: The code redundantly checks the boolean return of
controllerutil.RemoveFinalizer after already confirming
controllerutil.ContainsFinalizer(object, osacPublicIPFeedbackFinalizer); remove
the dead nested guard and always call controllerutil.RemoveFinalizer(object,
osacPublicIPFeedbackFinalizer) before updating via r.hubClient.Update(ctx,
object), or invert the logic to call RemoveFinalizer only when ContainsFinalizer
is true but without an additional if on RemoveFinalizer's return value; keep the
subsequent r.publicIPsClient.Signal call and the same logging behavior
unchanged.
In `@internal/controller/publicip_names.go`:
- Around line 27-30: The two vars osacPublicIPIDLabel and
osacPublicIPFeedbackFinalizer declare an explicit string type despite being
initialized with fmt.Sprintf (which returns string); remove the redundant
"string" type from both declarations so they are simply: var osacPublicIPIDLabel
= fmt.Sprintf(...) and var osacPublicIPFeedbackFinalizer = fmt.Sprintf(...).
🪄 Autofix (Beta)
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 UI
Review profile: CHILL
Plan: Pro
Run ID: 472169b7-3f65-4b5e-9f3f-19b912957f81
⛔ Files ignored due to path filters (36)
internal/api/osac/private/v1/access_key_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/access_key_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/access_keys_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/access_keys_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/access_keys_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/host_types_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/host_types_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/network_classes_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/network_classes_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pools_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pools_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pools_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ips_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ips_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ips_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_groups_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/security_groups_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/subnet_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/subnet_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/subnets_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/subnets_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/user_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/user_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/users_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/users_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/users_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/virtual_network_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/virtual_network_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/virtual_networks_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/virtual_networks_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
api/v1alpha1/publicip_types.goapi/v1alpha1/zz_generated.deepcopy.gobuf.gen.yamlcmd/main.goconfig/crd/bases/osac.openshift.io_publicips.yamlinternal/controller/publicip_feedback_controller.gointernal/controller/publicip_feedback_controller_test.gointernal/controller/publicip_names.go
DakCrowder
force-pushed
the
public-ip-feedback-controller
branch
from
April 24, 2026 18:48
d945705 to
22c9ce4
Compare
DakCrowder
force-pushed
the
public-ip-feedback-controller
branch
from
April 27, 2026 21:03
22c9ce4 to
741126b
Compare
DakCrowder
force-pushed
the
public-ip-feedback-controller
branch
from
April 27, 2026 21:41
741126b to
f90a281
Compare
DakCrowder
force-pushed
the
public-ip-feedback-controller
branch
from
April 29, 2026 20:08
f90a281 to
5e347c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/publicip_feedback_controller_test.go`:
- Around line 82-100: The test creates a gRPC client connection via
grpc.NewClient and assigns it to a local conn but never closes it; update the
BeforeEach to assign the connection to an outer-scope variable named conn (used
when constructing NewPublicIPFeedbackReconciler) and add cleanup in AfterEach to
call conn.Close() (guarded by nil-check) so the gRPC client is properly closed
along with grpcServer and listener to avoid resource leaks.
🪄 Autofix (Beta)
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 UI
Review profile: CHILL
Plan: Pro
Run ID: ea3ef699-1dbc-405a-aec4-aacd5b103e7e
📒 Files selected for processing (4)
cmd/main.gointernal/controller/publicip_feedback_controller.gointernal/controller/publicip_feedback_controller_test.gointernal/controller/publicip_names.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/publicip_names.go
Comment on lines
+82
to
+100
| conn, err := grpc.NewClient("passthrough:///bufnet", | ||
| grpc.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) { | ||
| return listener.Dial() | ||
| }), | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| reconciler = NewPublicIPFeedbackReconciler(k8sClient, conn, publicIPNamespace) | ||
| }) | ||
|
|
||
| AfterEach(func() { | ||
| if grpcServer != nil { | ||
| grpcServer.Stop() | ||
| } | ||
| if listener != nil { | ||
| _ = listener.Close() | ||
| } | ||
| }) |
There was a problem hiding this comment.
Missing gRPC connection cleanup in AfterEach.
The grpc.NewClient connection created at lines 82-88 is not closed in AfterEach. While the server and listener are cleaned up, the client connection should also be closed to avoid resource leaks and ensure clean test teardown.
🧹 Proposed fix to close the gRPC connection
Add conn to the test variables and close it in AfterEach:
var (
ctx context.Context
k8sClient client.Client
mockServer *mockPublicIPsServer
reconciler *PublicIPFeedbackReconciler
grpcServer *grpc.Server
listener *bufconn.Listener
+ conn *grpc.ClientConn
)Update BeforeEach to assign to the outer variable:
- conn, err := grpc.NewClient("passthrough:///bufnet",
+ var err error
+ conn, err = grpc.NewClient("passthrough:///bufnet",Add cleanup in AfterEach:
AfterEach(func() {
+ if conn != nil {
+ _ = conn.Close()
+ }
if grpcServer != nil {
grpcServer.Stop()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/publicip_feedback_controller_test.go` around lines 82 -
100, The test creates a gRPC client connection via grpc.NewClient and assigns it
to a local conn but never closes it; update the BeforeEach to assign the
connection to an outer-scope variable named conn (used when constructing
NewPublicIPFeedbackReconciler) and add cleanup in AfterEach to call conn.Close()
(guarded by nil-check) so the gRPC client is properly closed along with
grpcServer and listener to avoid resource leaks.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, DakCrowder 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 |
Contributor
Author
|
/unhold |
This was referenced May 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a feedback controller for the Public IP resource. Follows patterns present in other feedback controllers. However, please note this cannot be tested e2e until the ansible templates are fully implemented and we will need a pass once that is done to ensure proper functionality beyond unit testing and some hacky patching.
Summary by CodeRabbit
New Features
Tests