Skip to content

Commit

Permalink
Implement config validation for the application logger aspect. (#499)
Browse files Browse the repository at this point in the history 
Former-commit-id: dda088aa22625a7e0deabbe922ab78a4bd4e9992
  • Loading branch information
@ZackButcher
ZackButcher committed Mar 31, 2017
1 parent faac07f commit be8181e
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 72 deletions.
68 changes: 34 additions & 34 deletions mixer/pkg/aspect/applicationLogsManager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,29 +74,11 @@ func newApplicationLogsManager() ReportManager {
func (applicationLogsManager) NewReportExecutor(c *cpb.Combined, a adapter.Builder, env adapter.Env, df descriptor.Finder) (ReportExecutor, error) {
// TODO: look up actual descriptors by name and build an array
cfg := c.Aspect.Params.(*aconfig.ApplicationLogsParams)

desc := []*dpb.LogEntryDescriptor{
{
Name: "default",
DisplayName: "Default Log Entry",
Description: "Placeholder log descriptor",
},
}

metadata := make(map[string]*logInfo)
for _, d := range desc {
l, found := findLog(cfg.Logs, d.Name)
if !found {
env.Logger().Warningf("No application log found for descriptor %s, skipping it", d.Name)
continue
}

// TODO: remove this error handling once validate config is given descriptors to validate
t, err := template.New(d.Name).Parse(d.LogTemplate)
if err != nil {
env.Logger().Warningf("log descriptors %s's template '%s' failed to parse with err: %s", d.Name, d.LogTemplate, err)
}

for _, l := range cfg.Logs {
// validation ensures both that the descriptor exists and that its template is parsable by the template library.
d := df.GetLog(l.DescriptorName)
t, _ := template.New(d.Name).Parse(d.LogTemplate)
metadata[d.Name] = &logInfo{
format: payloadFormatFromProto(d.PayloadFormat),
severity: l.Severity,
Expand All @@ -110,7 +92,7 @@ func (applicationLogsManager) NewReportExecutor(c *cpb.Combined, a adapter.Build

asp, err := a.(adapter.ApplicationLogsBuilder).NewApplicationLogsAspect(env, c.Builder.Params.(adapter.Config))
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to construct application logs aspect with err: %v", err)
}

return &applicationLogsExecutor{
Expand All @@ -126,8 +108,35 @@ func (applicationLogsManager) DefaultConfig() config.AspectParams {
}

// TODO: validation of timestamp format
func (applicationLogsManager) ValidateConfig(config.AspectParams, expr.Validator, descriptor.Finder) (ce *adapter.ConfigErrors) {
return nil
func (applicationLogsManager) ValidateConfig(c config.AspectParams, v expr.Validator, df descriptor.Finder) (ce *adapter.ConfigErrors) {
cfg := c.(*aconfig.ApplicationLogsParams)
if cfg.LogName == "" {
ce = ce.Appendf("LogName", "no log name provided")
}

for _, log := range cfg.Logs {
desc := df.GetLog(log.DescriptorName)
if desc == nil {
ce = ce.Appendf("Logs", "could not find a descriptor for the log '%s'", log.DescriptorName)
continue // we can't do any other validation without the descriptor
}

if err := v.AssertType(log.Severity, df, dpb.STRING); err != nil {
ce = ce.Appendf(fmt.Sprintf("Logs[%s].Severity", log.DescriptorName), "failed type checking with err: %v", err)
}
if err := v.AssertType(log.Timestamp, df, dpb.TIMESTAMP); err != nil {
ce = ce.Appendf(fmt.Sprintf("Logs[%s].Timestamp", log.DescriptorName), "failed type checking with err: %v", err)
}
ce = ce.Extend(validateLabels(fmt.Sprintf("Logs[%s].Labels", log.DescriptorName), log.Labels, desc.Labels, v, df))

// TODO: how do we validate the log.TemplateExpressions against desc.LogTemplate? We can't just `Execute` the template
// against the expressions: while the keys to the template may be correct, the values will be wrong which could result
// in non-nil error returns even when things would be valid at runtime.
if _, err := template.New(desc.Name).Parse(desc.LogTemplate); err != nil {
ce = ce.Appendf(fmt.Sprintf("LogDescriptor[%s].LogTemplate", desc.Name), "failed to parse template with err: %v", err)
}
}
return
}

func (e *applicationLogsExecutor) Close() error { return e.aspect.Close() }
Expand Down Expand Up @@ -211,15 +220,6 @@ func (e *applicationLogsExecutor) Execute(attrs attribute.Bag, mapper expr.Evalu
return status.OK
}

func findLog(defs []*aconfig.ApplicationLogsParams_ApplicationLog, name string) (*aconfig.ApplicationLogsParams_ApplicationLog, bool) {
for _, def := range defs {
if def.DescriptorName == name {
return def, true
}
}
return nil, false
}

func payloadFormatFromProto(format dpb.LogEntryDescriptor_PayloadFormat) PayloadFormat {
switch format {
case dpb.JSON:
Expand Down
162 changes: 124 additions & 38 deletions mixer/pkg/aspect/applicationLogsManager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package aspect
import (
"fmt"
"reflect"
"strings"
"testing"
"text/template"
"time"
Expand All @@ -31,6 +32,7 @@ import (
"istio.io/mixer/pkg/aspect/test"
"istio.io/mixer/pkg/attribute"
"istio.io/mixer/pkg/config"
"istio.io/mixer/pkg/config/descriptor"
configpb "istio.io/mixer/pkg/config/proto"
"istio.io/mixer/pkg/expr"
"istio.io/mixer/pkg/status"
Expand All @@ -44,6 +46,26 @@ type (
}
)

var (
validDesc = dpb.LogEntryDescriptor{
Name: "logentry",
PayloadFormat: dpb.TEXT,
LogTemplate: "{{.foo}}",
Labels: []*dpb.LabelDescriptor{
{Name: "label", ValueType: dpb.STRING},
},
}

applogsDF = test.NewDescriptorFinder(map[string]interface{}{
validDesc.Name: &validDesc,
// our attributes
"duration": &dpb.AttributeDescriptor{Name: "duration", ValueType: dpb.DURATION},
"string": &dpb.AttributeDescriptor{Name: "string", ValueType: dpb.STRING},
"int64": &dpb.AttributeDescriptor{Name: "int64", ValueType: dpb.INT64},
"timestamp": &dpb.AttributeDescriptor{Name: "timestamp", ValueType: dpb.TIMESTAMP},
})
)

func evalErrOnStr(s string) expr.Evaluator {
return test.NewFakeEval(func(e string, _ attribute.Bag) (interface{}, error) {
if e == s {
Expand All @@ -66,7 +88,7 @@ func TestLoggerManager_NewLogger(t *testing.T) {
name: "istio_log",
aspect: tl,
metadata: map[string]*logInfo{
"default": {
validDesc.Name: {
timeFormat: time.RFC3339,
},
},
Expand All @@ -76,43 +98,29 @@ func TestLoggerManager_NewLogger(t *testing.T) {
name: "istio_log",
aspect: tl,
metadata: map[string]*logInfo{
"default": {
validDesc.Name: {
timeFormat: "2006-Jan-02",
},
},
}

emptyExec := &applicationLogsExecutor{
name: "istio_log",
aspect: tl,
metadata: make(map[string]*logInfo),
}

newAspectShouldSucceed := []aspectTestCase{
{"empty", &aconfig.ApplicationLogsParams{
LogName: "istio_log",
Logs: []*aconfig.ApplicationLogsParams_ApplicationLog{
{
DescriptorName: "default",
DescriptorName: validDesc.Name,
TimeFormat: time.RFC3339,
},
}}, defaultExec},
{"override", &aconfig.ApplicationLogsParams{
LogName: "istio_log",
Logs: []*aconfig.ApplicationLogsParams_ApplicationLog{
{
DescriptorName: "default",
DescriptorName: validDesc.Name,
TimeFormat: "2006-Jan-02",
},
}}, overrideExec},
// TODO: should this be an error? An aspect with no logInfo objects will never do any work.
{"no descriptors", &aconfig.ApplicationLogsParams{
LogName: "istio_log",
Logs: []*aconfig.ApplicationLogsParams_ApplicationLog{
{
DescriptorName: "no descriptor with this name",
},
}}, emptyExec},
}

m := newApplicationLogsManager()
Expand All @@ -123,7 +131,7 @@ func TestLoggerManager_NewLogger(t *testing.T) {
Builder: &configpb.Adapter{Params: &ptypes.Empty{}},
Aspect: &configpb.Aspect{Params: v.params, Inputs: map[string]string{}},
}
asp, err := m.NewReportExecutor(&c, tl, atest.NewEnv(t), nil)
asp, err := m.NewReportExecutor(&c, tl, atest.NewEnv(t), applogsDF)
if err != nil {
t.Fatalf("NewExecutor(): should not have received error for %s (%v)", v.name, err)
}
Expand All @@ -140,12 +148,11 @@ func TestLoggerManager_NewLogger(t *testing.T) {
}

func TestLoggerManager_NewLoggerFailures(t *testing.T) {

defaultCfg := &aconfig.ApplicationLogsParams{
LogName: "istio_log",
Logs: []*aconfig.ApplicationLogsParams_ApplicationLog{
{
DescriptorName: "default",
DescriptorName: validDesc.Name,
TimeFormat: time.RFC3339,
},
},
Expand Down Expand Up @@ -173,13 +180,108 @@ func TestLoggerManager_NewLoggerFailures(t *testing.T) {
},
}

if _, err := m.NewReportExecutor(cfg, v.adptr, atest.NewEnv(t), nil); err == nil {
if _, err := m.NewReportExecutor(cfg, v.adptr, atest.NewEnv(t), applogsDF); err == nil {
t.Fatalf("NewExecutor(): expected error for bad adapter (%T)", v.adptr)
}
})
}
}

func TestLoggerManager_DefaultConfig(t *testing.T) {
m := newApplicationLogsManager()
got := m.DefaultConfig()
want := &aconfig.ApplicationLogsParams{LogName: "istio_log"}
if !proto.Equal(got, want) {
t.Errorf("DefaultConfig(): got %v, wanted %v", got, want)
}
}

func TestLoggerManager_ValidateConfig(t *testing.T) {
wrap := func(name string, log *aconfig.ApplicationLogsParams_ApplicationLog) *aconfig.ApplicationLogsParams {
return &aconfig.ApplicationLogsParams{
LogName: name,
Logs: []*aconfig.ApplicationLogsParams_ApplicationLog{log},
}
}

validDesc := dpb.LogEntryDescriptor{
Name: "logentry",
PayloadFormat: dpb.TEXT,
LogTemplate: "{{.foo}}",
Labels: []*dpb.LabelDescriptor{
{Name: "label", ValueType: dpb.STRING},
},
}
invalidDesc := validDesc
invalidDesc.Name = "invalid"
invalidDesc.LogTemplate = "{{.foo"

df := test.NewDescriptorFinder(map[string]interface{}{
validDesc.Name: &validDesc,
invalidDesc.Name: &invalidDesc,
// our attributes
"duration": &dpb.AttributeDescriptor{Name: "duration", ValueType: dpb.DURATION},
"string": &dpb.AttributeDescriptor{Name: "string", ValueType: dpb.STRING},
"int64": &dpb.AttributeDescriptor{Name: "int64", ValueType: dpb.INT64},
"timestamp": &dpb.AttributeDescriptor{Name: "timestamp", ValueType: dpb.TIMESTAMP},
})

validLog := aconfig.ApplicationLogsParams_ApplicationLog{
DescriptorName: validDesc.Name,
Severity: "string",
Timestamp: "timestamp",
Labels: map[string]string{"label": "string"},
TemplateExpressions: map[string]string{"foo": "int64"},
}

noLogName := wrap("", &validLog)

missingDesc := validLog
missingDesc.DescriptorName = "not in the applogsDF"

invalidSeverity := validLog
invalidSeverity.Severity = "int64"

invalidTimestamp := validLog
invalidTimestamp.Timestamp = "duration"

invalidLabels := validLog
invalidLabels.Labels = map[string]string{
"not a label": "string", // correct type, but doesn't match desc's label name
}

invalidDescLog := validLog
invalidDescLog.DescriptorName = invalidDesc.Name

tests := []struct {
name string
cfg *aconfig.ApplicationLogsParams
applogsDF descriptor.Finder
err string
}{
{"valid", wrap("valid", &validLog), df, ""},
{"empty config", &aconfig.ApplicationLogsParams{}, df, "LogName"},
{"no log name", noLogName, df, "LogName"},
{"missing desc", wrap("missing desc", &missingDesc), df, "could not find a descriptor"},
{"invalid severity", wrap("sev", &invalidSeverity), df, "Severity"},
{"invalid timestamp", wrap("ts", &invalidTimestamp), df, "Timestamp"},
{"invalid labels", wrap("labels", &invalidLabels), df, "Labels"},
{"invalid logtemplate", wrap("tmpl", &invalidDescLog), df, "LogDescriptor"},
}

for idx, tt := range tests {
t.Run(fmt.Sprintf("[%d] %s", idx, tt.name), func(t *testing.T) {
if err := (&applicationLogsManager{}).ValidateConfig(tt.cfg, expr.NewCEXLEvaluator(), tt.applogsDF); err != nil || tt.err != "" {
if tt.err == "" {
t.Fatalf("Foo = '%s', wanted no err", err.Error())
} else if !strings.Contains(err.Error(), tt.err) {
t.Fatalf("Expected errors containing the string '%s', actual: '%s'", tt.err, err.Error())
}
}
})
}
}

func TestLogExecutor_Execute(t *testing.T) {
timeAttrName := "timestamp"
knownTime := time.Now()
Expand Down Expand Up @@ -359,22 +461,6 @@ func TestLogExecutor_Close(t *testing.T) {
}
}

func TestLoggerManager_DefaultConfig(t *testing.T) {
m := newApplicationLogsManager()
got := m.DefaultConfig()
want := &aconfig.ApplicationLogsParams{LogName: "istio_log"}
if !proto.Equal(got, want) {
t.Errorf("DefaultConfig(): got %v, wanted %v", got, want)
}
}

func TestLoggerManager_ValidateConfig(t *testing.T) {
m := newApplicationLogsManager()
if err := m.ValidateConfig(&ptypes.Empty{}, nil, nil); err != nil {
t.Errorf("ValidateConfig(): unexpected error: %v", err)
}
}

func TestPayloadFormatFromProto(t *testing.T) {
tests := []struct {
name string
Expand Down

0 comments on commit be8181e

Please sign in to comment.