Remove error condition on httproute when nginx reload fails (#3936) (#3938)

Problem: When nginx reload fails, all routes are marked invalid

Solution: Remove adding GatewayNotProgrammed condition to routes since the error should only be reflected on Gateway Listener conditions

Author: salonichf5

internal/controller/handler.go

@@ -337,7 +337,6 @@ func (h *eventHandlerImpl) updateStatuses(ctx context.Context, gr *graph.Graph,
 		gr.L4Routes,
 		gr.Routes,
 		transitionTime,
-		gw.LatestReloadResult,
 		h.cfg.gatewayCtlrName,
 	)
 

internal/controller/state/conditions/conditions.go

@@ -49,10 +49,6 @@ const (
 	// as another route.
 	RouteReasonHostnameConflict v1.RouteConditionReason = "HostnameConflict"
 
-	// RouteReasonGatewayNotProgrammed is used when the associated Gateway is not programmed.
-	// Used with Accepted (false).
-	RouteReasonGatewayNotProgrammed v1.RouteConditionReason = "GatewayNotProgrammed"
-
 	// RouteReasonUnsupportedConfiguration is used when the associated Gateway does not support the Route.
 	// Used with Accepted (false).
 	RouteReasonUnsupportedConfiguration v1.RouteConditionReason = "UnsupportedConfiguration"
@@ -472,17 +468,6 @@ func NewRouteUnsupportedConfiguration(msg string) Condition {
 	}
 }
 
-// NewRouteGatewayNotProgrammed returns a Condition that indicates that the Gateway it references is not programmed,
-// which does not guarantee that the Route has been configured.
-func NewRouteGatewayNotProgrammed(msg string) Condition {
-	return Condition{
-		Type:    string(v1.RouteConditionAccepted),
-		Status:  metav1.ConditionFalse,
-		Reason:  string(RouteReasonGatewayNotProgrammed),
-		Message: msg,
-	}
-}
-
 // NewRouteInvalidIPFamily returns a Condition that indicates that the Service associated with the Route
 // is not configured with the same IP family as the NGINX server.
 func NewRouteInvalidIPFamily(msg string) Condition {

internal/controller/status/prepare_requests.go

@@ -22,7 +22,6 @@ func PrepareRouteRequests(
 	l4routes map[graph.L4RouteKey]*graph.L4Route,
 	routes map[graph.RouteKey]*graph.L7Route,
 	transitionTime metav1.Time,
-	nginxReloadRes graph.NginxReloadResult,
 	gatewayCtlrName string,
 ) []UpdateRequest {
 	reqs := make([]UpdateRequest, 0, len(routes))
@@ -32,7 +31,6 @@ func PrepareRouteRequests(
 			gatewayCtlrName,
 			r.ParentRefs,
 			r.Conditions,
-			nginxReloadRes,
 			transitionTime,
 			r.Source.GetGeneration(),
 		)
@@ -55,7 +53,6 @@ func PrepareRouteRequests(
 			gatewayCtlrName,
 			r.ParentRefs,
 			r.Conditions,
-			nginxReloadRes,
 			transitionTime,
 			r.Source.GetGeneration(),
 		)
@@ -134,7 +131,6 @@ func prepareRouteStatus(
 	gatewayCtlrName string,
 	parentRefs []graph.ParentRef,
 	conds []conditions.Condition,
-	nginxReloadRes graph.NginxReloadResult,
 	transitionTime metav1.Time,
 	srcGeneration int64,
 ) v1.RouteStatus {
@@ -163,13 +159,6 @@ func prepareRouteStatus(
 			allConds = append(allConds, ref.Attachment.FailedConditions...)
 		}
 
-		if nginxReloadRes.Error != nil {
-			allConds = append(
-				allConds,
-				conditions.NewRouteGatewayNotProgrammed(conditions.RouteMessageFailedNginxReload),
-			)
-		}
-
 		conds := conditions.DeduplicateConditions(allConds)
 		apiConds := conditions.ConvertConditions(conds, srcGeneration, transitionTime)
 

internal/controller/status/prepare_requests_test.go

@@ -432,7 +432,6 @@ func TestBuildHTTPRouteStatuses(t *testing.T) {
 		map[graph.L4RouteKey]*graph.L4Route{},
 		routes,
 		transitionTime,
-		graph.NginxReloadResult{},
 		gatewayCtlrName,
 	)
 
@@ -511,7 +510,6 @@ func TestBuildGRPCRouteStatuses(t *testing.T) {
 		map[graph.L4RouteKey]*graph.L4Route{},
 		routes,
 		transitionTime,
-		graph.NginxReloadResult{},
 		gatewayCtlrName,
 	)
 
@@ -588,7 +586,6 @@ func TestBuildTLSRouteStatuses(t *testing.T) {
 		routes,
 		map[graph.RouteKey]*graph.L7Route{},
 		transitionTime,
-		graph.NginxReloadResult{},
 		gatewayCtlrName,
 	)
 
@@ -605,108 +602,6 @@ func TestBuildTLSRouteStatuses(t *testing.T) {
 	}
 }
 
-func TestBuildRouteStatusesNginxErr(t *testing.T) {
-	t.Parallel()
-	const gatewayCtlrName = "controller"
-
-	hr1 := &v1.HTTPRoute{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace:  "test",
-			Name:       "hr-valid",
-			Generation: 3,
-		},
-		Spec: v1.HTTPRouteSpec{
-			CommonRouteSpec: commonRouteSpecValid,
-		},
-	}
-
-	routeKey := graph.CreateRouteKey(hr1)
-
-	gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"}
-
-	routes := map[graph.RouteKey]*graph.L7Route{
-		routeKey: {
-			Valid:     true,
-			RouteType: graph.RouteTypeHTTP,
-			Source:    hr1,
-			ParentRefs: []graph.ParentRef{
-				{
-					Idx:     0,
-					Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName},
-					Attachment: &graph.ParentRefAttachmentStatus{
-						Attached: true,
-					},
-					SectionName: commonRouteSpecValid.ParentRefs[0].SectionName,
-				},
-			},
-		},
-	}
-
-	transitionTime := helpers.PrepareTimeForFakeClient(metav1.Now())
-
-	expectedStatus := v1.HTTPRouteStatus{
-		RouteStatus: v1.RouteStatus{
-			Parents: []v1.RouteParentStatus{
-				{
-					ParentRef: v1.ParentReference{
-						Namespace:   helpers.GetPointer(v1.Namespace(gwNsName.Namespace)),
-						Name:        v1.ObjectName(gwNsName.Name),
-						SectionName: helpers.GetPointer[v1.SectionName]("listener-80-1"),
-					},
-					ControllerName: gatewayCtlrName,
-					Conditions: []metav1.Condition{
-						{
-							Type:               string(v1.RouteConditionResolvedRefs),
-							Status:             metav1.ConditionTrue,
-							ObservedGeneration: 3,
-							LastTransitionTime: transitionTime,
-							Reason:             string(v1.RouteReasonResolvedRefs),
-							Message:            "All references are resolved",
-						},
-						{
-							Type:               string(v1.RouteConditionAccepted),
-							Status:             metav1.ConditionFalse,
-							ObservedGeneration: 3,
-							LastTransitionTime: transitionTime,
-							Reason:             string(conditions.RouteReasonGatewayNotProgrammed),
-							Message:            conditions.RouteMessageFailedNginxReload,
-						},
-					},
-				},
-			},
-		},
-	}
-
-	g := NewWithT(t)
-
-	k8sClient := createK8sClientFor(&v1.HTTPRoute{})
-
-	for _, r := range routes {
-		err := k8sClient.Create(context.Background(), r.Source)
-		g.Expect(err).ToNot(HaveOccurred())
-	}
-
-	updater := NewUpdater(k8sClient, logr.Discard())
-
-	reqs := PrepareRouteRequests(
-		map[graph.L4RouteKey]*graph.L4Route{},
-		routes,
-		transitionTime,
-		graph.NginxReloadResult{Error: errors.New("test error")},
-		gatewayCtlrName,
-	)
-
-	g.Expect(reqs).To(HaveLen(1))
-
-	updater.Update(context.Background(), reqs...)
-
-	var hr v1.HTTPRoute
-
-	err := k8sClient.Get(context.Background(), routeKey.NamespacedName, &hr)
-	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(helpers.Diff(expectedStatus, hr.Status)).To(BeEmpty())
-}
-
 func TestBuildGatewayClassStatuses(t *testing.T) {
 	t.Parallel()
 	transitionTime := helpers.PrepareTimeForFakeClient(metav1.Now())

tests/suite/snippets_filter_test.go

@@ -14,7 +14,6 @@ import (
 	v1 "sigs.k8s.io/gateway-api/apis/v1"
 
 	ngfAPI "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1"
-	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions"
 	"github.com/nginx/nginx-gateway-fabric/v2/tests/framework"
 )
 
@@ -28,7 +27,8 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter
 
 		namespace = "snippets-filter"
 
-		nginxPodName string
+		nginxPodName  string
+		gatewayNsName = types.NamespacedName{Name: "gateway", Namespace: namespace}
 	)
 
 	BeforeAll(func() {
@@ -230,9 +230,8 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter
 
 			Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed())
 
-			nsname := types.NamespacedName{Name: "tea", Namespace: namespace}
-			Eventually(checkHTTPRouteToHaveGatewayNotProgrammedCond).
-				WithArguments(nsname).
+			Eventually(checkGatewayToHaveGatewayNotProgrammedCond).
+				WithArguments(gatewayNsName).
 				WithTimeout(timeoutConfig.GetStatusTimeout).
 				WithPolling(500 * time.Millisecond).
 				Should(Succeed())
@@ -245,9 +244,8 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter
 
 			Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed())
 
-			nsname := types.NamespacedName{Name: "soda", Namespace: namespace}
-			Eventually(checkHTTPRouteToHaveGatewayNotProgrammedCond).
-				WithArguments(nsname).
+			Eventually(checkGatewayToHaveGatewayNotProgrammedCond).
+				WithArguments(gatewayNsName).
 				WithTimeout(timeoutConfig.GetStatusTimeout).
 				WithPolling(500 * time.Millisecond).
 				Should(Succeed())
@@ -257,42 +255,52 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter
 	})
 })
 
-func checkHTTPRouteToHaveGatewayNotProgrammedCond(httpRouteNsName types.NamespacedName) error {
+func checkGatewayToHaveGatewayNotProgrammedCond(gatewayNsName types.NamespacedName) error {
 	ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout)
 	defer cancel()
 
 	GinkgoWriter.Printf(
-		"Checking for HTTPRoute %q to have the condition Accepted/True/GatewayNotProgrammed\n",
-		httpRouteNsName,
+		"Checking for Gateway %q to have the condition Programmed/False/Invalid \n",
+		gatewayNsName,
 	)
 
-	var hr v1.HTTPRoute
+	var gw v1.Gateway
 	var err error
 
-	if err = k8sClient.Get(ctx, httpRouteNsName, &hr); err != nil {
+	if err = k8sClient.Get(ctx, gatewayNsName, &gw); err != nil {
 		return err
 	}
 
-	if len(hr.Status.Parents) != 1 {
-		return fmt.Errorf("httproute has %d parent statuses, expected 1", len(hr.Status.Parents))
+	gwStatus := gw.Status
+	if gwStatus.Conditions == nil {
+		nilConditionErr := fmt.Errorf("expected gateway conditions to not be nil")
+		GinkgoWriter.Printf("ERROR: %v\n", nilConditionErr)
+
+		return nilConditionErr
 	}
 
-	parent := hr.Status.Parents[0]
-	if parent.Conditions == nil {
-		return fmt.Errorf("expected parent conditions to not be nil")
+	for i := range gwStatus.Conditions {
+		GinkgoWriter.Printf("Gateway condition %d: Type=%s, Status=%s, Reason=%s\n",
+			i, gwStatus.Conditions[i].Type, gwStatus.Conditions[i].Status, gwStatus.Conditions[i].Reason)
 	}
 
-	cond := parent.Conditions[1]
-	if cond.Type != string(v1.GatewayConditionAccepted) {
-		return fmt.Errorf("expected condition type to be Accepted, got %s", cond.Type)
+	cond := gwStatus.Conditions[1]
+	if cond.Type != string(v1.GatewayConditionProgrammed) {
+		wrongTypeErr := fmt.Errorf("expected condition type to be Programmed, got %s", cond.Type)
+		GinkgoWriter.Printf("ERROR: %v\n", wrongTypeErr)
+
+		return wrongTypeErr
 	}
 
 	if cond.Status != metav1.ConditionFalse {
 		return fmt.Errorf("expected condition status to be False, got %s", cond.Status)
 	}
 
-	if cond.Reason != string(conditions.RouteReasonGatewayNotProgrammed) {
-		return fmt.Errorf("expected condition reason to be GatewayNotProgrammed, got %s", cond.Reason)
+	if cond.Reason != string(v1.GatewayReasonInvalid) {
+		wrongReasonErr := fmt.Errorf("expected condition reason to be GatewayReasonInvalid, got %s", cond.Reason)
+		GinkgoWriter.Printf("ERROR: %v\n", wrongReasonErr)
+
+		return wrongReasonErr
 	}
 
 	return nil