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.

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

fix: handle flattened errors related to whole entities #4813

Merged
merged 1 commit into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ Adding a new version? You'll need three changes:

- No more "log.SetLogger(...) was never called..." log entry during shutdown of KIC
[#4738](https://github.com/Kong/kubernetes-ingress-controller/pull/4738)
- When Kong returns a flattened error related to a Kong entity, the entity's type and name
will be included in the message reported in `KongConfigurationApplyFailed` Kubernetes event
generated for it.
[#4813](https://github.com/Kong/kubernetes-ingress-controller/pull/4813)

### Changed

Expand Down
2 changes: 1 addition & 1 deletion internal/dataplane/sendconfig/error_handling_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestParseFlatEntityErrors(t *testing.T) {
APIVersion: "v1",
UID: "e7e5c93e-4d56-4cc3-8f4f-ff1fcbe95eb2",
Problems: map[string]string{
"": "failed conditional validation given value of field protocol",
"service:67338dc2-31fd-47b6-85a9-9c11d347d090.httpbin.httpbin.80": "failed conditional validation given value of field protocol",
"path": "value must be null",
},
},
Expand Down
51 changes: 43 additions & 8 deletions internal/dataplane/sendconfig/inmemory_error_handling.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,48 @@ type ConfigErrorFields struct{}

// FlatEntityError represents a single Kong entity with one or more invalid fields.
type FlatEntityError struct {
Name string `json:"entity_name,omitempty" yaml:"entity_name,omitempty"`
ID string `json:"entity_id,omitempty" yaml:"entity_id,omitempty"`
Tags []string `json:"entity_tags,omitempty" yaml:"entity_tags,omitempty"`
Errors []FlatFieldError `json:"errors,omitempty" yaml:"errors,omitempty"`
// Name is the name of the Kong entity.
Name string `json:"entity_name,omitempty" yaml:"entity_name,omitempty"`

// ID is the ID of the Kong entity.
ID string `json:"entity_id,omitempty" yaml:"entity_id,omitempty"`

// Tags are the tags of the Kong entity.
Tags []string `json:"entity_tags,omitempty" yaml:"entity_tags,omitempty"`

// Type is the type of the Kong entity.
Type string `json:"entity_type,omitempty" yaml:"entity_type,omitempty"`

// Errors are the errors associated with the Kong entity.
Errors []FlatError `json:"errors,omitempty" yaml:"errors,omitempty"`
}

// FlatFieldError represents an error for a single field within a Kong entity.
type FlatFieldError struct {
// FlatErrorType tells whether a FlatError is associated with a single field or a whole entity.
type FlatErrorType string

const (
// FlatErrorTypeField is an error associated with a single field.
FlatErrorTypeField FlatErrorType = "field"

// FlatErrorTypeEntity is an error associated with a whole entity.
FlatErrorTypeEntity FlatErrorType = "entity"
)

// FlatError represents an error for a single field within a Kong entity or a whole entity.
type FlatError struct {
// Field is the name of the entity's field that has an error.
// Optional: Field can be empty if the error is associated with the whole entity.
Field string `json:"field,omitempty" yaml:"field,omitempty"`
// Message is the error associated with Field for single-value fields.

// Message is the error associated with Field (for single-value fields) or with a whole entity when Type is "entity".
Message string `json:"message,omitempty" yaml:"message,omitempty"`

// Messages are the errors associated with Field for multi-value fields. The array index in Messages matches the
// array index in the input.
Messages []string `json:"messages,omitempty" yaml:"messages,omitempty"`

// Type tells whether the error is associated with a single field or a whole entity.
Type FlatErrorType `json:"type,omitempty" yaml:"type,omitempty"`
}

// parseFlatEntityErrors takes a Kong /config error response body and parses its "fields.flattened_errors" value
Expand Down Expand Up @@ -81,7 +109,14 @@ func parseFlatEntityErrors(body []byte, log logrus.FieldLogger) ([]ResourceError
continue
}
if len(p.Message) > 0 {
raw.Problems[p.Field] = p.Message
switch p.Type {
case FlatErrorTypeField:
// If the error is associated with a single field, store it in the map under the field name.
raw.Problems[p.Field] = p.Message
case FlatErrorTypeEntity:
// If the error is associated with a whole entity, store it in the map under the entity type and name.
raw.Problems[fmt.Sprintf("%s:%s", ee.Type, ee.Name)] = p.Message
}
}
if len(p.Messages) > 0 {
for i, message := range p.Messages {
Expand Down
8 changes: 4 additions & 4 deletions internal/dataplane/sendconfig/sendconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,14 @@ func resourceErrorsToResourceFailures(resourceErrors []ResourceError, parseErr e
UID: k8stypes.UID(ee.UID),
},
}
for field, problem := range ee.Problems {
log.Debug(fmt.Sprintf("adding failure for %s: %s = %s", ee.Name, field, problem))
for problemSource, problem := range ee.Problems {
log.Debug(fmt.Sprintf("adding failure for %s: %s = %s", ee.Name, problemSource, problem))
resourceFailure, failureCreateErr := failures.NewResourceFailure(
fmt.Sprintf("invalid %s: %s", field, problem),
fmt.Sprintf("invalid %s: %s", problemSource, problem),
&obj,
)
if failureCreateErr != nil {
log.WithError(failureCreateErr).Error("could create resource failure event")
log.WithError(failureCreateErr).Error("could not create resource failure event")
} else {
out = append(out, resourceFailure)
}
Expand Down
57 changes: 31 additions & 26 deletions test/envtest/configerrorevent_envtest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,32 +92,37 @@ func TestConfigErrorEventGeneration(t *testing.T) {
}
t.Logf("got %d events", len(events.Items))

return true && // For the sake of equal indentation
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Ingress" &&
e.InvolvedObject.Name == ingress.Name &&
e.Message == "invalid methods: cannot set 'methods' when 'protocols' is 'grpc' or 'grpcs'"
}) &&
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid path: value must be null"
}) &&
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid : failed conditional validation given value of field 'protocol'"
}) &&
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
ok, err := regexp.MatchString(`failed to apply Kong configuration to http://[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+:[0-9]+: failed posting new config to /config: got status code 400`, e.Message)
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Pod" &&
e.InvolvedObject.Name == podName &&
ok && err == nil
})
matches := make([]bool, 4)
matches[0] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Ingress" &&
e.InvolvedObject.Name == ingress.Name &&
e.Message == "invalid methods: cannot set 'methods' when 'protocols' is 'grpc' or 'grpcs'"
})
matches[1] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid path: value must be null"
})
matches[2] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid service:httpbin.httpbin.80: failed conditional validation given value of field 'protocol'"
})
matches[3] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
ok, err := regexp.MatchString(`failed to apply Kong configuration to http://[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+:[0-9]+: failed posting new config to /config: got status code 400`, e.Message)
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Pod" &&
e.InvolvedObject.Name == podName &&
ok && err == nil
})
if lo.Count(matches, true) != 4 {
t.Logf("not all events matched: %+v", matches)
return false
}
return true
}, waitTime, tickTime)

t.Log("push failure events recorded successfully")
Expand Down
47 changes: 28 additions & 19 deletions test/integration/config_error_event_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package integration

import (
"context"
"regexp"
"testing"

"github.com/kong/go-kong/kong"
Expand Down Expand Up @@ -80,25 +81,33 @@ func TestConfigErrorEventGeneration(t *testing.T) {
}
t.Logf("got %d events", len(events.Items))

return true && // For the sake of equal indentation
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Ingress" &&
e.InvolvedObject.Name == ingress.Name &&
e.Message == "invalid methods: cannot set 'methods' when 'protocols' is 'grpc' or 'grpcs'"
}) &&
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid path: value must be null"
}) &&
lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid : failed conditional validation given value of field 'protocol'"
})
matches := make([]bool, 3)
matches[0] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Ingress" &&
e.InvolvedObject.Name == ingress.Name &&
e.Message == "invalid methods: cannot set 'methods' when 'protocols' is 'grpc' or 'grpcs'"
})
matches[1] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
e.Message == "invalid path: value must be null"
})
matches[2] = lo.ContainsBy(events.Items, func(e corev1.Event) bool {
ok, err := regexp.MatchString(`invalid service:.+\.httpbin\.80: failed conditional validation given value of field 'protocol'`, e.Message)
return e.Reason == dataplane.KongConfigurationApplyFailedEventReason &&
e.InvolvedObject.Kind == "Service" &&
e.InvolvedObject.Name == service.Name &&
ok && err == nil
})

if lo.Count(matches, true) != 3 {
t.Logf("not all events matched: %+v", matches)
return false
}

return true
}, statusWait, waitTick)

t.Log("push failure events recorded successfully")
Expand Down
Loading