fix(controller): apply service type from CR and default to ClusterIP

[jrse]

createArbiterService() never set Spec.Type from the CR's
spec.service.type, so the Service always defaulted to ClusterIP
regardless of user intent. determinePublicAddress() switched on the
Service type, but the actual object was always ClusterIP.

Additionally, when spec.service was nil the arbiter used
$(ROOK_POD_IP) as --public-addr, which is fragile across pod
rescheduling since the new IP is not propagated to the Ceph monmap.

• Set Spec.Type from spec.service.type in createArbiterService
• Default spec.service to {type: ClusterIP} in the webhook defaulter
  to eliminate the ephemeral pod IP path for new CRs
• Add controller integration test validating Service type propagation,
  port configuration, and --public-addr using the ClusterIP
• Add webhook test assertions for the new Service default

fixes: #86

Public address drift detection was considered but discarded: ClusterIP
is immutable for the Service lifetime, NodePort uses a static CR
field, and LoadBalancer IP changes are caught by the periodic
reconcile interval. Defaulting every
arbiter to a ClusterIP Service eliminates the most common drift
scenarios. A future enhancement could compare the
deployed --public-addr against determinePublicAddress() on each
reconcile to handle LoadBalancer IP changes.

[Copilot]

🟡 Changes recommended

Unresolved reconciliation and migration gaps can leave existing Services or deployments advertising stale addresses, and test coverage does not fully verify type and port propagation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes RemoteArbiter Service type propagation and defaults omitted services to stable ClusterIP addressing.

Changes:

• Default nil spec.service to ClusterIP.
• Apply configured Service types during creation.
• Add webhook and controller test coverage.

File summaries

File	Review summary
pkg/webhook/v1alpha1/remotearbiter_webhook.go	Adds Service defaulting. Moderate findings: persisted CRs are not migrated during reconciliation, and nil-to-default updates may not rebuild deployments with the new public address.
pkg/webhook/v1alpha1/remotearbiter_webhook_test.go	Tests Service defaulting and preservation.
pkg/controller/remotearbiter_controller.go	Applies the configured Service type. Moderate findings: existing Services are not reconciled, and LoadBalancer address changes can leave stale deployment arguments.
pkg/controller/remotearbiter_controller_test.go	Tests Service and public-address configuration. Moderate finding: the test does not prove non-default type propagation. Nit: expected port values should also be asserted.

Review details

Suppressed comments (4)

pkg/controller/remotearbiter_controller.go:983

• This only fixes Services created after the controller upgrade. Existing arbiters are merely fetched in fetchArbiterResources and their Service type is never reconciled, so a CR that already requested LoadBalancer or NodePort can retain the old ClusterIP Service and continue advertising the wrong address. Compare the existing Service with the CR and migrate/update it before deriving the public address.

			Type: s.remoteArbiter.Spec.Service.Type,

pkg/controller/remotearbiter_controller_test.go:695

• The new test only checks the number and names of the ports; a regression in the messenger port numbers, protocols, or target ports would still pass. Since this test is intended to cover port configuration, assert the expected values for both ServicePort entries as well.

			Expect(arbiterService.Spec.Ports).To(HaveLen(2))
			portNames := []string{arbiterService.Spec.Ports[0].Name, arbiterService.Spec.Ports[1].Name}
			Expect(portNames).To(ContainElements("tcp-msgr1", "tcp-msgr2"))
			// Verify ClusterIP was allocated

pkg/webhook/v1alpha1/remotearbiter_webhook.go:64

• Admission defaulting does not run when an already-persisted CR is merely reconciled. Thus existing arbiters with a nil spec.service remain on the no-Service path and continue using $(ROOK_POD_IP) after upgrading, leaving the stale-IP problem unresolved unless the user edits the CR. Consider applying the default in reconciliation or providing an explicit migration path.

	if remoteArbiter.Spec.Service == nil {
		remoteArbiter.Spec.Service = &v1alpha1.ServiceConfiguration{
			Type: corev1.ServiceTypeClusterIP,
		}
	}

pkg/webhook/v1alpha1/remotearbiter_webhook.go:64

• This defaulter also runs on updates, so an existing CR with a nil service is migrated to a Service on its first update. In that path the controller creates the Service but an existing arbiter Deployment is only rebuilt when source monitor resource versions change, so its command can continue advertising $(ROOK_POD_IP) despite the new ClusterIP. Detect this nil-to-default transition/service creation and update the Deployment, or explicitly handle migration of legacy CRs.

	if remoteArbiter.Spec.Service == nil {
		remoteArbiter.Spec.Service = &v1alpha1.ServiceConfiguration{
			Type: corev1.ServiceTypeClusterIP,
		}
	}

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[senolcolak]

P1 — repair Services created by the old controller. This applies the requested type only when a Service is first created. Existing installations affected by #86 already have a ClusterIP Service even when the CR requests NodePort or LoadBalancer; the fetch path keeps that object unchanged, so upgrading does not fix them. Please reconcile the existing Service type or provide an explicit migration/recreation path.

[senolcolak]

P1 — NodePort still advertises ports that are not reachable. These ports do not specify nodePort, so Kubernetes allocates high dynamic ports, while determinePublicAddress gives Ceph only the node IP and Ceph advertises 3300/6789. Traffic to those advertised endpoints will not reach the Service. Please model and advertise the allocated/configured NodePorts, or use an exposure strategy that preserves the Ceph ports.

[senolcolak]

P2 — migrate the existing deployment when this default is applied on update. Updating a legacy CR without spec.service now creates a ClusterIP Service, but deployment regeneration only follows source monitor resource-version changes. The existing deployment can therefore keep --public-addr=$(ROOK_POD_IP) and remain vulnerable to pod-IP drift even though the CR now contains a Service. Please detect this address change and rebuild the deployment immediately.

[senolcolak]

P2 — periodic reconciliation does not catch LoadBalancer address drift. The PR description says ingress changes are caught by the reconcile interval, but makeDeploymentSpec runs only when another tracked source resource changes. A new LoadBalancer IP therefore leaves the deployment and monmap on the old --public-addr. Please compare the calculated address with the deployed argument, or watch/reconcile Service changes explicitly.

[senolcolak]

Please add targeted coverage for the behavior this fix activates: an existing mismatched Service, defaulting a legacy deployment, NodePort address/port mapping, and LoadBalancer ingress changes. The current ClusterIP-only creation test would also pass when Kubernetes defaults an omitted Service type, so it does not exercise the non-default propagation that originally failed.