diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go index 77b7232d3e..9961f7820a 100644 --- a/internal/state/change_processor.go +++ b/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 diff --git a/internal/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go index 8de8baf17a..30650df628 100644 --- a/internal/state/dataplane/configuration.go +++ b/internal/state/dataplane/configuration.go @@ -97,25 +97,19 @@ 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, @@ -123,7 +117,7 @@ func BuildConfiguration( 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. diff --git a/internal/state/dataplane/configuration_test.go b/internal/state/dataplane/configuration_test.go index 4c448bc252..f313d6ffa3 100644 --- a/internal/state/dataplane/configuration_test.go +++ b/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", diff --git a/internal/state/dataplane/warnings.go b/internal/state/dataplane/warnings.go deleted file mode 100644 index 2bdad975c0..0000000000 --- a/internal/state/dataplane/warnings.go +++ /dev/null @@ -1,32 +0,0 @@ -package dataplane - -import ( - "fmt" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// Warnings stores a list of warnings for a given object. -type Warnings map[client.Object][]string - -func newWarnings() Warnings { - return make(map[client.Object][]string) -} - -// AddWarningf adds a warning for the specified object using the provided format and arguments. -func (w Warnings) AddWarningf(obj client.Object, msgFmt string, args ...interface{}) { - w[obj] = append(w[obj], fmt.Sprintf(msgFmt, args...)) -} - -// AddWarning adds a warning for the specified object. -func (w Warnings) AddWarning(obj client.Object, msg string) { - w[obj] = append(w[obj], msg) -} - -// Add adds new Warnings to the map. -// Warnings for the same object are merged. -func (w Warnings) Add(warnings Warnings) { - for k, v := range warnings { - w[k] = append(w[k], v...) - } -} diff --git a/internal/state/dataplane/warnings_test.go b/internal/state/dataplane/warnings_test.go deleted file mode 100644 index 08f0969db9..0000000000 --- a/internal/state/dataplane/warnings_test.go +++ /dev/null @@ -1,132 +0,0 @@ -package dataplane - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - "sigs.k8s.io/gateway-api/apis/v1beta1" -) - -func TestAddWarningf(t *testing.T) { - warnings := newWarnings() - obj := &v1beta1.HTTPRoute{} - - expected := Warnings{ - obj: []string{ - "simple", - "advanced 1", - }, - } - - warnings.AddWarningf(obj, "simple") - warnings.AddWarningf(obj, "advanced %d", 1) - - if diff := cmp.Diff(expected, warnings); diff != "" { - t.Errorf("AddWarningf mismatch (-want +got):\n%s", diff) - } -} - -func TestAddWarning(t *testing.T) { - warnings := newWarnings() - obj := &v1beta1.HTTPRoute{} - - expected := Warnings{ - obj: []string{ - "first", - "second", - }, - } - - warnings.AddWarning(obj, "first") - warnings.AddWarning(obj, "second") - - if diff := cmp.Diff(expected, warnings); diff != "" { - t.Errorf("AddWarning mismatch (-want +got):\n%s", diff) - } -} - -func TestAdd(t *testing.T) { - obj1 := &v1beta1.HTTPRoute{} - obj2 := &v1beta1.HTTPRoute{} - obj3 := &v1beta1.HTTPRoute{} - - tests := []struct { - warnings Warnings - addedWarnings Warnings - expected Warnings - msg string - }{ - { - warnings: newWarnings(), - addedWarnings: newWarnings(), - expected: newWarnings(), - msg: "empty warnings", - }, - { - warnings: Warnings{ - obj1: []string{ - "first", - }, - }, - addedWarnings: newWarnings(), - expected: Warnings{ - obj1: []string{ - "first", - }, - }, - msg: "empty added warnings", - }, - { - warnings: newWarnings(), - addedWarnings: Warnings{ - obj1: []string{ - "first", - }, - }, - expected: Warnings{ - obj1: []string{ - "first", - }, - }, - msg: "empty warnings", - }, - { - warnings: Warnings{ - obj1: []string{ - "first 1", - }, - obj3: []string{ - "first 3", - }, - }, - addedWarnings: Warnings{ - obj2: []string{ - "first 2", - }, - obj3: []string{ - "second 3", - }, - }, - expected: Warnings{ - obj1: []string{ - "first 1", - }, - obj2: []string{ - "first 2", - }, - obj3: []string{ - "first 3", - "second 3", - }, - }, - msg: "adding and merging", - }, - } - - for _, test := range tests { - test.warnings.Add(test.addedWarnings) - if diff := cmp.Diff(test.expected, test.warnings); diff != "" { - t.Errorf("Add() %q mismatch (-want +got):\n%s", test.msg, diff) - } - } -}