From e94cd33f1472004965df4555135226e8eb34fb41 Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Wed, 11 Sep 2024 15:59:53 +0200 Subject: [PATCH 1/2] filters/auth: benchmark jwtMetrics filter creation Signed-off-by: Alexander Yastrebov --- filters/auth/jwt_metrics_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/filters/auth/jwt_metrics_test.go b/filters/auth/jwt_metrics_test.go index ef76bef5fd..833a8bc82f 100644 --- a/filters/auth/jwt_metrics_test.go +++ b/filters/auth/jwt_metrics_test.go @@ -368,3 +368,17 @@ func marshalBase64JSON(t *testing.T, v any) string { } return base64.RawURLEncoding.EncodeToString(d) } + +func BenchmarkJwtMetrics_CreateFilter(b *testing.B) { + spec := auth.NewJwtMetrics() + args := []any{`{issuers: [foo, bar], optOutAnnotations: [oauth.disabled], optOutHosts: [ '^.+[.]domain[.]test$' ]}`} + + _, err := spec.CreateFilter(args) + require.NoError(b, err) + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = spec.CreateFilter(args) + } +} From 1ed2e452a15e16895fdc7ebeeaca226d0466b684 Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Mon, 9 Sep 2024 19:05:55 +0200 Subject: [PATCH 2/2] filters/auth: cache yaml config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cache parsed yaml config for `jwtMetrics` and `oauthTokeninfoValidate` filters to avoid parsing the same configuration over and over when these filters are appended as default filters to all routes. ``` │ HEAD~1 │ HEAD │ │ sec/op │ sec/op vs base │ JwtMetrics_CreateFilter-8 30777.00n ± 6% 18.45n ± 11% -99.94% (p=0.000 n=10) │ HEAD~1 │ HEAD │ │ B/op │ B/op vs base │ JwtMetrics_CreateFilter-8 24.33Ki ± 0% 0.00Ki ± 0% -100.00% (p=0.000 n=10) │ HEAD~1 │ HEAD │ │ allocs/op │ allocs/op vs base │ JwtMetrics_CreateFilter-8 180.0 ± 0% 0.0 ± 0% -100.00% (p=0.000 n=10) ``` Signed-off-by: Alexander Yastrebov --- filters/auth/jwt_metrics.go | 30 ++++----- filters/auth/tokeninfo.go | 59 +++++++++-------- filters/auth/yamlconfig.go | 84 +++++++++++++++++++++++ filters/auth/yamlconfig_test.go | 114 ++++++++++++++++++++++++++++++++ 4 files changed, 245 insertions(+), 42 deletions(-) create mode 100644 filters/auth/yamlconfig.go create mode 100644 filters/auth/yamlconfig_test.go diff --git a/filters/auth/jwt_metrics.go b/filters/auth/jwt_metrics.go index 1d426049fe..686a5da9d0 100644 --- a/filters/auth/jwt_metrics.go +++ b/filters/auth/jwt_metrics.go @@ -6,7 +6,6 @@ import ( "slices" "strings" - "github.com/ghodss/yaml" "github.com/opentracing/opentracing-go" "github.com/zalando/skipper/filters" "github.com/zalando/skipper/filters/annotate" @@ -14,8 +13,12 @@ import ( ) type ( - jwtMetricsSpec struct{} + jwtMetricsSpec struct { + yamlConfigParser[jwtMetricsFilter] + } + // jwtMetricsFilter implements [yamlConfig], + // make sure it is not modified after initialization. jwtMetricsFilter struct { Issuers []string `json:"issuers,omitempty"` OptOutAnnotations []string `json:"optOutAnnotations,omitempty"` @@ -27,7 +30,9 @@ type ( ) func NewJwtMetrics() filters.Spec { - return &jwtMetricsSpec{} + return &jwtMetricsSpec{ + newYamlConfigParser[jwtMetricsFilter](64), + } } func (s *jwtMetricsSpec) Name() string { @@ -35,26 +40,21 @@ func (s *jwtMetricsSpec) Name() string { } func (s *jwtMetricsSpec) CreateFilter(args []interface{}) (filters.Filter, error) { - f := &jwtMetricsFilter{} - - if len(args) == 1 { - if config, ok := args[0].(string); !ok { - return nil, fmt.Errorf("requires single string argument") - } else if err := yaml.Unmarshal([]byte(config), f); err != nil { - return nil, fmt.Errorf("failed to parse configuration") - } - } else if len(args) > 1 { - return nil, fmt.Errorf("requires single string argument") + if len(args) == 0 { + return &jwtMetricsFilter{}, nil } + return s.parseSingleArg(args) +} +func (f *jwtMetricsFilter) initialize() error { for _, host := range f.OptOutHosts { if r, err := regexp.Compile(host); err != nil { - return nil, fmt.Errorf("failed to compile opt-out host pattern: %q", host) + return fmt.Errorf("failed to compile opt-out host pattern: %q", host) } else { f.optOutHostsCompiled = append(f.optOutHostsCompiled, r) } } - return f, nil + return nil } func (f *jwtMetricsFilter) Request(ctx filters.FilterContext) {} diff --git a/filters/auth/tokeninfo.go b/filters/auth/tokeninfo.go index 7d33f50389..53323012cb 100644 --- a/filters/auth/tokeninfo.go +++ b/filters/auth/tokeninfo.go @@ -9,7 +9,6 @@ import ( "strings" "time" - "github.com/ghodss/yaml" "github.com/opentracing/opentracing-go" "github.com/zalando/skipper/filters" "github.com/zalando/skipper/filters/annotate" @@ -49,6 +48,8 @@ type ( tokeninfoSpec struct { typ roleCheckType options TokeninfoOptions + + tokeninfoValidateYamlConfigParser *yamlConfigParser[tokeninfoValidateFilterConfig] } tokeninfoFilter struct { @@ -60,11 +61,16 @@ type ( tokeninfoValidateFilter struct { client tokeninfoClient - config struct { - OptOutAnnotations []string `json:"optOutAnnotations,omitempty"` - UnauthorizedResponse string `json:"unauthorizedResponse,omitempty"` - OptOutHosts []string `json:"optOutHosts,omitempty"` - } + config *tokeninfoValidateFilterConfig + } + + // tokeninfoValidateFilterConfig implements [yamlConfig], + // make sure it is not modified after initialization. + tokeninfoValidateFilterConfig struct { + OptOutAnnotations []string `json:"optOutAnnotations,omitempty"` + UnauthorizedResponse string `json:"unauthorizedResponse,omitempty"` + OptOutHosts []string `json:"optOutHosts,omitempty"` + optOutHostsCompiled []*regexp.Regexp } ) @@ -167,9 +173,12 @@ func NewOAuthTokeninfoAnyKVWithOptions(to TokeninfoOptions) filters.Spec { } func NewOAuthTokeninfoValidate(to TokeninfoOptions) filters.Spec { + p := newYamlConfigParser[tokeninfoValidateFilterConfig](64) return &tokeninfoSpec{ typ: checkOAuthTokeninfoValidate, options: to, + + tokeninfoValidateYamlConfigParser: &p, } } @@ -244,10 +253,11 @@ func (s *tokeninfoSpec) CreateFilter(args []interface{}) (filters.Filter, error) } if s.typ == checkOAuthTokeninfoValidate { - if len(sargs) != 1 { - return nil, fmt.Errorf("requires single string argument") + config, err := s.tokeninfoValidateYamlConfigParser.parseSingleArg(args) + if err != nil { + return nil, err } - return createTokeninfoValidateFilter(ac, sargs[0]) + return &tokeninfoValidateFilter{client: ac, config: config}, nil } f := &tokeninfoFilter{typ: s.typ, client: ac, kv: make(map[string][]string)} @@ -274,22 +284,6 @@ func (s *tokeninfoSpec) CreateFilter(args []interface{}) (filters.Filter, error) return f, nil } -func createTokeninfoValidateFilter(client tokeninfoClient, arg string) (filters.Filter, error) { - f := &tokeninfoValidateFilter{client: client} - if err := yaml.Unmarshal([]byte(arg), &f.config); err != nil { - return nil, fmt.Errorf("failed to parse configuration") - } - - for _, host := range f.config.OptOutHosts { - if r, err := regexp.Compile(host); err != nil { - return nil, fmt.Errorf("failed to compile opt-out host pattern: %q", host) - } else { - f.optOutHostsCompiled = append(f.optOutHostsCompiled, r) - } - } - return f, nil -} - // String prints nicely the tokeninfoFilter configuration based on the // configuration and check used. func (f *tokeninfoFilter) String() string { @@ -444,6 +438,17 @@ func (f *tokeninfoFilter) Request(ctx filters.FilterContext) { func (f *tokeninfoFilter) Response(filters.FilterContext) {} +func (c *tokeninfoValidateFilterConfig) initialize() error { + for _, host := range c.OptOutHosts { + if r, err := regexp.Compile(host); err != nil { + return fmt.Errorf("failed to compile opt-out host pattern: %q", host) + } else { + c.optOutHostsCompiled = append(c.optOutHostsCompiled, r) + } + } + return nil +} + func (f *tokeninfoValidateFilter) Request(ctx filters.FilterContext) { if _, ok := ctx.StateBag()[tokeninfoCacheKey]; ok { return // tokeninfo was already validated by a preceding filter @@ -458,9 +463,9 @@ func (f *tokeninfoValidateFilter) Request(ctx filters.FilterContext) { } } - if len(f.optOutHostsCompiled) > 0 { + if len(f.config.optOutHostsCompiled) > 0 { host := ctx.Request().Host - for _, r := range f.optOutHostsCompiled { + for _, r := range f.config.optOutHostsCompiled { if r.MatchString(host) { return // opt-out from validation } diff --git a/filters/auth/yamlconfig.go b/filters/auth/yamlconfig.go new file mode 100644 index 0000000000..4d5f56dd96 --- /dev/null +++ b/filters/auth/yamlconfig.go @@ -0,0 +1,84 @@ +package auth + +import ( + "fmt" + + "github.com/ghodss/yaml" +) + +// yamlConfigParser parses and caches yaml configurations of type T. +// Use [newYamlConfigParser] to create instances and ensure that *T implements [yamlConfig]. +type yamlConfigParser[T any] struct { + initialize func(*T) error + cacheSize int + cache map[string]*T +} + +// yamlConfig must be implemented by config value pointer type. +// It is used to initialize the value after parsing. +type yamlConfig interface { + initialize() error +} + +// newYamlConfigParser creates a new parser with a given cache size. +func newYamlConfigParser[T any, PT interface { + *T + yamlConfig +}](cacheSize int) yamlConfigParser[T] { + // We want user to specify config type T but ensure that *T implements [yamlConfig]. + // + // Type inference only works for functions but not for types + // (see https://github.com/golang/go/issues/57270 and https://github.com/golang/go/issues/51527) + // therefore we create instances using function with two type parameters + // but second parameter is inferred from the first so the caller does not have to specify it. + // + // To use *T.initialize we setup initialize field + return yamlConfigParser[T]{ + initialize: func(v *T) error { return PT(v).initialize() }, + cacheSize: cacheSize, + cache: make(map[string]*T, cacheSize), + } +} + +// parseSingleArg calls [yamlConfigParser.parse] with the first string argument. +// If args slice does not contain a single string, it returns an error. +func (p *yamlConfigParser[T]) parseSingleArg(args []any) (*T, error) { + if len(args) != 1 { + return nil, fmt.Errorf("requires single string argument") + } + config, ok := args[0].(string) + if !ok { + return nil, fmt.Errorf("requires single string argument") + } + return p.parse(config) +} + +// parse parses a yaml configuration or returns a cached value +// if the exact configuration was already parsed before. +// Returned value is shared by multiple callers and therefore must not be modified. +func (p *yamlConfigParser[T]) parse(config string) (*T, error) { + if v, ok := p.cache[config]; ok { + return v, nil + } + + v := new(T) + if err := yaml.Unmarshal([]byte(config), v); err != nil { + return nil, err + } + + if err := p.initialize(v); err != nil { + return nil, err + } + + // evict random element if cache is full + if p.cacheSize > 0 && len(p.cache) == p.cacheSize { + for k := range p.cache { + delete(p.cache, k) + break + } + } + + p.cache[config] = v + + return v, nil +} diff --git a/filters/auth/yamlconfig_test.go b/filters/auth/yamlconfig_test.go new file mode 100644 index 0000000000..4a0df3662e --- /dev/null +++ b/filters/auth/yamlconfig_test.go @@ -0,0 +1,114 @@ +package auth + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testConfig struct { + Name string + Value int + Error bool + + initialized bool +} + +func (tc *testConfig) initialize() error { + if tc.Error { + return fmt.Errorf("error initializing %s", tc.Name) + } + tc.initialized = true + return nil +} + +func TestYamlConfig_parse(t *testing.T) { + const ( + fooConfig = `{name: foo, value: 42}` + barConfig = `{name: bar, value: 1984}` + bazConfig = `{name: baz, value: 3024}` + ) + + yc := newYamlConfigParser[testConfig](2) + + foo1, err := yc.parse(fooConfig) + require.NoError(t, err) + assert.Equal(t, "foo", foo1.Name) + assert.Equal(t, 42, foo1.Value) + assert.True(t, foo1.initialized) + + foo2, err := yc.parse(fooConfig) + require.NoError(t, err) + assert.True(t, foo1 == foo2, "expected cached instance") + + bar1, err := yc.parse(barConfig) + require.NoError(t, err) + assert.Equal(t, "bar", bar1.Name) + assert.Equal(t, 1984, bar1.Value) + assert.True(t, bar1.initialized) + + baz1, err := yc.parse(bazConfig) + require.NoError(t, err) + assert.Equal(t, "baz", baz1.Name) + assert.Equal(t, 3024, baz1.Value) + assert.True(t, baz1.initialized) + + // check either foo or bar was evicted + assert.Len(t, yc.cache, 2) + assert.Contains(t, yc.cache, bazConfig) + assert.Subset(t, map[string]*testConfig{ + fooConfig: foo1, + barConfig: bar1, + bazConfig: baz1, + }, yc.cache) +} + +func TestYamlConfig_parse_errors(t *testing.T) { + t.Run("invalid yaml", func(t *testing.T) { + yc := newYamlConfigParser[testConfig](1) + + config, err := yc.parse(`invalid yaml`) + assert.Error(t, err) + assert.Nil(t, config) + }) + + t.Run("initialize error", func(t *testing.T) { + yc := newYamlConfigParser[testConfig](1) + + config, err := yc.parse(`{name: foo, error: true}`) + assert.EqualError(t, err, "error initializing foo") + assert.Nil(t, config) + }) +} + +func TestYamlConfig_parseSingleArg(t *testing.T) { + yc := newYamlConfigParser[testConfig](1) + + t.Run("single string arg", func(t *testing.T) { + config, err := yc.parseSingleArg([]any{`{name: foo, value: 42}`}) + require.NoError(t, err) + assert.Equal(t, "foo", config.Name) + assert.Equal(t, 42, config.Value) + assert.True(t, config.initialized) + }) + + t.Run("single non-string arg", func(t *testing.T) { + config, err := yc.parseSingleArg([]any{42}) + assert.EqualError(t, err, "requires single string argument") + assert.Nil(t, config) + }) + + t.Run("empty args", func(t *testing.T) { + config, err := yc.parseSingleArg([]any{}) + assert.EqualError(t, err, "requires single string argument") + assert.Nil(t, config) + }) + + t.Run("too many args", func(t *testing.T) { + config, err := yc.parseSingleArg([]any{`{name: foo, value: 42}`, `{name: bar, value: 1984}`}) + assert.EqualError(t, err, "requires single string argument") + assert.Nil(t, config) + }) +}