Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant warnings #502

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Remove redundant warnings #502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 1 addition & 15 deletions internal/state/change_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
63 changes: 4 additions & 59 deletions internal/state/dataplane/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down
116 changes: 4 additions & 112 deletions internal/state/dataplane/configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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: "",
},
},
},
Expand All @@ -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",
},
{
Expand Down Expand Up @@ -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()
Expand All @@ -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)
}
})
}
}
Expand Down Expand Up @@ -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",
Expand Down
32 changes: 0 additions & 32 deletions internal/state/dataplane/warnings.go
View file
Open in desktop

This file was deleted.

Loading