Repository: nginx/nginx-gateway-fabric

Issue:
NGINX LoadBalancer Service IPFamily should match ipFamily from NginxProxy configuration
When provisioning the NGINX LoadBalancer Service, the IPFamily should be set based on the ipFamily that is set in the NginxProxy resource for the associated Gateway for that Service.

Right now we just create the Service with the Kubernetes default values, which will likely be SingleStack IPv4 in most cases.

Since a user can no longer manually edit the LoadBalancer Service as of our 2.0 release (due to the fact that the control plane provisions and controls it based on the NginxProxy config), this could block users from using dual stack IP traffic.

A user could set to SingleStack IPv6 if they configure their kube-apiserver with an IPv6 address range for the first argument in the `--service-cluster-ip-range` parameter, but I'm not sure if there's a way to configure k8s itself to default to dual stack.

### Acceptance
- When provisioning the NGINX Service, set the ipFamily and ipFamilyPolicy fields based on the ipFamily field in the NginxProxy resource (default is DualStack)

Discussion:
@aviallon: Thanks for reporting the issue.
Indeed, I am using the 2.0 release.
I guess I'll patch it myself for my own use.

By the way, would it make sense to pass-through any key Nginx doesn't manage to the LoadBalancer?@sjberman: > By the way, would it make sense to pass-through any key Nginx doesn't manage to the LoadBalancer?

Which keys are you referring to? Any of the fields in the Service spec?@aviallon: > > By the way, would it make sense to pass-through any key Nginx doesn't manage to the LoadBalancer?
> 
> Which keys are you referring to? Any of the fields in the Service spec?

Yes, indeed. This is the only way to cover everyone's use case.@sjberman: Our goal is to expose most (if not all) of the Service spec fields so that a user can configure them through our NginxProxy CRD. The current behavior is that if a user doesn't specify them, then we use the Kubernetes defaults. There aren't too many fields, but we missed the IPFamily here.

Unfortunately we decided against letting users edit the NGINX Service directly, because then we are stuck in a position that isn't very deterministic. When provisioning or updating the Service, we would have to decide whether to leave the fields empty (using Kubernetes defaults) or get the user's manual configuration and apply that (while having to know if the user's configuration is different from the k8s defaults so we know if we need to apply it or not). And then somehow reconcile that with the NginxProxy fields that tell our control plane how we need to configure the Service.

It gets messy :) so we decided that the NginxProxy resource is the source of truth, and if nothing is specified, then Kubernetes defaults are used by leaving those fields empty. We just need to ensure that all fields are available in there for users to set.@sjberman: Fix will be included in a patch release, hopefully next week. Thanks for bringing this to our attention!@sjberman: Once merged into main, you can also verify the fix in the `edge` release, before we release the patch.@aviallon: > Once merged into main, you can also verify the fix in the `edge` release, before we release the patch.

Very fast! Thank you!@aviallon: > Once merged into main, you can also verify the fix in the `edge` release, before we release the patch.

`Tested-By: Antoine VIALLON <antoine@lesviallon.fr>`

Solution patch:
diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go
index e4afe5b1..3b43ac8b 100644
--- a/internal/controller/provisioner/objects.go
+++ b/internal/controller/provisioner/objects.go
@@ -463,9 +463,12 @@ func buildNginxService(
 			Ports:                 servicePorts,
 			ExternalTrafficPolicy: servicePolicy,
 			Selector:              selectorLabels,
+			IPFamilyPolicy:        helpers.GetPointer(corev1.IPFamilyPolicyPreferDualStack),
 		},
 	}
 
+	setIPFamily(nProxyCfg, svc)
+
 	if serviceCfg.LoadBalancerIP != nil {
 		svc.Spec.LoadBalancerIP = *serviceCfg.LoadBalancerIP
 	}
@@ -479,6 +482,17 @@ func buildNginxService(
 	return svc
 }
 
+func setIPFamily(nProxyCfg *graph.EffectiveNginxProxy, svc *corev1.Service) {
+	if nProxyCfg != nil && nProxyCfg.IPFamily != nil && *nProxyCfg.IPFamily != ngfAPIv1alpha2.Dual {
+		svc.Spec.IPFamilyPolicy = helpers.GetPointer(corev1.IPFamilyPolicySingleStack)
+		if *nProxyCfg.IPFamily == ngfAPIv1alpha2.IPv4 {
+			svc.Spec.IPFamilies = []corev1.IPFamily{corev1.IPv4Protocol}
+		} else {
+			svc.Spec.IPFamilies = []corev1.IPFamily{corev1.IPv6Protocol}
+		}
+	}
+}
+
 func (p *NginxProvisioner) buildNginxDeployment(
 	objectMeta metav1.ObjectMeta,
 	nProxyCfg *graph.EffectiveNginxProxy,



Test patch:
diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go
index 061efc7c..96710f89 100644
--- a/internal/controller/provisioner/objects_test.go
+++ b/internal/controller/provisioner/objects_test.go
@@ -161,6 +161,7 @@ func TestBuildNginxResourceObjects(t *testing.T) {
 	validateMeta(svc)
 	g.Expect(svc.Spec.Type).To(Equal(defaultServiceType))
 	g.Expect(svc.Spec.ExternalTrafficPolicy).To(Equal(defaultServicePolicy))
+	g.Expect(*svc.Spec.IPFamilyPolicy).To(Equal(corev1.IPFamilyPolicyPreferDualStack))
 
 	// service ports is sorted in ascending order by port number when we make the nginx object
 	g.Expect(svc.Spec.Ports).To(Equal([]corev1.ServicePort{
@@ -260,6 +261,7 @@ func TestBuildNginxResourceObjects_NginxProxyConfig(t *testing.T) {
 
 	resourceName := "gw-nginx"
 	nProxyCfg := &graph.EffectiveNginxProxy{
+		IPFamily: helpers.GetPointer(ngfAPIv1alpha2.IPv4),
 		Logging: &ngfAPIv1alpha2.NginxLogging{
 			ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug),
 			AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug),
@@ -321,6 +323,8 @@ func TestBuildNginxResourceObjects_NginxProxyConfig(t *testing.T) {
 	g.Expect(svc.Spec.LoadBalancerIP).To(Equal("1.2.3.4"))
 	g.Expect(*svc.Spec.LoadBalancerClass).To(Equal("myLoadBalancerClass"))
 	g.Expect(svc.Spec.LoadBalancerSourceRanges).To(Equal([]string{"5.6.7.8"}))
+	g.Expect(*svc.Spec.IPFamilyPolicy).To(Equal(corev1.IPFamilyPolicySingleStack))
+	g.Expect(svc.Spec.IPFamilies).To(Equal([]corev1.IPFamily{corev1.IPv4Protocol}))
 
 	depObj := objects[5]
 	dep, ok := depObj.(*appsv1.Deployment)
@@ -961,3 +965,40 @@ func TestBuildNginxResourceObjectsForDeletion_OpenShift(t *testing.T) {
 	g.Expect(ok).To(BeTrue())
 	validateMeta(roleBinding, deploymentNSName.Name)
 }
+
+func TestSetIPFamily(t *testing.T) {
+	t.Parallel()
+	g := NewWithT(t)
+
+	newSvc := func() *corev1.Service {
+		return &corev1.Service{
+			Spec: corev1.ServiceSpec{},
+		}
+	}
+
+	// nProxyCfg is nil, should not set anything
+	svc := newSvc()
+	setIPFamily(nil, svc)
+	g.Expect(svc.Spec.IPFamilyPolicy).To(BeNil())
+	g.Expect(svc.Spec.IPFamilies).To(BeNil())
+
+	// nProxyCfg.IPFamily is nil, should not set anything
+	svc = newSvc()
+	setIPFamily(&graph.EffectiveNginxProxy{}, svc)
+	g.Expect(svc.Spec.IPFamilyPolicy).To(BeNil())
+	g.Expect(svc.Spec.IPFamilies).To(BeNil())
+
+	// nProxyCfg.IPFamily is IPv4, should set SingleStack and IPFamilies to IPv4
+	svc = newSvc()
+	ipFamily := ngfAPIv1alpha2.IPv4
+	setIPFamily(&graph.EffectiveNginxProxy{IPFamily: &ipFamily}, svc)
+	g.Expect(svc.Spec.IPFamilyPolicy).To(Equal(helpers.GetPointer(corev1.IPFamilyPolicySingleStack)))
+	g.Expect(svc.Spec.IPFamilies).To(Equal([]corev1.IPFamily{corev1.IPv4Protocol}))
+
+	// nProxyCfg.IPFamily is IPv6, should set SingleStack and IPFamilies to IPv6
+	svc = newSvc()
+	ipFamily = ngfAPIv1alpha2.IPv6
+	setIPFamily(&graph.EffectiveNginxProxy{IPFamily: &ipFamily}, svc)
+	g.Expect(svc.Spec.IPFamilyPolicy).To(Equal(helpers.GetPointer(corev1.IPFamilyPolicySingleStack)))
+	g.Expect(svc.Spec.IPFamilies).To(Equal([]corev1.IPFamily{corev1.IPv6Protocol}))
+}