Update service changed predicate

Author: bjee19

internal/controller/manager.go

@@ -440,7 +440,7 @@ func registerControllers(
 			objectType: &apiv1.Service{},
 			name:       "user-service", // unique controller names are needed and we have multiple Service ctlrs
 			options: []controller.Option{
-				controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}),
+				controller.WithK8sPredicate(predicate.ServiceChangedPredicate{}),
 			},
 		},
 		{

internal/framework/controller/predicate/service.go

@@ -7,20 +7,21 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
-// ServicePortsChangedPredicate implements an update predicate function based on the Ports of a Service.
-// This predicate will skip update events that have no change in the Service Ports and TargetPorts.
-type ServicePortsChangedPredicate struct {
+// ServiceChangedPredicate implements an update predicate function for a Service.
+// This predicate will skip update events that have no change in a Service's Ports, TargetPorts, or AppProtocols.
+type ServiceChangedPredicate struct {
 	predicate.Funcs
 }
 
-// ports contains the ports that the Gateway cares about.
-type ports struct {
+// portInfo contains the information that the Gateway cares about.
+type portInfo struct {
 	targetPort  intstr.IntOrString
+	appProtocol string
 	servicePort int32
 }
 
-// Update implements default UpdateEvent filter for validating Service port changes.
-func (ServicePortsChangedPredicate) Update(e event.UpdateEvent) bool {
+// Update implements default UpdateEvent filter for validating Service port information changes.
+func (ServiceChangedPredicate) Update(e event.UpdateEvent) bool {
 	if e.ObjectOld == nil {
 		return false
 	}
@@ -45,12 +46,30 @@ func (ServicePortsChangedPredicate) Update(e event.UpdateEvent) bool {
 		return true
 	}
 
-	oldPortSet := make(map[ports]struct{})
-	newPortSet := make(map[ports]struct{})
+	oldPortSet := make(map[portInfo]struct{})
+	newPortSet := make(map[portInfo]struct{})
 
 	for i := range len(oldSvc.Spec.Ports) {
-		oldPortSet[ports{servicePort: oldPorts[i].Port, targetPort: oldPorts[i].TargetPort}] = struct{}{}
-		newPortSet[ports{servicePort: newPorts[i].Port, targetPort: newPorts[i].TargetPort}] = struct{}{}
+		var oldAppProtocol, newAppProtocl string
+
+		if oldPorts[i].AppProtocol != nil {
+			oldAppProtocol = *oldPorts[i].AppProtocol
+		}
+
+		if newPorts[i].AppProtocol != nil {
+			newAppProtocl = *newPorts[i].AppProtocol
+		}
+
+		oldPortSet[portInfo{
+			servicePort: oldPorts[i].Port,
+			targetPort:  oldPorts[i].TargetPort,
+			appProtocol: oldAppProtocol,
+		}] = struct{}{}
+		newPortSet[portInfo{
+			servicePort: newPorts[i].Port,
+			targetPort:  newPorts[i].TargetPort,
+			appProtocol: newAppProtocl,
+		}] = struct{}{}
 	}
 
 	for pd := range oldPortSet {

internal/framework/controller/predicate/service_test.go

@@ -8,9 +8,11 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/event"
+
+	"github.com/nginx/nginx-gateway-fabric/internal/framework/helpers"
 )
 
-func TestServicePortsChangedPredicate_Update(t *testing.T) {
+func TestServiceChangedPredicate_Update(t *testing.T) {
 	t.Parallel()
 	testcases := []struct {
 		objectOld client.Object
@@ -221,9 +223,145 @@ func TestServicePortsChangedPredicate_Update(t *testing.T) {
 			},
 			expUpdate: false,
 		},
+		{
+			msg: "appProtocol changed",
+			objectOld: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							AppProtocol: helpers.GetPointer("oldAppProtocol"),
+						},
+					},
+				},
+			},
+			objectNew: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							AppProtocol: helpers.GetPointer("newAppProtocol"),
+						},
+					},
+				},
+			},
+			expUpdate: true,
+		},
+		{
+			msg: "appProtocol stayed the same",
+			objectOld: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							AppProtocol: helpers.GetPointer("sameAppProtocol"),
+						},
+					},
+				},
+			},
+			objectNew: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							AppProtocol: helpers.GetPointer("sameAppProtocol"),
+						},
+					},
+				},
+			},
+			expUpdate: false,
+		},
+		{
+			msg: "multiple appProtocols stayed the same",
+			objectOld: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							Port:        80,
+							TargetPort:  intstr.FromInt(80),
+							AppProtocol: helpers.GetPointer("sameAppProtocol80"),
+						},
+						{
+							Port:        81,
+							TargetPort:  intstr.FromInt(81),
+							AppProtocol: helpers.GetPointer("sameAppProtocol81"),
+						},
+						{
+							Port:        82,
+							TargetPort:  intstr.FromInt(82),
+							AppProtocol: helpers.GetPointer("sameAppProtocol82"),
+						},
+					},
+				},
+			},
+			objectNew: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							Port:        80,
+							TargetPort:  intstr.FromInt(80),
+							AppProtocol: helpers.GetPointer("sameAppProtocol80"),
+						},
+						{
+							Port:        81,
+							TargetPort:  intstr.FromInt(81),
+							AppProtocol: helpers.GetPointer("sameAppProtocol81"),
+						},
+						{
+							Port:        82,
+							TargetPort:  intstr.FromInt(82),
+							AppProtocol: helpers.GetPointer("sameAppProtocol82"),
+						},
+					},
+				},
+			},
+			expUpdate: false,
+		},
+		{
+			msg: "multiple appProtocols with one changing",
+			objectOld: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							Port:        80,
+							TargetPort:  intstr.FromInt(80),
+							AppProtocol: helpers.GetPointer("sameAppProtocol80"),
+						},
+						{
+							Port:        81,
+							TargetPort:  intstr.FromInt(81),
+							AppProtocol: helpers.GetPointer("sameAppProtocol81"),
+						},
+						{
+							Port:        82,
+							TargetPort:  intstr.FromInt(82),
+							AppProtocol: helpers.GetPointer("sameAppProtocol82"),
+						},
+					},
+				},
+			},
+			objectNew: &v1.Service{
+				Spec: v1.ServiceSpec{
+					Ports: []v1.ServicePort{
+						{
+							Port:        80,
+							TargetPort:  intstr.FromInt(80),
+							AppProtocol: helpers.GetPointer("sameAppProtocol80"),
+						},
+						{
+							Port:        81,
+							TargetPort:  intstr.FromInt(81),
+							AppProtocol: helpers.GetPointer("sameAppProtocol81"),
+						},
+						{
+							Port:        82,
+							TargetPort:  intstr.FromInt(82),
+							AppProtocol: helpers.GetPointer("differentAppProtocol"),
+						},
+					},
+				},
+			},
+			expUpdate: true,
+		},
 	}
 
-	p := ServicePortsChangedPredicate{}
+	p := ServiceChangedPredicate{}
 
 	for _, tc := range testcases {
 		t.Run(tc.msg, func(t *testing.T) {
@@ -243,7 +381,7 @@ func TestServicePortsChangedPredicate(t *testing.T) {
 	t.Parallel()
 	g := NewWithT(t)
 
-	p := ServicePortsChangedPredicate{}
+	p := ServiceChangedPredicate{}
 
 	g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeTrue())
 	g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue())

internal/framework/controller/register_test.go

@@ -142,7 +142,7 @@ func TestRegister(t *testing.T) {
 					test.fakes.mgr,
 					eventCh,
 					controller.WithNamespacedNameFilter(nsNameFilter),
-					controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}),
+					controller.WithK8sPredicate(predicate.ServiceChangedPredicate{}),
 					controller.WithFieldIndices(fieldIndexes),
 					controller.WithNewReconciler(newReconciler),
 					controller.WithOnlyMetadata(),