Remove redundant warnings (nginx#502)

The warnings reported by dataplane.BuildConfiguration() are now
redundant - they are reported as Conditions in the statuses of
Gateway API resources, which includes warnings-related cases for:
* invalid listener in an HTTPRoute
* unresolved backend ref an HTTPRoute

This commit removes the code, which generates and reports warnings.

Additionally, the commit fixes a related test TestBuildConfiguration
case "invalid listener": if a listener is invalid, it cannot include
any Routes and AcceptedHostnames, so those fields are removed.

Fixes nginx#467

Author: miledxz

internal/state/change_processor.go

@@ -168,21 +168,7 @@ func (c *ChangeProcessorImpl) Process(
 		c.cfg.Validators,
 	)
 
-	var warnings dataplane.Warnings
-	conf, warnings = dataplane.BuildConfiguration(ctx, g, c.cfg.ServiceResolver)
-
-	for obj, objWarnings := range warnings {
-		for _, w := range objWarnings {
-			// FIXME(pleshakov): report warnings via Object status
-			// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/467
-			c.cfg.Logger.Info("Got warning while building Graph",
-				"kind", obj.GetObjectKind().GroupVersionKind().Kind,
-				"namespace", obj.GetNamespace(),
-				"name", obj.GetName(),
-				"warning", w)
-		}
-	}
-
+	conf = dataplane.BuildConfiguration(ctx, g, c.cfg.ServiceResolver)
 	statuses = buildStatuses(g)
 
 	return true, conf, statuses

internal/state/dataplane/configuration.go

@@ -97,33 +97,27 @@ func (r *MatchRule) GetMatch() v1beta1.HTTPRouteMatch {
 
 // BuildConfiguration builds the Configuration from the Graph.
 // FIXME(pleshakov) For now we only handle paths with prefix matches. Handle exact and regex matches
-func BuildConfiguration(
-	ctx context.Context,
-	g *graph.Graph,
-	resolver resolver.ServiceResolver,
-) (Configuration, Warnings) {
+func BuildConfiguration(ctx context.Context, g *graph.Graph, resolver resolver.ServiceResolver) Configuration {
 	if g.GatewayClass == nil || !g.GatewayClass.Valid {
-		return Configuration{}, nil
+		return Configuration{}
 	}
 
 	if g.Gateway == nil {
-		return Configuration{}, nil
+		return Configuration{}
 	}
 
 	upstreamsMap := buildUpstreamsMap(ctx, g.Gateway.Listeners, resolver)
 	httpServers, sslServers := buildServers(g.Gateway.Listeners)
 	backendGroups := buildBackendGroups(g.Gateway.Listeners)
 
-	warnings := buildWarnings(g, upstreamsMap)
-
 	config := Configuration{
 		HTTPServers:   httpServers,
 		SSLServers:    sslServers,
 		Upstreams:     upstreamsMapToSlice(upstreamsMap),
 		BackendGroups: backendGroups,
 	}
 
-	return config, warnings
+	return config
 }
 
 func upstreamsMapToSlice(upstreamsMap map[string]Upstream) []Upstream {
@@ -139,55 +133,6 @@ func upstreamsMapToSlice(upstreamsMap map[string]Upstream) []Upstream {
 	return upstreams
 }
 
-func buildWarnings(graph *graph.Graph, upstreams map[string]Upstream) Warnings {
-	warnings := newWarnings()
-
-	for _, l := range graph.Gateway.Listeners {
-		for _, r := range l.Routes {
-			if !l.Valid {
-				warnings.AddWarningf(
-					r.Source,
-					"cannot configure routes for listener %s; listener is invalid",
-					l.Source.Name,
-				)
-
-				continue
-			}
-
-			for _, group := range r.GetAllBackendGroups() {
-				for _, backend := range group.Backends {
-					if backend.Name != "" {
-						upstream, ok := upstreams[backend.Name]
-
-						// this should never happen; but we check it just in case
-						if !ok {
-							warnings.AddWarningf(
-								r.Source,
-								"cannot resolve backend ref; internal error: upstream %s not found in map",
-								backend.Name,
-							)
-						}
-
-						if upstream.ErrorMsg != "" {
-							warnings.AddWarningf(
-								r.Source,
-								"cannot resolve backend ref: %s",
-								upstream.ErrorMsg,
-							)
-						}
-					}
-				}
-			}
-		}
-	}
-
-	if len(warnings) == 0 {
-		return nil
-	}
-
-	return warnings
-}
-
 func buildBackendGroups(listeners map[string]*graph.Listener) []graph.BackendGroup {
 	// There can be duplicate backend groups if a route is attached to multiple listeners.
 	// We use a map to deduplicate them.

internal/state/dataplane/configuration_test.go

@@ -262,10 +262,9 @@ func TestBuildConfiguration(t *testing.T) {
 	secretPath := "/etc/nginx/secrets/secret"
 
 	tests := []struct {
-		graph    *graph.Graph
-		expWarns Warnings
-		msg      string
-		expConf  Configuration
+		graph   *graph.Graph
+		msg     string
+		expConf Configuration
 	}{
 		{
 			graph: &graph.Graph{
@@ -371,15 +370,6 @@ func TestBuildConfiguration(t *testing.T) {
 						"invalid-listener": {
 							Source: invalidListener,
 							Valid:  false,
-							Routes: map[types.NamespacedName]*graph.Route{
-								{Namespace: "test", Name: "https-hr-1"}: httpsRouteHR1,
-								{Namespace: "test", Name: "https-hr-2"}: httpsRouteHR2,
-							},
-							AcceptedHostnames: map[string]struct{}{
-								"foo.example.com": {},
-								"bar.example.com": {},
-							},
-							SecretPath: "",
 						},
 					},
 				},
@@ -392,10 +382,6 @@ func TestBuildConfiguration(t *testing.T) {
 				HTTPServers: []VirtualServer{},
 				SSLServers:  []VirtualServer{},
 			},
-			expWarns: Warnings{
-				httpsHR1: []string{"cannot configure routes for listener invalid-listener; listener is invalid"},
-				httpsHR2: []string{"cannot configure routes for listener invalid-listener; listener is invalid"},
-			},
 			msg: "invalid listener",
 		},
 		{
@@ -978,7 +964,7 @@ func TestBuildConfiguration(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.msg, func(t *testing.T) {
-			result, warns := BuildConfiguration(context.TODO(), test.graph, fakeResolver)
+			result := BuildConfiguration(context.TODO(), test.graph, fakeResolver)
 
 			sort.Slice(result.BackendGroups, func(i, j int) bool {
 				return result.BackendGroups[i].GroupName() < result.BackendGroups[j].GroupName()
@@ -991,10 +977,6 @@ func TestBuildConfiguration(t *testing.T) {
 			if diff := cmp.Diff(test.expConf, result); diff != "" {
 				t.Errorf("BuildConfiguration() %q mismatch for configuration (-want +got):\n%s", test.msg, diff)
 			}
-
-			if diff := cmp.Diff(test.expWarns, warns); diff != "" {
-				t.Errorf("BuildConfiguration() %q mismatch for warnings (-want +got):\n%s", test.msg, diff)
-			}
 		})
 	}
 }
@@ -1471,96 +1453,6 @@ func TestBuildBackendGroups(t *testing.T) {
 	g.Expect(result).To(ConsistOf(expGroups))
 }
 
-func TestBuildWarnings(t *testing.T) {
-	createBackendRefs := func(names ...string) []graph.BackendRef {
-		backends := make([]graph.BackendRef, len(names))
-		for idx, name := range names {
-			backends[idx] = graph.BackendRef{Name: name}
-		}
-
-		return backends
-	}
-
-	createBackendGroup := func(sourceName string, backends []graph.BackendRef) graph.BackendGroup {
-		return graph.BackendGroup{
-			Source:   types.NamespacedName{Namespace: "test", Name: sourceName},
-			Backends: backends,
-		}
-	}
-
-	hrBackendGroup0 := createBackendGroup(
-		"hr",
-		createBackendRefs(""), // empty backend name should be skipped
-	)
-
-	hrBackendGroup1 := createBackendGroup(
-		"hr",
-		createBackendRefs("dne"),
-	)
-
-	hrInvalidGroup := createBackendGroup(
-		"hr-invalid",
-		createBackendRefs("invalid"),
-	)
-
-	hr := &v1beta1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Name: "hr", Namespace: "test"}}
-	hrInvalid := &v1beta1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Name: "hr-invalid", Namespace: "test"}}
-
-	invalidRoutes := map[types.NamespacedName]*graph.Route{
-		{Name: "invalid", Namespace: "test"}: {
-			Source: hrInvalid,
-			Rules:  groupsToValidRules(hrInvalidGroup),
-		},
-	}
-
-	routes := map[types.NamespacedName]*graph.Route{
-		{Name: "hr", Namespace: "test"}: {
-			Source: hr,
-			Rules:  groupsToValidRules(hrBackendGroup0, hrBackendGroup1),
-		},
-	}
-
-	upstreamMap := map[string]Upstream{
-		"foo":           {},
-		"bar":           {},
-		"resolve-error": {ErrorMsg: "resolve error"},
-	}
-
-	graph := &graph.Graph{
-		Gateway: &graph.Gateway{
-			Listeners: map[string]*graph.Listener{
-				"invalid-listener": {
-					Source: v1beta1.Listener{
-						Name: "invalid",
-					},
-					Valid:  false,
-					Routes: invalidRoutes,
-				},
-				"listener": {
-					Source: v1beta1.Listener{
-						Name: "valid",
-					},
-					Valid:  true,
-					Routes: routes,
-				},
-			},
-		},
-	}
-
-	expWarns := Warnings{
-		hr: []string{
-			"cannot resolve backend ref; internal error: upstream dne not found in map",
-		},
-		hrInvalid: []string{"cannot configure routes for listener invalid; listener is invalid"},
-	}
-
-	g := NewGomegaWithT(t)
-
-	warns := buildWarnings(graph, upstreamMap)
-
-	g.Expect(helpers.Diff(warns, expWarns)).To(BeEmpty())
-}
-
 func TestUpstreamsMapToSlice(t *testing.T) {
 	fooUpstream := Upstream{
 		Name: "foo",